gstack/test/fixtures/eng-required-parity-au.md

# Plan: Multi-tenant Auth Refactor (reviewed)

Reviewed by /plan-eng-review on 2026-09-10, branch `main`, commit `3522c32`.
Source plan: `PLAN.md`. Scope was reduced in Step 0 (decision D2) and every
finding below was accepted individually (D3 through D10). Lake Score: 7/7
decisions chose the complete option.

## Context

The service is moving from a single `legacyAuthFlow()` to a multi-tenant
auth path. The original sketch introduced five new types (AuthBroker,
TokenStore, SessionMint, AuthCache, RequestPolicy) across 12 files, shared
one mutable cache through a module-level export, rewrote the live login path
in place with no regression test, and left token validation as five
sequential IDP calls. This review keeps the goal (tenant-isolated brokering
and session minting over the existing cache adapter) and cuts the shape down
to what that goal needs, then hardens the two places where tenant isolation
can actually break: concurrent cache writes and swallowed errors.

## Step 0: Scope (decision D2, accepted)

**Accepted scope:** three new types and about 7 files.

| Original | Reviewed |
|---|---|
| TokenStore + AuthCache, both wrapping the existing adapter | One `AuthCache` facade. TokenStore is folded in. |
| RequestPolicy class | `requestPolicy(ctx)` pure function returning a policy value. Promote to a class only if per-tenant mutable state appears. |
| AuthBroker, SessionMint services | Kept. |
| 12 files | ~7: composition root, AuthCache, AuthBroker, SessionMint, requestPolicy, validate/dispatch module, flag routing in the entry point, plus tests. |

Plan text inconsistency fixed: the original listed "two new services" and
"four new classes" without AuthBroker in the class list. The real count was
five new types; it is now three.

Search check [Layer 1]: module-level mutable singletons are the documented
Node anti-pattern; composition-root injection is the boring fix. Strangler
fig with a per-tenant flag is the standard way to replace a live auth path.
`Promise.all` is correct when every call must succeed; `allSettled` only
when partial results are useful (they are not here).

TODOS.md does not exist in the repo. Distribution check: no new artifact
type, not applicable.

## Existing contracts retained

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` 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.

New in this plan: `AuthCache` is the only writer (see Architecture 2). The
adapter's validity and tenant-key rules are unchanged; the guard is layered
on top, not inside the adapter.

## Architecture

### Component wiring (decision 1A: constructor injection)

```
composition root (one per process)
  │
  ├── adapter = existingCacheAdapter()          (unchanged)
  ├── cache   = new AuthCache(adapter, tenantStatus)
  ├── idp     = new IdpClient({ timeoutMs })
  ├── broker  = new AuthBroker(cache, idp)
  └── mint    = new SessionMint(cache)
        │
        ▼
  entry point (route handler)
        │  flag.isEnabled(tenantId)?
        ├── false ──▶ legacyAuthFlow(req)                 (retained until parity)
        └── true  ──▶ validate(req, broker) ──▶ dispatch(result, mint)
```

No module-level `AuthCache` export. Tests build a fresh `AuthCache` per case.

### Write path (decision 2A: AuthCache owns all writes, guarded)

```
AuthBroker ──put(key, entry)──┐
                              ▼
                     AuthCache.put()
                       │ 1. tenantStatus.isActive(tenantId)?   no ──▶ drop + AuthError.TenantSuspended
                       │ 2. entry.policyVersion == current?    no ──▶ drop + AuthError.StalePolicy
                       │ 3. adapter.set(key, entry)
                       ▼
SessionMint ──put(key, session)┘

Interleaving that must be safe:
  t0  mint starts for tenant T
  t1  suspension hook wipes T's entries
  t2  mint calls cache.put()  ──▶ step 1 fails ──▶ nothing written
