gstack/test/fixtures/eng-retry-baseline-as.md

# Plan: Multi-tenant Auth Refactor (reviewed)

Reviewed by `/plan-eng-review` on 2026-09-10 against PLAN.md at commit 0d7f121.
Nine decisions (D1-D9) were made interactively; each is recorded inline where it
changes the plan. Original plan text is kept where it still stands and marked
**(revised)** where a decision changed it.

## Context

The auth path is being refactored for multi-tenancy. The original plan introduced
two services (`AuthBroker`, `SessionMint`) sharing a mutable module-level
`AuthCache`, added `TokenStore` and `RequestPolicy` classes, rewrote
`legacyAuthFlow()` in place, and parallelized five IDP calls. Review found the
shape was over-built relative to the existing cache adapter, had a fail-open race
between two cache writers, swallowed errors in the dispatcher, had no rollout or
rollback path, and no regression protection for the flow being rewritten.

The outcome after review: a thinner auth path (two new services, no new cache
layer), a single cache writer with versioned writes so revocation always wins,
a fail-closed dispatcher with typed errors, a per-tenant strangler-fig rollout
with a loud fallback, full unit + integration coverage, and faster logins with
fewer IDP calls.

## 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. Those validity and tenant-key rules are
unchanged. The adapter, its invalidation hooks, and their existing tests remain
in use.

**(revised, D2)** One narrow addition to the adapter: the write path accepts a
policy-version tag and rejects a write whose tag is older than the current
version for that tenant. Existing adapter tests stay green; new tests cover the
rejection.

**(revised, D1)** `AuthCache` and `TokenStore` are not built. The adapter is the
single cache layer and is passed to services by constructor injection.

## Architecture (revised, D1 + D2)

New types: `AuthBroker` (read path) and `SessionMint` (sole write path).
`RequestPolicy` is a pure function over request + tenant config, not a class.
No module-level exports of mutable state; both services receive the adapter,
the IDP client, and the flag reader in their constructors.

```
                     per-tenant flag (D3)
                            │
 request ──> router ────────┼──────────────> legacyAuthFlow()   (unflagged tenants,
                            │                                    flag-store-down fallback D9)
                            └──────────────> validateAndDispatch()   (flattened, D4)
                                                 │
                        ┌────────────────────────┴──────────────────────┐
                        ▼                                               ▼
                   AuthBroker  (READ only)                     SessionMint  (SOLE WRITER)
                        │  get(cacheKeyFor(...))                        │  put(key, entry, policyVersion)
                        ▼                                               ▼
                 ┌──────────────────── existing cache adapter ──────────────────────┐
                 │ keys: tenant|issuer|audience|policyVersion                        │
                 │ rejects put() whose policyVersion < current (D2)                 │
                 │ invalidates on logout / revocation / suspension (unchanged)      │
                 └───────────────────────────────────────────────────────────────────┘
                                                 ▲
                        SessionMint ──> IDP client (static cache + parallel + timeout, D7)
```

### Cache write ownership (D2)

Only `SessionMint` writes. `AuthBroker` reads and, on revocation or suspension
signals, calls the adapter's existing invalidation hooks. Every write carries
the policy version read at validation start via `readPolicyVersion()`. The
adapter rejects a write whose version is stale and the rejection is logged at
warn with tenant ID and key. This makes "revoke racing a mint resurrects the
token" structurally impossible rather than unlikely.

```
 time ──────────────────────────────────────────────────────────────>
 SessionMint:  read pv=7 ─── validate (IDP) ─────────── put(key, e, pv=7) ✗ rejected, logged
 Admin:                      revoke ──> invalidate(key), pv := 8
 AuthBroker:                                             get(key) → miss → deny ✓
```

### Rollout (D3 + D9)

`legacyAuthFlow()` stays callable. A per-tenant flag routes each tenant to the
new flow or legacy. Rollout is tenant by tenant, starting with an internal
tenant. Rollback is a flag flip.

Flag-store failure: lookup has a short timeout; on timeout or error the router
falls back to `legacyAuthFlow()`, emits a warn-level structured log and a
`auth.flag_fallback` metric, and an integration test stubs the flag store as
down. Legacy removal is tracked in TODOS.md (D8) with the exit condition
"all tenants flagged on, no flips for one full release cycle".

### Security architecture

