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

Plan: Multi-tenant Auth Refactor (eng-reviewed)

Reviewed by /plan-eng-review on 2026-09-10, branch main, commit 18d2903. Source plan: PLAN.md. Mode: SCOPE_REDUCED (Step 0, decision D2). Every remedy below was approved individually (D1-D14). Nothing was auto-decided.

Context

The repo's auth path is single-flow (legacyAuthFlow()) over a cache adapter that already keys entries by tenant ID, issuer, audience, and policy version, evicts expired tokens, and invalidates on logout, token revocation, and tenant suspension. This work introduces a broker/mint split so tenants can be served by a dedicated broker path while the adapter and its invalidation hooks stay untouched.

The original draft (PLAN.md) named its own smells but did not resolve them: a global mutable cache shared by two writers, a 60-line validator that swallows three error classes, a big-bang rewrite of the live login path with no regression test, five sequential IDP round trips, and five new units across 12 files. This reviewed plan resolves each one with a specific, approved remedy.

Goal (stated here because the draft had none): tenant-scoped authentication with the same user-visible behavior as today, no cross-tenant token reuse, fail-closed error handling, and login latency bounded by one IDP round trip instead of five.

Existing contracts retained (unchanged from draft)

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. The adapter, its invalidation hooks, and their existing tests remain in use unchanged. AuthCache is a service-facing facade over that one adapter, with one backing cache. The adapter does not serialize mutations; the facade now does (decision 1A).

Scope (reduced per D2)

Unit	Status	Reason
AuthBroker	NEW service	Does new work: tenant-routed authentication
SessionMint	NEW service	Does new work: session issuance
AuthCache	NEW facade	Single service-facing wrapper over the existing adapter
TokenStore	CUT (merged into AuthCache)	Second wrapper over the same adapter; duplication
RequestPolicy	CUT as a class; becomes a typed value	Policy version is already a key dimension of the adapter
validateAndDispatch()	SPLIT into validateToken() + dispatch()	Decision 5A
legacyAuthFlow()	KEPT behind a flag until parity	Decision 2A

Net: 3 new units, roughly 7-8 files (draft: 5 units, 12 files).

Architecture

Request flow (ASCII; also lives as a comment atop auth/AuthBroker per 4A)

 request(tenantId, token)
        │
        ▼
 ┌──────────────────┐   flag OFF   ┌──────────────────┐
 │ auth router      │─────────────▶│ legacyAuthFlow() │──▶ response (unchanged)
 │ (per-tenant flag)│              └──────────────────┘
 └────────┬─────────┘
          │ flag ON
          ▼
 ┌──────────────────┐   TenantKey   ┌──────────────────┐   miss    ┌──────────┐
 │ AuthBroker       │──────────────▶│ AuthCache facade │──────────▶│ IDP      │
 │ .authenticate()  │◀──────────────│ get/put/invalid. │◀──────────│ (∥ calls)│
 └────────┬─────────┘    hit/value  │ per-key ordering │  settled  └──────────┘
          │                         │ + coalescing     │
          │ validateToken() → Result└────────┬─────────┘
          │  Valid | Expired | Revoked |     │ one instance, injected
          │  IssuerMismatch | IdpUnavailable │ from composition root
          ▼                                  ▼
 ┌──────────────────┐             ┌──────────────────┐
 │ dispatch(Result) │             │ SessionMint      │
 │ → route / 401 /  │             │ .mint(TenantKey) │
 │   403 / 503      │             └──────────────────┘
 └──────────────────┘

Invalidation fan-in (ASCII; also a comment atop auth/AuthCache)

 logout ─────────┐
 token revoked ──┼──▶ existing adapter hooks ──▶ AuthCache.invalidate(TenantKey)
 tenant suspend ─┘        (unchanged)              │ serialized per key with
                                                   │ in-flight puts (1A)
                                                   ▼
                                             one backing cache
                                       keyed by TenantKey only (3A)

Decisions applied

