Skip to content

feat: add evidence-backed reconciliation completion command - #42

Closed
seonghobae wants to merge 26 commits into
fix/reconciliation-multi-match-conservationfrom
feat/reconciliation-run-reconciled-transition
Closed

feat: add evidence-backed reconciliation completion command#42
seonghobae wants to merge 26 commits into
fix/reconciliation-multi-match-conservationfrom
feat/reconciliation-run-reconciled-transition

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Stack position

This PR is intentionally stacked on open dependency-root PR #29: fix/reconciliation-multi-match-conservation@623e5bca48b036dcf1f4845b1f071fd7782c9fb2. Current exact head: 6ea56af77edf2e93b28cea9996db6e3780025dad. It must not merge to protected develop ahead of the parent. After #29 integrates, restack from that protected head and reacquire all exact-head evidence.

Product gap closed by this slice

Normal reconciliation-run creation produces an evaluating run, while authority-bearing close-package construction requires reconciled. Direct SQL status mutation is not a commercial owner-control path. This stack adds one named Reconciliation Completion command instead of a generic status editor.

Database and least-privilege authority

database/migrations/0020_reconciliation_completion_command.sql adds immutable tenant/run completion evidence containing the statement population hash, book population hash, approved-population hash, exact bridge-evidence hash, actor, purpose, tenant-scoped idempotency key and whole-command hash. The table is forced-RLS and immutable.

The migration creates accounting_reconciliation_completer as a separate NOLOGIN capability and reasserts NOLOGIN. Both completion-command insertion and the first transition into reconciled check pg_has_role(session_user, 'accounting_reconciliation_completer', 'MEMBER'). Caller fields, model output, GUCs and application SET ROLE are not authority. The capability is separate from accounting_closing_writer and receives only completion-table INSERT/SELECT, reconciliation_run.run_status_code column UPDATE and outbox INSERT mutation privileges. Deployment must also tenant-bind the authenticated login through the existing database-owned runtime binding.

The DB independently rejects completion unless the run is evaluating or review_required, no reconciliation exception is open, no match remains proposed, and immutable completion-command evidence exists before the run enters reconciled. Completion does not post/reverse journals, close a fiscal period, file tax, or alter accounting policy.

Exact-head authority hardening

Review of the new capability found a source-real privilege gap before integration: the column-level UPDATE (run_status_code) grant was narrow at the SQL privilege layer, but the transition trigger only special-cased NEW.run_status_code = 'reconciled'. A session holding accounting_reconciliation_completer could therefore have used direct SQL to manufacture another allowed status value such as review_required, not_reconciled, or superseded, contrary to ADR 0059's rule that those lifecycle edges require separately governed commands and evidence.

The repair followed RED→GREEN on this branch. tests/test_reconciliation_completion_contract_red.py first gained a failing contract requiring a target-state guard. Migration 0020 then changed so every changed target other than reconciled fails closed with reconciliation_completion_target_forbidden. The isolated real-PostgreSQL install test now reads the actual installed function definition with pg_get_functiondef(...) and verifies that the target restriction and diagnostic survive the public migration chain. ADR 0059 makes the state-machine boundary explicit, and docs/doctoring/2026-09-01-reconciliation-completion-status-authority.md records the causal defect, TDD repair, DDD interpretation and research basis.

This does not define review_required, not_reconciled, or superseded commands. It prevents the completion capability from silently becoming their authority. Future lifecycle commands must deliberately evolve the database state-machine guard with their own evidence and permissions.

Domain command

accept_reconciliation_completion() uses one PostgresPostingLedger._consistent_read_session() (REPEATABLE READ) for the tenant command lock, locked run, exception/match populations, current approved match/approval snapshots, #29's database-owned statement/book projection and exact bridge, immutable completion command, status transition and reconciliation_run.reconciled transactional-outbox evidence.

The caller does not supply target status, population identifiers, approval digests, or bridge money. Exact retry of the same key replays immutable command evidence; changed command evidence under that key raises IdempotencyConflictError. A second key cannot replace the tenant/run completion because the database owns one completion command per run. Public run IDs are restricted to one lowercase hyphenated canonical UUID spelling; brace, uppercase and compact aliases fail closed before database access.

Migration-chain repair

The historical persistence loader stopped at migration 0019. The exported production installer now fail-closes on a missing 0020 before any database work, applies the established 0001–0019 chain, then executes the checked-in completion migration and preserves any causal PostgreSQL failure.