```

The adapter's invalidation hooks still fire on logout, revocation, and
suspension. The guard closes the window between a hook firing and a late
write landing. Locks and queues were considered and rejected as
over-engineering for an in-process cache.

### Rollout (decision 3A: strangler fig, per-tenant flag)

1. Ship both paths. Flag default off for every tenant.
2. Enable for one internal tenant. Watch parity suite and error rates.
3. Widen by tenant cohort. Flip back per tenant on any divergence.
4. At 100% with parity green for one release, execute TODO 1 (delete
   `legacyAuthFlow()`, the flag branch, and the parity suite's legacy leg).

### Production failure scenarios per new codepath

| Codepath | Realistic failure | Plan accounts for it |
|---|---|---|
| Composition root | Two roots constructed (e.g. test and app) → two caches | Root is the only constructor call site; tests use the root's factory. |
| AuthCache.put() guard | tenantStatus lookup slow or down | Guard fails closed (deny write) and raises `AuthError.TenantStatusUnavailable`; test covers it. |
| Flag routing | Flag store unreachable | Default to legacy path; log; test covers it. |
| validate() parallel IDP | One call hangs | AbortSignal timeout, siblings aborted (6A). |

## Code quality (decision 4A)

`validateAndDispatch()` (60 lines, three nested try/catch blocks, each
swallowing a different error class) is split:

```
validate(req, broker): Promise<Validated>
  │  throws typed errors, never swallows
  ├── AuthError.IdpUnreachable   (network / timeout)
  ├── AuthError.BadSignature     (JWKS mismatch)
  ├── AuthError.UnknownTenant
  ├── AuthError.TenantSuspended
  └── AuthError.Expired

dispatch(validated, mint): Promise<Response>
  └── mint.mint(validated) ──▶ cache.put()

boundary (route handler)
  try { dispatch(await validate(req, broker), mint) }
  catch (e) { log(e); return mapAuthError(e) }   // one catch, one map table
```

`mapAuthError` is a single table from error class to response code and
user-facing message. Every class is a distinct, testable outcome. Callers
that depended on silent failure must be audited when the split lands.

DRY: TokenStore was a second wrapper over the same adapter as AuthCache; it
is gone (D2). Inline ASCII diagram comments go in `AuthCache` (write-path
guard), the composition root (wiring), and the validate/dispatch module
(error map).

## Tests

Test framework: none detected in this fixture repo (no `package.json`, zero
test files). File names below assume TypeScript with `*.test.ts`; adjust to
the real project's convention.

### Coverage diagram

```
CODE PATHS                                                   USER FLOWS
[+] composition-root.ts                                      [+] Login (flag off)
  └── build()                                                  └── [GAP][CRITICAL][→E2E] identical to pre-refactor — legacyAuthFlow.regression.test.ts
      └── [GAP] single AuthCache instance, no module export   [+] Login (flag on)
[+] auth-cache.ts                                              ├── [GAP][→E2E] happy path end to end
  └── put()                                                    ├── [GAP]       double-submit → one session
      ├── [GAP] active tenant, current policy → written        ├── [GAP]       token expires between validate and dispatch
      ├── [GAP] suspended tenant → dropped, TenantSuspended    └── [GAP]       flag store unreachable → legacy path
      ├── [GAP] stale policy version → dropped, StalePolicy  [+] Parity (flag on vs off)
      ├── [GAP] tenantStatus unavailable → fail closed          └── [GAP][→E2E] table: happy + each AuthError + suspended/revoked/expired
      └── [GAP] suspend-during-mint interleaving             [+] Tenant admin
[+] validate.ts                                                ├── [GAP][→E2E] suspend tenant → in-flight mint refused
  └── validate()                                               └── [GAP][→E2E] revoke token → next request rejected
      ├── [GAP] all 5 IDP calls succeed                      [+] Error states
      ├── [GAP] one call rejects → siblings aborted            ├── [GAP] each AuthError → specific code + message + log line
      ├── [GAP] one call times out → IdpUnreachable            └── [GAP] IDP slow → bounded failure, retryable message
      └── [GAP] each AuthError subclass raised
[+] dispatch.ts / boundary
  └── mapAuthError()
      └── [GAP] every AuthError class → distinct response
[+] request-policy.ts
  └── requestPolicy()  [GAP] pure: same ctx → same policy, unknown tenant → default-deny
[+] legacyAuthFlow (retained, existing tests) [★★ TESTED by existing adapter tests only]

