Repository navigation
Conversation
bee1cfc to
7b5aefa
Compare
7b5aefa to
0b35fe9
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean additive reporting-subscriptions feature, no blocking findings.
The reviewer surfaced no Critical/High/Medium findings. Walking the decision table top-down:
- Row 1 (critical/high): no findings — skip.
- Row 2 (gated_paths): false — skip.
- Row 3 (high-risk deletion): high_risk is true, but both reasons are
(modified), no(deleted)— skip. - Row 4 (medium data-loss/schema/infra): no medium findings — skip.
- Row 5 (modified high-risk + medium): no medium findings — skip.
- Row 6 (sticky escalate): no prior decision — skip.
- Row 7 (no-auto-approve team): no team match — skip.
- Row 8 (3+ mediums): zero mediums — skip.
- Row 9: approve.
The two high-risk (modified) files (src/adcp/server/init.py, principal.py) carry only additive wiring (ReportingSubscriptionOptions export, lazy submodule registration) with no removed/renamed public symbols. Reviewer confirmed correct semver signal (feat(reporting):), clean import layering (no _generated.py / numbered-class imports, no hand-edits under domains/**), real in-process parity testing rather than mocks, no credential surface, and docs consistent with code. No escalation trigger present.
0b35fe9 to
7e9ccf6
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean reviewer pass with no findings.
What I checked:
- No critical/high/medium findings (rows 1, 4, 5, 6, 8 do not fire).
gated_paths: falseandreview_decision: APPROVED— row 2 does not apply.high_risk: true, but reasons are(modified)entries onsrc/adcp/server/**with no medium-or-higher finding — row 3 (deletions) does not fire; row 5 requires a medium finding, which is absent. Modified high-risk files with no concerns are presumed safe.- No no-auto-approve team match — row 7 does not fire.
- Prior decision was
approve, so sticky-escalation (row 6) is inapplicable.
Substance: Purely additive buyer-client side — new consumer.py (412 lines) plus a one-site engine refinement. expected_subscription_periods delegation matches the coverage.py signature; readback parsing fail-closes on kind=="failed" and missing data; setdefault version retention correctly treats subscription versions as immutable. Engine _chain_start advances past post-retention re-grant per the coverage rule, confirmed by new test. feat(reporting): is the correct semver signal for additive public exports; new exports are client helpers (not server handlers), so AGENTS.md's handler table does not apply. Tests exercise the buyer-vs-seller period parity both unauthorized and after re-grant.
Falls through to row 9 → approve.
7e9ccf6 to
bf635a8
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean delta, no blocking findings.
This PR adds a reliable-reporting-subscriptions subsystem (buyer-side consumer, engine, coverage/resolution/registry modules) plus comprehensive tests, and makes minor modifications to src/adcp/server/__init__.py and src/adcp/server/principal.py.
Checks:
- No critical/high/medium findings from the reviewer.
- The new
consumer.pyis well-tested against a real in-process seller; theexcluded(required non-nullable list) vsgaps(nullable) asymmetry at consumer.py:476-477 is correct per the wire contract. - The engine.py
max_accountschange (sticky served-slots replacing_chain_startordering) is deterministic, matches the TS SDK canon, and leaves no dead imports; it is covered by updated tests. high_riskis true only from(modified)entries onsrc/adcp/server/**, but with zero medium-or-higher findings the modifications are presumed contract-preserving (row 5 does not fire).gated_paths: false;review_decision: APPROVED; no no-auto-approve team match.- Prior decision was
approve— consistent with this clean pass.
Decision table: rows 1–8 do not fire → row 9 approve.
bf635a8 to
7788425
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — buyer-client side of reporting subscriptions, all additive.
This PR adds the consumer/tracker surface for reporting subscriptions: new consumer.py (ReportingSubscriptionTracker, sync_reporting_subscriptions, read_reporting_subscriptions, load_subscription_coverage, is_subscription_change) plus an optional ReportingSubscriptionDeliveryHost hook in engine.py. The reviewer verified:
- The
_ended→ delivery-hook plumbing is safe:.pop("_ended")(engine.py:2371) runs beforeupdate_subscription_status, and_accounts_unauthorizedalways sets_ended, so the internal key never reaches the wire. _record_loss(consumer.py:445) is idempotent via itslast.unauthorized_at is Noneguard.- Import layering is clean — domain-path and
adcp.typesimports only, no_generated.py. - New helpers are SDK utilities over existing tasks (not task handlers), so AGENTS.md's handler table does not apply — no missing-handler-entry medium.
- Tracker/engine parity asserted against a real in-process seller.
Decision-table walk: no critical/high/medium findings (row 1, 4, 5, 6, 8 don't fire). gated_paths: false (row 2 N/A); review_decision is APPROVED anyway. high_risk is true but the two matched server files are (modified) with no medium-or-higher finding against them — row 3 requires (deleted), row 5 requires a medium finding; neither fires. No no-auto-approve team match (row 7 N/A). Prior decision was also approve. Falls through to row 9 → approve.
7788425 to
e998074
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — net-new additive experimental feature (AdCP 3.3 reporting-subscriptions), clean on a subsequent pass.
What I checked:
- No findings at any severity in this run; no blocking findings.
high_risk: truebut reasons are all(modified)onsrc/adcp/server/**and_forward_compat.pywith zero medium-or-higher findings — presumed safe per the modification rule. No(deleted)entries.gated_paths: falseandreview_decision: APPROVED— no gated-path escalation.- No no-auto-approve team match.
- Prior decision was
approve; this subsequent pass surfaces no new findings — a net-improvement/clean continuation, not sticky escalation.
Repo-gate spot checks (all clear):
- Reporting/subscriptions package is net-new against base, so edits to
resolution.can_produce/zone andUTC_ZONE_NAMESnarrowing touch unreleased code — no semver-against-released break; new exports additive (feat:). - Forward-compat preview shim patches generated models at runtime idempotently (
_add_fieldguards), not adomains/hand-edit or_generatedlayering violation. - PrincipalService preserves 3.2 behavior without the opt-in options.
- No credentials routed through
ctx_metadata. - engine.py withheld path (L1317-1327):
pop("_ended")runs before the delivered-is-None continue, so the internal key is stripped before payload validation — safe. The_evaluatelock-drop across host I/O is documented process-local/provisional, not a finding.
Decision table: no row 1-8 fired → row 9 approve.
e998074 to
cbcb336
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass on the AdCP 3.3 reporting-subscriptions slice.
No critical/high/medium findings surfaced. Walking the decision table top-down:
- Rows 1 (critical/high) — no findings.
- Row 2 (gated_paths) — gated_paths is false; also review_decision is APPROVED.
- Row 3 (high-risk deletion) — high_risk is true, but all high_risk_reasons entries are
(modified), none(deleted). - Row 4 (medium data-loss/schema/infra) — no medium findings.
- Row 5 (high-risk modified + medium finding) — high_risk modified files present, but no medium findings, so no trigger.
- Row 6 (sticky escalate) — prior decision was approve, not escalate.
- Row 7 (no-auto-approve team) — no team match.
- Row 8 (3+ medium) — zero medium findings.
- Row 9 — approve.
What was checked: engine.py grant/loss ordering and timestamp guards, registry.py versioning/scope/shared-destination validation matching schema allOf conditionals, consumer.py tracker grant-history and expected_periods. Buyer==seller conformance asserted across ~20 scenarios plus cross-SDK parity. Renamed coverage-cases re-added, not dropped. New public exports are additive (feat). No credential-in-ctx_metadata, type-layering, forward-compat, or disabled-test violations. In-memory engine store is a signposted placeholder, not a finding.
Checked: no critical/high/medium findings, no blocking issues, all guards clear.
cbcb336 to
7bc9a3f
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
This is a subsequent pass on a large additive AdCP 3.3 feature (principal-level reporting subscriptions, experimental). The prior decision was a clean approve; the delta adds new consumer/engine/registry/wire modules plus PrincipalService wiring and full buyer/seller conformance tests.
Checked the load-bearing concerns for this repo:
- Version gating is fail-closed (unparseable/absent adcp_version drops the adcp#8585 fields for older callers).
- beta.1↔beta.2 field gate is the DR-0032/0033 translation pathway and is tested in both directions.
- Section-level 3.2 handling is the option-off path.
- Domain-path imports only — no
_generatedlayering break. - Exports are purely additive (
feat:correct — no semver break). - No credentials in
ctx_metadata. - No disabled tests, blanket type-ignores, or dropped lint rules.
No critical/high/medium findings. high_risk is true only on (modified) server/types files, but the reviewer found no medium-or-higher concerns, so the modifications are presumed safe. gated_paths is false and review_decision is APPROVED. No no-auto-approve team match. None of decision rows 1–8 fire → row 9 approve.
bokelley
left a comment
There was a problem hiding this comment.
Review: buyer client helpers and ReportingSubscriptionTracker
This covers the commits new in this PR (consumer.py). Coverage and engine findings are on #1696 and #1698. Both confirmed bugs below were reproduced end to end with the in-process Seller and Client from test_reporting_subscription_consumer.py.
Confirmed bugs
1. [High] The tracker never starts a new authorization chain if it missed the loss. consumer.py:349-351
When the last recorded grant is still open, observe_account returns early and ignores a changed authorized_at. The spec defines authorized_at as "when the caller's current authorization chain for this account began", so any change means a new chain.
- Repro (retention 2 days): the tracker observes the grant, and authority is lost on 10-03. The tracker does not refresh during the loss: a notification was missed, or the process restarted with records reloaded. The account is re-granted on 10-08 with a new
authorized_at.- The seller owes 10-01, 10-02, then 10-09 to 10-11.
- The tracker expects 10-01 to 10-11, six periods the seller never owes.
- No
gaps[]entry rescues it, because a re-grant after expiry claims no gap. - Those six periods become
obligation_missingintents inplan_consumer_statuses.
- Fix: if
authorized_atdiffers from the open grant's start, close the open grant and append a new grant at the newauthorized_at. The old grant's end is unknown, but it is at or beforeauthorized_at - retention; using the newauthorized_atas an upper bound only under-expects. Also handleauthorized_atmoving earlier (raise).
2. [High] expected_periods ignores the seller's timezone_cutovers and uses the tracker's own observation time. consumer.py:332-340, :399-416
It passes exceptions=coverage.excluded and gaps=coverage.gaps, but not coverage.timezone_cutovers. Instead it uses changes recorded by observe_account at observed_at.
- Repro: the seller switches the account NY→LA at 10-03T12Z (
effective_at10-04T04Z), and the tracker pollslist_accountsat 10-06T12Z.- The seller owes LA days from 10-04T07Z.
- The tracker expects NY days 10-04 to 10-06, then LA days.
- For three days the two sides expect different periods, each missing the other's.
- This also feeds the overlap bug in #1696 finding 2, since local changes are recorded at T rather than E.
- Fix: when
coverageis given, passcoverage=coveragetoexpected_subscription_periods, and drop locally observed changes that a seller entry covers. Treat local observation as a fallback only.
3. [Medium] observe_account(timezone="UTC") silently defaults the zone. consumer.py:318
A caller that forgets the zone gets UTC periods for an account-timezone offering, which the spec's iana_timezone rule forbids. It also makes the next real observation look like a timezone change. Fix: make timezone required.
Lower severity and questions
- [Low] A continuing re-grant is stamped at the buyer's poll time.
consumer.py:361-368
The wire does not carry the re-grant instant. Without backfill, the seller'sgaps[]stay authoritative and the buyer only under-expects, which is safe. Withbackfill_on_regrant,expected_atbecomesobserved_at + delivery_sla, so deadlines drift with polling. Could the tracker use the time thereporting_subscription_changednotification arrived, or should the spec put aregranted_aton the readback? - [Low] Prior versions fall back to the current configuration after a restart.
_sentis in memory. After a restart,version_configurationsis empty and prior versions silently use the current configuration. That is acceptable on the buyer side only if it is documented. Consider raising when the readback lists a prior version that the tracker has no configuration for, unless the caller opts in.
Tests
test_tracker_follows_revocation_and_regrant_and_matches_the_seller refreshes at exactly the right moments, so it cannot catch finding 1. No test changes the timezone while using the tracker. Missing tests:
- a loss the tracker never saw, followed by a new-chain re-grant;
- a timezone change observed after the seller's
effective_at; observe_accountwithout a timezone;- a tracker restart with prior versions in the readback.
Merge readiness: not yet. Findings 1 and 2 make the buyer expect periods the seller does not owe, which turns into false obligation_missing statuses: billing disputes in the direction the SDK is meant to prevent.
755f2f4 to
cf564ce
Compare
|
Thanks. Here is each finding against the current head ( Fixed
Lows
Tests cover each case: a missed loss with a new chain, a late timezone change, a missing zone, and a restart with prior versions in the readback. |
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass over PR #1699 (prior outcome: approve), no blocking or medium findings.
This delta adds the buyer client plus seller engine/registry and down-projection for adcp#8585/#8588. The reviewer read and verified the largest delta files:
- engine.py grant-chain asserts (L1876/L1981) hold because
_apply_grantonly leaves the final grant open andzip(grants, grants[1:])never pairs the open tail asearlier. - consumer.py L746-747 re-grant placement under retention under-expects rather than over-claims (sound).
- registry.py L2615-2620 version-reuse check and L2584 inactive_intervals maxItems guard fail closed.
- wire.py L2856-2871 account-wide cutover collapse is safe.
- Forward-compat change widens (enum→extensible token) rather than narrows — no high finding per the discriminated-union rule.
- adcp#8585/#8588 fields gate behind 3.3.0-beta.2 with beta.1 down-projection and parity tests, satisfying the DR-0032/0033 pathway-back rule (pathway + test present).
- All new public exports are additive (feat:), no breaking semver signal required; no disabled tests, no blanket type:ignore (specific-code ignores only).
Gate checks:
- Author team gates: no no-auto-approve match.
- gated_paths: false; review_decision: APPROVED (no hard path gate).
- high_risk: true, but reasons are all (modified) files with NO medium-or-higher finding → row 5 does not fire; no (deleted) entries → row 3 does not fire.
- No critical/high/medium findings → rows 1, 4, 8 do not fire. Prior decision was approve, so row 6 sticky does not apply.
None of rows 1–8 fire; falls through to row 9 → approve.
cf564ce to
1da0400
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass on #1699 (buyer tracker + seller engine/registry for AdCP 3.3 reporting subscriptions).
What I checked:
- Semver signal: additive
feat(reporting)carries the correct signal — new public exports only, no removed/renamed/retyped public exports, no required→optional field flips. Consistent with a minor, not a break. - Type-system layering: no
_generated.pyimports outside the allowlist, no numbered-class imports, domain-path imports allowed. No hand-edits underdomains/**. - Forward-compat unions: no changes to the load-bearing fallback arms / discriminators.
- Credentials: none in
ctx_metadata;_CREDENTIAL_SHAPED_KEY_SUFFIXESintact. - CI gates: no disabled tests, no blanket
# type: ignore. - Version pathway (DR-0032/0033):
_gate_subscription_fields(principal.py:2573) drops adcp#8585 fields for callers below 3.3.0-beta.2 and fail-closes when version is unknown; parity fixture + tests cover the pathway. - Largest-file citations all verified safe: engine.py:1267 isolates a failing subscription into
evaluation_errors(degraded-completion by design, host I/O outside_store.lock); registry.py:2427 percent-encodes path-template values closing segment escape; coverage.py:729<=branch guarded by regranted-is-None at :743/:751; consumer.py:611 re-grant dating usesmax(seen, last.unauthorized_at).
Gates: gated_paths false; review_decision APPROVED; no no-auto-approve team match. high_risk is true but reasons are all (modified) with no medium-or-higher findings, so the heuristic flag alone does not escalate (rows 3/5 require a deletion or a medium finding, neither present).
Decision: Prior decision was approve; this run surfaces no critical/high/medium findings. Rows 1–8 do not fire → row 9 approve.
ba587db to
5159dd1
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Clean pass — no critical, high, or medium findings in the current run.
Decision trace (top-down):
- Row 1 (critical/high): no such findings. Reviewer verified semver signal (additive exports only,
feat:commits, no public-surface break), forward-compat unions untouched, no generated hand-edits, no credential-in-ctx_metadata regression, and CI gates armed (no skipped tests, only coded type-ignores). Not triggered. - Row 2 (gated_paths unsatisfied):
gated_paths: falseandreview_decision: APPROVED. Not triggered. - Row 3 (deleted high-risk path): all high-risk reasons are
(modified), no deletions. Not triggered. - Row 4 (medium data-loss/schema/infra): no medium findings. Not triggered.
- Row 5 (high_risk modified + medium):
high_risk: truewith modified entries (server/init.py, server/principal.py, types/_forward_compat.py) but no medium finding exists, so the conjunction fails. Not triggered. - Row 6 (sticky escalate): prior decision was
approve, not escalate. Not triggered. - Row 7 (no-auto-approve team): no team match. Not triggered.
- Row 8 (≥3 medium): zero medium findings. Not triggered.
- Row 9: approve.
The high-risk flag correctly drew extra scrutiny to the modified server and forward-compat files; the reviewer examined them per the largest-file rule and found the contracts preserved (principal gating fail-closes with UNSUPPORTED_FEATURE, path-template injection closed via percent-encoding, per-subscription failure isolation intentional). New-file scaffolding for the reporting-subscriptions system dominates the diff. Prior run on an earlier head was also a clean approve with no findings — consistent.
5159dd1 to
d36dad9
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass with no new findings; clean net improvement.
This is a follow-up to a prior clean approve. The delta adds buyer-side consumer.py (497 lines), its tests, a docs paragraph, and a registry.py path-hardening change.
What I checked:
- registry.py L594-597:
quote(value, safe="")now encodes\as well (..\..\secret→..%5C..%5Csecret), preventing SMB/UNC-style backslash values from escaping a path segment. Covered by a test at test_principal_reporting_subscriptions.py:615 — correct shape, with coverage. - consumer.py load-bearing control flow (
observe_accountchain/grant bookkeeping,refreshregrant dating,expected_periods) follows the under-expect-never-over-claim discipline and is validated against a real in-process seller engine viabuyer == seller_owesacross six scenarios. - Import layering respected; new exports are client helpers (not ADCPHandler methods) and are documented in docs/reliable-reporting-service.md.
Gate status: gated_paths: false; review_decision: APPROVED; no no-auto-approve team match. high_risk is true (server/** and _forward_compat.py modified), but the reviewer surfaced no medium-or-higher concerns on those modified files, so the modifications are presumed safe (per the high-risk flag reading rules). No critical/high/medium findings.
Decision table: rows 1-8 do not fire (no findings, gated_paths false, no deletions, no team gate). Falls through to row 9 → approve.
d36dad9 to
b0deb60
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
This PR adds AdCP 3.3 reporting subscriptions: buyer tracker (consumer.py), seller engine (engine.py), and sync_principal/get_principal versioning and validation (registry.py, principal.py). The reviewer verified:
- All changes are additive public exports under a correct
feat(reporting):signal; no breaking change (sync_principal's new context arg is keyword-only with a default), so the semver signal is satisfied. - Capacity ordering (engine.py:1333-1339), re-grant dating on both sides (consumer.py:509-519, engine.py:1126-1134), path-template segment encoding (registry.py:2330-2343), and beta.2 response gating (principal.py:2478-2506).
- The version-translation pathway is the Droppable one, gated below beta.2 with beta.1-schema validation tests; the section is Refused with UNSUPPORTED_FEATURE when the option is off — so DR-0032/DR-0033 pathway handling and its test are present.
- Import layering holds (domain-path/adcp.types only, no
_generated.pyimports, no hand-edited generated code). - Buyer/seller conformance tests are load-bearing and period-exact.
Decision-table walk: no critical/high findings (row 1 n/a). gated_paths is false and review_decision is APPROVED anyway (row 2 n/a). high_risk is true but both reasons are (modified) with no medium findings on them, so rows 3-5 do not fire. Prior decision was approve, so row 6 n/a. No no-auto-approve team match (row 7 n/a). Zero medium findings (row 8 n/a). Falls through to row 9 — approve.
… subscriptions Buyer side of AdCP 3.3 reporting subscriptions: - sync_reporting_subscriptions() writes the sync_principal section, optionally with reporting_destinations, and returns the readback. read_reporting_subscriptions() reads it back through get_principal. load_subscription_coverage() reads subscription_coverage[] from the get_reporting_status summary view. Seller rejections raise ReportingSubscriptionClientError carrying the error code. - ReportingSubscriptionTracker keeps the buyer's own records: - every registered version's configuration; - grant history from list_accounts and accounts_unauthorized[]; - timezone changes. It re-reads on a relevant principal.changed notification, and expected_periods() computes what the seller owes from those records alone. Results are ExpectedReportingPeriod values, so they feed evaluate_reporting_ledger and plan_consumer_statuses next to configuration-derived expectations; withheld periods carry no expected_at and are never claimed missing. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…5 settles A re-grant within status_retention_days continues the authorization chain and leaves list_accounts' authorized_at unchanged, so the wire carries no re-grant instant. ReportingSubscriptionTracker.observe_account records the re-grant at observed_at when an account it saw lose authority is readable again with the same authorized_at. A later authorized_at still starts a new chain. The seller's gaps[] remain authoritative for unowed periods. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ptions Fixes from the expert review of ReportingSubscriptionTracker: - authorized_at is the chain start (adcp#8585), so any change in it starts a new chain, even when the tracker never saw the loss. The unseen loss is then placed as early as the facts allow: the last instant the account was seen readable, or authorized_at minus retention. The tracker under-expects rather than claim periods the seller never owed. An earlier authorized_at is refused. - expected_periods applies the coverage entry's timezone_cutovers. When the seller lists them, they replace the tracker's own late observations. - observe_account requires an IANA timezone; the SDK never substitutes UTC. - A re-grant within retention is dated from the principal.changed notification's changed_at when the account leaves accounts_unauthorized[], not from poll time. - After a restart, prior versions come from the readback's prior_versions[].configuration (adcp#8588). A missing one is an error unless require_prior_configurations is false. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
b0deb60 to
1cd9b0e
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — new buyer-side reporting-subscription tracker (additive public exports, non-breaking semver signal), with tests and docs.
What I checked:
- Purely additive public surface (
feat(reporting)) — no removed/renamed exports, no required→optional flips, no wire-shape or forward-compat regression. Correct semver signal. - Authority-chain reconstruction in
consumer.py(observe_account/refresh/_chains) is intricate but pinned by a strong end-to-end suite: buyerexpected_periods== sellerowed_periodsperiod-by-period across revocation, re-grant within/after retention, timezone cutovers, and tracker restart. - No V1 contamination, no generated-code hand-edits, no credential-in-metadata concerns, no import-layering violations.
Medium findings (non-blocking):
src/adcp/reporting/subscriptions/consumer.py:438—expected_periods()applies the passedcoverageentry without checkingcoverage.subscription_id, whileload_subscription_coveragereturns all entries for the account; a wrong entry silently reshapes obligations. A one-line guard fixes it.
Decision: 1 medium finding, no critical/high, no escalation trigger (gated_paths false, high_risk false, review_decision APPROVED, no team gate, prior decision was approve). Fewer than three medium findings → row 9 approve.
Medium findings
- src/adcp/reporting/subscriptions/consumer.py:438 — expected_periods() applies coverage without checking its subscription_id
ReportingSubscriptionTracker.expected_periods applied whatever coverage entry it was given. A buyer with several subscriptions that passed the wrong subscription_coverage[] entry would silently apply that subscription's gaps and timezone cutovers to this one. The tracker now raises when coverage.subscription_id differs from the subscription asked about. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — prior Medium resolved, no new findings.
This is a subsequent review responding to the prior run's single Medium finding (consumer.py:438 — expected_periods() applied coverage without checking its subscription_id). The delta adds a fail-closed guard in expected_periods() that raises ValueError when coverage.subscription_id != subscription_id, placed ahead of every use of coverage, plus a test (test_coverage_for_another_subscription_is_refused) exercising it. Since ReportingSubscriptionCoverage carries no account identifier, subscription_id is the only available cross-check, so the fix is correctly scoped.
No current findings. high_risk is false; gated_paths is false and review_decision is APPROVED; no no-auto-approve team match. The prior blocking concern is gone and nothing new was flagged — net improvement. Rows 1–8 do not fire; falls through to row 9.
Third slice of #1695: the buyer's client side. It targets
nextand includes #1698's commits until that merges; review #1698 first. The new commits here are the buyer-side ones.What it adds
sync_reporting_subscriptions(client, subscriptions, reporting_destinations=..., expected_configuration_version=..., dry_run=...)writes thesync_principalsection and returns the readback.read_reporting_subscriptions(client)reads the readback throughget_principal.load_subscription_coverage(client, account=...)readssubscription_coverage[]from the summary view. Seller rejections raiseReportingSubscriptionClientErrorwith the error code.ReportingSubscriptionTrackerkeeps the buyer's own records:get_principalkeeps only the current configuration; see ambiguity 3 in feat(reporting): compute owed reporting-subscription periods and their capabilities #1696);list_accountsauthorized_atandaccounts_unauthorized[];handle_notification()re-reads on a relevantprincipal.changed.expected_periods()computes what the seller owes from those records alone.from_capabilities()readsbackfill_on_regrantandstatus_retention_days.Consumer runtime fit. Subscription expectations are
ExpectedReportingPeriods, so they go intoevaluate_reporting_ledger(expected_periods=...)andplan_consumer_statuses(missing_expected_periods=...)next to configuration-derived ones, with no change to either. A withheld period has noexpected_atand is never claimedobligation_missingwhile authority is absent.adcp#8585
A re-grant within
status_retention_daysnow leaveslist_accountsauthorized_atunchanged, so the wire carries no re-grant instant.observe_account()records such a re-grant atobserved_atwhen an account the tracker saw lose authority is readable again with the sameauthorized_at, and the seller'sgaps[]remain authoritative. A laterauthorized_atstill starts a new chain. The readback'sinactive_intervalsand the coverageprevious_timezoneflow throughexpected_subscription_periodsunchanged.Tests
tests/test_reporting_subscription_consumer.pyputs a real in-process seller (PrincipalServiceplusReportingSubscriptionEngine) behind a thin client adapter, so the buyer sees only wire responses. The cases:principal.changed, with the tracker's periods equal to the seller'sowed_periodsboth while unauthorized and after the re-grant gap;obligation_missingkeyed by the subscription;authorized_at, matching the seller period by period.Still remaining for #1695
These are listed in more detail in #1698:
offering_idin the generation identity formatch: alland per-product resolution.subscription_coverageintoReliableReportingService.adcp-reporting-storage.path_templateexpansion and per-account destinations.🤖 Generated with Claude Code