docs(10): add code review report

.planning/phases/10-pax8-client-auth-foundation/10-REVIEW.md

@@ -0,0 +1,189 @@
+---
+phase: 10-pax8-client-auth-foundation
+reviewed: 2026-07-10T22:06:39Z
+depth: standard
+files_reviewed: 7
+files_reviewed_list:
+  - lib/types/pax8.ts
+  - lib/services/pax8-client.ts
+  - lib/services/pax8-client.test.ts
+  - lib/services/pax8-factory.ts
+  - lib/services/pax8-factory.test.ts
+  - migrations/091_pax8_tables.sql
+  - scripts/verify-pax8-auth.ts
+findings:
+  critical: 0
+  warning: 4
+  info: 4
+  total: 8
+status: issues_found
+---
+
+# Phase 10: Code Review Report
+
+**Reviewed:** 2026-07-10T22:06:39Z
+**Depth:** standard
+**Files Reviewed:** 7
+**Status:** issues_found
+
+## Summary
+
+Reviewed the PAX8 client/factory/type/migration foundation added in this phase. No
+hardcoded secrets, injection vectors, or crashes were found — the OAuth2
+client-credentials flow, singleton factory pattern, and schema-only migration all
+follow existing Pulse conventions (`is<Name>Configured()` + lazy singleton,
+`snake_case` DB columns, `IF NOT EXISTS` migrations, soft-delete audit columns).
+
+The findings below are correctness/robustness gaps in `pax8-client.ts`'s token and
+retry handling that will matter once Phase 11/12 build sync services on top of this
+client — none are exploitable today because the only current caller is a manual
+verification script (`scripts/verify-pax8-auth.ts`), but they should be fixed before
+this client is wired into a scheduled sync job, since a sync job won't have a human
+watching stdout when the token/retry logic misbehaves.
+
+## Warnings
+
+### WR-01: Token response is used without validating its shape
+
+**File:** `lib/services/pax8-client.ts:46-49`
+**Issue:** `getToken()` calls `await res.json()` and assigns `data.access_token` /
+`data.expires_in` straight into client state with no check that the fields exist.
+If PAX8 ever returns a `200` with an unexpected body shape (proxy/CDN error page
+serialized as JSON, API version drift, etc.), `this.accessToken` becomes `undefined`
+while the non-null assertion on line 49 (`return this.accessToken!;`) tells the type
+system it's a `string`. Every subsequent call then sends `Authorization: Bearer
+undefined` and fails with a confusing `401` from PAX8's API instead of a clear
+"malformed token response" error at the source. `data` is also implicitly `any` —
+there's no `Pax8TokenResponse` type in `lib/types/pax8.ts` even though the file
+otherwise types every other PAX8 response shape.
+**Fix:**
+```ts
+interface Pax8TokenResponse {
+  access_token: string;
+  token_type: string;
+  expires_in: number;
+}
+// ...
+const data = (await res.json()) as Pax8TokenResponse;
+if (!data.access_token || !data.expires_in) {
+  throw new Error('PAX8 token response missing access_token/expires_in');
+}
+this.accessToken = data.access_token;
+this.tokenExpiry = Date.now() + data.expires_in * 1000;
+return this.accessToken;
+```
+
+### WR-02: Cached token is never invalidated on a `401` from a data-plane call
+
+**File:** `lib/services/pax8-client.ts:55-73`
+**Issue:** `fetchJson()` only special-cases `429`. If PAX8 revokes or rotates the
+token mid-lifetime (key rotation, admin revocation, etc.) a data call will start
+returning `401`, but `this.accessToken` / `this.tokenExpiry` are never cleared, so
+every subsequent call keeps reusing the same broken token until it naturally
+expires (up to ~24h per `expires_in`) or the process restarts. There's no recovery
+path in between.
+**Fix:** On `401`, clear the cached token and retry once with a fresh one:
+```ts
+if (res.status === 401 && retryCount === 0) {
+  this.accessToken = null;
+  return this.fetchJson(path, retryCount + 1);
+}
+```
+
+### WR-03: `Retry-After` parsing silently collapses to a near-zero wait on non-numeric values
+
+**File:** `lib/services/pax8-client.ts:62`
+**Issue:** `Math.max(30, parseInt(res.headers.get('Retry-After') || '30', 10))` assumes
+`Retry-After` is always a delay-in-seconds integer. Per HTTP spec, `Retry-After` may
+also be an HTTP-date string. `parseInt()` on a date string returns `NaN`, and
+`Math.max(30, NaN)` evaluates to `NaN` (any comparison against `NaN` is `false`, so
+`Math.max` can't select the `30` floor). `setTimeout(r, NaN * 1000)` then fires on
+essentially the next tick (Node coerces a `NaN`/falsy delay to `1`), so instead of
+backing off, the client hammers PAX8 immediately on the next retry attempt — the
+opposite of the intended behavior, and this is exactly the account-wide 1000/min
+rate limit path called out in 10-RESEARCH.md Pitfall 4.
+**Fix:**
+```ts
+const raw = res.headers.get('Retry-After');
+const parsed = raw ? parseInt(raw, 10) : NaN;
+const retryAfter = Number.isFinite(parsed) ? Math.max(30, parsed) : 30;
+```
+
+### WR-04: No coalescing of concurrent `getToken()` calls
+
+**File:** `lib/services/pax8-client.ts:23-50`
+**Issue:** `getToken()` checks the cache and, if empty/expired, kicks off a fresh
+`fetch` — but nothing marks "a fetch is in flight." If two callers invoke
+`listCompanies()` (or any future method) concurrently while the cache is cold (first
+call after boot, or right after expiry), both will independently see
+`this.accessToken` as falsy and issue duplicate token requests. This is a shared
+singleton (`pax8-factory.ts` caches one instance process-wide), so concurrent
+requests from multiple API routes hitting a cold cache is a realistic scenario, not
+a synthetic one.
+**Fix:** Cache the in-flight promise, not just the resolved token:
+```ts
+private tokenPromise: Promise<string> | null = null;
+
+private async getToken(): Promise<string> {
+  if (this.accessToken && Date.now() < this.tokenExpiry - 60000) return this.accessToken;
+  if (this.tokenPromise) return this.tokenPromise;
+  this.tokenPromise = this.fetchToken().finally(() => { this.tokenPromise = null; });
+  return this.tokenPromise;
+}
+```
+
+## Info
+
+### IN-01: Magic numbers without named constants
+
+**File:** `lib/services/pax8-client.ts:24, 62, 61`
+**Issue:** The 60-second expiry buffer, the 30-second minimum retry wait, and the
+4-retry cap are inline numeric literals. Fine individually, but they'll need to be
+referenced again once Phase 11/12 extend this client, and unnamed magic numbers
+invite drift between the token-buffer logic and any future rate-limit logic.
+**Fix:** Extract `const TOKEN_EXPIRY_BUFFER_MS = 60_000;`, `const MIN_RETRY_AFTER_SECONDS = 30;`, `const MAX_RETRIES = 4;` at module scope.
+
+### IN-02: Stale/contradictory comment above `fetchJson`
+
+**File:** `lib/services/pax8-client.ts:52-54`
+**Issue:** The comment states "Phase 11/12 will extend this with 429-aware
+Retry-After backoff ... — not needed for this phase's single auth-proof call," but
+the method immediately below it already implements 429-aware Retry-After backoff
+(lines 61-65). A future reader (or the Phase 11 implementer) could reasonably
+conclude no retry logic exists yet and duplicate it, or misjudge current behavior.
+**Fix:** Update the comment to describe what's actually deferred to Phase 11/12
+(e.g., "extend this with pagination cursors / cross-call rate-limit budget
+tracking"), not backoff that already exists.
+
+### IN-03: No test coverage for the 429 retry path or generic API error path
+
+**File:** `lib/services/pax8-client.test.ts`
+**Issue:** The test suite covers the token-fetch happy path, token caching, the
+token-failure path, and `listCompanies()`'s happy path, but never exercises
+`fetchJson`'s `429` retry branch or its generic `!res.ok` (non-token) error branch.
+Given WR-03 above, a test asserting the actual `setTimeout` delay used for a
+`429` (with `vi.useFakeTimers()`) would have caught the `Retry-After` parsing bug
+directly.
+**Fix:** Add cases: a `429` followed by a `200` (asserting retry + eventual
+success, with fake timers to avoid a real wait), and a non-`429` `!res.ok` (e.g.
+`500`) asserting the thrown error includes the path and status.
+
+### IN-04: `candidate_company_ids` / `match_confidences` array-length parity is unenforced
+
+**File:** `migrations/091_pax8_tables.sql:147-157`
+**Issue:** `pax8_company_match_review` stores parallel arrays
+(`candidate_company_ids BIGINT[]`, `match_confidences TEXT[]`) with no `CHECK`
+constraint that they stay the same length. This mirrors the pre-existing
+`device_link_review` table (migration 080) exactly, per the file's own comment, so
+it isn't a regression introduced by this migration — but it's worth a `CHECK
+(array_length(candidate_company_ids, 1) = array_length(match_confidences, 1))`
+before Phase 12's matcher starts writing rows, since a mismatch here would only
+surface as an off-by-index bug in whatever admin UI eventually renders the pairs.
+**Fix:** Add the `CHECK` constraint in this migration (or a follow-up one) rather
+than inheriting the gap silently.
+
+---
+
+_Reviewed: 2026-07-10T22:06:39Z_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_