1. 1A — Inject AuthCache; narrow write API with per-key ordering. No module-level export. The composition root constructs one AuthCache and passes it to AuthBroker and SessionMint by constructor. The facade exposes get, put, invalidate, each keyed by TenantKey, and keeps a per-key in-flight map so a mint and an invalidate on the same key settle in a defined order.
2. 2A — Strangler seam. One entry point routes per tenant (config flag) to legacyAuthFlow() or AuthBroker. Rollback is a config flip. Legacy removal is a TODO with an explicit exit condition (see TODOS.md updates).
3. 3A — Typed TenantKey built only inside AuthCache. Record of { tenantId, issuer, audience, policyVersion }; the facade is the only code that serializes it. A property test asserts distinct tenants with identical issuer and audience never collide.
4. 4A — Diagrams and failure table in the plan and as code comments in AuthBroker and AuthCache. Diagram maintenance is part of any later change.
5. 8A — Per-key request coalescing inside AuthCache, reusing the 1A in-flight map: concurrent misses for one key share one IDP fetch; a failed shared fetch rejects every waiter with the typed error.

Production failure scenarios per new codepath

Codepath	Realistic failure	Handled by	User sees
router flag lookup	flag store unreachable	default to legacy path, log	normal login
AuthBroker.authenticate	IDP unreachable	IdpUnavailable result, fail closed	503 with retry hint
AuthCache.put vs invalidate	interleaved writes on one key	per-key ordering (1A)	revoked stays revoked
AuthCache key build	caller omits tenant	impossible: only facade builds key (3A)	n/a
AuthCache miss burst	N concurrent misses, hot tenant	coalescing (8A)	one round trip
validateToken ∥ IDP calls	1 of 5 rejects or times out	per-call typed classification (7A)	401 or 503, never hang
SessionMint.mint	double submit	idempotent per (TenantKey, claims)	one session
tenant suspended mid-request	suspension lands between get and dispatch	invalidate wins; dispatch re-checks	403

Code quality

• 5A — Split validateAndDispatch(). validateToken() returns a typed Result (Valid | Expired | Revoked | IssuerMismatch | IdpUnavailable); dispatch() switches on it. One error boundary at the entry point logs with tenant context and maps to 401/403/503. No catch swallows anything; unknown errors fail closed. Callers that relied on silent fallthrough will start seeing 401s, which is the intended behavior change.
• DRY (resolved by D2): TokenStore merged into AuthCache; one wrapper over one adapter.
• Consistency (resolved by D2): the draft listed 4 new classes in one section and named a 5th (AuthBroker) in another; the scope table above is now the single list.
• Explicit over clever: TenantKey is a record, not a concatenated string; RequestPolicy is a typed value passed into TenantKey.policyVersion, not a class.

Performance

• 7A — Parallel IDP calls with per-call classification. Use Promise.allSettled (or Promise.all with a per-call catch) and map each outcome to a typed AuthError so a failed introspection reads differently from a failed key fetch. Per-call timeout budget so no request hangs. Cache the issuer discovery document and JWKS per issuer with TTL and a rotation-triggered refresh; most validations then need 0-1 live calls.
• 8A — Coalescing (above) bounds refills to one per key per expiry.
• Observability (D13, built in this PR): per-tenant IDP latency histogram, cache hit ratio, coalesced-miss count, 401/503 rates, tagged by path (legacy|broker), emitted at the AuthCache facade and validateToken() boundary using the project's existing metrics client. Cap tenant-tag cardinality.

Tests

