feat: bind financial report sources to AIS persistence - #52
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: true✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review Please review the database authority boundary rather than only SQL style. In particular verify that a caller cannot obtain stronger financial-report authority by supplying a balanced package, digest, reporting currency, period status, knowledge cutoff, run-status label, cross-scope snapshot, or future snapshot. This PR intentionally stops before source-set finalization, XBRL validation, approval, publication, filing, or regulator acceptance. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata =='
gh pr view 52 --json number,title,state,isDraft,baseRefName,headRefName,files \
--jq '{number,title,state,isDraft,baseRefName,headRefName,files:[.files[].path]}'
echo
echo '== Changed files with status =='
git diff --name-status "origin/$(gh pr view 52 --json baseRefName --jq .baseRefName)...HEAD"
echo
echo '== Candidate migration, installer, reporting, and test files =='
fd -t f . | rg -i '(0020|migration|financial.*report|report.*financial|trial.*balance|install|rls|source)'Length of output: 1639 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR patch =='
gh pr diff 52 --patch
echo
echo '== Python structure =='
ast-grep outline src/accounting_information_platform/migration_install.py --items all
ast-grep outline tests/test_financial_report_source_install.py --items all
ast-grep outline tests/test_financial_report_source_registry_red.py --items all
echo
echo '== Changed files with line numbers =='
for f in \
database/migrations/0020_financial_report_source_registry.sql \
src/accounting_information_platform/migration_install.py \
tests/test_financial_report_source_install.py \
tests/test_financial_report_source_registry_red.py \
docs/adr/0068-financial-report-source-authority.md
do
echo "----- $f -----"
cat -n "$f"
doneLength of output: 73481 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Candidate definitions and migration files =='
fd -t f . database src tests | rg '(migration|persistence|trial_balance_snapshot|fiscal_period|accounting_book|tenant)'
echo
echo '== Definition and mutation references =='
rg -n -i -C 5 \
'CREATE TABLE accounting_reporting\.trial_balance_snapshot|CREATE TABLE accounting_core\.fiscal_period|CREATE TABLE accounting_core\.accounting_book|CREATE( OR REPLACE)? FUNCTION accounting_core\.current_tenant_account_id|apply_foundation_migration|trial_balance_snapshot.*(UPDATE|DELETE)|BEFORE (UPDATE|DELETE).*trial_balance_snapshot' \
database src tests
echo
echo '== Migration sequence =='
fd -t f -e sql . database/migrations | sortLength of output: 42901 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Foundation: accounting book, fiscal period, and trial-balance snapshot =='
sed -n '27,135p;226,270p;300,345p' database/migrations/0001_accounting_foundation.sql
echo
echo '== Book-period control =='
cat -n database/migrations/0009_accounting_book_period_control.sql
echo
echo '== Snapshot creation and immutability references =='
rg -n -i -C 8 \
'trial_balance_snapshot|snapshot_generated_at|snapshot_currency_code|snapshot.*immutable|immutable.*snapshot' \
database/migrations/0001_accounting_foundation.sql \
database/migrations/0005_closed_period_guard.sql \
database/migrations/0009_accounting_book_period_control.sql \
database/migrations/0010_soft_close_command_evidence.sql \
src/accounting_information_platform/persistence.py
echo
echo '== Installer manifest and migration execution =='
sed -n '6190,6315p' src/accounting_information_platform/persistence.py
echo
echo '== Existing PostgreSQL integration-test fixtures for scope and RLS =='
rg -n -i -C 3 \
'ROW LEVEL SECURITY|FORCE ROW LEVEL SECURITY|cross-tenant|tenant.*deni|snapshot.*(entity|book|period|currency)|accounting_book_period_control' \
testsLength of output: 127500 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Status-update helper definitions and calls =='
rg -n -C 12 \
'def _set_book_period_closed|def _lock_book_period|UPDATE accounting_core\.accounting_book_period_control|UPDATE accounting_core\.fiscal_period|period_status_code.*accounting_book_period_control' \
src/accounting_information_platform/persistence.py
echo
echo '== Book-specific status behavior in integration tests =='
rg -n -C 10 \
'accounting_book_period_control|management.*(open|soft_closed|hard_closed)|statutory.*(open|soft_closed|hard_closed)|different.*book|book.*independent' \
tests/test_period_close_book_scope.py tests/test_postgres_posting.pyLength of output: 11335
The remaining reviewed controls have the intended design: the insert trigger overwrites caller currency, cutoff, and initial status; the source trigger checks tenant/entity/book/period/currency and cutoff; and the new tables use forced tenant RLS. The PR still needs PostgreSQL integration tests for these controls. The current new tests inspect SQL text or use mocks, so they cannot prove runtime enforcement.
|
|
@coderabbitai review The authority-boundary finding from comment 5533718322 is now repaired.
Please re-review the repaired authority boundary and the test isolation/transaction semantics. The PR remains Draft; queued CI is not treated as passing. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current HEAD and PR metadata =='
git rev-parse HEAD
gh pr view 52 --json number,isDraft,baseRefName,headRefName,commits,files \
--jq '{number,isDraft,baseRefName,headRefName,headCommit:.commits[-1].oid,files:[.files[].path]}'
echo
echo '== Migration and test candidate files =='
fd -t f . | rg '(^|/)(0020_financial_report_source_registry\.sql|test_.*financial.*report.*|migration_install\.py|0068-financial-report-source-authority\.md|0009_accounting_book_period_control\.sql)$'
echo
echo '== Outlines =='
ast-grep outline src/accounting_information_platform/migration_install.py --items all || true
fd -t f -e py tests | rg 'financial.*report|report.*financial|postgres.*report' | while read -r f; do
echo "----- $f -----"
ast-grep outline "$f" --items all || true
done
echo
echo '== Migration 0020 =='
cat -n database/migrations/0020_financial_report_source_registry.sql
echo
echo '== Book-period control migration =='
cat -n database/migrations/0009_accounting_book_period_control.sql
echo
echo '== Installer references =='
rg -n -C 8 '0020_financial_report_source_registry|apply_foundation_migration|migration' \
src/accounting_information_platform/migration_install.py \
src/accounting_information_platform/persistence.py
echo
echo '== Regression test implementations =='
fd -t f -e py tests | rg 'financial.*report|report.*financial|postgres.*report' | while read -r f; do
echo "----- $f -----"
cat -n "$f"
done
echo
echo '== ADR 0068 =='
cat -n docs/adr/0068-financial-report-source-authority.md
echo
echo '== Relevant recent commits =='
git show --stat --oneline --no-renames \
54944f238c29fa856bf3226dad610c2d74935eef \
fe7fc984b12797cb45709489e4b4af5ea3f12eeb \
d4752b8c9f534a22316ec3f6b4080201e78311fc \
d5b90240004e24fc2b40a01326fb9c747a4c2df1 \
8afbf102bfaeccd8ffacee83df8f8863a3676a03Length of output: 3919 🏁 Script executed: #!/bin/bash
set -u
echo '== Migration 0020 =='
cat -n database/migrations/0020_financial_report_source_registry.sql || true
echo
echo '== Book-period control =='
cat -n database/migrations/0009_accounting_book_period_control.sql || true
echo
echo '== PostgreSQL source-registry regression =='
cat -n tests/test_postgres_financial_report_source_registry.py || true
echo
echo '== Book-period authority regression =='
cat -n tests/test_financial_report_book_period_authority.py || true
echo
echo '== Migration installer =='
cat -n src/accounting_information_platform/migration_install.py || true
echo
echo '== Source installation test =='
cat -n tests/test_financial_report_source_install.py || true
echo
echo '== Snapshot and tenant helper definitions referenced by 0020 =='
rg -n -C 12 \
'CREATE TABLE accounting_reporting\.trial_balance_snapshot|CREATE( OR REPLACE)? FUNCTION accounting_core\.current_tenant_account_id|CREATE TABLE accounting_core\.accounting_book|CREATE TABLE accounting_core\.fiscal_period' \
database/migrations || true
echo
echo '== Other migrations that modify trial_balance_snapshot or account_book_period_control =='
rg -n -C 5 \
'(trial_balance_snapshot|accounting_book_period_control)' \
database/migrations/0001_accounting_foundation.sql \
database/migrations/0005_closed_period_guard.sql \
database/migrations/0009_accounting_book_period_control.sql \
database/migrations/0010_soft_close_command_evidence.sql || true
echo
echo '== Relevant PR diff only =='
git diff --no-ext-diff --unified=30 \
origin/feat/financial-reporting-xbrl-export...HEAD -- \
database/migrations/0020_financial_report_source_registry.sql \
src/accounting_information_platform/migration_install.py \
tests/test_postgres_financial_report_source_registry.py \
tests/test_financial_report_book_period_authority.py \
tests/test_financial_report_source_install.py \
docs/adr/0068-financial-report-source-authority.md || trueLength of output: 81273 🏁 Script executed: #!/bin/bash
set -u
echo '== PostgreSQL fixture lifecycle and cleanup =='
rg -n -C 25 \
'class PostgresPostingTests|def setUpClass|def tearDownClass|def setUp\(|def tearDown\(|def doCleanups|DATABASE_URL|psycopg\.connect|rollback\(|commit\(' \
tests/test_postgres_posting.py || true
echo
echo '== Fixture helper implementations used by the source-registry regression =='
ast-grep outline tests/test_postgres_posting.py --items all || true
rg -n -C 18 \
'tenant_id|policy|accounting_book_reference|INSERT INTO accounting_core\.fiscal_period|INSERT INTO accounting_core\.accounting_book_period_control|SET app\.tenant_account_id' \
tests/test_postgres_posting.py || true
echo
echo '== Period-state synchronization and fiscal-period mutation paths =='
rg -n -i -C 12 \
'UPDATE accounting_core\.fiscal_period|period_status_code.*fiscal_period|fiscal_period.*period_status_code|accounting_book_period_control.*period_status_code|CREATE( OR REPLACE)? FUNCTION.*period' \
database/migrations src/accounting_information_platform tests || true
echo
echo '== Base migration installer transaction behavior =='
rg -n -C 20 \
'def apply_foundation_migration|psycopg\.connect|autocommit|execute\(.*migration|read_text\(.*encoding' \
src/accounting_information_platform/persistence.py || true
echo
echo '== Registry test commits and transaction control =='
rg -n -C 5 \
'\.commit\(|\.rollback\(|autocommit|psycopg\.connect|_apply_registry_inside_transaction' \
tests/test_postgres_financial_report_source_registry.py || true
echo
echo '== Exact migration chain available in the checkout =='
fd -t f -e sql . database/migrations | sort || trueLength of output: 155443
The PostgreSQL regression creates a conflicting state. It sets the selected book-period row to The rollback-only test design is correct. The test removes migration 0020's outer transaction statements, runs the DDL in psycopg's default transaction, and calls I found one remaining installer-order defect.
When the stacks integrate, the public installer will skip migrations 0015 through 0019 and apply 0020 directly after 0014. Update the installer manifest and its failure-closed checks to require and execute migrations 0015 through 0019 before I did not execute the PostgreSQL test suite. Queued CI remains non-passing evidence. You are interacting with an AI system. |
|
Fresh foundation review promotes this branch's source-population freeze finding to the canonical Period Close owner boundary. Protected/inherited close schema currently documents Keep this PR and its RED. At the later non-force restack after the reconciliation/close foundation integrates, consume the protected snapshot-immutability invariant and rerun this report-source RED as consumer acceptance. If #53 fully makes hard-close snapshot header/line populations immutable and concurrency-safe, do not duplicate a narrower report-source-only mutation mechanism here; retain only linkage/scope/cutoff checks that are specific to |
|
Owner handoff from Period Close foundation PR #53: the underlying retained Do not duplicate that invariant here as a private |
|
#53 Period Close source-authority handoff: after the foundation is normally integrated and this Reporting lane non-force restacks, consume the exact book-period retained-snapshot pair rather than reimplementing it. The foundation now requires both directions at commit: retained snapshot => exact |
|
Follow-up Reporting boundary from #53: migration 0036 now also refuses a legacy one-sided hard-close/snapshot population before its future-write trigger is installed. Do not make Reporting “repair” either marker by synthesizing a snapshot, flipping book-period status, falling back to shared |
|
Foundation handoff from #53: when this reporting branch is eventually non-force restacked onto the integrated Period Close foundation, consume existing |
|
Reporting-Export handoff from #53: current-role catalog state must not be used to reinterpret historical posted journal semantics. Period Close now has RED evidence showing why |
|
Reporting-Export handoff from #53: when this lane is eventually non-force restacked onto the integrated Period Close foundation, do not reconstruct historical journal semantics from only the current effective |
|
Reporting-Export handoff from #53: when this lane eventually consumes integrated Period Close evidence, preserve exact historical |
|
Reporting-Export handoff from #53 current RED |
|
#53 Period Close downstream boundary update — current exact successor RED A new realistic code-reuse RED ( Keep #52 Draft and non-force restack only after the Period Close foundation integrates. Do not add a report-owned close writer, current-code historical identity fallback, shared |
|
#53 foundation handoff, current traceability head |
|
Period Close foundation handoff from #53: Reporting-Export must consume an authoritative |
|
/opencode Exact-log/source RCA for current RED head Foundation run Verified causal precursor:
Repair order remains TDD-strict: (1) repair only fixture/README precursor contracts; (2) rerun real PostgreSQL and require the population-freeze cases to reach their hostile UPDATE/INSERT and fail specifically because Dependency refresh: stored PR base remains |
|
Exact-head evidence correction (2026-09-07): The raw job log is now recoverable and fixes the earlier uncertainty: 736 tests ran with 8 failures, 0 errors. Seven failures are precursor fixture/authority failures rather than the intended snapshot-freeze RED: An existing |
|
Parent handoff refresh: #50 has advanced normally to exact |
|
Correction to the earlier fixture wording in comment The existing owner path is Keep the README contract repair separate: derive the executable endpoint from |
|
/opencode single-source-writer continuation — exact Do not create another competing lifecycle fix. Real-PostgreSQL RED Current exact-head behavior is 520 tests / 2 failures / 0 errors. The only failures are the existing canonical Period Close dependency REDs: After #53 normal protected integration, non-force rebuild/restack #52 from the resulting exact protected authority, preserve #50 valid deltas, renumber provisional migration 0020 to the next unclaimed forward number, reconcile installer/docs/tests, adapt the two dependency regressions to the canonical Period Close invariant, then reacquire real PostgreSQL behavior, 100% owned production statement/branch coverage, repository contracts, security/SAST/dependency/package/SBOM/provenance and current review gates on one unchanged exact head. No self-approval, bypass, force-push, destructive rebase, synthetic status or premature Ready transition. |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review on af95c4b098431e3c87e3dffa225de9e8c00c9d39 after hosted PostgreSQL Foundation 34268553044 completed.
The precursor #52-local defects are repaired: lawful book-period fixture materialization, README executable-endpoint contract, required close_idempotency_key, and imported-TestCase rediscovery no longer block the suite. Dependency diff, SAST and Security are GREEN.
The remaining two behavior failures are both valid and reach the intended hostile database mutation: the linked retained snapshot header can still be rewritten and the linked snapshot can still gain a line because no Period Close-owned immutability control rejects them. This is not permission to add a Reporting-local financial_report_source_population_frozen authority. Retained trial_balance_snapshot / trial_balance_line immutability belongs to Period Close #53, which is still Draft and outside protected lineage.
Keep #52 Draft and preserve these as dependency REDs. Do not skip/xfail/delete the tests, weaken constraints, copy mutable #53 SQL, or create a second Reporting-owned snapshot freeze. After #53 integrates normally, rebuild/restack #52 non-force on the resulting protected authority, renumber the provisional reporting migration, and adapt the regressions to assert the canonical Period Close invariant before reacquiring exact-head GREEN.
This COMMENT is review evidence only; it is not an approval or merge authorization.
Stack
This is a Draft successor to #50 and targets
feat/financial-reporting-xbrl-export. It does not bypass or replace #50. #50 remains the proposal-format/XBRL serializer boundary; this PR adds the first database-owned source-authority slice required by Issue #51.The current branch contains the live #50 parent through an ordinary merge and remains an isolated development stack. Its
0020_financial_report_source_registry.sqlidentity is provisional because unreleased reconciliation/Period Close migrations own intervening forward numbers. After those prerequisites integrate into protecteddevelop, rebuild this branch non-destructively from that exact protected authority, assign the next unclaimed migration number, and update installer/tests/docs together. Do not rewrite prerequisite history or close this PR because its provisional number becomes stale.Problem and authority boundary
A balanced caller-supplied report package and SHA-256 digest do not prove AIS origin. An authoritative financial report needs durable evidence of the exact tenant/entity/book/period population retained by AIS, with database-owned accounting-book period state and chronology.
financial_report_sourceis append-only, but the protected foundation consumed by this branch does not yet freeze a referencedtrial_balance_snapshotheader or itstrial_balance_linepopulation. That missing invariant belongs to retained Period Close evidence authority. Reporting may bind and consume retained close evidence; it may not synthesize close control, mutate retained snapshot truth, or create a narrower report-only substitute for Period Close immutability.Bounded delta
accounting_reporting.financial_report_runandfinancial_report_source;trial_balance_snapshotsource reference;recorded_atchronology;collecting_sources;run_status_code; later supersession remains reserved for a purpose-bound command owner;TDD / repair lineage
Predecessor
f4354a2d3c15b6270dbedb834372deb37e4be05dadded two real PostgreSQL source-population dependency REDs. Foundation run33876130106initially stopped earlier with 736 tests / 8 failures / 0 errors: seven tests lacked a lawfulaccounting_book_period_controlbecause their fixture was created after migration 0009's one-time backfill, and one docs contract promoted a provisional migration filename to canonical install truth.The ordinary repair sequence removed those precursor defects without changing production accounting authority:
33106ed3c00aaa81170dc968fe14e0a451eb7a90— fixture book-period control is materialized through the existing foundation owner helper_lock_book_period.fd1153fddc77edb0e7d6b81723ff82a03828cffe— population-freeze regressions consume that lawful owner-controlled fixture.8163909a1d79b29ffd52f65be05c50589170a540— recording-time regressions consume the same fixture boundary.a62a74d7b4b0d949b74a8c22fc181333edcf15cb— README derives the canonical foundation endpoint from the executable manifest rather than lexicographic migration filenames.ddb08b8ec5e05b3aa2cf3a3120e9b97625c341b2— ordinary merge adopts non-overlapping live feat: generate unverified financial report and XBRL proposals #50 reporting/XBRL deltas after exact comparison.8286b6cf0e9195eb97801a2b9cf952faa3587ddd— raw retained-snapshot test fixture supplies migration-0004-requiredclose_idempotency_keywithout weakening production constraints.1473e0c45fdb7a74967fb8cc041bea47c8203f81— population-freeze tests avoid importedunittest.TestCaserediscovery.af95c4b098431e3c87e3dffa225de9e8c00c9d39— recording-time tests use the same module-import boundary.c7d4ef300df6acf58bf8d5d5fd1799f82d509b23— a real PostgreSQL RED proves direct SQL could change a report run fromcollecting_sourcestosupersededwithout actor, command, idempotency, source-hash or outbox evidence.5a26eba357d09a0e65933da1e5ced99197130ecf— the database lifecycle guard now rejects rawrun_status_codemutation withfinancial_report_run_lifecycle_immutable; no supersession command or publication authority is invented in this slice.Steps 1–8 did not change
database/migrations/0020_financial_report_source_registry.sql. Steps 9–10 then exposed and repaired one Reporting-local lifecycle-authority gap without weakening Period Close, Posting, Billing, source-population or RLS controls.Exact current state — 2026-09-09
5a26eba357d09a0e65933da1e5ced99197130ecf;feat/financial-reporting-xbrl-export; this PR remains open / Draft / not merge-ready;34271427012is terminal RED on this exact head. Accounting job102213693861ran 520 tests in 85.603s with 2 failures / 0 errors. Exact-head dependency diff102213694040, SAST102213694259, and Security102213694269are GREEN; integrated-head attestations102214537378are skipped because this stack is not protected authority;test_direct_sql_cannot_supersede_report_run_without_command_evidenceis GREEN on this exact head and PostgreSQL emitsfinancial_report_run_lifecycle_immutablefor the hostile direct UPDATE. Initial lifecycle remains database-forced tocollecting_sources; Issue [Product Gap] Complete statutory report publishing and validated XBRL round trip #51 owns any later purpose-bound supersession/withdrawal command, command/outbox evidence, approval or publication lifecycle;close_idempotency_key, duplicate-test-discovery and raw lifecycle-mutation defects are now GREEN. Source-registry scope, chronology, FORCE RLS, database-owned recording time, installer failure handling, and ordinary reporting contracts reach and pass their intended assertions;test_linked_snapshot_header_cannot_be_rewrittenandtest_linked_snapshot_cannot_gain_new_lines. Both reach the hostile PostgreSQL UPDATE/INSERT and fail because the protected parent does not yet provide retainedtrial_balance_snapshot/trial_balance_linepopulation immutability;Explicit non-claims
This PR does not create an authoritative financial-report artifact or claim IFRS/DART conformance, XBRL validation, filing readiness, approval, publication, regulator acceptance, audit, or assurance. It establishes database-owned source provenance only.
Issue #51 continues to own authenticated report-run assembly/finalization, purpose-bound supersession/withdrawal, independent XBRL/Calculations/Formula/jurisdiction validation, official released taxonomy profiles, accessible HTML/PDF/XLSX/Inline XBRL, localized verified commentary, maker-checker approval, publication receipts, recovery, and release evidence.
Merge boundary
Keep Draft and preserve the two realistic dependency REDs. Do not add
financial_report_source_population_frozenor any equivalent Reporting-owned trigger/function merely to make this branch GREEN, and do not weaken/skip/xfail the tests to manufacture a pass.Foundation first: #53 must reach protected lineage through its own normal RED→GREEN→review→merge path. Then rebuild #52 non-force from the resulting protected authority while preserving #50 deltas, renumber the provisional reporting migration, reconcile the public installer order, adapt the two regressions to assert the canonical Period Close invariant rather than a report-local marker, and reacquire real PostgreSQL behavior, exact owned production statement/branch coverage, repository contracts, security/SAST/dependency/package/SBOM/provenance, current reviews and all live ruleset gates on one unchanged head.
No self-approval, bypass, force-push, destructive rebase, synthetic status, premature Ready transition, tag or release from this provisional state.