# Code Review Report — Batch 13

**Batch**: AZ-510 (Auth bootstrap refresh consolidation)
**Cycle**: 3
**Date**: 2026-05-13
**Verdict**: PASS

---

## Phase 1: Context Loading

- Task spec: `_docs/02_tasks/todo/AZ-510_auth_bootstrap_consolidation.md` — replace broken
  `GET /api/admin/auth/refresh` (no `credentials:'include'`) with `POST /api/admin/auth/refresh`
  (with credentials) chained to `GET /api/admin/users/me`. Closes Finding B3 / Vision P3.
- Architecture vision principle P3 (`bearer in memory, refresh in HttpOnly cookie`) requires the
  bootstrap path to send the HttpOnly refresh cookie; the prior code violated this.
- Architecture compliance baseline (`_docs/02_document/architecture_compliance_baseline.md`)
  carries B3 as the open downstream item AZ-510 was created to close.

## Phase 2: Spec Compliance

| AC | Mechanism | Test Evidence |
|----|-----------|---------------|
| AC-1 — POST refresh + `credentials:'include'`, no GET refresh | `runBootstrap()` direct `fetch(..., {method:'POST', credentials:'include'})` (`AuthContext.tsx:45-48`) | `AuthContext.test.tsx` FT-P-01 asserts method, credentials, chain |
| AC-2 — Successful refresh chains to `/users/me` and resolves `loading:false` | `setToken(refreshData.token)` then `api.get<AuthUser>(endpoints.admin.usersMe())` (`AuthContext.tsx:51-53`); `setUser(result)` + `setLoading(false)` (`:78-79`) | FT-P-01 asserts `usersMeHits === 1`; `getToken()` becomes `BEARER` in NFT-SEC-01 |
| AC-3 — Failed refresh → `/login` exactly once, no flash | `if (!refreshRes.ok) return null` (`:49`) → `setUser(null)` + `setLoading(false)` (`:78-79`) | `ProtectedRoute.test.tsx` covers spinner→`/login` paths under POST-refresh handlers |
| AC-4 — `/users/me` failure clears bearer + logs | `try/catch` around `api.get` calls `setToken(null)` + `console.error` + returns `null` (`:54-61`); top-level `.then` then sets `user:null` + `loading:false` | New `AC-4 (AZ-510)` test in `AuthContext.test.tsx:108-138` asserts `getToken()` becomes `null`, `console.error` carries `"/users/me failed"` |
| AC-5 — Returning user not bounced to `/login` | Successful bootstrap path sets `user` before `loading:false`; `ProtectedRoute` only redirects when `!loading && !user` | Implicit in `ProtectedRoute.test.tsx` admin-route success cases (no `/login` rendered) |
| AC-6 — 401-retry path unchanged | `runBootstrap` uses direct `fetch`, not `api`; `api/client.ts:73-98` unchanged | `NFT-SEC-01` exercises bootstrap → 401 on `/users/me` → POST refresh rotation → replay; `FT-P-03` covers refresh transparency |

**Constraints**:
- C1 `getApiBase()` is the only base-URL source — honored (`:45`).
- C2 No `api.post()` for refresh — honored; uses direct `fetch` per the same comment in `api/client.ts:88`.
- C3 MSW handlers exercise production paths — honored; no `vi.mock('api/client')`.
- C4 `setToken(null)` precedes `setUser(null)` on every failure path — honored:
  - `/users/me` failure: `setToken(null)` (`:59`) → return `null` → top-level `setUser(null)` (`:78`).
  - Outer fetch reject: `setToken(null)` (`:87`) → `setUser(null)` (`:88`).

**Risk 4 (StrictMode double-mount)**: addressed via module-scoped `bootstrapInflight` promise
(`AuthContext.tsx:25, 70-74`). Test-only escape hatch `__resetBootstrapInflightForTests`
exported via the `src/auth` barrel and called in `tests/setup.ts` afterEach to prevent
inter-test promise leakage (was the proximate cause of `ProtectedRoute.test.tsx` hangs during
implementation).

