docs(12): add code review report
This commit is contained in:
parent
2a4a27f1e0
commit
f9646c6480
1 changed files with 273 additions and 0 deletions
|
|
@ -0,0 +1,273 @@
|
|||
---
|
||||
phase: 12-orders-invoices-company-matching
|
||||
reviewed: 2026-07-11T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 11
|
||||
files_reviewed_list:
|
||||
- lib/services/pax8-client.test.ts
|
||||
- lib/services/pax8-client.ts
|
||||
- lib/services/pax8-company-matcher.test.ts
|
||||
- lib/services/pax8-company-matcher.ts
|
||||
- lib/services/pax8-sync-service.test.ts
|
||||
- lib/services/pax8-sync-service.ts
|
||||
- lib/types/pax8.ts
|
||||
- migrations/093_pax8_orders_company_matching.sql
|
||||
- migrations/094_pax8_order_items_quantity_numeric.sql
|
||||
- scripts/verify-pax8-invoice-items.ts
|
||||
- scripts/verify-pax8-orders-matching.ts
|
||||
findings:
|
||||
critical: 2
|
||||
warning: 3
|
||||
info: 4
|
||||
total: 9
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 12: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-07-11T00:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 11
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed the PAX8 orders/invoices ingestion and company-matching feature: the
|
||||
REST client (`pax8-client.ts`), the pg_trgm fuzzy matcher
|
||||
(`pax8-company-matcher.ts`), the full-sync orchestrator
|
||||
(`pax8-sync-service.ts`), the new schema (migrations 093/094), the shared
|
||||
types, and the two live-verification scripts, plus all three unit test files.
|
||||
|
||||
The read-only/GET-only discipline (PAX8-08), the SQL-injection avoidance for
|
||||
the fuzzy-matched company name (bound `$1`, never interpolated), and the
|
||||
never-overwrite-a-manual-match guard (D-05/SC#4) are all correctly
|
||||
implemented and covered by targeted unit tests. However, two defects will
|
||||
ship incorrect data to the database and to the admin UI if unaddressed: a
|
||||
precision-losing column type for the new dual-cost columns, and a metric
|
||||
mislabeling bug that surfaces fabricated "records deleted" counts in the
|
||||
existing Sync History dashboard for PAX8 runs. Several further robustness and
|
||||
maintainability gaps are noted below.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: `partner_cost` / `partner_cost_total` columns silently truncate real PAX8 cost precision
|
||||
|
||||
**File:** `migrations/093_pax8_orders_company_matching.sql:44-45`
|
||||
**Issue:**
|
||||
```sql
|
||||
ALTER TABLE pax8_order_items ADD COLUMN IF NOT EXISTS partner_cost NUMERIC(12,2);
|
||||
ALTER TABLE pax8_order_items ADD COLUMN IF NOT EXISTS partner_cost_total NUMERIC(12,2);
|
||||
```
|
||||
Both new dual-cost columns are declared with only 2 decimal digits of scale.
|
||||
But the sync service's own test fixture — explicitly annotated as "real
|
||||
live-verified sample values from 12-02-SUMMARY.md" — uses per-unit costs with
|
||||
3-4 decimal digits: `cost: 22.176` and `cost: 0.0182`
|
||||
(`lib/services/pax8-sync-service.test.ts:64,84`). `resolveCostColumns()`
|
||||
(`lib/services/pax8-sync-service.ts:35-53`) passes these values straight
|
||||
through to `partnerCost`/`partnerCostTotal`, which are then bound as plain
|
||||
JS numbers to the `INSERT INTO pax8_order_items` statement
|
||||
(`lib/services/pax8-sync-service.ts:452-471`).
|
||||
|
||||
Postgres does not reject values that exceed a column's declared *scale* — it
|
||||
silently rounds them (`NUMERIC(12,2)` stores `22.176` as `22.18` and `0.0182`
|
||||
as `0.02`). This is exactly the same class of bug that migration 094 already
|
||||
had to fix for `quantity` (there the type was too narrow in *kind*, INTEGER
|
||||
vs fractional; here it's too narrow in *scale*), except this failure mode
|
||||
does not throw — it corrupts stored financial data silently, and none of the
|
||||
existing tests (all mocked at the `postgresClient.query` boundary) can catch
|
||||
it. The Phase 12 feature is explicitly about "dual cost" (customer price vs.
|
||||
partner cost) reporting; per-unit partner cost is the number margin
|
||||
calculations depend on, and it will be systematically wrong at real-world
|
||||
scale (thousands of order items, rounding compounding rather than
|
||||
cancelling).
|
||||
|
||||
**Fix:** Widen both columns to match `quantity`'s fix, e.g.:
|
||||
```sql
|
||||
ALTER TABLE pax8_order_items ALTER COLUMN partner_cost TYPE NUMERIC(14,4);
|
||||
ALTER TABLE pax8_order_items ALTER COLUMN partner_cost_total TYPE NUMERIC(14,4);
|
||||
```
|
||||
in a new numbered migration (do not edit 093). Consider auditing
|
||||
`unit_price`/`line_total` (pre-existing `NUMERIC(12,2)` from migration 091)
|
||||
for the same risk given PAX8 line items can be usage-based/prorated.
|
||||
|
||||
### CR-02: `company_matches` "review" counts are added into `sync_history.records_deleted`, and the admin Sync History dashboard displays them as deletions
|
||||
|
||||
**File:** `lib/services/pax8-sync-service.ts:519-545` (`syncCompanyMatches`), `lib/services/pax8-sync-service.ts:109-113` (`fullSync` rollup)
|
||||
**Issue:** `syncCompanyMatches()` deliberately repurposes the
|
||||
`Pax8EntitySyncResult.tombstoned` field to mean "companies flagged for manual
|
||||
review this run" instead of an actual soft-delete count:
|
||||
```ts
|
||||
// Repurposed: not a soft-delete tombstone count — the number of
|
||||
// pax8_companies rows flagged for manual review this run (ambiguous
|
||||
// + no-candidate), surfaced through the same rollup field.
|
||||
tombstoned: result.flaggedAmbiguous + result.flaggedNoCandidate,
|
||||
```
|
||||
`fullSync()` then sums `tombstoned` across *all* entities unconditionally
|
||||
(`entities.reduce((sum, e) => sum + e.tombstoned, 0)`) into `totalTombstoned`,
|
||||
which is persisted as `sync_history.records_deleted`
|
||||
(`lib/services/pax8-sync-service.ts:113`). This is the exact same column that
|
||||
`components/admin/SyncHistoryTable.tsx:249-252` renders to admins as a red,
|
||||
negative "deleted" count:
|
||||
```tsx
|
||||
<TableCell className="text-right text-red-600">
|
||||
-{record.records_deleted}
|
||||
</TableCell>
|
||||
```
|
||||
An admin looking at a PAX8 full-sync row in the existing Sync History UI will
|
||||
see, e.g., "-3 deleted" when in fact zero companies were deleted and 3 were
|
||||
merely flagged for manual match review. Since ambiguous/no-candidate flags
|
||||
are an expected, ongoing steady-state outcome of this matcher (not a rare
|
||||
edge case), this will misreport on effectively every sync run that has any
|
||||
review-flagged company, undermining the dashboard's reliability for every
|
||||
integration it shows (not just PAX8).
|
||||
|
||||
**Fix:** Do not fold match-review counts into `tombstoned`/`records_deleted`.
|
||||
Either add a dedicated field to `Pax8EntitySyncResult`/`sync_history` for
|
||||
"flagged for review", or report `company_matches`'s `tombstoned` as `0` and
|
||||
surface the review count only in the `[Pax8Sync]` log line / a separate
|
||||
column, e.g.:
|
||||
```ts
|
||||
tombstoned: 0, // no soft-deletes performed by matching
|
||||
// surface flaggedAmbiguous/flaggedNoCandidate via a separate summary field
|
||||
```
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: A single failing invoice/item (or company/subscription/product) aborts the entire remaining batch for that sync run, silently skipping tombstoning
|
||||
|
||||
**File:** `lib/services/pax8-sync-service.ts:376-510` (`syncOrders`), same pattern in `syncCompanies` (153-213), `syncSubscriptions` (215-293), `syncProducts` (302-365)
|
||||
**Issue:** Each entity sync wraps its *entire* fetch-and-upsert loop (all
|
||||
invoices, then all items per invoice) inside one `try/catch`. Any thrown
|
||||
error partway through — a constraint violation, an unexpected null, a
|
||||
transient network error on `listAllInvoiceItems` — aborts all remaining
|
||||
invoices/items for that run *and* skips the tombstoning step entirely,
|
||||
silently truncating coverage for anything not yet processed. This is
|
||||
precisely the failure mode migration 094's comment documents as having
|
||||
already happened in production ("aborted the entire orders/order_items sync
|
||||
loop on the first such row... silently truncating SC#1's item coverage to a
|
||||
tiny partial subset instead of the full ~56k-row history"). Migration 094
|
||||
fixed that one specific trigger (integer overflow on fractional quantity),
|
||||
but the underlying architecture — no per-item isolation, no partial-progress
|
||||
continuation — is unchanged, so any *other* new data shape PAX8 returns in
|
||||
the future will reproduce the same silent truncation.
|
||||
**Fix:** Wrap the per-invoice (and per-item) body in its own try/catch that
|
||||
logs and `continue`s past a single bad record, accumulating a
|
||||
best-effort partial result instead of aborting the whole run. At minimum,
|
||||
run tombstoning in a `finally`-equivalent step so a mid-loop failure doesn't
|
||||
also suppress reconciliation of the records that *did* succeed.
|
||||
|
||||
### WR-02: `matchPax8Companies()`'s fixed `limit=1000` + `ORDER BY name` can permanently starve later companies once flagged rows accumulate
|
||||
|
||||
**File:** `lib/services/pax8-company-matcher.ts:184-214`, called with no `limit` override from `lib/services/pax8-sync-service.ts:522`
|
||||
**Issue:** The eligibility query is `ORDER BY name LIMIT $1` (default 1000).
|
||||
Companies that end up flagged ambiguous/no-candidate remain "eligible"
|
||||
indefinitely (their review row's `resolved_at` stays NULL until a human acts
|
||||
on it), so they keep occupying slots in every subsequent run's top-1000 by
|
||||
name. If the number of permanently-unresolved review rows plus not-yet-matched
|
||||
companies ever exceeds 1000 (plausible as the PAX8/Autotask company universe
|
||||
grows), companies sorting alphabetically after that point will never be
|
||||
scored by an automatic sync, with no visible symptom besides an
|
||||
ever-growing "eligible but never reached" tail. At the current scale (118
|
||||
companies) this is inert, but there's no safety valve if it grows.
|
||||
**Fix:** Either raise/parametrize the limit generously relative to expected
|
||||
company-count growth, or replace the `ORDER BY name` cursor with something
|
||||
that rotates fairness across runs (e.g., `ORDER BY COALESCE(matched_at,
|
||||
'epoch') ASC` so already-attempted-but-still-open rows sink to the back of
|
||||
the queue instead of a static alphabetical order).
|
||||
|
||||
### WR-03: `Pax8Client.getToken()` does not validate the token response shape
|
||||
|
||||
**File:** `lib/services/pax8-client.ts:53-56`
|
||||
**Issue:**
|
||||
```ts
|
||||
const data = await res.json();
|
||||
this.accessToken = data.access_token;
|
||||
this.tokenExpiry = Date.now() + data.expires_in * 1000;
|
||||
return this.accessToken!;
|
||||
```
|
||||
If PAX8 ever returns a 200 with an unexpected body (e.g. a proxy/CDN error
|
||||
page reshaped as JSON, or a future API version renaming the field), `res.ok`
|
||||
is still `true`, so the `!res.ok` guard never fires. `data.access_token`
|
||||
would be `undefined`, `this.accessToken` would be set to `undefined`, and the
|
||||
non-null assertion (`!`) would suppress the type error — every subsequent
|
||||
call would then send `Authorization: Bearer undefined` and fail with a
|
||||
confusing 401 from the data endpoint instead of a clear "malformed token
|
||||
response" error from `getToken()` itself. `expires_in * 1000` would also
|
||||
produce `NaN` for `tokenExpiry`, and `Date.now() < NaN - 60000` is always
|
||||
`false`, so it would (correctly, if accidentally) refetch every time — but
|
||||
only by luck of `NaN` comparisons, not by validation.
|
||||
**Fix:**
|
||||
```ts
|
||||
const data = await res.json();
|
||||
if (!data.access_token || typeof data.expires_in !== 'number') {
|
||||
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;
|
||||
```
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: `flaggedAmbiguous` metric conflates "true tie" with "single low-confidence candidate, no tie at all"
|
||||
|
||||
**File:** `lib/services/pax8-company-matcher.ts:216-236`, `decide()` at 91-99
|
||||
**Issue:** `decide()` returns `{ kind: 'review', top3 }` both for genuine
|
||||
near-ties (D-02) and for a lone candidate that simply scores below
|
||||
`AUTO_LINK_THRESHOLD` with no second candidate at all
|
||||
(`pax8-company-matcher.test.ts`'s "review (below threshold)" case: a single
|
||||
0.80 candidate). Both land in `result.flaggedAmbiguous` in the caller
|
||||
(`decision.top3.length === 0 ? flaggedNoCandidate++ : flaggedAmbiguous++`).
|
||||
Operators reading `Pax8CompanyMatchResult`/the `[Pax8Match]` log line would
|
||||
reasonably read "ambiguous" as "multiple plausible candidates," which isn't
|
||||
true for the low-confidence-single-candidate case.
|
||||
**Fix:** Track a third bucket (`flaggedLowConfidence`) or rename to something
|
||||
scope-neutral like `flaggedReview`, and only call out true ties separately
|
||||
if that distinction matters operationally.
|
||||
|
||||
### IN-02: `CANDIDATE_FLOOR` is interpolated into the SQL string rather than bound as a parameter
|
||||
|
||||
**File:** `lib/services/pax8-company-matcher.ts:71-85`
|
||||
**Issue:** The file's own doc comment states "The PAX8 name is always bound
|
||||
as $1 — never string-interpolated," yet the very same query interpolates
|
||||
`CANDIDATE_FLOOR` directly: `similarity($1, company_name) >
|
||||
${CANDIDATE_FLOOR}`. Harmless today since `CANDIDATE_FLOOR` is a hardcoded
|
||||
module constant (never derived from user/request input), but it's an
|
||||
inconsistent pattern next to a comment explicitly calling out injection
|
||||
discipline, and a future edit that makes this threshold configurable (e.g.
|
||||
via an admin setting) could reintroduce a real injection vector by copying
|
||||
this pattern.
|
||||
**Fix:** Bind as a second parameter: `similarity($1, company_name) > $2`,
|
||||
passing `[pax8Name, CANDIDATE_FLOOR]`.
|
||||
|
||||
### IN-03: `Pax8InvoiceItem.forCompanyId` and `.companyName` are typed but never read
|
||||
|
||||
**File:** `lib/types/pax8.ts:81-109`, `lib/services/pax8-sync-service.ts:452-471`
|
||||
**Issue:** The sync service maps `item.companyId` to `pax8_company_id`
|
||||
(`lib/services/pax8-sync-service.ts:461`), but the type also declares a
|
||||
separate `forCompanyId: string | null` field that is never referenced
|
||||
anywhere in the reviewed files. Given the header-vs-item company mapping was
|
||||
specifically identified as a live-verification risk (12-RESEARCH.md Pitfall
|
||||
1/2, per the file's own comments), having two similarly-named,
|
||||
unreconciled candidate fields for "the customer this line item belongs to"
|
||||
is worth a second look — confirm `companyId` (not `forCompanyId`) is
|
||||
definitively the correct field for every observed item type before this
|
||||
scales past the spot-checked sample.
|
||||
**Fix:** Either remove `forCompanyId` from the type if it's confirmed
|
||||
irrelevant, or add a one-line comment recording why `companyId` (not
|
||||
`forCompanyId`) was chosen, matching the documentation rigor already applied
|
||||
elsewhere in this file.
|
||||
|
||||
### IN-04: Redundant type assertion on `p.category`
|
||||
|
||||
**File:** `lib/services/pax8-sync-service.ts:343`
|
||||
**Issue:** `(p.category as string | null) ?? p.vendorName ?? null` — `
|
||||
Pax8Product.category` is already declared as `string | null` in
|
||||
`lib/types/pax8.ts:58`, making the `as string | null` cast a no-op.
|
||||
**Fix:** Drop the assertion: `p.category ?? p.vendorName ?? null`.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-07-11T00:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Loading…
Add table
Add a link
Reference in a new issue