Review: hooks.server.ts + app.d.ts — westside-admin#15

Verdict: APPROVED

Template Completeness

• [x] Type — Feature
• [x] Lineage — cites parent #2, dependency #14, siblings #16/#17
• [x] Repo — forgejo_admin/westside-admin
• [x] User Story — story-westside-admin-admin-row-crud with funnel-auth rationale
• [x] Context — explains the per-request enforcement role, lazy-refresh behavior, how it consumes #14, does-not-define for #16/#17
• [x] File Targets — 1 create, 1 update, 3 explicit do-not-touch directories with their owning sub-tasks named
• [x] Acceptance Criteria — 8 testable items
• [x] Test Expectations — manual integration matrix (4 cases) with stated rationale for skipping vitest target until #17 lands
• [x] Constraints — SvelteKit cookie API, locals-not-globalThis, exact cookie-name string, rewrite-not-redirect for 403, no route allowlist, import path style
• [x] Checklist — includes funnel-auth review per feedback_funnel_requires_auth
• [x] Related — project page, parent, dep, dataflow note, funnel-auth memory

Traceability

• [x] story:admin-row-crud label — matches project Safety Constraints contract
• [x] story note verified — entry present in project-westside-admin user-stories table (story-westside-admin-admin-row-crud)
• [x] arch:westside-admin label — umbrella satisfied by arch-dataflow-westside-admin Flow 1, which sequences this hook explicitly (browser to SK to hooks.server.ts to JWKS validation to role parse to 403 OR locals.user to load())
• [!] arch:keycloak label has NO backing note — same gap as siblings #14 (review-1134), parent #1132, dep #1131. arch-keycloak still does not exist; arch-dataflow-westside-admin Flow 1 jointly covers the Keycloak surface for this ticket. Per the carry-over [SCOPE] recommendation in review-1134, Ava's call whether to author arch-keycloak now or defer. Not a per-ticket blocker.
• [x] Forgejo issue valid and open. Target repo forgejo_admin/westside-admin, sub-task 2 of 4.

File Targets

• [x] src/hooks.server.ts (create) — does not exist yet (verified via find). Correct location: SvelteKit picks this path up automatically. OK to create.
• [x] src/app.d.ts (update) — exists with the unmodified scaffold (App namespace block with all interfaces commented out). Adding App.Locals.user here is the one-line uncomment + fill, exactly the right surface.
• [x] Do-NOT-touch:
• [x] vitest.config.ts + package.json test scripts landed via #14/PR #18 (verified). The 'no vitest target' note in Test Expectations is accurate scoping (manual integration only for this sub-task), but vitest IS now available if the agent wants to add a unit test for cookie parsing or 30s-window calculation as a free-throw.

Repo Placement

Dependencies

• westside-admin#14 (keycloak.ts lib) — MERGED as PR #18 (982df5b). Board item #1134 sits in validation column. Direct unblock.
• pal-e-deployments#147 (KEYCLOAK_CLIENT_SECRET landing) — MERGED. ArgoCD reconciling per prompt. The hook's env-var reads (KEYCLOAK_URL, KEYCLOAK_REALM, KEYCLOAK_CLIENT_ID, KEYCLOAK_CLIENT_SECRET, COOKIE_SIGNING_KEY) all happen at request time via the lib's requireEnv; absent envs throw MissingEnvError. For dev validation against npm run dev on archbox, the agent will need a local .env shim — not a blocker but worth flagging.
• Sibling sub-task #16 (auth endpoints) — OPEN, in backlog. The hook redirects to /auth/login?redirect={url}; sub-task #16 must create this endpoint. Hook does NOT block on #16 to land code, but end-to-end manual ACs cannot fully pass until #16 + #17 ship together. Acknowledged in the issue body's Test Expectations.
• Sibling sub-task #17 (403 page) — OPEN, in backlog. Hook rewrites to the (unauthorized) route group; #17 creates the page itself. Hook code can ship; the rewrite target is a 404 until #17 lands.
• Implicit ordering: #15 then #16 then #17, with full E2E only after #17. Acknowledged.