COVERAGE: 0/22 new paths tested (0%)  |  Code paths: 0/13  |  User flows: 0/9
QUALITY: existing adapter tests ★★  |  GAPS: 22 (7 E2E, 1 CRITICAL regression, 0 eval)
```

Legend: ★★★ behavior + edge + error | ★★ happy path | ★ smoke | [→E2E] needs integration test

### Required tests (all written alongside the feature code)

**CRITICAL (regression rule, mandatory, no decision needed):**
`legacyAuthFlow.regression.test.ts`. Pin current behavior of
`legacyAuthFlow()` before any change: valid token → session shape and
lifetime; expired, bad signature, unknown tenant → the exact current
response codes and bodies; logout and revocation clear the cache. This is
the oracle for the parity suite and the guard for the flag-off path. What
breaks without it: the rewrite changes existing behavior for every tenant
still on the legacy path with nothing to catch it.

**Decision 5A, parity suite:** `auth-parity.test.ts`. One fixture table,
each row run through both paths (flag off, flag on), assert identical
outcome. Rows: valid token; each `AuthError` class; tenant suspended,
revoked, expired; policy version bumped. Exit criterion for TODO 1.

**Decision 1A:** `composition-root.test.ts`. Two services from one root
share one `AuthCache`; two roots do not. Grep test: no module exports an
`AuthCache` instance.

**Decision 2A:** `auth-cache.test.ts`. Five `put()` branches above,
including the suspend-during-mint interleaving (start mint, fire suspension
hook, complete mint, assert no entry) and tenantStatus unavailable → deny.

**Decision 4A:** `validate.test.ts`, `dispatch.test.ts`. One test per
`AuthError` subclass raised by `validate()`; one per row of `mapAuthError`;
assert a log line is emitted for each; assert nothing is swallowed (a
non-AuthError propagates).

**Decision 6A:** in `validate.test.ts` with a fake IDP: all succeed;
one rejects → others receive abort; one hangs past timeout →
`IdpUnreachable` within the bound; latency of the happy path ≈ max, not
sum (assert call overlap via fake timestamps).

**User flows [→E2E]:** `auth.e2e.test.ts`: login flag on → authenticated
request → logout; double-submit; suspend tenant during login; revoke then
retry; flag store unreachable → legacy path.

QA test plan artifact written to
`~/.gstack/projects/gstack-plan-count-tTVLFw/vercel-sandbox-main-eng-review-test-plan-20260910-210603.md`.

## Performance (decision 6A)

Token validation's five independent IDP calls run in parallel:

```
validate()
  signal = AbortSignal.timeout(timeoutMs)  (one per call, plus a shared controller)
  Promise.all([discovery, jwks, introspect, userinfo, tenantLookup].map(c => c(signal)))
    │ first rejection ──▶ controller.abort() ──▶ siblings cancelled
    │ timeout          ──▶ AuthError.IdpUnreachable
    ▼
  latency: max(call) instead of sum(call)