No spec-gap findings.

## Phase 3: Code Quality

- **SOLID / SRP**: `runBootstrap` has one responsibility (refresh + chain + clear-on-failure);
  `AuthProvider`'s effect orchestrates the inflight guard and react state — clean separation.
- **Error handling**: explicit `try/catch` around `/users/me`; outer `.catch` handles network
  errors on the POST refresh itself. Both log via `console.error` with diagnostic prefix.
  No bare catches introduced. (Pre-existing `try { await api.post(authLogout()) } catch {}` in
  `logout` is out of scope.)
- **Naming**: `bootstrapInflight`, `runBootstrap`, `__resetBootstrapInflightForTests` are
  precise and self-documenting. Test export name carries the `__…ForTests` convention.
- **Defensive `hasPermission`**: `user?.permissions?.includes(perm) ?? false` — correctly
  guards against legacy `/users/me` payloads that omit `permissions`. Required because
  several existing test fixtures returned the bare `User` shape without `permissions`.
- **Comments**: comments explain *why* (StrictMode race, CORS posture for `api.post`,
  Constraint #4 ordering) — not *what*. Conforms to coderule.mdc.
- **Test quality**: AC-4 test asserts `getToken() === null` AND that `console.error` was
  called with the diagnostic prefix — meaningful state + log assertion, not just "no throw".

No findings.

## Phase 4: Security Quick-Scan

- No hardcoded secrets, no SQL/string-interp queries, no `eval`/`exec`.
- `console.error('[AuthContext] Refresh succeeded but /users/me failed:', err)` logs the error
  object. The error object originates from `api.get` which throws a structured error without
  bearer material; the bearer was set via `setToken` before the try block but is not in the
  thrown error. No bearer leak.
- HttpOnly refresh cookie continues to flow via `credentials:'include'` — never touched in JS.
  NFT-SEC-02 explicitly verifies `document.cookie` carries no refresh-prefixed cookie.

No findings.

## Phase 5: Performance

- Two sequential network calls (POST refresh → GET `/users/me`) on every cold mount. Spec NFR
  budgets 200 ms p95 for the chain on dev compose; same nginx/auth/host. Within budget.
- Module-scoped inflight promise prevents double-bootstrap under StrictMode dev double-mount,
  removing the wasted second round-trip.

No findings.

## Phase 6: Cross-Task Consistency

Single-task batch — N/A.

## Phase 7: Architecture Compliance

| Check | Result |
|-------|--------|
| Layer direction | `src/auth/AuthContext.tsx` imports from `../api` (barrel) and `../types` only — auth → api allowed per architecture |
| Public API respect | All cross-component imports go through `src/api/index.ts` and `src/types/index.ts` barrels; no deep imports |
| New cyclic deps | None introduced |
| Duplicate symbols | None |
| Cross-cutting in component dir | `bootstrapInflight` is auth-specific state; correctly lives in the auth component |

**STC-ARCH-01 (cross-component deep imports)** static gate: passed after fixing the
`tests/setup.ts → src/auth/AuthContext` deep import by re-exporting
`__resetBootstrapInflightForTests` from `src/auth/index.ts` (barrel) and switching the import
to `../src/auth`.

**STC-ARCH-02 (no hardcoded API literals)** static gate: passed; new `endpoints.admin.usersMe`
builder added (`src/api/endpoints.ts`) and used at the only callsite.

### Baseline Delta

| Status | Finding | Notes |
|--------|---------|-------|
| Resolved | B3 — Auth bootstrap missing `credentials:'include'` | Was open in `_docs/02_document/04_verification_log.md`; bootstrap now POST + `credentials:'include'` + chained `/users/me`. |
| Carried over | (none in this file's scope) | — |
| Newly introduced | (none) | — |

## Verdict

**PASS** — no Critical / High / Medium / Low findings. All ACs covered with tests; constraints
honored; static and fast profiles green (231 passed, 13 quarantined skips unchanged); Finding
B3 resolved.