Skip to main content

Reconciliation pipeline — ranked fix plan

1. Ranked defects

#Defectfile:lineMoney impactFix size
1RPC over-applies past invoice total on stale pool (race with manual payment / concurrent confirm)src/db/migrations/0018_check_reconciliation.sql:82Permanent double-pay; check money trapped on a paid invoice, no reversalS (migration)
2Confirm recomputes allocation with no binding to the preview the owner approvedsrc/server/actions/checks.ts:503Money applied to invoices the owner never saw; permanentM
3Void/reissue allowed on partially-paid invoices — payment stranded on void row, facility re-billed in fullsrc/server/actions/invoiceManage.ts:152 + 0097:164Facility over-billed by the paid amount; applied dollars point at void rowS (migration)
4Owner-typed amount never checked against OCR check amountsrc/server/actions/checks.ts:501Order-of-magnitude fat-finger marks a whole pool paid against one depositS/M
5correctInvoice edits totals on paid/partial invoices without reconciling paid/statussrc/server/actions/invoiceManage.ts:258Invisible overcollection or uncollectable balance, permanent desyncM
6Ref-named invoice forfeited when line amount > outstanding — line rerouted to a neighboursrc/lib/reconcile/target.ts:174Wrong invoice paid; named invoice goes overdue/dunnedS (deletion)
7Line with a ref matching nothing still claims a different invoice by date/amountsrc/lib/reconcile/target.ts:141Duplicate payment cascades across sibling facilitiesS
8Identical sibling invoices tie in PASS 2, resolved by created_at, silentlysrc/lib/reconcile/target.ts:193Wrong facility's invoice marked paid; common case under Mon–Sun billing, not an edgeS

Ranking is by blast radius × silence. 1–5 are money-loss; 6–8 are wrong-allocation but equally permanent given the missing reversal path. Nothing here is not worth fixing — the only cuts are inside the fixes (see per-item notes).

2. Concrete changes

All fixes below use the skeptic's corrected versions, which are correctly sized. Shared-choke-point rule applied: fixes 1 and 3 go in the SQL functions so every caller (present and future) is covered by one guard.

Fix 1 — cap check applied at outstanding (RPC raise) — migration 0099

  • New file src/db/migrations/0099_confirm_check_outstanding_guard.sql: create or replace function public.confirm_check_reconciliation(...) identical to 0018 except, after the per-invoice SELECT ... FOR UPDATE, add: if v_applied > greatest(v_total - v_paid, 0) then raise exception 'Invoice % balance changed since reconciliation was computed; re-run reconciliation', v_invoice_id; end if; One transaction — the raise aborts the whole confirm atomically. Do not cap-and-continue (function returns void; silent capping strands money exactly like gap (c)).
  • src/server/actions/checks.ts: map the new raise text in the error handler next to the existing "already been reconciled" case so the operator sees "re-run", not the generic failure.
  • Test that fails today, passes after: none runnable without a DB — the TS tests only cover allocation math. Minimum honest check: a Vitest spec on the error-mapping branch (mock the RPC rejecting with the new message, assert the mapped error), plus manual SQL verification in a branch DB. Flag this in the PR.

Fix 2 — bind confirm to the reviewed allocation

  • src/components/admin/ReconcileCheck.tsx: add one hidden field expectedApplications = JSON.stringify(result.applications.map(a => ({ invoiceId, appliedCents }))) inside the confirm form.
  • src/types/schemas/check.ts: extend confirmReconciliationSchema — JSON string parsed to z.array(z.object({ invoiceId: z.string().uuid(), appliedCents: z.number().int().positive() })).
  • src/server/actions/checks.ts (confirmReconciliation): after computing result.applications (:507), compare against the posted pairs as sorted sets; on mismatch return { ok:false, error: 'The outstanding invoices changed since this page loaded. Refresh and re-review the allocation before confirming.' }. Also move the remittance_lines update at :514 to after the comparison (or after the RPC succeeds) — today it mutates audit state even when the RPC fails, which is a second bug this fix removes for free.
  • Test: confirm action with expectedApplications naming invoice X while the recomputed pool yields invoice Y → returns the refresh error and the RPC is never called. Fails today (no such field exists); passes after.

Fix 3 — forbid voiding an invoice with money on it — migration 0100

  • New file src/db/migrations/0100_void_invoice_paid_guard.sql: recreate void_invoice (0097 version); in the existing FOR UPDATE select also read amount_paid_cents; after the 'paid' guard add if v_paid > 0 then raise exception 'Invoice has payments applied and cannot be voided'; end if;. Guard on amount_paid_cents, not status='partial' — covers Wise/manual money too. One DB guard fixes voidInvoiceById, reissueInvoice, and any future caller.
  • src/server/actions/invoiceManage.ts: map the new raise text in both error handlers (:75 and :183 currently regex only /paid invoice/i), and add if (inv.status === 'partial') return { ok:false, error: "This invoice has a payment applied — it can't be reissued until the payment is unapplied." } at :153 so reissue fails before preflight work.
  • Do not build a payment-transfer/unapply flow here — the guard makes the loss impossible; the unapply feature is the open item.
  • Test: same caveat as Fix 1 (SQL guard); TS test asserts the 'partial' early-return in reissueInvoice and the new error mapping. Hiding the buttons for 'partial' in InvoiceActions.tsx is optional polish, skip unless free.

