docs(phase-11): add code review report
This commit is contained in:
parent
d4630dd7de
commit
dee3bdf28d
1 changed files with 274 additions and 0 deletions
|
|
@ -0,0 +1,274 @@
|
|||
---
|
||||
phase: 11-company-catalog-subscription-sync
|
||||
reviewed: 2026-07-10T23:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 6
|
||||
files_reviewed_list:
|
||||
- migrations/092_pax8_subscription_costs.sql
|
||||
- lib/types/pax8.ts
|
||||
- lib/services/pax8-client.ts
|
||||
- lib/services/pax8-client.test.ts
|
||||
- lib/services/pax8-sync-service.ts
|
||||
- app/api/pax8/sync/route.ts
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 5
|
||||
info: 6
|
||||
total: 11
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 11: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-07-10T23:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 6
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed the Phase 11 company-catalog/subscription-sync deliverables: migration
|
||||
092, the extended `lib/types/pax8.ts`, the client's new read-only pagination
|
||||
helpers, `Pax8SyncService`, and `POST/GET /api/pax8/sync`.
|
||||
|
||||
The load-bearing invariants called out in the plans hold up under inspection:
|
||||
|
||||
- **PAX8-08 (read-only invariant):** confirmed by grep — the only `method:` in
|
||||
`pax8-client.ts` is the `/v1/token` OAuth POST; `pax8-sync-service.ts` contains
|
||||
no `fetch(` call at all and only invokes the client's `listAll*` read methods.
|
||||
- **SQL injection:** every query in `pax8-sync-service.ts` is parameterized
|
||||
(`$1`…`$N`); the three tombstone `UPDATE`s use `id <> ALL($1::uuid[])` with
|
||||
the id array passed as a bound parameter, never interpolated.
|
||||
- **Soft-delete correctness:** all three tombstone passes (companies,
|
||||
subscriptions, products) correctly short-circuit to `0` when the "seen" id
|
||||
array is empty, so a failed/empty pull cannot mass-delete a table.
|
||||
- **Auth gating:** `/api/pax8/sync` is not present in `middleware.ts`'s public
|
||||
allowlist; it stays behind the session-cookie check.
|
||||
|
||||
No critical (security/data-loss) findings. The warnings below are mostly about
|
||||
error-reporting accuracy and one carried-over, still-unfixed bug in the
|
||||
client's rate-limit backoff that Phase 11 now exercises far more heavily than
|
||||
Phase 10 did (a single auth-proof call vs. potentially hundreds of paginated
|
||||
GETs per full sync across 118 companies / 445 subscriptions / 46+ products,
|
||||
per the live-run numbers in 11-03-SUMMARY.md).
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `Retry-After` backoff still breaks on non-integer values — and is now exercised at scale
|
||||
|
||||
**File:** `lib/services/pax8-client.ts:62`
|
||||
**Issue:** `Math.max(30, parseInt(res.headers.get('Retry-After') || '30', 10))`
|
||||
was already flagged in the Phase 10 review (`10-REVIEW.md` WR-03) and was not
|
||||
fixed. Per the HTTP spec, `Retry-After` may be an HTTP-date string instead of
|
||||
a delay-in-seconds integer. `parseInt()` on a date string (e.g. `"Wed, 21 Oct
|
||||
2026 00:00:00 GMT"`) returns `NaN`; `Math.max(30, NaN)` evaluates to `NaN`
|
||||
(any comparison against `NaN` is `false`, so the `30` floor is never applied);
|
||||
`setTimeout(r, NaN * 1000)` then fires on essentially the next tick instead of
|
||||
backing off. This is exactly the account-wide 1000/min rate-limit scenario
|
||||
11-01-PLAN.md's threat model (T-11-07) claims is mitigated: "existing 429
|
||||
Retry-After backoff in the client's fetchJson is inherited." That mitigation
|
||||
claim is false whenever PAX8 sends a date-form `Retry-After`. Phase 11's new
|
||||
`paginateAll()` calls `fetchJson()` in a tight sequential loop — for the
|
||||
subscriptions entity alone that's 3 pages at `size=200` for 445 rows, and
|
||||
every page walk inherits this same broken backoff — so a single malformed
|
||||
`Retry-After` header during a real 429 can now cause a request-hammering loop
|
||||
against a rate limit PAX8 shares across the whole partner account, not just
|
||||
Pulse.
|
||||
**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-02: Per-entity sync loops wrap the entire loop in one try/catch, so a single bad row misreports (or drops) the whole entity's result
|
||||
|
||||
**File:** `lib/services/pax8-sync-service.ts:113-173` (companies), `175-253`
|
||||
(subscriptions), `262-325` (products)
|
||||
**Issue:** Each `syncX()` method wraps its whole per-row loop *and* the
|
||||
subsequent tombstone query in one `try { … } catch { return { …, upserted: 0,
|
||||
tombstoned: 0, error: msg } }`. If any single row throws mid-loop — e.g. a
|
||||
company with a `NOT NULL name` violation, or a subscription/product whose id
|
||||
comes back as a non-UUID string from PAX8 (column is `UUID`, insert would
|
||||
throw `invalid input syntax for type uuid`) — the method:
|
||||
1. Discards every row already successfully upserted in that loop iteration
|
||||
from the reported count (the catch hardcodes `upserted: 0`, even though
|
||||
those rows are already committed in Postgres, since each `postgresClient.query`
|
||||
call auto-commits individually — the data isn't lost, but the reported
|
||||
metric is wrong).
|
||||
2. Never runs that entity's tombstone pass for the run, since the tombstone
|
||||
query sits after the loop, inside the same try. A transient bad row from
|
||||
PAX8 therefore delays soft-delete reconciliation for that entire entity by
|
||||
at least one sync cycle.
|
||||
|
||||
This pattern mirrors `qbo-sync-service.ts`'s existing per-entity `syncInvoices`/
|
||||
etc. methods (same whole-loop try + hardcoded `recordsUpserted: 0` on catch),
|
||||
so it isn't a novel defect introduced by this phase, but it's present in all
|
||||
three of the new PAX8 entity methods and worth fixing here since PAX8-04/05
|
||||
explicitly rely on getting a truthful `Pax8EntitySyncResult` back per run.
|
||||
**Fix:** Track upserted count outside a narrower per-row `try/catch` (log +
|
||||
`continue` on a single row's failure instead of letting it abort the whole
|
||||
loop), or at minimum report the actual `upserted`/`tombstoned` counters
|
||||
accumulated so far in the catch block instead of hardcoded `0`.
|
||||
|
||||
### WR-03: `POST /api/pax8/sync` has no try/catch — a missing/misconfigured PAX8 credential throws an unhandled exception instead of a 503
|
||||
|
||||
**File:** `app/api/pax8/sync/route.ts:5-20`
|
||||
**Issue:** `getPax8SyncService()` → `new Pax8SyncService()` → `getPax8Client()`
|
||||
throws synchronously (`'PAX8 is not configured — set PAX8_CLIENT_ID and
|
||||
PAX8_CLIENT_SECRET'`) when the env vars are unset. `POST()` calls
|
||||
`getPax8SyncService()` with no surrounding `try/catch`, so this throws out of
|
||||
the route handler entirely instead of returning the CLAUDE.md-documented `503`
|
||||
for missing/bad config. `GET()` in the same file *does* wrap its body in
|
||||
try/catch and would handle this correctly. This mirrors an existing gap in
|
||||
`app/api/itglue/sync/route.ts`'s `POST` (same missing try/catch), so it's a
|
||||
systemic pattern rather than something new to this phase — but it's a real
|
||||
inconsistency worth closing here since Phase 11 is net-new code.
|
||||
**Fix:**
|
||||
```ts
|
||||
export async function POST(req: NextRequest) {
|
||||
try {
|
||||
const body = await req.json().catch(() => ({}));
|
||||
const triggeredBy = body.triggeredBy || 'manual';
|
||||
const svc = getPax8SyncService();
|
||||
if (svc.isSyncInProgress()) {
|
||||
return NextResponse.json({ error: 'Sync already in progress' }, { status: 409 });
|
||||
}
|
||||
svc.fullSync(triggeredBy).catch(err => console.error('[Pax8Sync] Background sync error:', err.message));
|
||||
return NextResponse.json({ ok: true, message: 'PAX8 sync started' });
|
||||
} catch (err) {
|
||||
return NextResponse.json(
|
||||
{ error: err instanceof Error ? err.message : 'PAX8 sync unavailable' },
|
||||
{ status: 503 }
|
||||
);
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
### WR-04: `getToken()` still has no shape validation or in-flight coalescing (carried over from Phase 10, unfixed)
|
||||
|
||||
**File:** `lib/services/pax8-client.ts:23-50`
|
||||
**Issue:** Both `10-REVIEW.md` WR-01 (no validation that `data.access_token`/
|
||||
`data.expires_in` exist before assigning + non-null-asserting
|
||||
`this.accessToken!`) and WR-04 (no coalescing of concurrent cold-cache
|
||||
`getToken()` calls) remain unaddressed. Phase 11 didn't touch `getToken()`,
|
||||
but it did add three new call sites (`listAllCompanies/Subscriptions/Products`)
|
||||
that all funnel through it via `fetchJson()`. The malformed-response case is
|
||||
still unguarded: a `200` with an unexpected body shape (proxy error page,
|
||||
API version drift) sets `this.accessToken = undefined` while the type system
|
||||
is told it's `string`, and every subsequent PAX8 call fails with a confusing
|
||||
`401` instead of a clear error at the source.
|
||||
**Fix:** As in `10-REVIEW.md` WR-01 — type the token response and validate
|
||||
`access_token`/`expires_in` are present before assigning.
|
||||
|
||||
### WR-05: `sync_history.triggered_by` accepts an unvalidated request-body value of any type
|
||||
|
||||
**File:** `app/api/pax8/sync/route.ts:5-7`
|
||||
**Issue:** `const triggeredBy = body.triggeredBy || 'manual';` — `body` is
|
||||
untyped (`any`, from `req.json().catch(() => ({}))`). If a caller POSTs
|
||||
`{ "triggeredBy": { "foo": "bar" } }` or `{ "triggeredBy": 42 }`, that value
|
||||
flows unchanged into `Pax8SyncService.fullSync(triggeredBy)` and then into
|
||||
`INSERT INTO sync_history (..., triggered_by) VALUES (..., $2)` against a
|
||||
`VARCHAR(255)` column. `pg` will attempt to stringify a non-string/number
|
||||
parameter, and for an object this can throw at query time (mid-`fullSync`,
|
||||
inside the already-try/catch-guarded `insertHistoryStarted`, so it's swallowed
|
||||
as a warning) or insert an unhelpful value. Low impact (this is an
|
||||
authenticated, internal trigger endpoint, not attacker-facing input in the
|
||||
traditional sense), but CLAUDE.md calls for validating inputs "when it
|
||||
matters," and a malformed `triggered_by` silently breaks sync-history
|
||||
attribution.
|
||||
**Fix:** `const triggeredBy = typeof body.triggeredBy === 'string' && body.triggeredBy.trim() ? body.triggeredBy : 'manual';`
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Unused type imports in `pax8-sync-service.ts`
|
||||
|
||||
**File:** `lib/services/pax8-sync-service.ts:16-17`
|
||||
**Issue:** `Pax8Company` and `Pax8Subscription` are imported but never
|
||||
referenced as explicit type annotations anywhere in the file (confirmed via
|
||||
`npx eslint`, which reports both as `@typescript-eslint/no-unused-vars`
|
||||
warnings). `Pax8Product` (line 268) is the only one actually used.
|
||||
**Fix:** Drop the two unused imports, or add explicit `: Pax8Company[]` /
|
||||
`: Pax8Subscription[]` annotations where `listAllCompanies()`/
|
||||
`listAllSubscriptions()` results are consumed if the intent was
|
||||
self-documentation.
|
||||
|
||||
### IN-02: Stale/contradictory comment above `fetchJson` still unfixed
|
||||
|
||||
**File:** `lib/services/pax8-client.ts:52-54`
|
||||
**Issue:** Also carried over from `10-REVIEW.md` IN-02. The comment reads
|
||||
"Phase 11/12 will extend this with 429-aware Retry-After backoff … not needed
|
||||
for this phase's single auth-proof call," directly above a method that
|
||||
already implements 429/Retry-After backoff. Phase 11 is the phase the comment
|
||||
refers to, and it built `paginateAll()` on top of this method's *existing*
|
||||
backoff (per `11-01-SUMMARY.md`: "requesting size=200 and looping through
|
||||
fetchJson() (inheriting existing 429/Retry-After backoff)") without updating
|
||||
or removing the now-doubly-stale comment.
|
||||
**Fix:** Replace with something accurate, e.g. "429/Retry-After backoff below
|
||||
is inherited by all listAll* pagination helpers (Phase 11)."
|
||||
|
||||
### IN-03: `GET /api/pax8/sync` returns raw `snake_case` DB columns instead of the project's camelCase API convention
|
||||
|
||||
**File:** `app/api/pax8/sync/route.ts:34-46`
|
||||
**Issue:** CLAUDE.md: "API responses are camelCase — handlers transform
|
||||
manually (no ORM)." The `history` array here is returned as raw
|
||||
`sync_history` rows (`sync_type`, `started_at`, `completed_at`,
|
||||
`records_added`, `records_updated`, `records_deleted`, `error_message`,
|
||||
`triggered_by`), untransformed. This mirrors `app/api/itglue/sync/route.ts`'s
|
||||
`GET` (same pattern), so it's a pre-existing convention gap rather than new
|
||||
in this phase, but it's worth flagging since Phase 11 is the direct place a
|
||||
consistent camelCase transform could have been introduced for this response
|
||||
shape.
|
||||
**Fix:** Map `history.rows` to camelCase keys before returning, e.g.
|
||||
`history.rows.map(r => ({ id: r.id, syncType: r.sync_type, status: r.status, startedAt: r.started_at, … }))`.
|
||||
|
||||
### IN-04: `records_added` conflates "upserted" (insert + update), not "added"
|
||||
|
||||
**File:** `lib/services/pax8-sync-service.ts:69, 351`
|
||||
**Issue:** `totalUpserted` (the sum of every successful `INSERT ... ON
|
||||
CONFLICT DO UPDATE`) is written into `sync_history.records_added`. The column
|
||||
name (and the base `sync_history` schema's original Autotask-sync intent)
|
||||
implies net-new rows, but this value counts every row touched, added or
|
||||
merely refreshed. Not a functional bug — the plan explicitly specifies
|
||||
"records_added = total upserted" and the base `sync_history` table has no
|
||||
separate "matched/updated-only" column to split this into — but worth noting
|
||||
for anyone reading `sync_history` cross-integration expecting `records_added`
|
||||
to mean "new rows only."
|
||||
**Fix:** None required if the semantic is documented; consider a one-line
|
||||
comment at the `updateHistory()` call site noting `records_added` here means
|
||||
"total upserted," not "net-new."
|
||||
|
||||
### IN-05: `currencyCode ?? 'USD'` doesn't guard against an empty-string value
|
||||
|
||||
**File:** `lib/services/pax8-sync-service.ts:218`
|
||||
**Issue:** `s.currencyCode ?? 'USD'` only falls back when `currencyCode` is
|
||||
`null`/`undefined`, not when PAX8 returns `""`. An empty string would be
|
||||
inserted into the `CHAR(3) NOT NULL` `currency` column, right-padded to three
|
||||
spaces rather than defaulting to `'USD'`. Purely theoretical given PAX8's
|
||||
subscription objects always carry a real ISO currency code in practice (per
|
||||
the live-run confirmation in `11-03-SUMMARY.md`), but worth a defensive
|
||||
one-liner given `currencyCode: string | null` is the modeled type, not `string
|
||||
| null | ''`.
|
||||
**Fix:** `(s.currencyCode || 'USD')` or an explicit empty-string check.
|
||||
|
||||
### IN-06: No automated test coverage for `pax8-sync-service.ts` or the route
|
||||
|
||||
**File:** `lib/services/pax8-sync-service.ts`, `app/api/pax8/sync/route.ts`
|
||||
**Issue:** Per CLAUDE.md, automated test coverage in this codebase is scoped
|
||||
to `analyzer/`, `rmm/`, `b2/` — so this isn't a deviation from project
|
||||
convention, and matches `qbo`/`itglue`/`appgate` sync services (also
|
||||
untested). Noting it anyway because the two behaviors this review spent the
|
||||
most time verifying by hand — the tombstone empty-set guard (WARNING focus
|
||||
area #3) and the referenced-only product filtering (D-01/D-02) — are exactly
|
||||
the kind of logic a couple of `postgresClient`-mocked unit tests would pin
|
||||
down far more cheaply than the live-run verification `11-03-PLAN.md` requires
|
||||
each time this service changes.
|
||||
**Fix:** Optional — a small `vi.mock('./postgres-client')`-based test for
|
||||
`syncCompanies()`'s empty-seen-set tombstone skip would be the highest-value
|
||||
single test to add if this file gets touched again.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-07-10T23:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Loading…
Add table
Add a link
Reference in a new issue