Acceptance Criteria

• AC #4 (tampered cookie treated as anonymous, never throws) — directly maps to decryptCookiePayload's null-on-tamper contract verified above. Crisp.
• AC #3 (missing-admin = rewrite, NOT redirect) — explicit and re-stated in Constraints. Prevents the common bug of redirecting to /403 (which would create a redirect loop if /403 itself is admin-gated).
• AC #6 (cookie attributes on every refresh) — covers the failure mode where a refresh path forgets to re-set HttpOnly+Secure+SameSite.
• AC #7 (no tokens / refresh tokens / ciphertext in any log) — mechanical grep verification post-implementation.
• AC #8 (event.locals.user typed end-to-end) — couples the hook to the app.d.ts update; agent cannot half-finish either side.

Blast Radius

• Health-check exclusion: issue body says 'every route is admin-gated by default; the only exclusions are /auth/* and /(unauthorized)'. src/routes/health already exists (verified via ls). Either it must be admin-gated (and Kubernetes liveness/readiness probes need to be configured to skip auth, which they cannot directly — the funnel does not know about probes), OR the hook needs a third exclusion for /health. This is a real implementation question that the issue body does not resolve. See Recommendation.
• Cookie attribute symmetry: the lib does not enforce cookie attributes (it returns ciphertext strings only). Every hook code path that calls event.cookies.set(...) must include { httpOnly: true, secure: true, sameSite: 'lax', path: '/' }. SvelteKit's secure default is only true in HTTPS contexts; httpOnly defaults to true but explicit is better. AC #6 covers this conceptually.
• Locals shape lock-in: the App.Locals.user type defined in this PR becomes a load-bearing interface for every +page.server.ts in the project. Renaming roles to realmRoles later means touching every consumer. Issue body's explicit shape — { sub: string; email: string; name: string; roles: string[] } — is fine; just call it out as locked once shipped.

Decomposition Assessment

• File count: 1 create + 1 update = 2 files. Well under threshold.
• AC count: 8 — over the 5-AC paper rule but, like #14, the ACs all map to a single file's behavior. They are facets of one handler function (handle), not orthogonal user-visible features.
• Estimated time: 5-8 min agent run. The hook is roughly 60-100 lines of TypeScript.
• Cohesion: handle is necessarily a single function in one file. Splitting (e.g., one PR for happy path, one for refresh, one for 403) would create N partially-broken intermediate states. Anti-pattern.

Recommendation

• [BODY] (should-fix, not blocker) Resolve the /health route question. Add a third exclusion to the 'every route is admin-gated by default; the only exclusions are /auth/* and /(unauthorized)' sentence — either explicitly include /health as a third exclusion, or explicitly state '/health is admin-gated; Kubernetes probes hit the pod via a cluster-internal path.' Without this clarification the agent has to guess. Recommend: exclude /health (the simpler answer).
• [BODY] (optional) Add to AC #4 or as a new AC: 'If refreshTokensIfNeeded throws TokenRefreshError, the request is treated as anonymous (cookie cleared, 302 to /auth/login).' Currently inferable but explicit is better.
• [BODY] (optional, cosmetic) Tighten the Constraints sentence 'Cookie name MUST be exactly westside_admin_session (matches #14's expectation and the spec in #2)' to 'Cookie name MUST be exactly westside_admin_session (defined here for the first time; #14's lib is cookie-name-agnostic by design — the hook owns this constant; matches the spec in #2).' More accurate.
• [SCOPE] (carry-over from review-1134) Decide whether to create arch-keycloak note now or continue deferring. Same status as before: arch-dataflow-westside-admin Flow 1 covers the surface this ticket touches. Ava's call.
• Sequencing note for whoever dispatches: #15 is unblocked NOW (#14/PR #18 merged, env vars merged via #147). Ready to move backlog to todo to next_up to in_progress. Sibling sub-tasks #16/#17 can start in parallel if Ava wants — they don't share files with #15 (do-not-touch fences are clean) and the import surface they need from $lib/server/keycloak is already on main. The only constraint is end-to-end manual ACs require all four landed.

Key Finding Summary