* Tenant isolation boundary is the cache key. One builder (D5) makes the tenant
  field structurally required.
* Every error class in the dispatcher denies (D4). Unknown errors deny.
* Revocation always wins the race (D2).
* Flag-store outage degrades to the known-good legacy path, never to an
  unvalidated pass (D9).

### Production failure scenarios per new codepath

| Codepath | Realistic failure | Test | Handling | User sees |
|---|---|---|---|---|
| `validateAndDispatch()` boundary | Unexpected exception mid-validation | yes (T4/T6) | deny + structured log (D4) | clear 401/403 |
| `AuthBroker` read | Adapter throws / cache backend down | yes (T6) | deny, log, metric | clear 503 |
| `SessionMint` write | Stale policy version after revoke | yes (T2/T6) | write rejected + warn log (D2) | denied on next request |
| `SessionMint` write | Adapter write throws | yes (T6) | deny, no partial state | clear 503 |
| IDP client | One of N calls rejects | yes (T7) | Promise.all rejects, all outcomes logged, deny | clear 503 |
| IDP client | Call hangs | yes (T7) | per-call timeout → deny | clear 503, retry safe |
| IDP static cache | JWKS key rotation, unknown `kid` | yes (T7) | one forced refetch, then deny | brief retry, then works |
| Flag router | Flag store unreachable | yes (T3/T6) | legacy fallback + metric (D9) | nothing, legacy behavior |
| `cacheKeyFor()` | Two tenants collide | yes (T5) | impossible by construction | n/a |

**Critical gaps (no test, no handling, silent): 0.**

## Code quality (revised, D4 + D5)

`validateAndDispatch()` was 60 lines with three nested try/catch blocks, each
swallowing a different error class. It is rewritten as a linear pipeline of
small steps:

```
 parseToken ──> resolveTenant ──> checkPolicy ──> validateWithIdp ──> mintSession ──> dispatch
     │               │                │                 │                  │
  TokenError     TenantError      PolicyError        IdpError          CacheError
     └───────────────┴────────────────┴─────────────────┴──────────────────┘
                                      │
                        single boundary catch:
                        map error → explicit DENY result
                        structured log {tenant, step, errorClass}
                        metric auth.deny{reason}
                        unknown Error → DENY (fail closed)
```

Typed errors: `TokenError`, `TenantError`, `PolicyError`, `IdpError`,
`CacheError`, all extending `AuthError`. Callers that relied on a swallowed
error to continue now receive an explicit deny and are updated in this PR.

DRY: `cacheKeyFor(tenantId, issuer, audience, policyVersion)` and
`readPolicyVersion(tenantId)` live in one shared module used by `AuthBroker`
and `SessionMint`. One test asserts every field is present and ordered and that
two different tenants never produce the same key.

Inline ASCII diagram comments to add at implementation:
* adapter write path: the versioned-write timeline above
* `validateAndDispatch()`: the pipeline diagram above
* router: flag decision tree including the fallback branch
* `SessionMint`: mint pipeline and the sole-writer contract

## Tests (revised, D6 + REGRESSION RULE)

**CRITICAL regression suite (mandatory, IRON RULE).** `legacyAuthFlow()` is
existing behavior being rewritten and the original plan had no regression
coverage. Before any rewrite, write a characterization suite that pins current
behavior: valid token → allow, expired → deny, wrong issuer/audience → deny,
suspended tenant → deny, logout invalidates. The suite runs against both the
legacy path and the new flow (via the flag) for the whole rollout window.

Coverage target: every branch in the diagram below, unit and integration.

