From d1e5a522c41e188767f5786c526ade3012b839b2 Mon Sep 17 00:00:00 2001 From: lorentz Date: Fri, 10 Jul 2026 18:07:33 -0400 Subject: [PATCH] docs(10): add code review report --- .../10-REVIEW.md | 189 ++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 .planning/phases/10-pax8-client-auth-foundation/10-REVIEW.md diff --git a/.planning/phases/10-pax8-client-auth-foundation/10-REVIEW.md b/.planning/phases/10-pax8-client-auth-foundation/10-REVIEW.md new file mode 100644 index 0000000..e6c53e1 --- /dev/null +++ b/.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 (`isConfigured()` + 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 | null = null; + +private async getToken(): Promise { + 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_