gstack/test/fixtures/eng-retained-corpus-au.md

Plan: Multi-tenant Auth Refactor (reviewed)

Reviewed by /plan-eng-review on 2026-09-10, branch main, commit e51cff2. Mode: SCOPE_REDUCED (Step 0 accepted). Source plan: PLAN.md. Caveat: the repository contains only PLAN.md and CLAUDE.md; no source was available to read. Every finding below is grounded in plan text (PLAN.md:line) and marked with its confidence. Module paths under src/ are placeholders to be mapped onto the real tree at implementation time.

Context

Auth is being reworked so two services, AuthBroker (validates tokens and dispatches requests) and SessionMint (issues sessions), serve multiple tenants over the existing tenant-keyed cache adapter. The original plan (PLAN.md:18-36) shared one module-level mutable AuthCache between both services, rewrote legacyAuthFlow() in place with no regression test, carried a 60-line validateAndDispatch() with three error-swallowing catches, made five sequential IDP calls per validation, and introduced four new classes across twelve files. This review keeps the goal (multi-tenant auth on the existing adapter) and hardens how it gets there: injected single-writer cache, staged cutover, explicit error results, parallel validation, and full test coverage including the legacy regression.

Existing contracts retained (unchanged from source plan)

The existing cache adapter keys entries by tenant ID, issuer, audience, and policy version. It evicts expired tokens and invalidates entries on logout, token revocation, or tenant suspension. AuthCache retains these validity and tenant-key rules and is a service-facing facade over that same adapter, with one backing cache. The adapter, its invalidation hooks, and their existing tests remain in use unchanged (PLAN.md:7-13).

Step 0: Scope decision (D4, accepted)

Complexity check triggered: 12 files, 4 new classes (PLAN.md:35-36).

Decision: reduce.

• TokenStore is cut. The adapter plus the AuthCache facade already own storage, eviction, and invalidation; a second storage abstraction had no stated job. (Plan-text evidence, confidence 6/10.)
• RequestPolicy starts as a module of pure functions (src/auth/request-policy.ts), promoted to a class only if it grows per-tenant state.
• New units: AuthBroker, SessionMint, AuthCache facade, plus the policy function module. Target footprint about 8 files.

Logged as decision 0fdd2895 via gstack-decision-log. Scope is settled; later sections do not re-argue it.

Architecture

Component and data flow

                 request (tenant, token)
                          |
                          v
   +----------------------------------------------+
   |  composition root (one place, app startup)   |
   |  authCache = new AuthCache(existingAdapter)  |
   |  broker    = new AuthBroker(authCache, idp,  |
   |                             policyFns)       |
   |  mint      = new SessionMint(authCache, idp) |
   +----------------------------------------------+
          |  read / invalidate            | write (single writer)
          v                               v
   +-------------+                 +--------------+
   |  AuthBroker |  ---mint req--> |  SessionMint |
   +-------------+                 +--------------+
          \                               /
           \        AuthCache facade     /
            +---------------------------+
            | get(key) / invalidate(key)|
            | putIfVersion(key, entry,  |
            |   expectedPolicyVersion)  |
            +---------------------------+
                          |
                existing cache adapter
          (tenant, issuer, audience, policyVersion)
          eviction + invalidation hooks (unchanged)

Issue 1 [P1] (confidence 8/10) PLAN.md:19-20, :10 — shared global mutable cache (D5, approved: A)

Problem: both services mutate one module-level AuthCache and the facade "does not serialize mutations". A tenant-suspension invalidation that fires mid-mint can be followed by the mint's write, leaving a suspended tenant with a live session. Module-level exports also leak state across tests and double up under hot reload. [Layer 1] constructor injection is the proven answer; the write-ordering problem needs an explicit rule on top.

Remedy (in plan):