```
CODE PATHS                                                   USER FLOWS
[~] legacyAuthFlow()  (flag-routed, kept)                    [+] Login / token validation
  ├── [CRITICAL] regression: valid token → allow               ├── [→E2E] Login on flagged tenant → new flow
  ├── [CRITICAL] regression: expired → deny                    ├── [→E2E] Login on unflagged tenant → legacy
  ├── [CRITICAL] regression: wrong audience/issuer → deny      ├── [→E2E] Flag flipped mid-session → no lockout
  └── [CRITICAL] regression: suspended tenant → deny           └── [→E2E] Flag store down → legacy + metric (D9)
[+] validateAndDispatch()  (flattened, D4)                   [+] Revocation / logout
  ├── happy path → dispatch                                    ├── [→E2E] Revoke → next request denied
  ├── TokenError → deny + log                                  ├── [→E2E] Revoke racing mint → stale write rejected (D2)
  ├── TenantError → deny + log                                 └──        Tenant suspended → all tokens denied
  ├── PolicyError → deny + log
  ├── IdpError → deny + log                                  [+] Tenant isolation
  ├── CacheError → deny + log                                  ├── [→E2E] Tenant A token never validates for B
  └── unknown Error → deny + log (fail closed)                 └──        Same issuer/audience, different tenant → miss
[+] AuthBroker (read path)
  ├── cache hit → allow                                      [+] Error states
  ├── cache miss → IDP validate                                ├── IDP timeout → clear 503, not 401
  └── adapter throws → deny                                    ├── IDP 5xx → clear 503, retry safe
[+] SessionMint (sole writer, D2)                              └── Partial IDP failure → deny, all outcomes logged
  ├── mint → write with policy version
  ├── stale version → write rejected + logged
  └── adapter write throws → deny, no partial state
[+] cacheKeyFor() / readPolicyVersion()  (D5)
  ├── all four fields required and ordered
  └── two tenants never collide
[+] IDP client (D7)
  ├── static responses served from cache within TTL
  ├── unknown kid → one forced JWKS refetch
  ├── all parallel calls succeed
  ├── one rejects → aggregate error, no hang
  └── per-call timeout fires

TARGET: 34/34 paths tested (100%)  |  Code paths: 22/22  |  User flows: 12/12
GAPS BEFORE REVIEW: 27 (7 E2E, 4 CRITICAL regression)  |  GAPS AFTER PLAN: 0
```

Test harness: a fake adapter with an injectable write delay (for the D2 race
test) and an IDP stub that can fail, hang, or rotate keys per call. Integration
flows run against those fakes; no live IDP in CI.

## Performance (revised, D7)

Token validation issued 5 sequential IDP calls. Revised:

1. Static IDP responses (OIDC discovery document, JWKS) are cached with a TTL in
   the existing adapter; unknown `kid` triggers one forced refetch.
   Assumption: two of the five calls are these static fetches. If none are,
   step 2 still applies.
2. Remaining calls run with `Promise.all`, each wrapped in a per-call timeout.
   Any rejection denies the request; all outcomes are logged so a partial
   failure is diagnosable.
3. Expected result: login latency drops from 5 round trips to 1, IDP request
   volume drops by up to 40 percent, and a hung IDP call cannot hang a login.

## Scope (revised, D1)

Complexity check triggered on the original plan (12 files, 4 new classes plus
`AuthBroker`). Reduced to: 2 new service types, 1 shared helper module, 1 typed
error module, 1 narrow adapter change, 1 router change, the rewritten
dispatcher, and tests. Roughly 8 source files plus tests.

## What already exists

| Sub-problem | Existing code | Plan now |
|---|---|---|
| Tenant-scoped cache keying, expiry, invalidation | cache adapter + hooks + tests | reused unchanged, one write-path addition (D2) |
| Current auth behavior | `legacyAuthFlow()` | kept as flag fallback and regression oracle (D3) |
| Service-facing cache facade | none needed; adapter API suffices | `AuthCache` dropped (D1) |
| Token storage | adapter already stores tokens | `TokenStore` dropped (D1) |
| Request policy evaluation | none; was a proposed class | pure function (D1) |

## NOT in scope

* **Deleting `legacyAuthFlow()` and the per-tenant flag** — tracked in
  TODOS.md (D8); happens after 100 percent rollout plus one release of bake.
* **Adding an `AuthCache` facade** — only if a future consumer needs a narrower
  API than the adapter; not justified today (D1).
* **Rewriting the cache adapter itself** — one write-path addition only (D2);
  its keying and invalidation rules are unchanged.
* **IDP-side rate-limit negotiation or client-credential changes** — the
  static cache (D7) reduces load; anything beyond that is separate work.
* **Multi-region cache consistency** — out of scope for this refactor; the
  versioned write (D2) is single-backing-cache correct as the plan states.
* **New artifact distribution** — no new binary, package, or image; N/A.

## TODOS.md updates (apply at implementation start; plan mode forbade the edit)

```markdown
# TODOS

## Auth

### Remove legacyAuthFlow() and the per-tenant new-flow flag

**What:** Delete legacyAuthFlow(), the per-tenant new-flow flag, the flag-store
fallback path, and the legacy branch of the regression suite.

**Why:** Two auth code paths double the test and review cost of every future
auth change and keep a fallback alive that no longer has anything to fall back
from.

**Context:** /plan-eng-review D3 (2026-09-10) chose a strangler-fig rollout:
legacyAuthFlow() stays callable behind a per-tenant flag while the new
AuthBroker + SessionMint flow rolls out tenant by tenant. D9 added a
flag-store-down fallback to legacy. The regression suite pins legacy behavior
and runs against both paths during rollout. Start in the router and the
regression suite; the flag reader and fallback metric go with them.

**Effort:** S
**Priority:** P2
**Depends on:** All tenants flagged on to the new flow with no flag flips for
one full release cycle.
```

## Worktree parallelization strategy

| Step | Modules touched | Depends on |
|---|---|---|
| S1 Regression suite for `legacyAuthFlow()` | tests/auth/legacy | — |
| S2 Shared helpers + typed errors | auth/shared | — |
| S3 Adapter versioned-write rejection | cache adapter module | S2 (policy version helper) |
| S4 IDP client: static cache, parallel, timeout | auth/idp | — |
| S5 `AuthBroker` + `SessionMint` | auth/services | S2, S3, S4 |
| S6 Flattened dispatcher + flag router + fallback | auth/dispatch | S2, S5 |
| S7 Integration flows | tests/auth/integration | S5, S6 |

Lanes:
* Lane A: S1 (independent)
* Lane B: S2 → S3 → S5 → S6 (sequential, shared auth/ services and adapter)
* Lane C: S4 (independent)
* Lane D: S7 (after B and C merge)

Execution order: launch A, B, C in parallel worktrees. Merge A first (it is
pure tests and gates the rewrite). Merge C, then B. Then D.

Conflict flags: Lanes B and C both live under auth/; keep S4 confined to
auth/idp and its own test file to avoid merge conflicts with auth/services.

