feat: add evidence-backed reconciliation completion command - #42
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 |
|
@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. |
|
@opencode-agent Fresh review request for exact stacked head |
|
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 The purpose-limited 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). |
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 protecteddevelopahead 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
evaluatingrun, while authority-bearing close-package construction requiresreconciled. 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.sqladds 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_completeras a separateNOLOGINcapability and reassertsNOLOGIN. Both completion-command insertion and the first transition intoreconciledcheckpg_has_role(session_user, 'accounting_reconciliation_completer', 'MEMBER'). Caller fields, model output, GUCs and applicationSET ROLEare not authority. The capability is separate fromaccounting_closing_writerand receives only completion-table INSERT/SELECT,reconciliation_run.run_status_codecolumn 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
evaluatingorreview_required, no reconciliation exception is open, no match remainsproposed, and immutable completion-command evidence exists before the run entersreconciled. 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-casedNEW.run_status_code = 'reconciled'. A session holdingaccounting_reconciliation_completercould therefore have used direct SQL to manufacture another allowed status value such asreview_required,not_reconciled, orsuperseded, 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.pyfirst gained a failing contract requiring a target-state guard. Migration 0020 then changed so every changed target other thanreconciledfails closed withreconciliation_completion_target_forbidden. The isolated real-PostgreSQL install test now reads the actual installed function definition withpg_get_functiondef(...)and verifies that the target restriction and diagnostic survive the public migration chain. ADR 0059 makes the state-machine boundary explicit, anddocs/doctoring/2026-09-01-reconciliation-completion-status-authority.mdrecords the causal defect, TDD repair, DDD interpretation and research basis.This does not define
review_required,not_reconciled, orsupersededcommands. 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 onePostgresPostingLedger._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 andreconciliation_run.reconciledtransactional-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
0020before 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.pyproves that installer control flow with executable fakes.tests/test_postgres_reconciliation_completion_migration.pycreates 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-completionsstdlib 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
6ea56af77edf2e93b28cea9996db6e3780025dad.623e5bca48b036dcf1f4845b1f071fd7782c9fb2.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.