1. No module-level export. AuthCache is constructed once in the composition root and passed to AuthBroker and SessionMint by constructor.
2. Single writer: SessionMint is the only component that writes session entries. AuthBroker reads and calls invalidate.
3. Version-checked writes: AuthCache.putIfVersion(key, entry, expectedPolicyVersion) rejects the write when the entry's policy version changed or the key was invalidated since the read. Returns an explicit Rejected result; SessionMint surfaces it as TenantSuspended/Stale, never retries blindly.
4. Unit test forces the interleaving (read → invalidate → write) and asserts the write is rejected.

   Write protocol (per tenant key)
   SessionMint            AuthCache                 adapter
      |  read(key) ----------> |  get ----------------> |
      |  <-- {entry, v=7} ---- |                        |
      |                        |  <== invalidate(key) ==|  (suspension hook)
      |  putIfVersion(key,     |                        |
      |     entry', expect=7)->|  compare v: gone/!=7   |
      |  <-- Rejected -------- |                        |
      |  => TenantSuspended, no session issued

Issue 2 [P1] (confidence 8/10) PLAN.md:27-28 — big-bang rewrite of legacyAuthFlow() (D6, approved: A)

Problem: the legacy path is replaced in one shot with no rollback other than a redeploy. Multi-tenant auth means one wrong branch locks out a customer.

Remedy (in plan): strangler-fig cutover.

1. Per-tenant flag auth.brokerPath (values: legacy, shadow, broker).
2. shadow: run both paths, serve the legacy decision, log any mismatch (allow/deny, tenant, policy version, reason code) to a dedicated auth.shadow.mismatch event.
3. broker: serve the new path. Rollback is a flag flip, no deploy.
4. Legacy deletion is a follow-up PR (see TODOS) once mismatches are zero across all tenants for the agreed window.

   Cutover state machine (per tenant)
   [legacy] --enable shadow--> [shadow] --0 mismatches over window--> [broker]
      ^                          |                                      |
      +------- flag flip --------+------------ flag flip ---------------+
   Exit: all tenants in [broker] for N days  ==> delete legacy + flag (TODO)

Production failure scenarios considered:

• New path denies a valid token for one tenant: caught in shadow as a mismatch before any user is affected; in broker, flag flip restores legacy in seconds.
• Shadow doubles IDP load: acceptable for the window; metadata cache from Issue 5 keeps it to about one extra call per request.

Code quality

Issue 3 [P1] (confidence 8/10) PLAN.md:23-24 — validateAndDispatch() swallows three error classes (D7, approved: A)

Problem: 60 lines, three nested try/catch blocks, each swallowing a different error class. In auth a swallowed error is a silent deny at best and a silent allow at worst, with no log line to tell a bad token from an IDP outage.

Remedy (in plan):

1. Split into four small steps: parseToken, validateToken, resolveTenant, dispatch, each about 10 lines and unit-tested alone.
2. Each step returns a discriminated union: Ok<T> | InvalidToken | TenantSuspended | IdpUnavailable | Unexpected. No nested try/catch; a single try at the step that performs I/O maps the thrown error to one of these variants.
3. One boundary at the top (validateAndDispatch) maps each variant to a response code and a structured log with tenant id and request id. Unexpected always logs at error level and denies.
4. Callers consume the result type; no exceptions cross the boundary.

   validateAndDispatch (pipeline)
   parseToken -> validateToken -> resolveTenant -> dispatch
       |             |               |               |
     InvalidToken  IdpUnavailable  TenantSuspended  Ok
       \_____________|_______________|______________/
                              |
                    boundary: map -> response + log
                    Unexpected => 500 + error log + deny

DRY: key construction (tenant, issuer, audience, policyVersion) lives only in the adapter; AuthCache exposes typed keys so neither service rebuilds them.

Tests

Test framework: none detectable in this repository (no package.json, config, or test files). Test file names below follow *.test.ts; adjust to the real project convention.

Coverage diagram (planned code, after approved remedies)

CODE PATHS                                              USER FLOWS
[+] src/auth/auth-cache.ts                              [+] Login (tenant A, tenant B side by side)
  ├── get()                                               ├── [GAP] [→E2E] A logs in, B logs in, no cross read
  │   └── [GAP] hit / miss / expired                      ├── [GAP] [→E2E] Double-submit login → one session
  ├── invalidate()                                        └── [GAP] [→E2E] IDP timeout → clear retry message
  │   └── [GAP] logout / revoke / suspend (hook wiring)  [+] Logout
  └── putIfVersion()                                      └── [GAP] [→E2E] A logs out, B unaffected
      ├── [GAP] version matches → stored                 [+] Suspension / revocation
      ├── [GAP] version differs → Rejected                ├── [GAP] [→E2E] suspend A → next request denied
      └── [GAP] key invalidated since read → Rejected     ├── [GAP] [→E2E] revoke token → next request denied
[+] src/auth/session-mint.ts                              └── [GAP] [→E2E] suspend during mint → no session
  ├── mint()
  │   ├── [GAP] happy path                               [+] Cutover
  │   ├── [GAP] Rejected → TenantSuspended                ├── [GAP] flag=legacy serves legacy path
  │   └── [GAP] IDP failure → IdpUnavailable              ├── [GAP] flag=shadow logs mismatch, serves legacy
[+] src/auth/auth-broker.ts                               └── [GAP] flag=broker serves new path
  ├── parseToken()      [GAP] valid / malformed / empty
  ├── validateToken()   [GAP] ok / expired / bad sig     [+] Error states
  │   ├── [GAP] Promise.all one-fails → fail fast         ├── [GAP] IDP 500 → clear error, logged w/ tenant+req id
  │   ├── [GAP] per-call timeout → IdpUnavailable         ├── [GAP] malformed JWKS → deny, logged, no crash
  │   └── [GAP] metadata cache hit / miss / TTL expiry    └── [GAP] Unexpected → 500, error log, deny
  ├── resolveTenant()   [GAP] known / suspended / unknown
  ├── dispatch()        [GAP] ok / downstream error
  └── validateAndDispatch() boundary
      └── [GAP] every variant → response + log
[+] src/auth/request-policy.ts (pure fns)
  └── [GAP] each policy fn: allow / deny / edge inputs
[~] src/auth/legacy-auth-flow.ts (kept behind flag)
  └── [GAP] [CRITICAL REGRESSION] recorded corpus: legacy vs broker parity
[=] existing cache adapter                              (★★★ TESTED — existing suite, unchanged)

COVERAGE: 1/34 paths tested (3%)  |  Code paths: 1/22  |  User flows: 0/12
QUALITY: ★★★:1  |  GAPS: 33 (10 E2E, 1 CRITICAL regression, 0 eval)

Legend: ★★★ behavior + edge + error | ★★ happy path | ★ smoke | [→E2E] integration test | [+] new | [~] modified | [=] unchanged

Issue 4 [P1] (confidence 9/10) PLAN.md:14-16 — no end-to-end coverage of the auth flows (D8, approved: A)

Remedy (in plan): test/e2e/multi-tenant-auth.e2e.test.ts with a two-tenant fixture and a fake IDP server.

• Login/logout/revoke/suspend/expired for tenant A while tenant B stays logged in; assert B never sees A's session and no cross-tenant cache read.
• Fake IDP injects timeout, 500, malformed JWKS; assert the user-visible error is explicit and the log carries tenant id and request id.
• Interleaving test: trigger suspension between read and write inside SessionMint.mint(); assert putIfVersion rejects and no session exists.
• Cutover tests: each flag value routes as specified; shadow mismatch event is emitted on a deliberately divergent case and absent on the happy path.

CRITICAL regression test (mandatory under the regression rule, no question asked)

legacyAuthFlow() is existing behavior being replaced (PLAN.md:27-28) and the source plan explicitly omitted a regression test (PLAN.md:14-16). Add test/auth/legacy-parity.regression.test.ts:

• Record a corpus of real-shaped requests (valid, expired, revoked, suspended tenant, wrong audience, wrong issuer, malformed) with the legacy decision for each.
• Run the corpus through the new AuthBroker path and assert identical allow/deny and reason code for every entry.
• This test is also the shadow-mode oracle; it stays after legacy deletion, re-pointed at the recorded decisions.

Unit tests (one file per module, every branch in the diagram)

• auth-cache.test.ts: get hit/miss/expired; invalidate per hook; putIfVersion stored/rejected-version/rejected-invalidated.
• session-mint.test.ts: happy, Rejected → TenantSuspended, IDP failure.
• auth-broker.test.ts: each step's variants; boundary mapping for every variant including Unexpected; Promise.all fail-fast; per-call timeout; metadata cache hit/miss/TTL expiry.
• request-policy.test.ts: every policy function, allow/deny/boundary inputs, null/empty tenant.

QA test plan artifact (for /qa and /qa-only): ~/.gstack/projects/gstack-plan-count-kpTDIg/vercel-sandbox-main-eng-review-test-plan-20260910-211647.md

Performance

Issue 5 [P2] (confidence 8/10) PLAN.md:31-32 — five sequential IDP calls per validation (D9, approved: A)

Remedy (in plan):

1. Promise.all over the independent calls [Layer 1, standard library]. Fail-fast is correct: any failed check means the token is invalid, so partial results have no value (Promise.allSettled not appropriate).
2. Every IDP call wrapped with AbortSignal.timeout(ms); a timeout maps to IdpUnavailable, never a hung login.
3. Per-issuer metadata cache (discovery document, JWKS) with TTL and key-rotation handling (on unknown kid, refresh once, then fail). Steady-state validation makes one IDP call instead of five.
4. Tests: one-fails fail-fast, timeout path, cache hit/miss/expiry, unknown kid refresh.

   before: IDP1 -> IDP2 -> IDP3 -> IDP4 -> IDP5      (5 RTT)
   after:  [discovery, JWKS from cache] + Promise.all([introspect, ...])  (~1 RTT)
           each call: AbortSignal.timeout -> IdpUnavailable

Failure modes

New codepath	Realistic failure	Test	Handling	User sees
AuthCache.putIfVersion	suspension between read and write	interleaving unit + E2E	Rejected → TenantSuspended	explicit deny
SessionMint.mint	IDP timeout mid-mint	unit + fake IDP E2E	IdpUnavailable	clear retry message
AuthBroker.validateToken	one of N calls fails	unit (fail-fast)	Promise.all rejects → InvalidToken/IdpUnavailable	explicit deny
AuthBroker.validateToken	hung IDP endpoint	unit (timeout)	AbortSignal.timeout	retry message, not a spinner
metadata cache	stale JWKS after key rotation	unit (unknown kid)	refresh once, then fail	brief deny, self-heals
validateAndDispatch boundary	unknown exception	unit (Unexpected)	500 + error log + deny	generic error, logged
shadow mode	paths disagree	cutover test	mismatch event, legacy served	nothing (by design)
flag broker	new path wrong for a tenant	regression corpus	flag flip rollback	recovers in seconds

Critical gaps in the source plan (no test, no handling, silent): swallowed errors in validateAndDispatch and write-after-invalidate on the shared cache. Both are closed by approved remedies (Issues 1 and 3). Open critical gaps after review: 0.

What already exists

• Existing cache adapter: tenant/issuer/audience/policy-version keying, eviction, invalidation hooks, and tests. Reused unchanged; AuthCache is a thin facade. TokenStore would have rebuilt this and is cut.
• legacyAuthFlow(): retained behind the per-tenant flag as the shadow oracle and regression baseline until deletion.
• validateAndDispatch(): exists; refactored, not rewritten from scratch.
• Promise.all, AbortSignal.timeout: platform built-ins, no dependency.

NOT in scope

• TokenStore class: cut; storage is the adapter's job (D4).
• RequestPolicy as a class: deferred until it holds state (D4).
• Deleting legacyAuthFlow() and the cutover flag: follow-up PR after the shadow window (TODO below).
• Replacing or re-keying the existing cache adapter: out of scope; its contracts are retained by design.
• Adapter-level mutation serialization (locks): not needed once single writer + version-checked writes are in place.
• New distribution artifacts: none introduced; no pipeline work needed.

Diagrams to embed in code

• src/auth/auth-cache.ts: the write-protocol sequence (Issue 1).
• src/auth/auth-broker.ts: the validateAndDispatch pipeline (Issue 3).
• src/auth/cutover.ts (flag routing): the cutover state machine (Issue 2).
• test/e2e/multi-tenant-auth.e2e.test.ts: two-tenant fixture layout. Update these diagrams in the same commit as any change to the code they describe.

Worktree parallelization strategy

Step	Modules touched	Depends on
S1 AuthCache facade + putIfVersion + tests	src/auth/auth-cache, test/auth	—
S2 validateAndDispatch split + typed results + request-policy fns	src/auth/auth-broker, src/auth/request-policy, test/auth	—
S3 Promise.all + timeout + metadata cache	src/auth/auth-broker (validateToken), test/auth	S2
S4 SessionMint on injected cache	src/auth/session-mint, test/auth	S1
S5 Cutover flag + shadow compare + regression corpus	src/auth/cutover, src/auth/legacy-auth-flow, test/auth	S2, S4
S6 Two-tenant E2E + fake IDP	test/e2e	S1–S5

Lanes:

• Lane A: S1 → S4 (sequential, shared cache contract)
• Lane B: S2 → S3 (sequential, both in auth-broker)
• Lane C: S5 (after A and B merge)
• Lane D: S6 (after C)

Execution: launch A and B in parallel worktrees; merge both; then C; then D. Conflict flag: A and B both add tests under test/auth/ in different files; keep file names distinct to avoid merge noise.

TODOS.md updates

TODOS.md does not exist; create it after plan mode exits with this entry (approved D10):

• What: Delete legacyAuthFlow(), the auth.brokerPath flag, and the shadow-compare harness.
• Why: Two auth paths double surface area and drift risk; the flag is scaffolding with a planned exit.
• Pros: One path to reason about; smaller codebase.
• Cons: Must not happen before the mismatch window closes.
• Context: Issue 2 keeps legacy behind a per-tenant flag with shadow logging. Exit criteria: all tenants on broker, zero auth.shadow.mismatch events over the agreed window. Start in the composition root (remove flag routing), delete legacy and shadow harness, keep legacy-parity.regression.test.ts pointed at recorded decisions.
• Depends on / blocked by: this PR merged; shadow window elapsed.

Post-plan-mode actions (approved, not plan-file edits)

• D1: append gstack skill routing rules to CLAUDE.md and commit (chore: add gstack skill routing rules to CLAUDE.md).
• D10: create TODOS.md with the entry above.

Implementation Tasks

Synthesized from this review's findings. Each task derives from a specific finding above. Run with Claude Code or Codex; checkbox as you ship.

• T1 (P1, human: ~1 day / CC: ~20 min) — AuthCache — Inject cache by constructor; single writer; add putIfVersion with rejection on version change or invalidation; interleaving unit test
  • Surfaced by: Architecture — Issue 1, PLAN.md:19-20, :10
  • Files: src/auth/auth-cache.ts, composition root, test/auth/auth-cache.test.ts
  • Verify: interleaving test rejects write after invalidate; no module-level export const authCache
• T2 (P1, human: ~3 days / CC: ~30 min) — Cutover — Per-tenant auth.brokerPath flag (legacy/shadow/broker); shadow mismatch event; legacy retained
  • Surfaced by: Architecture — Issue 2, PLAN.md:27-28
  • Files: src/auth/cutover.ts, src/auth/legacy-auth-flow.ts, test/auth/cutover.test.ts
  • Verify: each flag value routes correctly; mismatch event fires on divergent case only
• T3 (P1, human: ~1 day / CC: ~20 min) — AuthBroker — Split validateAndDispatch into parse/validate/resolveTenant/dispatch returning a discriminated union; single boundary with tenant+request-id logging; Unexpected denies
  • Surfaced by: Code Quality — Issue 3, PLAN.md:23-24
  • Files: src/auth/auth-broker.ts, src/auth/request-policy.ts, test/auth/auth-broker.test.ts, test/auth/request-policy.test.ts
  • Verify: no nested try/catch; every variant has a boundary test; grep shows no empty catch
• T4 (P1, human: ~1 day / CC: ~15 min) — Regression — CRITICAL: recorded-corpus parity test legacy vs broker
  • Surfaced by: Tests — regression rule, PLAN.md:14-16, :27-28
  • Files: test/auth/legacy-parity.regression.test.ts, test/fixtures/auth-corpus.json
  • Verify: 100% decision + reason-code parity across the corpus
• T5 (P1, human: ~2 days / CC: ~30 min) — E2E — Two-tenant suite with fake IDP: login/logout/revoke/suspend/expired, cross-tenant isolation, IDP timeout/500/malformed JWKS, suspend-during-mint
  • Surfaced by: Tests — Issue 4, PLAN.md:14-16
  • Files: test/e2e/multi-tenant-auth.e2e.test.ts, test/support/fake-idp.ts
  • Verify: suite green; isolation assertions present for every flow
• T6 (P2, human: ~1 day / CC: ~15 min) — AuthBroker — Promise.all over IDP calls, AbortSignal.timeout per call, per-issuer discovery/JWKS cache with TTL and unknown-kid refresh
  • Surfaced by: Performance — Issue 5, PLAN.md:31-32
  • Files: src/auth/auth-broker.ts, src/auth/idp-metadata-cache.ts, test/auth/auth-broker.test.ts
  • Verify: fail-fast, timeout, cache hit/miss/expiry, unknown-kid tests pass; steady-state call count is 1
• T7 (P2, human: ~2h / CC: ~5 min) — Scope — Remove TokenStore from the design; implement RequestPolicy as pure functions
  • Surfaced by: Step 0 — D4, PLAN.md:35-36
  • Files: src/auth/request-policy.ts (no TokenStore file)
  • Verify: no TokenStore symbol; request-policy has no class or module state
• T8 (P3, follow-up) — Cleanup — Delete legacy path, flag, and shadow harness after the mismatch window
  • Surfaced by: TODOS — D10
  • Files: src/auth/cutover.ts, src/auth/legacy-auth-flow.ts, composition root
  • Verify: parity regression test still green against recorded decisions

Suppressed findings (appendix)

• [P3] (confidence 4/10) PLAN.md:35 — TokenStore may have an undisclosed job (e.g. refresh-token persistence). Unverifiable without source; if so, reopen D4 for that one responsibility.
• [P3] (confidence 4/10) PLAN.md:36 — RequestPolicy may need per-tenant state from day one. Unverifiable; promotion path documented in D4.

Completion summary

• Step 0: Scope Challenge — scope reduced per recommendation (TokenStore cut, RequestPolicy as functions)
• Architecture Review: 2 issues found (both resolved: A)
• Code Quality Review: 1 issue found (resolved: A)
• Test Review: diagram produced, 33 gaps identified (1 CRITICAL regression, 10 E2E); all added to plan
• Performance Review: 1 issue found (resolved: A)
• NOT in scope: written
• What already exists: written
• TODOS.md updates: 1 item proposed to user (accepted)
• Failure modes: 2 critical gaps flagged in source plan, 0 open after remedies
• Outside voice: skipped (codex_reviews disabled)
• Parallelization: 4 lanes, 2 parallel / 2 sequential
• Lake Score: 5/5 recommendations chose complete option
• Unresolved decisions: 0

GSTACK REVIEW REPORT

Review	Trigger	Why	Runs	Status	Findings
CEO Review	/plan-ceo-review	Scope & strategy	0	—	—
Outside Review	codex via /plan-eng-review (host: claude, phase: plan-review)	Independent 2nd opinion	1	disabled	skipped by config (codex_reviews=disabled)
Eng Review	/plan-eng-review	Architecture & tests (required)	1	clean (SCOPE_REDUCED)	5 issues, 0 critical gaps open, 33 test gaps added to plan
Design Review	/plan-design-review	UI/UX gaps	0	—	—
DX Review	/plan-devex-review	Developer experience gaps	0	—	—

• OUTSIDE COVERAGE: provider codex, phase plan-review, status disabled (user opt-out via codex_reviews=disabled), no findings; no native fallback dispatched because disabled is terminal. Missing outside coverage is recorded, not counted as clean.
• VERDICT: ENG CLEARED — ready to implement. Re-enable outside voice with gstack-config set codex_reviews enabled if a second model's read is wanted before build.

NO UNRESOLVED DECISIONS