Conversation
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — currency-freezing on reporting obligations verified clean.
Checked: trust boundary holds (currency resolves from trusted seller state at obligation creation, persists before source work, reuses frozen value across retries/restarts/restatements/adjustments); fail-closed on unresolved legacy history (CURRENCY_UNRESOLVED) with no backfill or USD default; definition/monetary-unit Postgres round-trip via to_storage; hash stability for existing configs; idempotent legacy replay; exact-sum precision; deep-copy before staging awaits; idempotent SQL migration with shape check and immutability trigger.
No blocking gates apply: high_risk false, gated_paths false, no no-auto-approve team match, first review. No critical/high/medium findings surfaced by the reviewer. Falls through to row 9.
Non-blocking nit (not a finding): the additive-but-behavior-changing currency requirement ships under a fix: prefix on the beta train — worth confirming the semver signal is intentional, but not a public-surface break, so it does not gate.
Independent review of #1175 found four defects in the currency freeze and fixed them. An obligation with no retained currency aborted the whole worker turn. `_acquire_pending` raised out of `run_worker`, so one legacy period starved every later period under the same configuration on every turn, forever. Acquisition now gates on the frozen code only once it actually needs it -- after the terminal/satisfied checks -- so a settled legacy period stays the no-op it already was, and an unresolved one is recorded in `WorkerTurn.slices_failed` and escalated like any other stuck slice. A direct `acquire_obligation` call still raises `CURRENCY_UNRESOLVED`; nothing is read or written for a quarantined obligation. A money column that only some rows carried was rejected outright, even though `InlineReportingSource` deliberately declines to publish a control total for exactly that shape. Sparse columns now validate every reported value and are left unsummed. A published total still reconciles exactly against every row, a column every row carries still requires one, and a pinned monetary control total is still always required. `null` reads as "not reported" -- never zero, never a currency label -- for money cells and row `currency` alike. Reading `GetMediaBuyDeliveryResponse.currency` emitted a DeprecationWarning on every fetch; an adopter running warnings as errors saw an opaque `PROVIDER_TRANSIENT` failure instead of a published slice. The label is still checked against the frozen currency, now read from stored state, and typed, duck and slotted holders all behave the same. `InMemoryReportingLedgerStore.commit_revision` reached its monetary invariant before its row-count invariant, so the two stores answered the same miscounted revision with different codes. Memory now checks the count first, exactly as `PgReportingLedgerStore` does. `adcp.reporting.currency` is also a declared lazy public submodule, and the docs describe the migration and partial-money behavior as implemented. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean subsequent pass, no blocking findings.
This is a follow-up review of PR #1175 (reporting ledger / currency handling). The reviewer found no Critical/High/Medium findings across the delta: currency.py sparse-metric handling, producer.py currency-gate move with scoped ReportingCurrencyError catch, store.py row_count hoist, inline_source.py deprecated-currency read, additive init/ledger exports, docs, and tests. All logic paths are internally consistent and well-tested, escalation is operator-visible, timeouts are preserved, and no public-surface removal, forward-compat, import-layering, or ctx-metadata mandatories were touched.
Decision-table walk (top-down):
- Row 1 (critical/high): no such findings.
- Row 2 (gated_paths unsatisfied): gated_paths=false; also review_decision=APPROVED. N/A.
- Row 3 (deleted high-risk): high_risk=false. N/A.
- Row 4 (medium data-loss/schema/infra): no medium findings. N/A.
- Row 5 (modified high-risk + medium): high_risk=false. N/A.
- Row 6 (sticky escalate): prior decision was approve, not escalate. N/A.
- Row 7 (no-auto-approve team): no team match. N/A.
- Row 8 (≥3 medium): zero medium findings. N/A.
- Row 9: approve.
Prior decision was also approve; this pass confirms the delta remains clean. No author-team gate and no path gate constrain the outcome.
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>
1 participant
The producer previously copied one process-wide currency into every source request, so an EUR account could be reported as USD. This change resolves currency from trusted seller account/configuration/definition history at obligation creation, persists it before acquisition, and reuses the stored value for retries, restarts, snapshot restatements, official publications, adjustments, status reads and audits.
Closes #1171.
Dependency and stacked base
conductor/reporting-account-scoped-keys-1169until fix(reporting)!: scope configuration generations by account #1174 merges; retarget tomainafterward.91fa2786efdd3117ce2c1a875ede7431767d7e87.6c74a458b8ee2a34c47c154186b950faed50f4fa.Behavior and compatibility
CurrencyResolvercallbacks andFixedCurrencyResolver.ProducerOfferings(currency=...), including its USD default for single-currency deployments, remains supported.spendis monetary by default. New low-level obligation writes explicitly supply currency. Existing wire schemas remain unchanged.Migration policy
The new idempotent in-place migration follows #1169 under the same advisory lock and transaction. It adds a nullable currency column with no default, validates code shape, and prevents updates to a stored currency.
Preserve unknown history and fail closed. Neither beta.15 nor #1169 retained enough trusted evidence to prove arbitrary historical currency. All existing obligations remain
NULL; no USD/current-account/adapter-derived backfill occurs. Their rows, revisions, adjustments, hashes, consumer statements, issues and change sequences are preserved. Exact legacy record replays stay idempotent, and status/content reads remain available. New acquisition, revisions and adjustments for those obligations fail withCURRENCY_UNRESOLVED; status projectsHISTORY_UNAVAILABLE/action_required/contact_seller.Backfilling USD or today's account currency was rejected as invented evidence. Recovery from authenticated external historical definition/account/scope evidence requires a separate reviewed adopter migration with an audit trail; the SDK supplies no unchecked repair API. Stop and drain all older writers before upgrading. Deployment steps and alternatives are documented in
docs/reporting-ledger-migration.md.Reviewer fixes
An independent review of this PR reproduced and fixed four defects in the delta;
ff584b6fcarries them with their regressions._acquire_pendingletCURRENCY_UNRESOLVEDescaperun_worker, so one pre-upgrade period starved every later period under the same configuration on every turn, forever. Acquisition now gates on the frozen code where it is first needed -- after the terminal and satisfied checks -- so a settled legacy period stays the no-op it already was, and an unresolved one is reported inWorkerTurn.slices_failedand escalated like any other stuck slice. A directacquire_obligationcall still raises, and nothing is read or written for a quarantined obligation.InlineReportingSourceproduces when it declines to publish a control total, so conforming slices could not commit. A sparse column now validates every reported value and is left unsummed; a published total still reconciles exactly against every row, a column every row carries still requires one, and a pinned monetary control total is still always required.nullreads as "not reported" -- never zero, never a currency label.DeprecationWarningper fetch. An adopter running warnings as errors saw an opaquePROVIDER_TRANSIENTfailure instead of a published slice. The label is still checked against the frozen currency, now read from stored state; typed, duck-typed and slotted holders all behave identically.InMemoryReportingLedgerStore.commit_revisionreached its monetary invariant before its row-count invariant, so a miscounted revision returned a different code thanPgReportingLedgerStoregave. Memory now checks the count first.adcp.reporting.currencyis also declared as a lazy public submodule, and the migration and partial-money documentation now matches the implemented behavior, including that a period with no rows owes no derived monetary total.Validation
tests/conformance/reporting/plus the ledger, inline-source and source-contract unit suites. 341 before the reviewer fixes, 350 with their regressions, 354 with the empty-period boundary case.make ci-localwithADCP_PG_TEST_URLenabled: 8,566 passed, 34 skipped, 9 deselected, 1 xfailed in 21m19s, 84.98% coverage. Generated-code validation passed. Noinline_sourcedeprecation warnings remain anywhere in the run.src/and on every changed file, Black on every changed file: passed. Strict mypy: 1,353 source files and all 27 adopter fixtures passed, with the no-suppression contract check passing.py.typedis included. Installing the wheel into an isolated environment resolvesadcp.reporting.currencylazily and runs the partial-money path.The shared memory/real-PostgreSQL cases cover concurrent EUR/USD accounts in one producer, retry and restart (including closing every application connection), restatement, official publication and adjustment, invalid codes, adapter and total mismatches, mixed scope, mutable source rows, and literal beta.15/#1169 migration with concurrent and repeated upgrades. The diff was reviewed against the updated stacked base and
origin/mainfor trust boundaries, immutable resolution, migration evidence and compatibility.Residual risks
WorkerTurn.did_workstays true every turn. A supervisor that backs off only on an idle turn will keep polling;WorkerTurn.escalatedpast the recovery deadline is the alerting signal.Open workspace in Conductor