## 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: ~15min)** — tests/auth/legacy — Write the CRITICAL characterization suite for `legacyAuthFlow()` before any rewrite; run it against legacy and new flow
  - Surfaced by: Test review — REGRESSION RULE, PLAN.md:27-28 and :14-16
  - Files: tests/auth/legacy/*, router flag stub
  - Verify: suite green on legacy path before S5/S6 land; green on both paths after
- [ ] **T2 (P1, human: ~1.5 days / CC: ~25min)** — cache adapter + auth/services — Single writer: only `SessionMint` writes; writes carry policy version; adapter rejects stale writes with warn log
  - Surfaced by: Architecture — D2, PLAN.md:10, 19-20
  - Files: cache adapter write path, auth/services/SessionMint, auth/services/AuthBroker
  - Verify: unit test for stale-write rejection; integration test "revoke racing mint → denied"
- [ ] **T3 (P1, human: ~1.25 days / CC: ~23min)** — auth/dispatch — Per-tenant flag router with legacy fallback on flag-store failure, timeout, warn log, `auth.flag_fallback` metric
  - Surfaced by: Architecture — D3 + D9, PLAN.md:27-28
  - Files: auth/dispatch/router, flag reader interface
  - Verify: integration tests for flagged, unflagged, mid-session flip, flag store down
- [ ] **T4 (P1, human: ~1 day / CC: ~20min)** — auth/dispatch — Flatten `validateAndDispatch()` into a linear pipeline with typed errors and one fail-closed boundary
  - Surfaced by: Code quality — D4, PLAN.md:23-24
  - Files: auth/dispatch/validateAndDispatch, auth/shared/errors
  - Verify: one unit test per error class plus unknown-error → deny; no catch without a deny
- [ ] **T5 (P2, human: ~2h / CC: ~5min)** — auth/shared — `cacheKeyFor()` and `readPolicyVersion()` helpers used by both services, with ordering and no-collision tests
  - Surfaced by: Code quality — D5, PLAN.md:7-8
  - Files: auth/shared/cacheKey, tests
  - Verify: unit tests; grep shows no other key construction in auth/
- [ ] **T6 (P1, human: ~2 days / CC: ~30min)** — tests/auth/integration — Integration flows: revocation race, tenant isolation, flag routing, IDP partial failure, flag store down, adapter failures
  - Surfaced by: Test review — D6 coverage diagram, 7 [→E2E] flows
  - Files: tests/auth/integration/*, fake adapter with write delay, IDP stub
  - Verify: all 12 user flows in the diagram green
- [ ] **T7 (P2, human: ~1 day / CC: ~20min)** — auth/idp — TTL-cache static IDP responses, `Promise.all` with per-call timeout for the rest, forced JWKS refetch on unknown kid
  - Surfaced by: Performance — D7, PLAN.md:31-32
  - Files: auth/idp/client, tests
  - Verify: unit tests for cache hit, rotation refetch, one-rejects, timeout; latency measurement 5 RTT → 1 RTT
- [ ] **T8 (P2, human: ~0.5 day / CC: ~10min)** — auth/services — Drop `AuthCache` and `TokenStore`; `RequestPolicy` as pure function; constructor injection for adapter, IDP client, flag reader
  - Surfaced by: Step 0 — D1, PLAN.md:19-20, 35-36
  - Files: auth/services/*, auth/shared/requestPolicy
  - Verify: no module-level mutable exports in auth/ (grep); services unit-testable with fakes
- [ ] **T9 (P3, human: ~5min / CC: ~1min)** — TODOS.md — Add the legacy-removal TODO with its exit condition
  - Surfaced by: TODOS.md updates — D8
  - Files: TODOS.md
  - Verify: entry present in the format above

## Completion summary

- Step 0: Scope Challenge — scope reduced per recommendation (D1: 4-5 new classes → 2 services + helpers)
- Architecture Review: 2 issues found (D2 cache write race, D3 rollout); 1 follow-on failure mode (D9)
- Code Quality Review: 2 issues found (D4 swallowed errors, D5 key builder DRY)
- Test Review: diagram produced, 27 gaps identified (4 CRITICAL regression, 7 E2E); all added to plan (D6)
- Performance Review: 1 issue found (D7)
- NOT in scope: written
- What already exists: written
- TODOS.md updates: 1 item proposed to user (D8, accepted)
- Failure modes: 0 critical gaps flagged
- Outside voice: skipped (codex_reviews disabled)
- Parallelization: 4 lanes, 3 parallel / 1 sequential follow-on
- Lake Score: 7/7 coverage-scored recommendations chose the complete option (D1, D8 were kind-only)

Setup prompts deferred this run (not approvals): CLAUDE.md routing rules (plan
mode forbids the edit and commit), /office-hours design-doc offer (explicit
review request), cross-project learnings toggle (learnings store empty).

## GSTACK REVIEW REPORT

| Review | Trigger | Why | Runs | Status | Findings |
|--------|---------|-----|------|--------|----------|
| CEO Review | `/plan-ceo-review` | Scope & strategy | 0 | — | — |
| Outside Review | codex via `/plan-eng-review` | Independent 2nd opinion | 1 | disabled | skipped (codex_reviews=disabled) |
| Eng Review | `/plan-eng-review` | Architecture & tests (required) | 1 | clean (PLAN) | 32 issues, 0 critical gaps |
| Design Review | `/plan-design-review` | UI/UX gaps | 0 | — | — |
| DX Review | `/plan-devex-review` | Developer experience gaps | 0 | — | — |

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

- (confidence: 5/10) PLAN.md:19 vs :35 — `AuthBroker` is a new service but is missing from the "4 new classes" list; the real count was 5. Informational; resolved by D1.
- (confidence: 5/10) PLAN.md:31-32 — the five IDP calls are not named; the D7 static-cache step assumes two are OIDC discovery and JWKS. Verify at implementation.
- (confidence: 4/10) PLAN.md:35 — `RequestPolicy` contents are undefined; treated as a pure function (D1). Revisit if it needs state.

**OUTSIDE COVERAGE:** provider codex, phase plan-review, status disabled by config (`codex_reviews=disabled`), no findings; no native fallback was dispatched because disabled is an intentional opt-out, not a provider failure.

**VERDICT:** ENG CLEARED — ready to implement (1 clean plan-eng-review run within 7 days, 0 unresolved, 0 critical gaps). Outside review disabled by config; CEO, Design, DX reviews not run and not required.

NO UNRESOLVED DECISIONS