From e92268f38c5f0ec7594f9046d218ed8fcafb0be1 Mon Sep 17 00:00:00 2001 From: lorentz Date: Sat, 11 Jul 2026 07:25:58 -0400 Subject: [PATCH] docs(12): add code review fix report --- .../12-REVIEW-FIX.md | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 .planning/phases/12-orders-invoices-company-matching/12-REVIEW-FIX.md diff --git a/.planning/phases/12-orders-invoices-company-matching/12-REVIEW-FIX.md b/.planning/phases/12-orders-invoices-company-matching/12-REVIEW-FIX.md new file mode 100644 index 0000000..cff9159 --- /dev/null +++ b/.planning/phases/12-orders-invoices-company-matching/12-REVIEW-FIX.md @@ -0,0 +1,63 @@ +--- +phase: 12-orders-invoices-company-matching +fixed_at: 2026-07-11T11:30:00Z +review_path: .planning/phases/12-orders-invoices-company-matching/12-REVIEW.md +iteration: 1 +findings_in_scope: 5 +fixed: 5 +skipped: 0 +status: all_fixed +--- + +# Phase 12: Code Review Fix Report + +**Fixed at:** 2026-07-11T11:30:00Z +**Source review:** .planning/phases/12-orders-invoices-company-matching/12-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope: 5 (2 Critical, 3 Warning; Info findings excluded per fix_scope) +- Fixed: 5 +- Skipped: 0 + +## Fixed Issues + +### CR-01: `partner_cost` / `partner_cost_total` columns silently truncate real PAX8 cost precision + +**Files modified:** `migrations/095_pax8_order_items_partner_cost_numeric.sql` (new) +**Commit:** `591fc5c` +**Applied fix:** Added new migration 095 (093 is a committed migration and was not edited) widening `pax8_order_items.partner_cost` and `partner_cost_total` from `NUMERIC(12,2)` to `NUMERIC(14,4)`, matching the precision already used for `quantity` (migration 094). Applied directly to the running dev Postgres container (`pulse-postgres`, per project convention for existing volumes) via `docker exec -i pulse-postgres psql -U pulse_user -d pulse_autotask < migrations/095_....sql`, and verified via `\d pax8_order_items` that both columns now report `numeric(14,4)`. + +### CR-02: `company_matches` "review" counts fabricate deletion counts in the Sync History UI + +**Files modified:** `lib/services/pax8-sync-service.ts`, `lib/services/pax8-sync-service.test.ts`, `lib/types/pax8.ts` +**Commit:** `bddf612` +**Applied fix:** Added a dedicated `flaggedForReview?: number` field to `Pax8EntitySyncResult`. `syncCompanyMatches()` now reports `tombstoned: 0` (no soft-deletes are ever performed by matching) and surfaces the ambiguous/no-candidate review count separately via `flaggedForReview`, plus a `[Pax8Sync] Company match: N flagged for manual review (...)` log line. `fullSync()`'s summary log line now also reports the total flagged-for-review count. Updated the existing unit test (`syncCompanyMatches delegates to matchPax8Companies...`) which previously asserted the buggy `tombstoned === 1` behavior — it now asserts `tombstoned === 0` and `flaggedForReview === 1`. All 5 tests in `pax8-sync-service.test.ts` pass. + +### WR-01: A single failing record aborts the entire remaining batch for that sync run + +**Files modified:** `lib/services/pax8-sync-service.ts` +**Commit:** `cb8ae85` +**Applied fix:** Wrapped the per-row upsert body in its own try/catch in all four entity syncs (`syncCompanies`, `syncSubscriptions`, `syncProducts`, `syncOrders`), including a separate per-item try/catch nested inside the per-invoice try/catch in `syncOrders` (invoice header failure and item-fetch/item-upsert failure are isolated independently). Each failed row is logged (`console.error`, non-fatal) and the loop continues; the id is still recorded as "seen" so a transient write failure doesn't get misread as "gone from PAX8" by the tombstone step. Tombstoning now always runs over whatever was actually seen this run, regardless of mid-loop row failures. Each entity result now reports `success: false` with a summary `error` message (count of failures + first error) when any row failed, while still returning the partial `upserted`/`tombstoned` counts from the rows that did succeed — previously a single throw discarded all counts for the entire method and skipped tombstoning entirely. All 5 tests in `pax8-sync-service.test.ts` pass. + +### WR-02: `matchPax8Companies()`'s fixed `limit=1000` + `ORDER BY name` can permanently starve later companies + +**Files modified:** `lib/services/pax8-company-matcher.ts` +**Commit:** `76a652d` +**Applied fix:** Applied both halves of the review's suggested fix: (1) raised the default `limit` from 1000 to 10000 (generous headroom above the validated 118-company scale); (2) replaced the static `ORDER BY name` cursor with `ORDER BY COALESCE(r.detected_at, c.matched_at, 'epoch'::timestamptz) ASC`, joining the open (`resolved_at IS NULL`) `pax8_company_match_review` row so companies are re-scored in order of "least recently considered" rather than alphabetically — a permanently-open review row now sinks behind any row that hasn't been reconsidered as recently, instead of camping in the same alphabetical slot forever. All 6 tests in `pax8-company-matcher.test.ts` pass. + +### WR-03: `Pax8Client.getToken()` does not validate the token response shape + +**Files modified:** `lib/services/pax8-client.ts` +**Commit:** `1bb2b8b` (plus follow-up `be6f07b` fixing a TS narrowing error the initial edit introduced — `npx tsc --noEmit` flagged `this.accessToken` as `string | null` on the return statement; fixed by capturing the validated token into a local `const token: string` and returning that instead of re-reading the mutable instance field) +**Applied fix:** Applied the review's suggested fix essentially verbatim: after parsing the token response body, throw `PAX8 token response missing access_token/expires_in` if `access_token` is falsy or `expires_in` isn't a number, before ever assigning to `this.accessToken`/`this.tokenExpiry`. This closes the silent-`undefined`-token / `NaN`-expiry failure mode described in the finding. All 12 tests in `pax8-client.test.ts` pass. + +## Skipped Issues + +None — all 5 in-scope findings were fixed. + +--- + +_Fixed: 2026-07-11T11:30:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_