Reconciliation pipeline — ranked fix plan
1. Ranked defects
| # | Defect | file:line | Money impact | Fix size |
|---|---|---|---|---|
| 1 | RPC over-applies past invoice total on stale pool (race with manual payment / concurrent confirm) | src/db/migrations/0018_check_reconciliation.sql:82 | Permanent double-pay; check money trapped on a paid invoice, no reversal | S (migration) |
| 2 | Confirm recomputes allocation with no binding to the preview the owner approved | src/server/actions/checks.ts:503 | Money applied to invoices the owner never saw; permanent | M |
| 3 | Void/reissue allowed on partially-paid invoices — payment stranded on void row, facility re-billed in full | src/server/actions/invoiceManage.ts:152 + 0097:164 | Facility over-billed by the paid amount; applied dollars point at void row | S (migration) |
| 4 | Owner-typed amount never checked against OCR check amount | src/server/actions/checks.ts:501 | Order-of-magnitude fat-finger marks a whole pool paid against one deposit | S/M |
| 5 | correctInvoice edits totals on paid/partial invoices without reconciling paid/status | src/server/actions/invoiceManage.ts:258 | Invisible overcollection or uncollectable balance, permanent desync | M |
| 6 | Ref-named invoice forfeited when line amount > outstanding — line rerouted to a neighbour | src/lib/reconcile/target.ts:174 | Wrong invoice paid; named invoice goes overdue/dunned | S (deletion) |
| 7 | Line with a ref matching nothing still claims a different invoice by date/amount | src/lib/reconcile/target.ts:141 | Duplicate payment cascades across sibling facilities | S |
| 8 | Identical sibling invoices tie in PASS 2, resolved by created_at, silently | src/lib/reconcile/target.ts:193 | Wrong facility's invoice marked paid; common case under Mon–Sun billing, not an edge | S |
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-invoiceSELECT ... 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 fieldexpectedApplications=JSON.stringify(result.applications.map(a => ({ invoiceId, appliedCents })))inside the confirm form.src/types/schemas/check.ts: extendconfirmReconciliationSchema— JSON string parsed toz.array(z.object({ invoiceId: z.string().uuid(), appliedCents: z.number().int().positive() })).src/server/actions/checks.ts(confirmReconciliation): after computingresult.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 theremittance_linesupdate 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
expectedApplicationsnaming 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: recreatevoid_invoice(0097 version); in the existing FOR UPDATE select also readamount_paid_cents; after the 'paid' guard addif v_paid > 0 then raise exception 'Invoice has payments applied and cannot be voided'; end if;. Guard onamount_paid_cents, notstatus='partial'— covers Wise/manual money too. One DB guard fixesvoidInvoiceById,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 addif (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
reissueInvoiceand the new error mapping. Hiding the buttons for 'partial' inInvoiceActions.tsxis optional polish, skip unless free.
Fix 4 — OCR amount override gate
src/server/actions/checks.ts: addocr_extracted_centsto 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, andformData.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 + aconfirmAmountDifferscheckbox 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')); rejecttotalCents < paidwith 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 < paidis 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: deleteline.amountCents <= outstandingCentsfrom the PASS 1 predicate (:174); change :140 to return the score-100 match onrefMatchalone.invoice_numberis globally unique — the gate can only reject the one correct candidate, never disambiguate;allocateCheckalready 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 basisinvoice_ref, sibling untouched. Fails today (sibling steals it), passes after.
Fix 7 — contradicting ref disqualifies the candidate
src/lib/reconcile/target.ts(scoreCandidate): hoistconst lineRef = normRef(line.invoiceRef); before scoring:if (lineRef !== null && lineRef !== normRef(inv.invoiceNumber)) return null;(refMatchsimplifies tolineRef !== 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
#6990pool with a spare candidate named by no line → assert the absent-ref line'sinvoiceIdis still null. Fails today, passes after.
Fix 8 — cross-facility ties go unmatched
src/lib/reconcile/target.tsPASS 2 loop:tiedAcrossFacilitiesboolean; reset whenbestis replaced; set when a candidate ties on (score, dist, outstandingCents) with a different facilityId; claim onlyif (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', emptyorderedInvoiceIds. 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
- 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.
- Overpayment/leftover (gap c): when a confirmed check exceeds the outstanding pool, where should the excess live — a payer credit balance, a flagged
check_paymentsremainder column, or reject the confirm entirely? Fix 1+4 shrink the window but the leftover from a legitimately oversized check still evaporates. - 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.) - 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.)
- Soft-delete gaps (a)/(b): given as known — but the same "money on it" rule applies: should
deleteInvoicerefuse whenamount_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.
| PR | Contents | Migration | Why this grouping |
|---|---|---|---|
| PR 1 | Fixes 6 + 7 + 8 (all target.ts + tests) | none | One 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 2 | Fix 1 (0099) + Fix 2 | 0099 | Both 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 3 | Fix 4 | none | Small, 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 4 | Fix 3 (0100) + error mapping | 0100 | Different domain surface (invoice lifecycle, not check confirm). Must be isolated from PR 2 so each migration is individually revertible. |
| PR 5 | Fix 5 | none | Same 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:
- 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.
- 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.
- Use raw remittance lines only — date/ref/amount as OCR'd.
mergeLineMatchesoverwrote 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. - Reconstruct the payer's facility set as of confirm time if
billing_payersmappings 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):
correctInvoiceedits: 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
recordInvoicePaymentand 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_amount→unmatched) 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-XXXXlinkage) to classify these, or they show as false unmatched. - Fix-1-class races already baked in: any invoice where
amount_paid_cents > total_amount_centstoday 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.