Conversation
Per-cell evidence withdrew a metric's control total whenever *any* cell for it was unavailable, and dropped every total whenever a staged row matched no requested constituent. Both conditions are reachable with no `cell_availability` at all, so five documented legacy shapes -- a `None` not-ready answer, `covered_constituent_ids`, `unavailable_constituents`, a non-default `unavailable_status`, and bare rows with one unmatched row -- silently changed their sealed bytes, `content_fingerprint`, ledger `control_totals`, and `revision_content_sha256` for adopters who never opted in. A full-coverage manifest whose every cell was `present` lost its totals entirely because one extra row was staged. A control total is a checksum over the staged rows: `reporting_inspection` and `_reconcile` verify it by recomputing from the revision's rows, so unmatched rows belong in it and a derived constituent status says nothing about it. Withdraw a column only when the adapter explicitly declared a cell of that metric unavailable -- the case where row values exist but the adapter has said they are not a measurement. Also stop the zero-row mixing guard from naming `cell_availability`: a derived result reaches it too, and an adopter who never set that field should not be sent looking for it. Verified all twelve representative legacy shapes now seal byte-identical to origin/main, and re-ran 10,800 randomized override/fallback combinations through the execution conformance validator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean integration of two previously reviewed roots (#1173 per-metric evidence, #1176 currency) plus a durable-records test harness.
Checked:
- Core
inline_source.pycell resolution, batch-watermark advance, control-total withdrawal, and error classification. - Batch-watermark
max()is safe: constituents and metrics both havemin_length=1, so no empty-array edge case. - Multi-tenant staging isolation, clock injection, and semver signal verified.
- Currency going keyword-only breaks no published caller since it never shipped positionally — no semver break, so no
feat!:/migration-note requirement is triggered.
No Critical/High/Medium findings reported. No gated paths, no high-risk flag, no no-auto-approve team match, no prior decision. None of decision-table rows 1–8 fire, so this falls through to row 9.
A cell declaring a metric `missing`/`delayed`/`unsupported` withdrew that metric's control total unconditionally, including when the declaring constituent staged no rows at all. `validate_monetary_content` requires the total for a monetary column every retained row carries, so the composition of #1173's per-metric evidence with #1175's obligation currency sealed an immutable publication the ledger then refused with `MONETARY_TOTAL_MISMATCH` on the replay of every retry -- permanently wedging the obligation for the answer per-metric evidence exists to give: a buy that delivered nothing and whose billing is pending. Distinguish the two shapes rather than exempting money from the withdrawal: - A withdrawal by a constituent that staged rows still removes the total. Those rows carry values the adapter disclaimed, so summing them would contradict its own evidence, and the payload stays staged byte-for-byte. - A withdrawal by a constituent that staged no rows reaches no sum and removes nothing. The checksum over the rows that were measured is retained. A row-empty batch still withdraws, so unavailability is never published as an observed zero. No total ever sums a disclaimed value. Monetary columns have no third option: dropping the total is refused by the ledger and keeping it would falsify the evidence. A withdrawn monetary cell whose own constituent's rows report that metric now raises before anything is staged or sealed. `ReportingDefinitionBinding.to_wire()` deliberately keeps the frozen unit declarations off the wire, so the frozen slice request cannot carry them; `InlineReportingSource(monetary_metrics=...)` lets a trusted caller declare #1171's custom `monetary_metric_units` columns, with `spend` always included. An undeclared custom money column keeps the non-monetary behavior. Manifest and control-total bytes for a result with no `cell_availability` are unchanged, byte-identical to the #1176 base. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass on the delta, no blocking or medium findings.
Checked:
- Withdrawal invariant traced through
_publish,_validate_cell_rows, and_control_totals: narrowing the control-total withdrawal rule so a no-row constituent's withdrawal no longer drops neighbours' subtotal is sound. - New fail-closed guard rejecting a withdrawn monetary cell whose own staged rows report that metric (before staging/sealing) prevents ledger wedge on the monetary/sparsity interaction.
- New keyword-only
monetary_metricsparam is additive/non-breaking on the public surface — no semver signal required. - Docs (
docs/reporting-source-adapters.md,README.md) match behavior. - Three new parametrized conformance tests cover the new branches.
No gated paths, high_risk false, review_decision APPROVED, no author-team gate. No critical/high/medium findings — falls through to row 9.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the explicit join of two parallel, reviewed Reliable Reporting roots: #1173's per-metric source evidence and #1176's durable records on top of #1175's obligation currency. This branch will be the parent of #1168A.
The integration keeps the six legacy positional
InlineFetchResultarguments and makes bothcurrencyandcell_availabilitykeyword-only. Configuration/obligation currency stays authoritative. Sparse unavailable monetary cells retain available nonmonetary measurements, explicit zero retains its own evidence, and unavailable spend suppresses its total wherever that withdrawal can reach the sum. Both lines' currency/evidence validators, row snapshotting, error classification, control-total bytes, and sealed replay behavior are preserved.The new private
_reliable_support.pyharness supplies manual time, finite failure plans and barriers, bounded worker draining, scripted sync/async sources, deterministic destination/receiver stores, and memory/real-PostgreSQL factories. Its trusted publisher preparation helper supplies typed evidence before the real Core store write, composing the worker with the #1167A record APIs. All domain validation still runs in the existing stores.The combined scenarios exposed an account collision in memory staging: identical rows under the same execution key let a later account replace the earlier account's ownership. Staging now includes the account in its internal key while retaining the same public object references and bytes. The clock audit also corrected memory Core change appends and PostgreSQL Core/reconciliation change timestamps to honor injected clocks, and added an optional clock to source conformance execution/object-read deadlines. Default clocks retain their existing real-time behavior.
Cross-foundation wedge fixed on this branch
The combined scenarios also exposed a defect that neither reviewed root can see alone. #1173 withdrew a metric's control total whenever any cell declared it
missing/delayed/unsupported, including when the declaring constituent staged no rows at all. #1175'svalidate_monetary_contentrequires the total for a monetary column every retained row carries. So the join sealed an immutable publication the obligation ledger then refused withMONETARY_TOTAL_MISMATCHon the replay of every retry -- permanently wedging the obligation for the one answer per-metric evidence exists to give: a buy that delivered nothing and whose billing is pending.The fix distinguishes the two shapes rather than exempting money from the withdrawal:
No control total ever sums a value a cell disclaims, and no sealed publication can be one the ledger only ever refuses.
Monetary columns have no third option: dropping the total is refused by the ledger, and keeping it would falsify the evidence. A withdrawn monetary cell whose own constituent's rows report that metric now raises
ValueErrorbefore anything is staged or sealed.ReportingDefinitionBinding.to_wire()deliberately keeps the frozen unit declarations off the wire so retained contract hashes do not move, soReportingContractIdentityV1carries no units and the source cannot derive them. A new keyword-onlyInlineReportingSource(monetary_metrics=...)lets a trusted caller declare #1171's custommonetary_metric_unitscolumns;spendis always included and the default is unchanged behavior.Reachability boundary: this requires
coverage.expected == "partial".ReportingProducer._build_sliceemitsexpected="full", and afullslice returns retryablePARTIAL_RESULTbefore sealing, so the wedge sits on the adopter-built partial-coverage path -- which is the only path on which #1173's withdrawn cells can publish at all. Manifest and control-total bytes for a result with nocell_availabilityare byte-identical to the #1176 base.There are 69 new cases across the integration module and harness regressions. They cover colliding EUR/USD identities, sparse per-metric evidence, zero versus unavailable spend, obligation-currency totals, validation before staging/sealing, immutable rows during an awaited stage, post-seal retry with changed adapter currency/evidence, store and fresh-process restart, worker-to-record publication, populated legacy-schema upgrade followed by exact-revision reconciliation, zero-row snapshot/official coexistence, a no-row unavailable monetary cell that still commits its neighbour's subtotal, a withdrawn monetary cell contradicted by its own rows failing before any seal, and a custom frozen monetary metric distinguished on both branches. These cases are not marked
integration; real PostgreSQL variants run wheneverADCP_PG_TEST_URLis set. Memory restart explicitly restores a test fixture image.Reviewed roots and preserved history:
3c405a21f978ed9d3208611bb4a7a8434a0569337fd88c6710c26211484032bdd0e51fcd1077e6edff584b6fbe3436e7afa4980a14e2e5e4004a06a36c7ffa506612e5fa33a6adc4b097987aea2303c5d0308e6e22f3329f95ff8d558dff95601f81c6ff037de4ac822ecefb2f95d32c15c297fb4c45d683037de4ac822ecefb2f95d32c15c297fb4c45d683#1176 was approved at that exact remote head before branch creation. The branch was created from that head and joined with:
Both reviewed histories are parents of the integration merge. The foundation PRs retain their branches and bases.
Validation ran on static commit
037de4ac822ecefb2f95d32c15c297fb4c45d683, with Python 3.12.14 and real PostgreSQL 16.14.ADCP_PG_TEST_URLpointed at the local PostgreSQL server; each test owns an isolated schema.Both installed environments ran the workspace's
check_installed_resources.pywithpython -I, verifying imports from their ownsite-packages, the combined constructor signature, the keyword-onlymonetary_metricsseam,py.typed, all four bundled SQL files and all 3,147 schema JSON files byte-for-byte against the static checkout. Each installed artifact also bootstrapped the full 12-table PostgreSQL ledger twice in a fresh isolated schema.Artifact SHA-256 values:
adcp-8.0.0b15-py3-none-any.whl7f3f7eea17d90d91a3c38df1929566835de7de730fb7f56e5b7a19d09d10e15eadcp-8.0.0b15.tar.gz5eeb944b48201538b4e7df1fa069182b8f14ae48b3742465db320f68da5ce178Byte drift was measured directly at each root with one legacy no-opt-in input.
origin/mainand #1173's head7fd88c67both produce manifest bytesb1d4ab57...0912f42; #1175ff584b6f, #11763c405a21,d0308e6eand the verified head037de4acall produce38afa6c0...fb286233. #1173 introduces no drift, the"unit":"USD"on the spend total is #1175's intentional currency pin, and the wedge fix adds none.Residual boundaries for reviewers:
monetary_metrics, because the frozen slice request cannot carry the trusted unit declarations. An undeclared custom money column keeps feat(reporting): support per-metric source availability evidence #1173's non-monetary behavior and can still wedge for that column; a producer-side wiring belongs in a follow-up. Both branches are regression-covered.ReportingProducerhas no partial-coverage path yet, so per-metric unavailability is reachable only on adopter-built slices._control_totalspinsunitonly onspend, so the manifest unit check is a no-op for a custom monetary total. Changing that moves manifest bytes for those adopters.reporting_ledger_changes.committed_atdefaults tonow()whilereporting_reconciliation_changes.committed_atdefaults toclock_timestamp(). Each is self-consistent with its own feed and predates this stack; ordering usesseq.test_a_covered_zero_row_constituent_keeps_zeros_only_for_its_available_metricsnow retains the neighbour's checksum, and a newtest_a_withdrawal_only_removes_the_total_its_own_rows_could_corruptpins the shape feat(reporting): support per-metric source availability evidence #1173 did test.Open workspace in Conductor