Repository navigation
Portfolio upgrade: reconciliation evidence and reliability - #1
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1eaa7fc8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,13 @@ | |||
| \set ON_ERROR_STOP on | |||
| \copy processors FROM 'sql/seed/processors.csv' WITH (FORMAT csv, HEADER true) | |||
There was a problem hiding this comment.
Reset identity sequences after loading explicit seed IDs
The seed CSVs provide explicit IDs to every identity-backed table, but COPY does not advance the associated PostgreSQL sequences. After the documented schema/seed setup, the first ordinary insert without an ID (for example a new raw_events, recon_runs, or qb_sync_jobs row) therefore generates ID 1 and collides with the seeded row. Advance each affected sequence to the table maximum after loading the CSVs.
Useful? React with 👍 / 👎.
| if len(valid_groups) == 1: | ||
| group = valid_groups[0] |
There was a problem hiding this comment.
Treat split groups as ambiguous across all target records
A group is accepted when it is unique only for the current right record, without checking whether the same left group also reconciles another right record. For example, left amounts 40 and 60 with two separate right amounts of 100 produce an arbitrary match to whichever right ID sorts first and an exception for the other, even though neither target is distinguishable. This is a false financial match; split candidates need global collision/ambiguity resolution before any group is committed.
Useful? React with 👍 / 👎.
| stats JSONB NOT NULL DEFAULT '{}'::jsonb, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| CHECK (time_window_end >= time_window_start), | ||
| UNIQUE (input_fingerprint) |
There was a problem hiding this comment.
Make reconciliation-run creation replay-safe
The new unique fingerprint constraint conflicts with the Create Recon Run node in n8n/workflows/06_reconcile_match_engine.json, which still performs an unconditional insert. If a run is retried after its run row was created but before matches or exceptions finished writing, the retry stops on this constraint and can never resume the partial run. The workflow should upsert or retrieve the existing run by fingerprint before continuing with its idempotent child writes.
Useful? React with 👍 / 👎.
| if candidate.left_id in matched_left or candidate.right_id in matched_right: | ||
| continue |
There was a problem hiding this comment.
Try the next eligible candidate after a preferred target is taken
When two left records prefer the same right record and their proposal scores differ by more than ambiguity_delta, the higher-scoring proposal claims the target and the other proposal is simply skipped here. Even if the skipped left record has a second candidate above minimum_score, that candidate is never considered, so a valid two-pair assignment becomes one match plus two discrepancies. Resolve the one-to-one assignment globally or retry each contender's remaining ranked candidates.
Useful? React with 👍 / 👎.
| "reference": record.reference or "", | ||
| } |
There was a problem hiding this comment.
Include partial-refund inputs in the run fingerprint
The serialized fingerprint omits original_gross_amount, although _match_classification uses that field to decide between partial_refund and refund_or_reversal. Consequently, otherwise identical refund inputs with and without an original gross amount produce different reconciliation output but the same fingerprint, causing the unique recon_runs.input_fingerprint constraint to conflate semantically different runs. Include every input field that can affect matching or classification.
Useful? React with 👍 / 👎.
Summary
This PR upgrades the repository into a tested financial-systems reference implementation while preserving explicit evidence labels. It does not claim a live client deployment, business savings, production throughput, or accounting acceptance.
Engineering changes
Verified evidence
Remaining production debt