fix(gl): scope account-ledger reads to one accounting book - #57
fix(gl): scope account-ledger reads to one accounting book#57seonghobae wants to merge 9 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughADR 0019가 장부 범위 조회 계약을 문서화했습니다. PostgreSQL RED 테스트가 명시적 장부 조회, 누락된 Changes원장 장부 범위 검증
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change documents and tests the required accounting-book scoping behavior for account-ledger reads. It introduces no production behavior change, and the RED cases intentionally capture the existing cross-book leakage for follow-up implementation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation [56]의 PostgreSQL 회귀 테스트, 명시적 Resolution [56]의 최소 인과 수리를 구현하십시오. 라이브러리와 HTTP 경계에서
✨ 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 |
|
Hosted precursor failure was verified on exact I repaired only that fixture identity on ordinary descendant |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review Please review exact current head |
|
✏️ Learnings added
✅ Action performedReview finished.
|
|
Retired source handoff. It produced no observed descendant from |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Retired source handoff on 2026-09-09 after a fresh live check: PR #57 is still exact |
|
@codex address that feedback Continue PR #57 as the single active source writer for the admission slice only. Re-read the live PR before writing. Expected head is Implement only the non-overlapping General Ledger admission repair proven by the checked-in REDs. Allowed production files: Required behavior:
Validation sequence: run the focused admission tests first ( |
|
You have reached your Codex usage limits. You can see your limits in the Codex usage dashboard. |
|
RETIRED 2026-09-09 after fresh exact-head verification: this handoff produced no source descendant and no acknowledgement while |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head admission review on 89b8bcaa6ae3268554171abfebd2a9b65b8cab80: the existing alias-conflict RED is necessary but not sufficient for an authoritative accounting-book query boundary. Current _first_query(fields, name) returns values[0], so a request carrying book_reference=A&book_reference=B can silently choose one of two distinct accounting scopes after parse_qs has retained both values. For a financial ledger query this is an ambiguity/control defect, not a harmless HTTP parsing detail; upstream proxies/clients must not be able to change effective book scope by parameter-collapse convention.
The ordinary child on this head adds the realistic HTTP/PostgreSQL RED and requires 400 with book_reference named when a repeated canonical key has distinct values. Identical repetition need not fail merely for duplication. Keep the repair in the non-persistence admission slice: inspect the complete value lists for canonical and alias spellings before _first_query, establish one logical book identity, normalize to library book_reference, and only then enter lookup_account_ledger. Do not touch persistence.py or weaken the existing two-book 50000.000000 vs 25000 RED.
This is a COMMENT review, not approval or GREEN. Current exact-head workflows are queued and the active source writer must ordinary-adopt this descendant before producing the minimal admission repair.
|
/opencode Own PR #57 as the single active source writer for the non-persistence General Ledger admission slice only. The prior handoff Implement the smallest causal admission repair and nothing else:
Hard boundary: do not edit Acceptance for this slice: empty library book fails before DB; missing HTTP book is 400; conflicting alias is 400; repeated distinct canonical values are 400; explicit valid book reaches current persistence; the remaining failure is the book-unscoped SQL population/totals owned by the later persistence slice after #53 ownership is reconciled. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Fresh persistence-slice finding on exact 89b8bcaa6ae3268554171abfebd2a9b65b8cab80: the planned phrase “resolve exactly one active accounting book” needs an executable ambiguity guard before the later SQL repair.
The protected schema does not make accounting_book.book_name unique for a tenant/legal entity. Its uniqueness is on (tenant_account_id, legal_entity_id, book_role_code, valid_from) plus identifier keys; later migrations contain no book_name uniqueness. Meanwhile the existing _require_book_for_close pattern selects accounting_book_id, reporting_currency_code by tenant + legal entity + book_name + valid_to IS NULL and calls .fetchone(). Therefore reusing that helper/pattern for #57 can silently choose one of two simultaneously active rows that share the requested book name/reference but differ in role or effective history. That would replace the current cross-book leak with nondeterministic book authority.
Before the persistence GREEN, add a real PostgreSQL RED that creates two active accounting-book rows for the same tenant/legal entity with the same requested book_name but distinct legal book_role_code/identities, then calls the public account-ledger inquiry and requires fail-closed ambiguity with no sibling ledger disclosure. Keep a positive case proving one exact active match succeeds. The causal implementation must either (a) resolve an actually unique immutable book identity or (b) explicitly detect 0/1/>1 active matches and reject >1; never rely on .fetchone(), implicit row order, book_role_code inference, or current account mappings.
This finding belongs to the later persistence.py slice. Do not expand the active admission writer 5599374088 or compete with Period Close #53. Keep ADR 0019 Proposed until this ambiguity case and the existing two-book 50000.000000 vs 25000 population/totals RED are both GREEN on one unchanged descendant.
|
Foundation prerequisite for the #57 persistence phase is now #58 / Draft #59 exact The original “one active row” framing was incomplete because Keep the current #57 admission writer scoped to |
|
Foundation prerequisite update for the later persistence slice: #58/#59 has tightened durable |
|
#58/#59 persistence prerequisite strengthened on exact |
1 participant
RED-only General Ledger repair lane
Fixes #56. Period Close #53 is referenced only for the shared
persistence.pysingle-writer boundary; it is not this PR's implementation issue.Protected parent/current base:
develop@239008c4edc7d305c97704c5102b593c6622b36f.Current exact head:
89b8bcaa6ae3268554171abfebd2a9b65b8cab80.State: open / Draft / mergeable / intentionally RED.
Buyer/control defect
GET /account-ledgers/lookup_account_ledgercurrently binds tenant + legal entity + chart-account code (+ optional period) but not an accounting book, while PostgreSQL population/totals omitgeneral_journal.accounting_book_id. Chart-account codes are book-scoped, so statutory and management books under one legal entity may legitimately reuse110100; current reads can combine their immutable journal lines and totals into one buyer-visible ledger.Real PostgreSQL evidence already proves the defect: exact
7b22dde45af55d6402cfc99be3c95751b6b622ce, Accounting Foundation34303435636, returned statutoryperiod_debit_total = 50000.000000instead of the statutory-only25000in a same-legal-entity/two-book/same-account-code scenario.Admission is also fail-closed: the library must accept one explicit canonical
book_referenceand reject empty/non-canonical identity before persistence; HTTP must reject a missing book, conflictingbook_reference/accounting_book_referencealiases, and repeated canonical keys carrying distinct values instead of selecting first/last arbitrarily. Identical duplicate values may normalize to one logical identity.accounting_book_referenceis an HTTP alias only; it is not the canonical library parameter.TDD lineage
b052715d...failed before the accounting assertion because the fixture called.hexon a string tenant id.7b22dde...repaired only that fixture and then reached the valid PostgreSQL RED: 461 tests / 1 failure,50000.000000vs25000; same-head dependency/SAST/security GREEN.aca6723...added missing-book HTTP RED.64e467f...repaired touched fixture docstrings only.0c32d88...added explicit-book library RED.bb032c3...made ADR 0019 code-current while preserving the historical Accepted contract and keeping the book-scope amendment Proposed.2126d67...added the pre-persistence empty-book RED.f078d58...added real-HTTP conflicting-alias RED. Current89b8bcaa...adds parameter-pollution RED: repeated canonicalbook_referencekeys with different values must return 400.accounting_book_reference. Exact source disproved both: currentlookup_account_ledgerhas no book parameter at all. Both threads were answered and resolved. Current unresolved inline review-thread count: 0.Current exact-head evidence —
89b8bcaa...Accounting Foundation
34331096008is terminal FAILURE. Exact-head security102399604235, SAST102399604423, and dependency diff102399604432are terminal GREEN. Accounting job102399604522passed container initialization, exact checkout, reproducible timestamp setup, pinned Python and hash-locked dependencies, then failed atRun behavior and repository tests. Coverage/denominator enforcement, repository contracts, compile/import, reproducible package, SBOM/provenance continuation were skipped after behavior RED. Integrated-head attestations were skipped because this is not protected integration.Standalone SAST
34331096110and Security34331096129are terminal GREEN. Required CodeQL PR34331096131is terminal FAILURE at the central required-workflow compatibility boundary, not a leaf source-analysis result: Python receiver102400983266and Actions receiver102400983324failed terminal enforcement by08:58:56Z, while the same-run authoritative dispatch job102402891472did not start until09:06:59Zand then succeeded. That exact downstream canary was handed to canonical.github#2040in comment5599403046; do not rerun historical CodeQL or manufacture leaf status.This head is not whole-head GREEN.
Source-writer boundary
Prior Codex handoff terminated on usage limits with no source delta. Prior
/opencodehandoff5598678587also produced no descendant and is explicitly retired.The single active source writer is successor
/opencodehandoff5599374088, scoped only tosrc/accounting_information_platform/accept.pyandsrc/accounting_information_platform/http_api.py. It must add canonical librarybook_reference, fail closed beforePostgresPostingLedgeron missing/empty/non-canonical identity, normalize the HTTP alias, reject conflicting aliases and repeated same-key distinct values, and update only directly affected call sites/tests. It must not editpersistence.py, migrations, #53 source, or shared #37 docs.The intended admission partial GREEN deliberately leaves the real PostgreSQL two-book population/totals RED (
50000.000000vs25000) intact.Planned causal GREEN
general_journal.accounting_book_id; compose optional period inside the same book scope; preserve exact Decimal and immutable facts; valid empty book returns exact zeros; unknown/cross-entity book fails closed; cursor continuation cannot cross books.src/accounting_information_platform/persistence.pyremains concurrently mutable in Period Close #53. If #53 produces an ordinary descendant first, read and preserve that intervening delta before the GL persistence repair. No force-push, destructive rebase, source copy, synthetic status, no-op queue churn, self-approval, premature merge, tag, version, or release.Shared
CHANGELOG.md,docs/doctoring/STANDARD_TRACEABILITY.md, anddocs/product-technical-gap-baseline.mdremain #37's canonical single-writer surfaces and are post-integration handoff only.