work-club-manager/.sisyphus/notepads/club-work-manager/decisions.md

# Decisions — Club Work Manager

_Architectural choices and technical decisions made during implementation_

---

## Decision 1: Comma-Separated Clubs Format (2026-03-05)

### Context
Keycloak sends clubs as comma-separated UUIDs instead of JSON dictionary, but the system expected dictionary format `{"uuid": "role"}`.

### Decision: Accept Comma-Separated UUIDs, Lookup Roles from Database

**Rationale:**
- Cleaner claim format (less space in JWT)
- Single source of truth: Member table in database
- Decouples authorization from Keycloak configuration
- Simplifies Keycloak setup (no complex mapper needed)

**Implementation:**
1. TenantValidationMiddleware: Split comma-separated string, validate requested tenant
2. ClubRoleClaimsTransformation: Split clubs claim, lookup Member role from database
3. Both use synchronous `.FirstOrDefault()` for consistency and performance

**Implications:**
- Database must be accessible during authentication (fast query with indexed user + tenant)
- Role changes require database update (not JWT refresh)
- Members table is authoritative for role assignment

## Decision 2: Synchronous Database Query in IClaimsTransformation (2026-03-05)

### Context
Initially implemented async database query with `FirstOrDefaultAsync()` in IClaimsTransformation.

### Decision: Use Synchronous `.FirstOrDefault()` Query

**Rationale:**
- IClaimsTransformation.TransformAsync() must return Task<ClaimsPrincipal>
- Hot reload (dotnet watch) fails when making method async (ENC0098)
- Synchronous query is acceptable: single database lookup, minimal blocking
- Avoids async/await complexity in authentication pipeline

**Implications:**
- Slightly less async but prevents hot reload issues
- Query performance is milliseconds (indexed on ExternalUserId + TenantId)
- Error handling via try/catch returns null role (fallback behavior)


## Decision 3: Entity Framework Connection Interceptor Architecture (2026-03-05)

### Context
Attempted to set PostgreSQL session variable (`SET LOCAL app.current_tenant_id`) in `ConnectionOpeningAsync` but failed with "Connection is not open" exception.

### Decision: Move SQL Execution to ConnectionOpened Lifecycle Phase

**Rationale:**
- `ConnectionOpeningAsync` executes before actual connection establishment
- `ConnectionOpened` is guaranteed to have an open connection
- Synchronous execution is acceptable in ConnectionOpened callback
- Logging/validation can happen in ConnectionOpeningAsync without SQL

**Implementation:**
- ConnectionOpeningAsync: Only logs warnings about missing tenant
- ConnectionOpened: Executes SET LOCAL command synchronously
- Both methods check for tenant context, only opened executes SQL

**Implications:**
- Tenant isolation guaranteed at connection open time
- RLS (Row-Level Security) policies see correct tenant_id
- Error handling via try/catch with logging
- Synchronous operation in callback is expected pattern


---

## Decision 4: actions/upload-artifact@v3 Over v4 for Gitea Compatibility (2026-03-06)

### Context
Gitea CI pipeline needs artifact uploads for test results and build logs on failure. GitHub Actions has v3 and v4 of upload-artifact available.

### Decision: Use actions/upload-artifact@v3

**Rationale:**
- v3: Stable across Gitea 1.18-1.22+ (verified by community reports)
- v4: Breaking changes in cache format (requires Gitea 1.21+)
- Project may deploy to various Gitea instances (internal/external)
- CI reliability > performance improvement (~30% upload speed gain in v4)

**Tradeoffs Considered:**
- v4 Performance: 30% faster uploads, better compression
- v3 Compatibility: Works on wider range of Gitea versions
- Decision: Prioritize compatibility for this infrastructure-critical workflow

**Implications:**
- Slightly slower artifact uploads (non-critical for failure-only uploads)
- If Gitea version known to be 1.21+, can upgrade to v4
- Document decision to prevent confusion during future reviews