Fix 4 — OCR amount override gate

  • src/server/actions/checks.ts: add ocr_extracted_cents to the select at :484 (without this the guard cannot run — the original proposal forgot it). After :501: if non-null, amountCents !== check.ocr_extracted_cents, and formData.get('confirmAmountDiffers') !== 'true' → return error showing the OCR amount and asking to verify against the check image. Exact-cents equality, no tolerance — the field is prefilled from OCR, so any difference is a deliberate edit; a 1% tolerance constant is added surface that weakens the guard.
  • src/components/admin/ReconcileCheck.tsx: when typed amount ≠ ocrAmountCents, render a warning showing both amounts + a confirmAmountDiffers checkbox in the confirm form; disable submit until checked.
  • No migration.
  • Test: confirm with amountCents ≠ ocr and no override flag → rejected; with flag → proceeds. Fails today, passes after.

Fix 5 — correctInvoice must respect amount_paid

  • src/server/actions/invoiceManage.ts (correctInvoice): swap the lines-only preread for one select on the invoice (status, amount_paid_cents, sent_at, paid_at, company-scoped, .neq('status','void')); reject totalCents < paid with an explicit error; in the same update recompute with the RPC's own rule: status: paid >= totalCents ? 'paid' : paid > 0 ? 'partial' : inv.status, paid_at: paid >= totalCents ? (inv.paid_at ?? now) : null; and fix the sent_at bug: sent_at: inv.status === 'draft' ? inv.sent_at : issueDate.... Raising a 'paid' total flips it to 'partial' so the balance re-enters the pool — that's the point.
  • No overpayment-credit path — rejecting totalCents < paid is sufficient; anything more belongs with the unapply feature.
  • Test: correct a 'partial' invoice (paid 500000) down to 400000 → rejected. Fails today (update succeeds), passes after.

Fix 6 — drop the amount gate from ref matches (deletion)

  • src/lib/reconcile/target.ts: delete line.amountCents <= outstandingCents from the PASS 1 predicate (:174); change :140 to return the score-100 match on refMatch alone. invoice_number is globally unique — the gate can only reject the one correct candidate, never disambiguate; allocateCheck already caps applied at outstanding, so no over-credit is possible.
  • Test (tests/lib/reconcile/target.test.ts): ref line with amount 100000 against a candidate with outstanding 60000 plus a same-amount in-window sibling → ref-named invoice claimed with basis invoice_ref, sibling untouched. Fails today (sibling steals it), passes after.

Fix 7 — contradicting ref disqualifies the candidate

  • src/lib/reconcile/target.ts (scoreCandidate): hoist const lineRef = normRef(line.invoiceRef); before scoring: if (lineRef !== null && lineRef !== normRef(inv.invoiceNumber)) return null; (refMatch simplifies to lineRef !== null). One place, covers PASS 2 and any future caller. A named line matches its invoice or stays unmatched — the amount then flows to the oldest-first fallback (single-facility) or held-for-review (multi-facility), instead of being guessed onto a neighbour.
  • Test: extend the existing #6990 pool with a spare candidate named by no line → assert the absent-ref line's invoiceId is still null. Fails today, passes after.

Fix 8 — cross-facility ties go unmatched

  • src/lib/reconcile/target.ts PASS 2 loop: tiedAcrossFacilities boolean; reset when best is replaced; set when a candidate ties on (score, dist, outstandingCents) with a different facilityId; claim only if (best && !tiedAcrossFacilities). Same-facility ties stay arbitrary — right payer, harmless. No new 'ambiguous' basis; 'unmatched' already renders in review.
  • Test: two sibling facilities, identical same-week 60000c invoices, ref-less date+amount line, singleFacility:false → basis 'unmatched', empty orderedInvoiceIds. Fails today, passes after.

Migrations: only fixes 1 and 3 (0099, 0100 — 0098 is taken by the uncommitted audit migration). Both are create or replace function, no data change, trivially revertible by re-applying the prior definition.

3. DO NOT FIX YET — owner decisions needed

  1. Unapply/reversal for confirmed checks: fixes 3 and 5 now return "unapply the payment first" errors for a flow that does not exist. Do you want an unapply feature (delete application rows + walk back amount_paid/status, audit-logged), and who is allowed to use it? Everything above only prevents new damage; it can't undo old damage.
  2. Overpayment/leftover (gap c): when a confirmed check exceeds the outstanding pool, where should the excess live — a payer credit balance, a flagged check_payments remainder column, or reject the confirm entirely? Fix 1+4 shrink the window but the leftover from a legitimately oversized check still evaporates.
  3. Multi-facility held-for-review (gap d): should confirm be blocked while remainderHeldForReview > 0, or is partial confirm with a visible held amount acceptable? (Blocking changes operator workflow — your call.)
  4. Foreign ref schemes: fix 7's known trade-off — a payer whose stub refs never match our invoice numbers degrades from date-matching to unmatched/manual review. Acceptable, or do you want a per-payer "ignore refs" flag? (I'd ship without the flag; manual review beats possibly-wrong money.)
  5. Soft-delete gaps (a)/(b): given as known — but the same "money on it" rule applies: should deleteInvoice refuse when amount_paid_cents > 0, mirroring fix 3? One-line guard if yes; wants your confirmation because it changes existing delete behavior.

