feat(reconciliation): add maker-checker exception resolution command - #46
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
github-project-automation
Bot
moved this from Backlog
to Done
in Accounting Information System
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| CREATE TRIGGER accounting_reconciliation_exception_resolution_authority_guard | ||
| BEFORE UPDATE OR DELETE ON accounting_core.reconciliation_exception | ||
| FOR EACH ROW | ||
| EXECUTE FUNCTION accounting_core.enforce_reconciliation_exception_resolution_authority(); |
There was a problem hiding this comment.
🟡 Terminal exceptions bypass command authority
When an exception is inserted as resolved or superseded, accounting_reconciliation_exception_resolution_authority_guard never runs. The command API rejects that non-open exception, blocking reconciliation until privileged cleanup.
Prompt for agents
Migration 0020 guards reconciliation_exception status changes only on UPDATE or DELETE. A direct INSERT can therefore create resolution_status_code='resolved' or 'superseded' without a reconciliation_exception_resolution_command. The later command API cannot repair that row because assign_reconciliation_exception_resolution_hash accepts only an open exception, while lifecycle finalization rejects the missing command. Extend the database state guard so new exception rows must begin open, and add PostgreSQL coverage for direct terminal INSERT attempts. Consider how the forward migration must handle any pre-existing commandless terminal rows.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # ADR 0062: Reconciliation exception resolution uses named immutable maker-checker commands | ||
|
|
||
| - Status: Proposed | ||
| - Date: 2026-09-02 | ||
| - Bounded context: Bank Reconciliation / Evidence and Audit | ||
| - Depends on: ADR 0058 reconciliation-run command evidence and the stacked reconciliation lifecycle transition | ||
| - Follow-ups: #34/#9 for authenticated HTTP command authorization; #44 for least-privilege database capability |
There was a problem hiding this comment.
🔍 Required release records are missing
The new public command changes reconciliation authority, but CHANGELOG.md and STANDARD_TRACEABILITY.md omit it. Repository rules require both updates for authority changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def apply_foundation_migration(database_url: str, migration_path: Path) -> None: | ||
| """Apply the complete checked-in foundation chain through the canonical loader.""" | ||
| resolution_migration_path = ( | ||
| migration_path.parent / "0020_reconciliation_exception_resolution_command.sql" | ||
| ) | ||
| if not resolution_migration_path.is_file(): | ||
| raise AccountingValidationError( | ||
| "Reconciliation exception-resolution command migration is missing at " | ||
| f"{resolution_migration_path}. Restore " | ||
| "database/migrations/0020_reconciliation_exception_resolution_command.sql, " | ||
| "then retry." | ||
| ) | ||
| _apply_foundation_migration(database_url, migration_path) | ||
| psycopg = _import_psycopg() | ||
| try: | ||
| with psycopg.connect( | ||
| database_url, autocommit=True, cursor_factory=psycopg.ClientCursor | ||
| ) as connection: | ||
| connection.execute(resolution_migration_path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
🔍 Migration loaders now disagree
The public wrapper installs migration 0020, while persistence.apply_foundation_migration still stops at 0019. Callers now receive different schemas from the same-named loader.
Was this helpful? React with 👍 or 👎 to provide feedback.
| CREATE CONSTRAINT TRIGGER reconciliation_exception_resolution_status_pair_guard | ||
| AFTER INSERT ON accounting_core.reconciliation_exception_resolution_command | ||
| DEFERRABLE INITIALLY DEFERRED | ||
| FOR EACH ROW | ||
| EXECUTE FUNCTION accounting_core.enforce_reconciliation_exception_resolution_pair(); |
There was a problem hiding this comment.
📝 Info: Deferred pairing supports command-first writes
The command-first write order remains atomic. The status guard sees same-transaction evidence, and the deferred pair guard validates the final status at commit.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 523b6fc2af
ℹ️ 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".
| "database/migrations/0020_reconciliation_exception_resolution_command.sql, " | ||
| "then retry." | ||
| ) | ||
| _apply_foundation_migration(database_url, migration_path) |
There was a problem hiding this comment.
Callers that use the existing accounting_information_platform.persistence.apply_foundation_migration entry point—including several repository PostgreSQL fixtures—still install only through migration 0019 because migration 0020 is known only to this new wrapper. A database initialized through that established entry point therefore lacks reconciliation_exception_resolution_command, and the newly exported resolution command fails with an undefined-relation error. Add 0020 to the canonical persistence migration chain rather than applying it solely in the wrapper.
Useful? React with 👍 / 👎.
| evidence_hash = command.get("resolution_evidence_hash") | ||
| if not isinstance(evidence_hash, str) or _HASH_PATTERN.fullmatch(evidence_hash) is None: | ||
| raise AccountingValidationError( | ||
| "resolution_evidence_hash must be a canonical sha256 digest. Retain the reviewed " | ||
| "resolution evidence, supply its exact digest, then retry." |
There was a problem hiding this comment.
This command accepts only resolution_evidence_hash, which identifies the supporting review evidence rather than the source command payload, and neither the API nor the new table stores a source_payload_hash. Consequently, retries can alter ignored payload members while retaining the same idempotency key and still replay successfully, leaving no immutable digest of the received command payload. Require, validate, persist, and compare a source-payload hash independently of the evidence and database-derived command hashes.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| ## Verification and traceability | ||
|
|
||
| Acceptance evidence must demonstrate at least these cases on real PostgreSQL: raw terminal status update fails; owner-as-reviewer fails; decision before exception time fails; valid command changes status and emits the matching outbox event atomically; exact replay returns the same receipt; changed replay conflicts; terminal exception evidence cannot be rewritten; a terminal status without matching command still blocks run reconciliation; a terminal status with matching command can proceed when all other bridge/review controls pass; tenant RLS and shared command idempotency remain enforced. |
There was a problem hiding this comment.
This commit changes reconciliation authority from “every exception blocks finalization” to maker-checker terminal commands, but none of CHANGELOG.md or the repository's standards traceability records are updated. The ADR alone does not update those required project-wide records, so release and control documentation will continue to describe the pre-0020 authority boundary. Record this behavior change in both locations.
AGENTS.md reference: AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
| IF TG_OP = 'DELETE' THEN | ||
| IF command_exists THEN | ||
| RAISE EXCEPTION | ||
| 'resolved reconciliation exception evidence is immutable (reconciliation_exception_resolution_immutable)' |
There was a problem hiding this comment.
When a privileged caller deletes an open exception, command_exists is false, so this trigger returns OLD and permits the deletion. The run then has no exception row for either the application or database finalization checks to inspect and can be reconciled without any maker-checker resolution command. Reject deletion regardless of whether a command already exists, or replace it with an explicit immutable supersession command.
Useful? React with 👍 / 👎.
| RETURN OLD; | ||
| END IF; | ||
|
|
||
| IF command_exists |
There was a problem hiding this comment.
The immutability checks for owner_reference and effective_at run only when command_exists is true, so both fields remain mutable while the exception is open. A privileged caller can change the owner away from themselves or move the exception time backward and then submit a resolution whose actor/time passes both the application and database checks, defeating the advertised maker-checker and temporal-causality controls. Preserve immutable original owner and effective-time evidence from exception creation rather than freezing it only after resolution.
Useful? React with 👍 / 👎.
1 participant
Buyer outcome
Replace mutable reconciliation-exception terminal status with a named, immutable maker-checker command. The command retains reviewed evidence, exact idempotency identity and database-owned provenance, then emits the matching accounting outbox event atomically. It cannot post/reverse journals, close periods, change accounting policy, or write Billing-owned truth.
This Draft is stacked on #43 exact parent
48a1857449303e754a928b98828d114ce7ca1669, which is itself stacked on dependency-root #29. It must not bypass or leapfrog either parent.RED first
The first child commit
0fa553438eae4d7e9a542f62a7fd628f343643aeadds a real-PostgreSQL regression proving privileged rawreconciliation_exception.resolution_status_code = 'resolved'must fail without named command authority. Parent #43 permits that raw status rewrite, so the regression is a real RED against the parent behavior rather than a synthetic assertion.Narrow causal repair
0020_reconciliation_exception_resolution_command.sqladds tenant/run/exception-scoped immutable command evidence with the shared reconciliation idempotency namespace;PUBLICtable privilege;open -> resolved/supersededstatus changes fail unless the matching command already exists in the same transaction;resolve_reconciliation_exception()performs the command/status/outbox writes in one transaction and exact retries replay the retained receipt while changed retries conflict;Evidence and boundaries
Real PostgreSQL acceptance covers raw-SQL bypass, valid maker-checker resolution, atomic outbox evidence, exact replay/conflict, owner-as-reviewer rejection and terminal-evidence immutability. Unit contracts cover validation, run/exception scope, temporal causality, database-hash fail-closed behavior, supersession, shared idempotency and lifecycle snapshot binding. ADR 0062 records the accounting/security boundary and primary-source traceability without claiming IFRS, ISO, SOC 2 or CSAP compliance/certification.
HTTP authentication/authorization remains deferred to #34 / issue #9. Least-privilege database capability remains issue #44 and must expose the named command rather than raw command/status/outbox DML. Context Graph Contracts and Enterprise Architecture Core remain read-only sibling authorities; this PR consumes no open-PR bytes and does not copy financial facts into EA/context-graph data.
Merge boundary
Current child head at PR creation:
523b6fc2af49cc047678c99cf6079c043a162789. Treat all queued/pending/absent/stale/predecessor evidence as non-passing. Keep Draft until one unchanged exact head passes real PostgreSQL integration, exact 100% owned production statement/branch coverage and public docstrings, repository contracts, SAST/security/dependency/package/SBOM/provenance gates, live rulesets and qualifying independent review. Do not merge this child before #43/#29 stack order is satisfied.