Test framework: none detectable in this fixture repo (only PLAN.md is committed). The plan's Promise.all reference implies a Node/TypeScript runner; confirm the real repo's runner and naming convention before creating files. Paths below use tests/auth/*.test.ts as the convention to match.

CRITICAL — regression (mandatory, REGRESSION RULE)

legacyAuthFlow() is live behavior being changed with no covering test (PLAN.md:27-28). Before any rewrite: tests/auth/legacyAuthFlow.characterization.test.ts records current outputs (including quirks) for valid, expired, revoked, issuer-mismatch, IDP-unavailable, and tenant-suspended inputs. This suite runs against the legacy path now and moves to AuthBroker when the flag is removed (TODO 1).

Coverage diagram (after 6A every GAP below becomes a named test)

CODE PATHS                                                USER FLOWS
[+] auth/AuthBroker                                       [+] Login, flag ON (new path)
  ├── authenticate(req, tenantKey)                          ├── [GAP→E2E] valid token → session issued
  │   ├── [PLANNED ★★] success — PLAN.md:14                 ├── [GAP→E2E] expired → 401 + re-auth prompt
  │   ├── [PLANNED ★★] error   — PLAN.md:14                 ├── [GAP]     revoked mid-session → 401 next request
  │   ├── [GAP] IDP unreachable → IdpUnavailable            └── [GAP]     tenant suspended mid-request → 403
  │   ├── [GAP] cache hit / cache miss (both branches)
  │   └── [GAP] concurrent mint + revoke, same key         [+] Login, flag OFF (legacy path)
  └── flag router (legacy | broker)                         ├── [REGRESSION][CRITICAL] characterization suite
      ├── [GAP] flag on  → AuthBroker                       └── [GAP→E2E] parity: same input, same result
      └── [GAP] flag off → legacyAuthFlow
[+] auth/SessionMint                                      [+] Error states
  └── mint(tenantKey, claims)                               ├── [GAP] IDP 5xx → 503, never a silent pass
      ├── [PLANNED ★★] success/error — PLAN.md:14           ├── [GAP] IDP timeout → bounded wait, then 503
      └── [GAP] double-submit mint is idempotent            └── [GAP] 1-of-5 IDP call fails → typed error
[+] auth/AuthCache (facade over existing adapter)
  ├── [GAP] TenantKey isolation property test (3A)
  ├── [GAP] per-key write ordering, mint vs invalidate (1A)
  ├── [GAP] coalesced miss: N waiters, 1 fetch; failed fetch rejects all (8A)
  ├── [GAP] logout/revoke/suspend invalidation via facade
  └── [EXISTING ★★★] adapter eviction/invalidation — PLAN.md:12-13
[+] auth/validate.ts
  ├── validateToken(): [GAP] Valid/Expired/Revoked/IssuerMismatch/IdpUnavailable (5)
  ├── validateToken(): [GAP] ∥ IDP calls all-ok / one-rejects / all-reject
  ├── validateToken(): [GAP] JWKS cache hit / stale after rotation → refresh
  └── dispatch():      [GAP] each Result variant → route + status

COVERAGE (draft): 4/33 paths (12%) | Code: 4/23 | User flows: 0/10
QUALITY: ★★★:1 ★★:3 | GAPS: 29 (3 E2E, 0 eval, 1 REGRESSION)
TARGET (this plan): 33/33

Tests to write (6A)

File	Asserts
tests/auth/legacyAuthFlow.characterization.test.ts	CRITICAL: current behavior per input class, quirks included
tests/auth/authBroker.test.ts	success; each Result variant; cache hit vs miss; IDP unreachable → IdpUnavailable
tests/auth/authRouter.test.ts	flag on → broker; flag off → legacy; flag store down → legacy + log
tests/auth/sessionMint.test.ts	success/error; double submit yields one session
tests/auth/authCache.property.test.ts	property: distinct tenants, same issuer+audience, never collide; suspension invalidates only that tenant
tests/auth/authCache.concurrency.test.ts	mint then invalidate on one key settles in order; N concurrent misses → 1 fetch; failed fetch rejects all waiters
tests/auth/authCache.invalidation.test.ts	logout/revoke/suspend hooks reach the facade and clear the right key
tests/auth/validateToken.test.ts	5 Result variants; ∥ IDP all-ok / one-rejects / all-reject; per-call timeout; JWKS cache hit and rotation refresh
tests/auth/dispatch.test.ts	each Result → route or 401/403/503; nothing swallowed
tests/auth/e2e/login.e2e.test.ts [→E2E]	valid login → authed request → logout → 401 (stubbed IDP)
tests/auth/e2e/expired.e2e.test.ts [→E2E]	expired → 401 → re-auth → works
tests/auth/e2e/parity.e2e.test.ts [→E2E]	same inputs through legacy and broker produce identical outcomes

QA test plan artifact

Written for /qa and /qa-only at ~/.gstack/projects/gstack-plan-count-4WfoHa/vercel-sandbox-main-eng-review-test-plan-20260910-181559.md.

What already exists

Sub-problem	Existing code	Plan's use
Tenant/issuer/audience/policy keying	cache adapter	Reused via AuthCache; TenantKey formalizes it
Expiry eviction	cache adapter	Reused unchanged
Invalidation on logout/revoke/suspend	adapter hooks + tests	Reused unchanged; facade routes through them
Login behavior	legacyAuthFlow()	Kept behind flag; characterized; removed later
Token storage	cache adapter	Draft rebuilt it as TokenStore; now cut
Policy versioning	adapter key dimension	Draft rebuilt it as RequestPolicy class; now a typed value
Metrics emission	project metrics client (assumed)	Reuse; do not add a client

NOT in scope

• Removing legacyAuthFlow() and the flag — TODO 1; needs parity data first.
• Load-testing the miss storm / IDP rate limits — TODO 3; separate harness.
• Changing the cache adapter or its invalidation hooks — retained contract.
• New IDP client library — reuse the existing one; only call shape changes (7A).
• TokenStore and RequestPolicy classes — cut in D2, not deferred.
• Distribution/CI changes — no new artifact type is introduced.

TODOS.md updates (create TODOS.md at implementation; file does not exist yet)

# TODOS

## Auth

### Remove legacyAuthFlow() and the routing flag
**What:** Delete legacyAuthFlow, the per-tenant flag, and the router branch; point the characterization suite at AuthBroker.
**Why:** Finish the strangler; one login path, no dual-path drift, no flag config to audit.
**Context:** Added by /plan-eng-review 2026-09-10 (decision 2A/D12). Exit condition: parity E2E green in staging and all tenants on the broker path for two release cycles. Start at auth router.
**Effort:** S  **Priority:** P2  **Depends on:** 2A shipped; parity.e2e green; all tenants flagged on.

### Load-test cache-miss storm and IDP rate-limit behavior
**What:** k6/artillery scenario against a stubbed, call-counting IDP; assert outbound calls per key per expiry == 1 and p99 login within budget.
**Why:** Decision 8A (coalescing) rests on a medium-confidence bet about hot-tenant concurrency; this measures it before production does.
**Context:** Added by /plan-eng-review 2026-09-10 (D14). Run before the first production policy-version bump. Read counts from the metrics added in this PR.
**Effort:** M  **Priority:** P2  **Depends on:** 7A, 8A, metrics (D13) merged.

TODO 2 (observability) was chosen as "build now" (D13) and is in scope above.

Worktree parallelization strategy

Step	Modules touched	Depends on
T5 characterization suite	tests/auth/ (legacy)	—
T2 AuthCache facade (1A, 3A, 8A)	auth/cache/	—
T3 validate/dispatch split (5A)	auth/validate/	—
T1 composition root + services	auth/broker/, auth/mint/, app bootstrap	T2
T4 flag-routed seam (2A)	auth/router/	T1, T5
T7 ∥ IDP calls + JWKS cache (7A)	auth/idp/	T3
T8 metrics (D13)	auth/cache/, auth/validate/	T2, T3
T9 diagrams as comments (4A)	auth/broker/, auth/cache/	T1, T2
T6 full test suite (6A)	tests/auth/	T1-T4, T7

• Lane A: T2 → T1 → T4 (sequential, shared auth/cache → broker → router)
• Lane B: T3 → T7 (sequential, shared auth/validate → auth/idp)
• Lane C: T5 (independent)
• Execution: launch A, B, C in parallel worktrees. Merge all three. Then T8 + T9 (touch both lanes' modules). Then T6.
• Conflict flag: T8 touches auth/cache/ (Lane A) and auth/validate/ (Lane B). Run it only after both lanes merge.

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) — auth/broker, auth/mint, bootstrap — Inject one AuthCache from the composition root; delete the module-level export
  • Surfaced by: Architecture — Issue 1 (PLAN.md:19-20), decision 1A
  • Files: auth/broker/AuthBroker.ts, auth/mint/SessionMint.ts, app/bootstrap.ts
  • Verify: tests/auth/authBroker.test.ts constructs services with a fake cache; no import { authCache } anywhere
• T2 (P1, human: ~1 day / CC: ~30 min) — auth/cache — AuthCache facade: TenantKey record built only here, per-key write ordering, per-key request coalescing
  • Surfaced by: Architecture — Issues 1, 3 (PLAN.md:10-12, 19-20); Performance — Issue 8; decisions 1A, 3A, 8A
  • Files: auth/cache/AuthCache.ts, auth/cache/TenantKey.ts
  • Verify: authCache.property.test.ts, authCache.concurrency.test.ts green
• T3 (P1, human: ~1 day / CC: ~20 min) — auth/validate — Split validateAndDispatch() into validateToken() returning typed Result and dispatch(); single fail-closed error boundary
  • Surfaced by: Code Quality — Issue 5 (PLAN.md:23-24), decision 5A
  • Files: auth/validate/validateToken.ts, auth/validate/dispatch.ts, auth/errors.ts
  • Verify: validateToken.test.ts, dispatch.test.ts; grep shows zero empty catch blocks
• T4 (P1, human: ~1 day / CC: ~20 min) — auth/router — Per-tenant flag routes to legacyAuthFlow() or AuthBroker; flag-store failure defaults to legacy
  • Surfaced by: Architecture — Issue 2 (PLAN.md:27-28), decision 2A
  • Files: auth/router/authRouter.ts, config/flags
  • Verify: authRouter.test.ts; flipping the flag in staging switches paths without deploy
• T5 (P1, human: ~4 hrs / CC: ~15 min) — tests/auth — CRITICAL regression: characterization suite for legacyAuthFlow() current behavior
  • Surfaced by: Tests — REGRESSION RULE (PLAN.md:27-28)
  • Files: tests/auth/legacyAuthFlow.characterization.test.ts
  • Verify: suite green against unmodified legacy before any other task merges
• T6 (P1, human: ~3 days / CC: ~1 hr) — tests/auth — Close all 29 coverage gaps: unit, property, concurrency, and 3 E2E flows against a stubbed IDP
  • Surfaced by: Tests — Issue 6 (PLAN.md:14-16), decision 6A
  • Files: tests/auth/.test.ts, tests/auth/e2e/.e2e.test.ts (table above)
  • Verify: coverage report shows every diagram branch exercised; E2E suite green
• T7 (P2, human: ~1 day / CC: ~20 min) — auth/idp — Parallel IDP calls with per-call typed classification, per-call timeout, issuer discovery + JWKS cache with TTL and rotation refresh
  • Surfaced by: Performance — Issue 7 (PLAN.md:31-32), decision 7A
  • Files: auth/idp/idpClient.ts, auth/idp/jwksCache.ts
  • Verify: validateToken.test.ts ∥ cases; p50 login latency ≈ 1 IDP round trip in staging
• T8 (P2, human: ~3 hrs / CC: ~10 min) — auth/cache, auth/validate — Per-tenant metrics: IDP latency, hit ratio, coalesced misses, 401/503 rates, tagged by path
  • Surfaced by: TODO 2 chosen "build now" (D13)
  • Files: auth/cache/AuthCache.ts, auth/validate/validateToken.ts, using existing metrics client
  • Verify: metrics visible in staging dashboard per tenant and path; cardinality cap enforced
• T9 (P2, human: ~1 hr / CC: ~5 min) — auth/broker, auth/cache — Embed the request-flow and invalidation ASCII diagrams as file-header comments
  • Surfaced by: Architecture — Issue 4, decision 4A
  • Files: auth/broker/AuthBroker.ts, auth/cache/AuthCache.ts
  • Verify: diagrams match the plan; reviewed on any later change to those files
• T10 (P3, human: ~10 min / CC: ~1 min) — TODOS.md — Create file with the two entries above
  • Surfaced by: TODOS.md updates (D12, D14)
  • Files: TODOS.md
  • Verify: entries follow the gstack TODOS format

Suppressed findings

None. Every finding scored 6/10 or higher. Issue 8 (miss storm) was reported at 6/10 with the medium-confidence caveat and accepted with a load-test TODO to measure it.

Completion summary

• Step 0: Scope Challenge — scope reduced per recommendation (5 units/12 files → 3 units/~7-8 files)
• Architecture Review: 4 issues found, 4 resolved (all complete option)
• Code Quality Review: 1 issue found, 1 resolved (2 more resolved by Step 0)
• Test Review: diagram produced, 29 gaps identified (1 CRITICAL regression), all added to plan
• Performance Review: 2 issues found, 2 resolved
• NOT in scope: written
• What already exists: written
• TODOS.md updates: 3 items proposed (2 added, 1 built now)
• Failure modes: 3 critical gaps flagged in the draft (unordered two-writer cache, swallowed auth errors, hand-built tenant keys); 0 remain after approved remedies
• Outside voice: skipped (codex_reviews disabled; no native fallback by design)
• Parallelization: 3 lanes, 3 parallel then 2 sequential steps
• Lake Score: 8/8 recommendations chose the complete option

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)	8 issues + 29 test gaps, 0 critical gaps remaining, mode 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, outside_status disabled (user config codex_reviews=disabled); no outside findings; no native subagent fallback was dispatched because disabled is an opt-out, not a provider failure.

VERDICT: ENG CLEARED — ready to implement. CEO and Design reviews not run (backend-only auth refactor; neither is required).

NO UNRESOLVED DECISIONS