Not worth doing: hiding void/correct buttons for 'partial' in InvoiceActions.tsx (server guard is the fix; UI polish only), a diff-and-re-confirm UI for fix 2 (refresh-and-re-review is enough), a 1% tolerance on fix 4, and a CHECK constraint relating amount_paid to total (fix 5's guard + RPC raise cover the live paths; a constraint would fail on existing bad rows before the backfill audit runs).

4. Shipping order

Every PR touches payments/invoicing → each needs the security-reviewer subagent + human approval per CLAUDE.md. Order chosen so pure-TS, no-migration fixes land first and the matcher is correct before the historical backfill runs.

PRContentsMigrationWhy this grouping
PR 1Fixes 6 + 7 + 8 (all target.ts + tests)noneOne file, one test file, pure functions, reviewed as one coherent matcher-semantics change. Must land before the backfill re-target runs or the audit diffs against a still-buggy matcher.
PR 2Fix 1 (0099) + Fix 20099Both guard the same race (stale pool at confirm); fix 2's set-comparison and fix 1's raise are the app-level and DB-level halves of one invariant. Same files.
PR 3Fix 4noneSmall, independent, isolate so the UX (override checkbox) can be reviewed on its own. Could fold into PR 2 if review bandwidth is tight — same files — but the override checkbox is a product decision worth its own approval.
PR 4Fix 3 (0100) + error mapping0100Different domain surface (invoice lifecycle, not check confirm). Must be isolated from PR 2 so each migration is individually revertible.
PR 5Fix 5noneSame file as PR 4 (invoiceManage.ts) — ship after to avoid conflicts; independent behavior change (correct dialog), own approval.

Do not batch PR 2 and PR 4: two SQL-function replacements in one PR means one revert takes out both guards.

5. Historical backfill (re-target pre-ref-first checks) — report only

What it must do: for each check_payments row confirmed before commit 8a39d34 (and ideally before PR 1 lands — use the deploy timestamp, not the commit date), re-run targeting with the post-PR-1 matcher against a reconstructed pool, diff the resulting allocation against the actual check_payment_applications rows, and report per-check: matched-as-recorded / would-differ (with both allocations side by side) / cannot-reconstruct. Never write.

What it must check to be trustworthy:

  1. Pool reconstruction, not current state. The candidate pool must be "outstanding as of that check's confirm time": start from invoice totals, subtract only applications and payments timestamped before the confirm. Using today's pool is the #1 way to produce garbage — invoices paid/voided/reissued/corrected since then would create false diffs everywhere.
  2. Include soft-deleted and voided invoices in the reconstructed pool (gap a): the old matcher saw them, so excluding them changes the diff for the wrong reason. Flag any check whose recorded application points at a now-deleted/void invoice — those are findings, not noise.
  3. Use raw remittance lines only — date/ref/amount as OCR'd. mergeLineMatches overwrote match metadata at confirm time with confirm-time results (fix 2's finding), so any stored basis/invoiceId on the lines describes the old matcher's opinion and must be ignored as input.
  4. Reconstruct the payer's facility set as of confirm time if billing_payers mappings have been edited since; if mapping history isn't kept, flag those checks as cannot-reconstruct rather than diffing against today's set.

What makes the output untrustworthy (each must be flagged per-check, not papered over):

  • correctInvoice edits: totals changed since confirm with no history table → outstanding-at-confirm is unrecoverable for those invoices. Any check whose pool touched a corrected invoice is cannot-reconstruct.
  • Missing/unauditable timestamps: manual recordInvoicePayment and Wise applications whose applied-at time can't be ordered against the check confirm make the pool snapshot ambiguous.
  • OCR quality: refs the vision model garbled will now hard-fail matching (post-fix-7 semantics) where the greedy matcher "succeeded" — a diff there may mean the old allocation was right by luck. Report basis changes (date_amountunmatched) separately from target changes (invoice A → invoice B); only the latter are candidate misallocations.
  • Reissued invoices: a stub naming a pre-reissue invoice number can't match its replacement; the report needs the reissue chain (the Replaces INV-XXXX linkage) to classify these, or they show as false unmatched.
  • Fix-1-class races already baked in: any invoice where amount_paid_cents > total_amount_cents today is direct evidence of the over-apply bug — report those unconditionally, no reconstruction needed; simplest and highest-confidence signal in the whole audit.

Output shape: one row per check — check id, confirm date, recorded allocation, re-targeted allocation, diff class (same / different-target / now-unmatched / cannot-reconstruct + reason). All correction is a human decision downstream, gated on the (missing) unapply path — which is why owner question #1 comes first.