diff --git a/.planning/phases/12-orders-invoices-company-matching/12-REVIEW.md b/.planning/phases/12-orders-invoices-company-matching/12-REVIEW.md new file mode 100644 index 0000000..0e75db2 --- /dev/null +++ b/.planning/phases/12-orders-invoices-company-matching/12-REVIEW.md @@ -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 + + -{record.records_deleted} + +``` +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_