tests/test_reconciliation_completion_install_contract.py proves that installer control flow with executable fakes. tests/test_postgres_reconciliation_completion_migration.py creates an isolated real PostgreSQL database with a non-super/non-bypass migration login, applies the public chain, then verifies forced RLS, the NOLOGIN/non-super/non-BYPASSRLS capability, all three completion triggers, exact minimum mutation privileges, and the installed target-state guard from PostgreSQL catalogs/function definition. docs/ARCHITECTURE.md, docs/OPERABILITY.md, and ADR 0059 list migration 0020 and its provisioning/revocation boundary.

Research / traceability

ADR 0059 and the doctoring note retain the existing PostgreSQL 18 / 2025 VLDB transaction-isolation basis and add a current peer-reviewed RBAC integrity reference: Logrippo (2025), Data flow security in role-based access control, Journal of Information Security and Applications. The research supports constraining the effective operation set of a role rather than treating a coarse underlying privilege as business authority; it does not replace the repository's PostgreSQL tests or grant accounting authority.

Deliberate transport split

The buyer-facing POST /reconciliation-completions stdlib HTTP route is a successor slice. It is not claimed as integrated here. Separating transport from the authority-bearing DB/domain command keeps the security-critical state transition independently reviewable. No generic run-status mutation route will be introduced. Purpose-bound application authorization remains a separate integration dependency; the completion command must not be exposed as a generic tenant-authenticated mutation.

Current review / evidence rule

  • Exact head: 6ea56af77edf2e93b28cea9996db6e3780025dad.
  • Exact parent: 623e5bca48b036dcf1f4845b1f071fd7782c9fb2.
  • No inline review thread is currently open on this PR.
  • Exact-head Accounting Foundation CI is queued and therefore non-passing.
  • Any head/base movement invalidates predecessor checks/reviews. Draft auto-review/status signals are not promoted to independent approval evidence.

Merge boundary

Keep Draft. Do not auto-merge, merge, tag, version or release until the parent lands and this stack is restacked onto the protected integrated head, then one unchanged exact head passes real PostgreSQL behavior, exact 100% owned production statement/branch coverage and edge-case denominator, public docstrings/repository contracts, SAST/security/dependency checks, migration-chain proof, current-head reviews, stack/base correctness and every applicable protection rule. No predecessor check/review transfers to this head and no protection bypass is authorized.

@coderabbitai

coderabbitai Bot commented Sep 1, 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.

Copy link
Copy Markdown
Contributor Author

@opencode-agent Review this exact current stacked PR head against its live non-default base. Treat this as review-only while the PR remains Draft: validate the reconciliation completion aggregate/state-transition authority, database trigger/RLS/immutability contracts, idempotency, exact PostgreSQL snapshot/population/bridge binding, transactional outbox, migration safety, tests/coverage/docstrings, and ADR traceability. Do not merge, retarget, bypass protection, or rewrite the parent stack. If the head or base moves, stop and require fresh exact-head evidence.

Copy link
Copy Markdown
Contributor Author

@opencode-agent Fresh review request for exact stacked head bf610fc270a9d611cedc68e927e583d2365bd9b3 against live parent fix/reconciliation-multi-match-conservation@623e5bca48b036dcf1f4845b1f071fd7782c9fb2. Review-only while Draft. In addition to the earlier authority review, validate the new accounting_reconciliation_completer NOLOGIN/session_user capability boundary, public 0020 migration installer, isolated real-PostgreSQL migration/RLS/privilege test, architecture/operability deployment contract, and whether any existing fixture/runtime path can bypass or cannot lawfully exercise completion. If either head/base moves, require fresh exact-head evidence. Do not merge, retarget, bypass protection, or rewrite the parent stack.

Copy link
Copy Markdown
Contributor Author

Superseded after cross-branch causal review by the canonical lifecycle stack in #43. I preserved the useful database-status finding instead of discarding concurrent work: #43 now rejects every changed run_status_code target without a named lifecycle command, including raw evaluating -> not_reconciled and post-reconciliation downgrades, and adds a real PostgreSQL regression. #43 retains the stronger run-level advisory-lock / pre-snapshot serialization and post-transition evidence-freeze model that this branch does not provide.

The purpose-limited NOLOGIN database capability-role idea is also preserved as explicit follow-up issue #44, scoped to the canonical reconciliation_run aggregate and purpose-bound authorization rather than a second completion aggregate/migration. The sibling 0020 completion table/service, installer fork, and duplicate command vocabulary should therefore not be integrated independently.

No commits from this branch are being force-applied or treated as transferable CI evidence. Closing this PR removes the competing lifecycle implementation while retaining its two durable contributions (target-state hardening in #43 and least-privilege capability requirements in #44).

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