```

`Promise.all` is the right semantics: validation is all-or-nothing.
Per-issuer caching of discovery and JWKS is TODO 2, deliberately out of this
PR. If the IDP client does not accept an `AbortSignal`, wrap it rather than
skipping the timeout.

## NOT in scope

- **TokenStore as a separate class.** Second wrapper over the same adapter; folded into AuthCache (D2). Revisit only if a non-cache backing store is actually needed.
- **RequestPolicy as a class.** No described state; a pure function. Promote when per-tenant mutable policy state appears.
- **Deleting `legacyAuthFlow()` and the flag.** Follow-up TODO 1, gated on parity at 100%.
- **Per-issuer discovery/JWKS caching.** Follow-up TODO 2; independent of tenant correctness and would blur the parity comparison.
- **Locks or a write queue for AuthCache.** Write-time guard chosen instead (2A); a queue is over-engineering for an in-process cache.
- **Changes to the existing cache adapter or its invalidation hooks.** Retained unchanged by the plan's own contract.
- **Distribution / packaging.** No new artifact type.

## What already exists

- **Existing cache adapter** (tenant/issuer/audience/policy-version keys, expiry eviction, invalidation on logout/revocation/suspension, with tests): reused unchanged behind `AuthCache`. The original plan rebuilt a second wrapper (TokenStore) over it; removed.
- **`legacyAuthFlow()`**: retained as the flag-off path and as the parity oracle instead of being rewritten in place.
- **Existing adapter tests**: still run; they do not cover the new writer or the guard, hence the new `auth-cache.test.ts`.
- **IDP client**: reused; gains an `AbortSignal` parameter or a thin wrapper.

## TODOS (approved D9, D10; create TODOS.md at implementation time)

### TODO 1: Remove the per-tenant auth flag and delete legacyAuthFlow()
- **What:** Delete `legacyAuthFlow()`, the flag branch in the entry point, and the parity suite's legacy leg.
- **Why:** Two live auth paths are a maintenance and audit burden once the new path is proven.
- **Pros:** Single code path, smaller test matrix. **Cons:** Needs a real signal before it is safe.
- **Context:** Start at the composition root / entry-point flag routing. The parity suite from decision 5A is the gate.
- **Depends on:** Flag at 100% for all tenants; parity suite green for one full release.

### TODO 2: Cache per-issuer IDP discovery and JWKS
- **What:** Issuer-keyed cache for the discovery document and JWKS with TTL and refresh on unknown `kid`.
- **Why:** Two of five IDP calls per login are static per issuer; cuts latency and IDP quota.
- **Pros:** Lower p50, fewer rate-limit hits. **Cons:** A second cache with its own staleness rules; key rotation must trigger refresh.
- **Context:** Lives in the IDP client, not AuthCache. Key on issuer URL.
- **Depends on:** Decision 6A (parallel calls) landing first as the baseline.

## Failure modes

| New codepath | Realistic failure | Test | Handling | User sees |
|---|---|---|---|---|
| AuthCache.put() | Suspension hook races a mint | auth-cache interleaving | guard drops write | clear "account suspended" |
| AuthCache.put() | tenantStatus lookup down | auth-cache unavailable case | fail closed, TenantStatusUnavailable | clear retryable error |
| validate() | One IDP call hangs | validate timeout case | AbortSignal timeout | bounded, retryable error |
| validate() | One IDP call rejects, siblings leak | validate abort case | controller.abort() | specific error |
| dispatch/boundary | Unknown error class | validate non-AuthError case | propagates, logged | 500 with log line (not silent) |
| Flag routing | Flag store unreachable | e2e flag-unreachable | default to legacy | unchanged legacy behavior |
| Composition root | Second root built | composition-root test | test fails | n/a |
| legacyAuthFlow (flag off) | Behavior drift from rewrite | CRITICAL regression test | n/a (no code change on this path) | identical to today |

Critical gaps (no test, no handling, silent): **0** after the accepted
remedies. Before review there were two: the suspend-during-mint race and
the swallowed error classes.

## Worktree parallelization strategy

| Step | Modules touched | Depends on |
|---|---|---|
| S1 Composition root + AuthCache (fold TokenStore, guarded put) | auth/cache, app bootstrap | — |
| S2 validate()/dispatch() split, AuthError classes, parallel IDP calls | auth/validate, auth/idp-client | — |
| S3 AuthBroker + SessionMint + requestPolicy | auth/services | S1 (cache API), S2 (AuthError types) |
| S4 Flag routing + regression test + parity suite + e2e | entry point, test/ | S1, S2, S3 |

Lane A: S1 (independent). Lane B: S2 (independent). Lane C: S3 → S4
(sequential, waits on A and B).

Execution order: launch A and B in parallel worktrees. Merge both. Then C.

Conflict flags: A and B both touch the shared `AuthError` type if it is
placed under auth/cache; put `AuthError` in its own module in S2 and have S1
import it, or agree the file name up front.

## 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: ~3h / CC: ~10min)** — composition root — Build one AuthCache in a composition root and constructor-inject it into AuthBroker and SessionMint; delete the module-level export
  - Surfaced by: Architecture issue 1 (D3) — PLAN.md:19-20 "global mutable AuthCache instance via module-level export"
  - Files: composition-root.ts, auth-broker.ts, session-mint.ts, composition-root.test.ts
  - Verify: composition-root.test.ts; grep confirms no exported AuthCache instance
- [ ] **T2 (P1, human: ~4h / CC: ~15min)** — AuthCache — Route all writes through AuthCache.put() with tenant-status and policy-version guard; fail closed on status lookup failure
  - Surfaced by: Architecture issue 2 (D4) — PLAN.md:10 "they do not serialize mutations"
  - Files: auth-cache.ts, auth-cache.test.ts
  - Verify: auth-cache.test.ts including suspend-during-mint interleaving
- [ ] **T3 (P1, human: ~1 day / CC: ~20min)** — entry point — Per-tenant feature flag routes to the new path; legacyAuthFlow() retained; flag store failure defaults to legacy
  - Surfaced by: Architecture issue 3 (D5) — PLAN.md:27-28 "rewritten as part of this work"
  - Files: entry-point route handler, flags config, auth.e2e.test.ts
  - Verify: e2e flag on/off and flag-unreachable cases
- [ ] **T4 (P1, human: ~3h / CC: ~10min)** — legacyAuthFlow — CRITICAL regression test pinning current legacyAuthFlow() behavior before any change
  - Surfaced by: Test review, mandatory regression rule — PLAN.md:27-28 "no regression test for the prior behavior is planned"
  - Files: legacyAuthFlow.regression.test.ts
  - Verify: test passes against unmodified legacyAuthFlow() first
- [ ] **T5 (P1, human: ~4h / CC: ~15min)** — validate/dispatch — Split validateAndDispatch() into validate() + dispatch(), typed AuthError subclasses, one boundary catch with a mapAuthError table and a log line per class; audit callers that relied on silent failure
  - Surfaced by: Code quality issue 4 (D6) — PLAN.md:23-24 "each catch swallows a different error class"
  - Files: validate.ts, dispatch.ts, auth-error.ts, validate.test.ts, dispatch.test.ts
  - Verify: one test per AuthError class; non-AuthError propagates
- [ ] **T6 (P2, human: ~1 day / CC: ~20min)** — tests — Table-driven parity suite running each fixture row through flag-off and flag-on paths
  - Surfaced by: Test issue 5 (D7) — PLAN.md:14-16 "does not exercise legacyAuthFlow() or assert compatibility"
  - Files: auth-parity.test.ts
  - Verify: suite green for every row; becomes the exit criterion for TODO 1
- [ ] **T7 (P2, human: ~3h / CC: ~10min)** — IDP client — Parallelize the five IDP calls with Promise.all, per-call AbortSignal timeout, abort siblings on first failure, map to AuthError.IdpUnreachable
  - Surfaced by: Performance issue 6 (D8) — PLAN.md:31-32 "5 sequential API calls to the IDP"
  - Files: validate.ts, idp-client.ts, validate.test.ts
  - Verify: fake-IDP tests for hang, single rejection, and call overlap
- [ ] **T8 (P2, human: ~2h / CC: ~10min)** — scope — Fold TokenStore into AuthCache; implement requestPolicy() as a pure function with default-deny for unknown tenant
  - Surfaced by: Step 0 scope challenge (D2) — PLAN.md:35-36 "4 new classes (TokenStore, SessionMint, AuthCache, RequestPolicy)"
  - Files: auth-cache.ts, request-policy.ts, request-policy.test.ts
  - Verify: no TokenStore symbol remains; requestPolicy tests
- [ ] **T9 (P3, human: ~2h / CC: ~10min)** — cleanup — TODO 1: remove flag and delete legacyAuthFlow() after parity at 100%
  - Surfaced by: TODO 1 (D9)
  - Files: entry point, legacyAuthFlow module, auth-parity.test.ts
  - Verify: parity suite green for one release before starting
- [ ] **T10 (P3, human: ~4h / CC: ~15min)** — IDP client — TODO 2: per-issuer discovery + JWKS cache with kid-miss refresh
  - Surfaced by: TODO 2 (D10)
  - Files: idp-client.ts, idp-client.test.ts
  - Verify: second login for same issuer makes 3 network calls, not 5

## Completion summary

- Step 0: Scope Challenge — scope reduced per recommendation (5 types / 12 files → 3 types / ~7 files)
- Architecture Review: 3 issues found (all resolved: 1A, 2A, 3A)
- Code Quality Review: 1 issue found (resolved: 4A)
- Test Review: diagram produced, 22 gaps identified; 1 CRITICAL regression test added under the mandatory rule; parity suite accepted (5A)
- Performance Review: 1 issue found (resolved: 6A)
- NOT in scope: written
- What already exists: written
- TODOS.md updates: 2 items proposed to user, 2 accepted (file to be created at implementation)
- Failure modes: 0 critical gaps flagged after remedies (2 before)
- Outside voice: skipped (codex_reviews disabled; recorded as outside_status disabled)
- Parallelization: 3 lanes, 2 parallel / 1 sequential
- Lake Score: 7/7 recommendations chose complete option

## Suppressed findings (confidence below 7, appendix only)

- (4/10) TokenStore may have been intended for a future non-cache backing store; the plan does not say so, so it was folded. Re-split if that requirement surfaces.
- (4/10) RequestPolicy may need per-tenant mutable state; nothing in the plan describes it. Pure function until proven otherwise.
- (3/10) The five IDP calls may not all be independent (e.g. introspection may need the discovery document's endpoint). If so, two-stage: discovery + JWKS first, then the remaining three in parallel. Verify against the IDP client before T7.
- (4/10) Callers of validateAndDispatch() may depend on the silent-failure behavior; no call sites were available in this fixture to check. Audit is folded into T5.

## GSTACK REVIEW REPORT

| Review | Trigger | Why | Runs | Status | Findings |
|--------|---------|-----|------|--------|----------|
| CEO Review | `/plan-ceo-review` | Scope & strategy | 0 | — | — |
| Outside Review | codex via `/plan-eng-review` (plan-review phase) | Independent 2nd opinion | 1 | disabled | outside_status: disabled (codex_reviews=disabled); no outside coverage |
| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 1 | clean (PLAN) | 7 issues, 0 critical gaps, 7/7 resolved, scope reduced |
| 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, disabled by config (`codex_reviews=disabled`); no findings. Native review only. Re-enable with `gstack-config set codex_reviews enabled`.

**VERDICT:** ENG CLEARED — ready to implement (scope reduced, 7 findings resolved, regression test mandatory). No outside coverage.

NO UNRESOLVED DECISIONS