Skip to content

feat: persist durable reconciliation approval evidence - #30

Closed
seonghobae wants to merge 10 commits into
fix/reconciliation-multi-match-conservationfrom
feat/reconciliation-approval-evidence
Closed

feat: persist durable reconciliation approval evidence#30
seonghobae wants to merge 10 commits into
fix/reconciliation-multi-match-conservationfrom
feat/reconciliation-approval-evidence

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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 head fb8223bd82d51ed5337feae4c0dfa114ce964971 and carries the durable 0016_reconciliation_approval_evidence.sql slice 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 7f81376aac4bc10fdcd4348d8d595f018108edb8 on an obsolete recorded base 5f237f8dda7c6269504e8cc818059f7012767ca5. 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.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae seonghobae changed the title feat: require durable reconciliation approval evidence feat: persist durable reconciliation approval evidence Aug 27, 2026

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant