feat: persist durable reconciliation approval evidence - #30
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review on 7de795a5cac40a77f36de5d3ebe95c59a9a7c92b: the new durable approval table correctly guards proposed -> approved, but the same accounting-control boundary is not enforced for rejection or for terminal-state reopening.
reconciliation_approval explicitly models both approval_decision_code IN ('approved','rejected'), and this PR states that a rejected approval is terminal for the match. However reconciliation_match_requires_approval() only checks NEW.match_status_code = 'approved'. A caller can therefore update a proposed match directly to rejected with no durable approver/purpose/source-hash evidence. After an approved or rejected decision, the same row can also be moved back to proposed because neither 0015 nor 0016 enforces a terminal match-state transition graph. That breaks Issue #6/#8's requirement that reviewed status transitions retain actor/purpose/evidence/effective/system-time provenance and lets a rejected decision cease to be terminal without creating a new reviewed match.
Please add real-PostgreSQL RED coverage before changing production: (1) status-only proposed -> rejected must fail; (2) a durable rejected approval for the same tenant/run/match enables exactly the rejected transition; (3) approved -> proposed and rejected -> proposed fail closed; (4) explicit approved|rejected -> superseded remains allowed so historical evidence can be retired without mutation; and (5) approval rows remain append-only and unique per match. The narrow GREEN should make reviewed terminal transitions database-owned and decision-code-consistent, not merely add application convention. Preserve the no-posting/no-close authority boundary and exact current migration installer contracts.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head accounting-control review on 7f81376aac4bc10fdcd4348d8d595f018108edb8: durable approval exists and terminal transitions are now decision-code-consistent, but the approval is not immutably bound to the exact allocation/candidate state it is supposed to approve.
reconciliation_approval.source_payload_hash is only format-checked. reconciliation_approval_insert_guard() requires the match to be proposed, but a proposed match can still receive additional append-only statement/journal allocation rows after the approval row is recorded. Later reconciliation_match_requires_approval() checks only that an approval row with the requested decision exists; it does not prove that the approval's source hash commits to the final candidate/allocation evidence. A controller can therefore approve evidence A, the proposal can append evidence B while still proposed, and the match can then enter approved using the stale approval fact. The resulting durable approval is real but does not attest to the state that was actually approved.
Please add a real-PostgreSQL RED before production changes: record a proposed match and its allocations, insert approval evidence, then append or otherwise extend allocation evidence while the match remains proposed and prove the subsequent proposed -> approved transition fails closed. Cover statement and journal allocation changes, and preserve the existing balanced/non-empty allocation invariant. The narrow GREEN should bind the approval to one immutable proposal snapshot—e.g. a database-owned canonical proposal/evidence digest or immutable proposal-version identity—and verify that binding at terminal transition. Do not accept caller-supplied hash format alone as authority, and do not solve this by making reconciliation approval capable of posting/reversing/closing journals. Because this PR is currently 10 commits ahead / 14 behind parent #29 with merge base 5f237f8..., keep it Draft and re-run this RED/GREEN after the non-destructive post-#29 restack; predecessor green evidence must not transfer.
Superseded duplicate — closed without merge
This stale Draft successor is superseded by PR #31 (
fix: bind reconciliation approval to database snapshot). PR #31 is based directly on the current PR #29 headfb8223bd82d51ed5337feae4c0dfa114ce964971and carries the durable0016_reconciliation_approval_evidence.sqlslice plus the missing database-owned reconciliation snapshot binding, immutable payload reference, upgrade fail-close, allocation freeze, terminal identity/evidence immutability, and current PostgreSQL regression coverage.PR #30 remains at exact head
7f81376aac4bc10fdcd4348d8d595f018108edb8on an obsolete recorded base5f237f8dda7c6269504e8cc818059f7012767ca5. Its historical PostgreSQL/coverage/package/security evidence is stale-stack evidence and is not transferred to #31.Closing this PR only removes the duplicate integration lane. No branch history is rewritten, no predecessor evidence is promoted, and no accounting authority changes. PR #29 remains the dependency root; PR #31 cannot integrate ahead of #29 through the protected default-branch path.