Repository navigation
feat(reporting): serve reporting subscriptions from the principal runtime (#3504, 2/3) - #3508
Conversation
Add the pure expectedSubscriptionPeriods rule for AdCP 3.3 principal-level reporting subscriptions (experimental media_buy.reporting_subscriptions), the normative period_generation calendar it uses, selector resolution (exact, finest, all with byte-order ties), scope membership, and a builder for the 3.3 registration capabilities (configuration_tasks, the deprecated configuration_task, and the subscriptions block). createAdcpServerFromPlatform adds media_buy.reporting_subscriptions to experimental_features only beside a subscriptions block. installConfiguration now throws a typed ReportingConfigurationImmutableError with the unchanged message. The cross-SDK parity table is test/fixtures/reporting-subscriptions/coverage-cases.json. Refs #3504 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…al ships Advertising media_buy.reporting_subscriptions also requires adcp.principal.supported_sections to list reporting_subscriptions, which the SDK does not emit yet. Keep the capability builder pure in this slice and add the feature id together with the principal block in the runtime slice, so an adopter can never produce a 3.3-invalid capabilities response. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — AdCP 3.3 principal reporting subscriptions (opt-in), 19 files, additive.
No critical/high findings. Two medium findings only, neither in data-loss/schema/infra categories; high_risk is false and gated_paths is false, so no escalation row fires. With fewer than three medium findings and no team gate, this falls through to row 9 (approve).
Checked:
- Changeset present and correctly typed (minor, additive exports/options) — satisfies changeset-vs-wire-impact.
- Opt-off path byte-for-byte unchanged, proven by golden transcript.
- Witness-not-translator holds; version-downgrade (3.2/3.3) handling and tests present.
- CAS mutate callbacks side-effect-free (safe retries); host hooks thread AbortSignal.
Medium findings (non-blocking):
- src/lib/reporting/subscriptions/engine.ts:480 — readFacts silently truncates authorized accounts at 10k-page cap, which can fabricate accounts_unauthorized/withheld receipts.
- src/lib/server/create-adcp-server.ts:9844 — protocol.principal not stripped from experimental_features for <3.2 callers (backward-compat completeness).
Follow-up (non-blocking): confirm withSubscriptionCoverage's subscription_coverage field is stripped from get_reporting_status responses to <3.3 callers via response projection.
Medium findings
- src/lib/reporting/subscriptions/engine.ts:480 — readFacts silently truncates authorized accounts at 10k-page cap
- src/lib/server/create-adcp-server.ts:9844 — protocol.principal not stripped from experimental_features for <3.2 callers
The base branch was changed.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass on PR #3508 resolves both prior Medium findings with no new blockers.
This is a follow-up pass (delta: 233 lines, 5 files, mostly tests). The two Medium findings from the prior approve run are cleanly resolved:
engine.ts:480—readFactsnow fails closed: returns on terminal cursor and throwsrefusing a partial rosteron 10k-page exhaustion, with a test confirming no account is read as a revocation after rejection.create-adcp-server.ts:9844—protocol.principalis now stripped fromexperimental_featuresfor <3.2 callers, matching the adjacent 3.3-gate filters, with a 3.1 test.
The new subscription_coverage 3.3 gate in service/index.ts reads servedAdcpVersion via the same immutable-property pattern as ledger/response-version.ts; undefined-defaults-to-include is consistent with the ADCP_VERSION fallback elsewhere — no witness-not-translator concern. Changesets are present for the src/lib/** changes.
No Critical/High/Medium findings in the current run. Not high-risk, no gated paths, no author team gate. Falls through the decision table to row 9.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean refactor with no blocking findings.
This PR refines path_template placeholder validation in the PR-new reporting-subscriptions.ts, replacing a two-step regex approach with a single linear-scan templatePlaceholders helper that returns undefined on unpaired braces. Logic is correct: nested {, orphan }, and unclosed trailing { all yield undefined, and fail() throws (returns never) before the placeholders! loop, so there is no null-deref risk. New tests cover all three unbalanced cases.
Checked:
- No critical/high/medium findings surfaced by the reviewer.
- Changeset coverage: two
.changeset/*.mdentries present for the reporting-subscription changes touchingsrc/lib/**; the refactor refines a file already introduced in this PR, so no additional changeset is required. - Witness-not-translator: no mock/fallback data or wire-shape re-shaping introduced.
- No high-risk paths, no gated paths, no author team gate.
Decision table: rows 1–8 do not fire (no findings at all, high_risk false, gated_paths false, no team gate). Falls through to row 9 → approve.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
This is a subsequent review on a prior clean approve. The delta since the prior approve (head cb2c8a3) is a single line: removal of an unused DAY constant from test/lib/reporting-subscription-engine.test.js. The reviewer confirmed zero remaining \bDAY\b references (live timing uses DAY_MS), making this a safe dead-code cleanup with no breakage.
Checks applied:
- No critical/high findings (row 1 does not fire).
gated_pathsis false (row 2 does not fire) — notereview_decisionis REVIEW_REQUIRED but the gate is path-based and not triggered here.high_riskis false, no deletions/modifications of sensitive files (rows 3–5 do not fire).- Prior decision was
approve, notescalate(row 6 does not apply). - No no-auto-approve team match (row 7 does not fire).
- Zero medium findings (row 8 does not fire).
Falls through to row 9 → approve. Changeset coverage is present (.changeset/reporting-subscription-*.md). No blocking findings.
Adopt the rules adcontextprotocol/adcp#8585 settled: - Selection reads each offering's published standard_dimensions and unmapped_dimensions, and fails closed on an offering without them. - period_timezone: a utc offering matches only UTC or Etc/UTC; a source_timezone offering only its own published zone (never an account-resolved one). - The coverage function takes the readback's inactive_intervals in wire shape, per-offering timezone_cutovers with previous_timezone (account.timezone is now the current zone), and per-offering gaps. - The parity table fixes the billing-cycle case's scope (both keys, which maxProperties 1 forbids), adds cases for the settled rules, and its subscriptions are now schema-validated. The new wire fields are hand-modeled in subscriptions/wire.ts until 3.3.0-beta.2 ships. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
4097da6 to
b7268ff
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean delta, no blocking or medium findings.
This PR adds AdCP 3.3 principal-level reporting subscriptions: an opt-in seller runtime + coverage engine. The change is additive — responses are unchanged without the new options.
Checks that passed:
- Witness-not-translator: No fabrication/re-shaping. Version gating in create-adcp-server.ts and from-platform.ts correctly strips 3.3 fields below 3.2/3.3 and post-beta.1 settled fields below 3.3.0-beta.2, matched by wire-strip helpers and tested on the wire.
- DR-0033 pathway: Version gating + wire-strip helpers provide the backward pathway for the new wire fields, tested on the wire.
- Changesets: Two changesets present and correct for opt-in additive changes in beta pre-mode; package-size bumps measured.
- Fail-closed behavior: engine.ts readFacts bounds the roster walk at 10,000 pages and refuses a partial roster rather than reading missing accounts as revocations. Durable-store production guard sound.
- Path safety: encodePathSegment percent-encodes '/' and '.'/'..'; PATH_TEMPLATE rejects traversal/unknown placeholders.
- Ops: abort signals threaded, bounded CAS retry. Buyer/seller parity enforced by assertAgree harness.
One non-blocking note (calendarFor source_timezone asymmetry in engine.ts vs coverage.ts) was traced to an unreachable path and is harmless.
gated_paths is false and no no-auto-approve team match, so review_decision: REVIEW_REQUIRED does not force a gate. Prior decision was approve; no new findings. Falls through to row 9.
bokelley
left a comment
There was a problem hiding this comment.
Review: principal runtime and subscription engine (#3508, commits 5776a9b..b7268ff)
Checked against adcontextprotocol/adcp main (42dc2467, includes #8585). I drove createReportingSubscriptionEngineV1 directly from src/ with a stub principal runtime, an in-memory store and a controllable clock. Every item marked "verified" below is the engine's actual output.
Overall. Validation, versioning and the readback shape are solid. The engine's owed set is not stable over time, though. Elapsed periods disappear or reappear when host facts change, when an offering is withdrawn, or when internal state is pruned. Several cutover instants are stamped at reconcile time instead of when the change happened. owedObligations() is documented as "the seller's authoritative owed set", and billing settles on it, so I would fix the critical and high items before anything consumes it.
Confirmed bugs
1. Critical: withdrawing an offering, or a product dropping it, erases a buy's entire history. At engine.ts:721 and engine.ts:477, a buy-level offering_unsupported exception opens with from = max(accepted_at, confirmed_at). That is a raw past instant, not the cutover E at the moment the resolution changed. computeOwed and every buyer then remove every period from from on.
Verified with v1 accepted 11-01, a buy confirmed 11-02 with a flight from 11-03 to 12-31, and a UTC P1D offering A:
- reconcile at 11-15: owed 11-03 through 11-14
Agetswithdrawn_at: 2026-11-20(the supported path), or the product dropsA; reconcile at 11-21- owed: nothing.
excluded: [{reason: offering_unsupported, media_buy_id: mb, from: 2026-11-02T00:00:00Z}]
Per x-adcp-validation.cutover (a resolution change is a cutover) and "Exception from and until values are such E instants", 11-03 through 11-19 stay owed. These are elapsed and possibly billed periods.
Fix: open the exception at the cutover E of the change. That is ceil of the change instant on the outgoing calendars: the withdrawn_at or available_from instant, or the observation instant for product edits. Use max(accepted_at, confirmed_at) only when the buy never resolved under the current version. Do the same for a reopened exception: today a resolve → unresolve → resolve → unresolve sequence reopens from the original past instant.
2. High: pruning closed exceptions, gaps and timezone cutovers changes elapsed history. engine.ts:482-488 prunes them status_retention_days after until or effective_at. computeOwed (engine.ts:914-947) recomputes the entire history from whatever state remains.
- Capacity. Verified with
maxAccounts: 1andstatusRetentionDays: 7. Accountbhas capacity from 11-02 to 11-11. On 11-12 the owed periods forbare11-01, 11-11. After a reconcile on 11-25, the owed periods forbup to 11-12 are11-01 … 11-11, so nine periods the seller refused are now "owed". - Timezone change. Verified with an account that moves from New York to Los Angeles on 11-04. Before the prune, the owed starts are
11-02T05, 11-03T05, 11-04T05, 11-05T08, …. After the prune they are11-01T07, 11-02T08, 11-03T08, 11-04T08, 11-05T08, …: history is re-bucketed into the new zone, and a period appears that was never owed.
Spec: "an elapsed period never becomes owed retroactively", and exceptions only ever remove periods. Fix: retention applies to the wire projection, not to the state that defines the owed set. Either keep internal state for as long as any affected obligation is retained, or freeze owed periods at scope_resolved_at (the ledger materialization planned for slice 3) and stop recomputing elapsed history.
3. High: cutover instants are stamped at reconcile time, not when the change happened. The spec defines E from the instant T of the change. The engine uses nowMs in several places:
- Override closing (
engine.ts:466-470).until = ceil_subscription(now). It ignores the config'sactive_until, and it uses the subscription's calendar instead of the outgoing configuration's. Verified: a config is active from 11-01 until 11-10T12:00 (asreplace_account_configswould set it), and the reconcile runs at 11-12T03:00. The result isuntil: 2026-11-13, and the subscription owes from 11-13. Per the spec, the configuration owes through its period starting before T, so E = 11-11T00:00Z and the subscription owes from 11-11. 11-11 and 11-12 are owed by nobody.ReportingSubscriptionAccountConfigV1.schedule(engine.ts:105-111) and theuntil?: {at, calendar}slot (engine.ts:616) are declared for exactly this but never used. - Capacity and
action_required(engine.ts:691,702) open atceil(now), so periods between the account's coverage start and the first reconcile are owed. Verified in the capacity scenario above: accountbowes 11-01 although it was overmax_accountsfrom the start. - Revocation and re-grant. Revocation (
engine.ts:586-589) recordsunauthorized_atat the reconcile instant. A re-grant under an unchangedauthorized_at(engine.ts:530) is stamped at the reconcile instant. Verified: a revocation at an unknown time, observed 11-07T10, and a re-grant observed 11-09T10 give a gap[11-07, 11-10). With a slow reconcile cadence, data for periods ending after the real revocation counts as owed, and possibly deliverable, until the engine notices. - Account timezone change (
engine.ts:786): E =ceil_old(now)rather than the first old boundary after the actual change.
Fix: let the hooks report event instants (unauthorized_at, a re-grant instant, timezone_changed_at, plus the config's active_until and schedule), and compute E from them. Treat authorizationChanged as the primary path, and reconcile-time stamping as a documented fallback that reports a lag. When a capacity or action_required condition is first observed for a newly covered account, open it at the account's coverage start.
4. High (design): the owed set is recomputed from current host facts. owedPeriods (engine.ts:949-964) and readbackFor call listAccountBuys now and resolve against the static options.offerings.
- Removing a withdrawn offering from
offerings, instead of keeping it withwithdrawn_at, makes all its historical periods vanish. - A product whose
reporting_delivery_offering_idschanges, or a package added to or removed from a buy, re-resolves every elapsed period without a cutover.
These are separate from item 1. Until obligations are materialized, owedObligations() should not be described as authoritative. Either freeze the per-period resolution, or require hosts to supply dated product and offering history.
5. Medium: engine readback advances configuration_version. At runtime.ts:980, recordReportingSubscriptionCoverage issues a new configurationVersion whenever accounts_with_exceptions or accounts_unauthorized change. sync_principal: "configuration_version advances on every successful mutation … through this task or through sync_agent_notification_configs — and never on seller-driven setup-state transitions." Failure scenario: a buyer reads version V and the engine reconciles a revocation. The buyer's guarded sync_principal with expected_configuration_version: V returns CONFLICT for any section, even though it changed nothing. Fix: do not touch configurationVersion; principal.changed already signals the change.
6. Medium: if the replace_account_configs hook fails after commit, the replacement is lost. At runtime.ts:359-362 the hook runs after the principal row commits, with replaceAppliedVersion already persisted. If the hook throws, the caller gets an error for a write that was applied. A retry with the same body finds replaceAppliedVersion === version and never calls the hook again. The configs stay active, so the subscription stays overridden indefinitely.
Separately, the hook must deactivate configs only "on in-scope accounts where the subscription resolves an offering … never accounts it excludes, or with offering_unsupported or capacity". The host has none of that state; the engine does.
Fix: use a durable outbox and retry, or mark the version applied only after the hook succeeds. Pass the target account list from the engine, or provide an engine helper that computes it.
7. Medium: receiptRoster(accountId) crosses tenants. At engine.ts:1008-1030, store.listPrincipals() returns principals of every tenant, and the filter is only state.accounts[accountId]. If account IDs are tenant-scoped, tenant B's principals land on the receipt roster for tenant A's acct_1. Fix: take tenantId (or a full account scope) and filter listPrincipals by it.
8. Medium: a new authorization chain is ignored when the revocation was never observed. At engine.ts:525, if the last grant is open, applyGrant(authorized=true) returns without looking at fact.authorized_at. Failure scenario: the account is revoked and re-granted after retention between two reconciles. The host now reports a later authorized_at (a new chain), but the engine keeps the old chain. The seller owes the revoked interval; a buyer computing from the wire authorized_at owes from the new chain start. Fix: treat a later authorized_at on an open grant as a revocation followed by a new chain. If the revocation instant is unknown, bound it by the previous observation.
9. Medium: the destination method is frozen at acceptance, but the buyer reads the current destination. At reporting-subscriptions.ts:284-285,315, destinationMethod is captured once per version. Failure scenario: the buyer replaces destination archive under the same destination_id, moving from s3 to gcs, without bumping the subscription version. The seller keeps resolving s3 offerings, while a buyer computing with the destination's current pattern and transport resolves gcs offerings, or nothing. Fix: treat a destination method change as a resolution-change cutover, and re-freeze with a dated record. Alternatively, reject a pattern or transport change for a destination named by an active subscription unless the subscription version is bumped.
10. Medium: the release gate fails open, and is keyed to a version literal. wire.ts:83 serves the beta.2 fields when servedAdcpVersion === undefined. Every protocol dispatch path I traced sets validationVersion. A host that calls platform.getReportingStatus or the principal handlers from its own router, which is a supported composition, gets beta.2 fields validated against a beta.1 bundle.
The gate is also a semver literal (3.3.0-beta.2, with a TODO). If beta.2 is cut without #8585, the gate opens on a bundle whose closed schemas reject the fields.
Fix:
- Fail closed on
undefined, with an explicit opt-in for direct calls in tests. - Gate on the bundle's schema instead of a release literal: for example, whether the bundled
reporting-delivery-offering.jsondefinesstandard_dimensions, the waybundleSupportsAdcpVersionFieldalready works.
The gate is otherwise correct for protocol traffic. The served value is the validating bundle key, and get_adcp_capabilities, get_principal/sync_principal and get_reporting_status all strip the fields below beta.2.
Low
subscription_idcollision check is optional (runtime.ts:431). WithlistDeliveryConfigIdsabsent, asubscription_idequal to one of the caller'sdelivery_config_idvalues is silently accepted, although sellers MUST reject it. Require the hook wheneverconfiguration_tasksincludessync_accounts.- Readback truncation.
reporting-subscriptions.ts:444keeps only the last 64inactive_intervals, while the engine computes with all of them. After 64 toggles, a buyer recomputing from the readback owes periods the seller doesn't.accounts_with_exceptionsandaccounts_unauthorizedare silently cut at 1000 (runtime.ts:964,967). - Path templates can collide after expansion. At
reporting-subscriptions.ts:521, "templates must differ" is checked textually.{account_id}/{subscription_id}for subscriptionxand{account_id}/xfor subscriptionyboth expand toacct/x, so the second feed overwrites the first. Consider requiring{subscription_id}or distinct literal prefixes. Spec question:./..are encoded as%2E(reporting-subscriptions.ts:541), and RFC 3986 §2.3 tells normalizers to decode%2E. A transport that normalizes would turn.%2E(template.{account_id}, account.) back into... That is safe on S3 but worth a spec note for path-based transports. withheld_period_countheuristic (engine.ts:896,902-911). WithoutisPeriodDelivered, every past-due period is assumed delivered, so a late seller under-reports withheld periods.slaMsalso ignores Y/M components.- Multi-calendar override
from(engine.ts:450-452,699). Onefromequal to the maxceilacross offerings makes the earlier-ending offering owe periods that start after T.excluded[]has nooffering_id, so this is a spec limitation; please document it. - Capability refusal is one-way.
from-platform.tsrefusessubscriptionswithout the principal section, but not the reverse (reporting_subscriptionsinsupported_sectionswithoutconfiguration_tasksincludingsync_principal). Nothing checks thatlist_accountspopulatesauthorization.authorized_at, or that buys populatecompleted_at. Sellers advertising subscriptions MUST do both, and buyers cannot compute coverage without them. - Stale scope in
subscription_coverage. Entries are returned for every subscription with stored state for the account, including subscriptions whose current scope no longer includes it. The spec scopes entries to subscriptions whose scope includes the account, plus retained inactive ones. - "Option off is byte-identical" holds on the wire for the principal runtime (golden) and the service (
registrationkeeps key order). It does not cover:- the new per-release capability projection in
create-adcp-server.ts, which runs for every seller and is not in the golden PrincipalLifecycleRuntime, which now extendsPrincipalLifecycleRuntimeSubscriptionswith required members, a type-level break for custom implementations and mocks- a principal row written with the option on and later served with it off, which still returns
reporting_subscriptions
- the new per-release capability projection in
Tests
reporting-subscription-engine.test.js compares the buyer and seller, but both sides call the same expectedSubscriptionPeriods, so they agree by construction. The suite tests whether facts are plumbed through, not the rule itself. None of the divergences above is exercised. Please add:
- offering withdrawal and a product change mid-flight
- pruning after
status_retention_days - override close with a config
active_untilearlier than the reconcile - capacity for an account first observed after its coverage start
- an unobserved revocation followed by a new chain
- a destination transport change
- engine readback followed by a guarded
sync_principal
- An empty flight (stop at or before start) overlaps nothing. - A cutover carries the outgoing stretch's own owing start into E, so two cutovers inside one period never owe a period twice. - Sub-daily periods under a local alignment restart at each local midnight on wall-clock multiples (adcp#8588); calendars gain next() and the walk iterates by instant with an iteration budget and skip-ahead. - A billing anchor in a DST fold is interval zero itself. - A wire authorized_at equal to the chain start continues the chain. - The start rule lives in one function for the pending owner decision. - Schema validation accepts unknown reporting-dimension values. - The parity table grows to 79 cases with obligations on every case and non-wire inputs labelled. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
b7268ff to
24186cd
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — AdCP 3.3 principal reporting subscriptions (subsequent pass).
The single current finding is a Medium (not critical/high), so row 1 does not fire. gated_paths is false (row 2 N/A). high_risk is false (rows 3, 5 N/A). The Medium finding's category is a runtime reconcile edge case, not data-loss/schema/infra (row 4 N/A). Prior decision was approve, not escalate (row 6 N/A). No no-auto-approve team match (row 7 N/A). Only one Medium finding — fewer than three (row 8 N/A). Falls through to row 9 → approve.
Medium findings
- src/lib/server/principal/runtime.ts:1021 — recordReportingSubscriptionCoverage throws uncaught when a subscription owes >1000 capacity exceptions; engine.ts readbackFor builds those lists unbounded, so an all_authorized_accounts subscription on a roster larger than max_accounts can halt the principal's reconcile. Worth a follow-up to bound/guard, but a single Medium with no blocking trigger.
What I verified from the reviewer: the change is additive/opt-in (byte-for-byte unchanged without options); the ordinal→next() refactor in coverage.ts advances start strictly on every path and is bounded by max_iterations (no infinite loop); changesets are minor and correct; release-field stripping is the documented DR-0032/0033 version-translation exception. 1 Medium, no blocking issues → approve.
Medium findings
- src/lib/server/principal/runtime.ts:1021 — Reconcile fails permanently when a subscription owes >1000 capacity exceptions
The start bound is the first boundary at or after the later of the version's accepted_at and the account's authorized_at; a buy is owed every period that ends after its confirmed_at and overlaps its flight, so the period in which the buy is confirmed is owed in full. The rule stays in firstOwedBoundary. Parity table: the mid-period confirmed_at and hourly cases change, plus every case whose first period contains confirmed_at; new cases for an ASAP buy confirmed mid-day and a monthly billing buy confirmed on the 12th. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…time
Opt-in support for the AdCP 3.3 sync_principal reporting_subscriptions
section (experimental media_buy.reporting_subscriptions):
- createPrincipalLifecycle({ reportingSubscriptions }) validates, versions
and reads back subscriptions (core/reporting-subscription-state.json),
honours dry_run and expected_configuration_version, applies
replace_account_configs once per version through a host hook, derives
action_required from the destination, and emits principal.changed with
reason reporting_subscription_changed. It also exposes the adcp.principal
capability block.
- createReportingSubscriptionEngineV1 turns three host hooks into grant
history, max_accounts slots, precedence, exceptions at cutover instants,
timezone cutovers, re-grant gaps, accounts_unauthorized, the receipt
roster and the post-loss receipt window, and computes owed periods with
the same expectedSubscriptionPeriods rule buyers run.
- createReliableReportingService({ reportingSubscriptions }) advertises
configuration_tasks and the subscriptions block and adds
subscription_coverage to summary and periods views.
- createAdcpServerFromPlatform passes capabilities.principal and
capability_changes through, adds protocol.principal and
media_buy.reporting_subscriptions to experimental_features, refuses
subscriptions without the reporting_subscriptions section, and strips
3.3-only fields for callers pinned below 3.3.
Without the options every response is byte-for-byte unchanged; a golden
transcript captured from the pre-change runtime proves it.
Refs #3504
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… by release - readFacts refuses a roster that does not finish within the page cap, as it already did for a repeating cursor: a partial roster would read as revocations and fabricate accounts_unauthorized entries. - Callers pinned below 3.2 no longer see protocol.principal once the adcp.principal block is removed for them. - subscription_coverage is added only for callers served 3.3 or later; earlier get_reporting_status response shapes are closed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The placeholder regex backtracked polynomially on runs of '{' (CodeQL).
Tokenize braces in a single pass instead, which also rejects nested and
unbalanced braces explicitly.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Clean delta. Removes engine.acceptsReceipt (interface + impl) per adcp#8607 — revocation means no access, so there is no post-loss receipt window. The removal is tracked by changeset prose, an absence assertion, and a new replace_account_configs idempotency test. Reviewer confirmed no dangling callers (only the test's 'acceptsReceipt' in w.engine check remains) and that retentionMs stays load-bearing elsewhere in engine.ts.
No critical/high/medium findings. Only a single Low (stale JSDoc at engine.ts:1786 still referencing the removed "post-loss receipt window", outside the delta hunk) — non-blocking nit.
Changeset present (two .changeset/*.md files covering the reporting-subscription coverage + runtime changes), satisfying changeset-vs-wire-impact. No no-auto-approve team match; gated_paths false; high_risk false. Prior decision was also approve with no findings — net state unchanged / clean. Falls through to row 9.
Nit (non-blocking): src/lib/reporting/subscriptions/engine.ts:1786 — update stale JSDoc that still advertises the removed "post-loss receipt window".
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Checked against the decision table top-down:
- No critical/high findings (row 1 N/A).
- gated_paths is false (row 2 N/A); review_decision REVIEW_REQUIRED does not force escalation absent a gate.
- high_risk is false, no deletions (rows 3, 5 N/A).
- No medium findings in data-loss/schema/infra categories (row 4 N/A).
- Prior decision was approve, not escalate (row 6 N/A).
- No no-auto-approve team match (row 7 N/A).
- Zero medium findings, so fewer than three (row 8 N/A).
- Falls through to row 9 → approve.
Reviewer reports a clean pass: delta dominated by mechanical version bump (beta.7→beta.9), maintained schema sync (3.1.29→3.1.31), docs, and a dev-only performance harness. Load-bearing additions (shared content-keyed MCP validator cache with per-entry Ajv; compact-schema.ts remote-schema factoring with return-schema fallbacks) were verified. No witness-not-translator concern — validation/compliance fixtures do not reshape buyer wire data. Changesets present and correctly patch-scoped under beta pre-mode. Prior clean approval still holds.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean subsequent pass, no blocking findings.
This is a subsequent review following a prior approve (no findings). The current run surfaces no critical, high, or medium findings.
What I checked:
- Changesets present:
.changeset/reporting-subscription-coverage.mdand.changeset/reporting-subscription-runtime.mdcover thesrc/lib/**changes with a typed patch appropriate for an additive internal optimization. No hand-editedversionline. - Witness-not-translator: The core change (shared content-keyed MCP output-schema validators via
compactMcpSchemaForValidation) is fail-safe —normalize()returns the original schema on any unsupported graph ($async, dynamic/recursive scopes, relative/fragment IDs, duplicate resources, refs into annotation data) and only hoists relocatable nodes after refs are made absolute, so relocation cannot change validation. Validity-equivalence invariant is asserted against the official provider under fast-check fuzzing. No wire-shape re-shaping or fabrication. - Schema bump: 3.1.29→3.1.31 threaded consistently across package.json/scripts/docs/tests (maintained minor bump).
- Transport: No reimplemented HTTP/SSE; change is in reporting/validation layers.
Decision-table trace: no critical/high (row 1 n/a); gated_paths false (row 2 n/a); high_risk false (rows 3,5 n/a); no medium findings (rows 4,8 n/a); prior decision was approve, not escalate (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve.
…bscription-coverage
…ndle Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…/reporting-subscription-runtime
…cription tests Main adopted 3.3.0-beta.2, which no longer ships the beta.1 bundle and defines the fields settled after beta.1 (standard_dimensions, inactive_intervals, prior_versions[].configuration, previous_timezone, per-offering entries), so the release gate now emits them to beta.2 callers. inactive in coverage stays gated until beta.3. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Checked: this delta is a mechanical AdCP 3.3.0-beta.1→beta.2 regen (generated types, docs, changesets, version pins) plus three reviewed source changes — the legacy-ref constraint-satisfaction reconciler in creative-delivery.ts (size/duration from resolved refs only, fail-closed on unknown constraints), the typed RequestValidationError / dispatched:false pre-dispatch marker (no submitted-value leak, message preserved, per-site markers on the four VersionUnsupportedError throws), and the envelope-status ESM/CJS loader binding fix for #3529 (cache cleared on rebind, unresolved versions no longer cached). Changesets present and correctly typed.
Decision table walkthrough: no critical/high findings (row 1 n/a); gated_paths false (row 2 n/a); high_risk false (rows 3/5 n/a); no medium findings (rows 4/8 n/a); prior decision was approve so row 6 n/a; no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve.
Note: review_decision is REVIEW_REQUIRED, but since gated_paths is false this does not force escalation — row 2 only fires when gated_paths is true.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean subsequent pass, no blocking or medium findings.
This is a subsequent review (prior outcome: approve, no findings). The delta is the AdCP 3.3.0-beta.1→beta.2 adoption (version pins, regenerated generated types, docs) plus three load-bearing source changes merged from main: typed RequestValidationError/isPreDispatchError with dispatched: false markers, the legacy sync_creatives per-package ref reconciliation by constraint satisfaction with playerSizedRefs deprecation, and the #3529 ESM envelope-status loader binding.
Verified clean:
- Reconciliation logic sound: per-axis width/height/duration union is intentional and documented, acceptors fail closed on unknown size/duration keys, cache only stores resolved versions — no witness-not-translator / fabrication violation.
- Changesets present and correctly typed (minor for beta adoption/typed error, patch for regression fix) under beta pre-mode — no changeset-vs-wire-impact violation.
- No official-client or version-pathway violations.
Decision table: no critical/high findings (row 1 n/a); gated_paths false (row 2 n/a); high_risk false (rows 3/5 n/a); no medium findings (rows 4/8 n/a); prior decision was approve (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve.
Note: review_decision is REVIEW_REQUIRED, but gated_paths is false, so row 2 does not fire.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…/reporting-subscription-runtime
|
Ladon cannot review this PR until merge conflicts are resolved. |
…bscription-coverage Brings in #3548. The packed-byte cap keeps the higher of main's and this branch's. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…/reporting-subscription-runtime # Conflicts: # scripts/check-package-size.mjs
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Checked:
- No findings at critical/high/medium severity in this run (row 1 does not fire).
gated_paths: false→ row 2 does not apply despitereview_decision: REVIEW_REQUIRED.high_risk: false, no deletions/modifications triggers (rows 3–5 inert).- Prior decision was
approve, so sticky escalation (row 6) does not apply. - No no-auto-approve team match (row 7 inert).
- Zero medium findings, well under the three-medium threshold (row 8 inert).
Delta is CI/test-scoped: comment-only additions to scripts/check-package-size.mjs, a retired version-pin swap to the live ADCP_VERSION in a test, a one-line string collapse in a coverage test, and an empty-frontmatter changeset correctly declaring no bump for a CI-only change touching no published library code. No witness-not-translator, version-pathway, wire-shape, or changeset-impact concerns in scope. Falls through to row 9 → approve.
A sub-daily billing_cycle was routed to the local sub-daily calendar, which aligns to local midnight and ignores period_anchor, so every boundary was off by the anchor's time of day. adcontextprotocol/adcp#8622 makes a sub-daily billing_cycle invalid; refuse it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…/reporting-subscription-runtime
…nt_configs A later write that merely omitted replace_account_configs cleared both the applied marker and a committed-but-unconfirmed replacement, so a failed host hook was silently abandoned. An omitted flag now leaves both untouched; an explicit false clears them, so a later true applies again. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…t owed period end - An open action_required exception now takes the host's current pending_action (url, message, reason) when it changes, keeps its from, and notifies principal.changed. It used to keep the first action forever. - accounts_unauthorized last_owed_period_end takes the latest frozen period_end. Frozen periods are not stored in end order, so the last entry could under-state the window. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean delta pass with no blocking or medium findings.
Checked:
- No critical/high/medium findings surfaced by the reviewer.
gated_paths: falseand no no-auto-approve team match, so no hard gates apply (row 2/7 do not fire despitereview_decision: REVIEW_REQUIRED, since that only gates whengated_pathsis true).high_risk: false— no deletion/modification escalation triggers (rows 3/5).- Prior decision was
approve, so sticky escalation (row 6) does not apply. - Three focused bug fixes to the new, unreleased reporting-subscriptions feature, each tested and citing upstream basis (adcp#8622, JCS canonical comparison, ===false retry-preservation). Changesets present for the work, satisfying changeset-vs-wire-impact.
- No witness-not-translator violations: fixes fail-close/preserve rather than fabricate wire shapes.
None of rows 1–8 fire; falls through to row 9 → approve.
The merge-base changed after approval.
Second of the series for #3504. It contains #3505's commits, so merge #3505 first and review only the commits from
feat(reporting): serve reporting subscriptions from the principal runtimeonward. It targetsmainbecause the CI workflow runs only for PRs intomain. This slice adds the seller principal runtime, the coverage engine that turns subscriptions into owed periods and exceptions, status, lost authority, and capabilities. Everything is opt-in. Without the new options, behaviour is byte-for-byte unchanged, and a golden transcript captured from the pre-change runtime proves it (principal-reporting-subscriptions.test.js, "option off").What ships
1.
sync_principal/get_principal(issue item 1)createPrincipalLifecycle({ reportingSubscriptions: { listDeliveryConfigIds?, replaceAccountConfigs?, maxSubscriptions? } })sits besideprepareReportingDestinationanddeclarations. Without it the section is refused withUNSUPPORTED_FEATURE, as before.Validation (
src/lib/server/principal/reporting-subscriptions.ts) covers:all_authorized_accountsoraccount_ids;exclude_account_idsonly with all-accounts scopeperiod_timezoneonly with exactly one accountofficialfinality andconsumer_receipt;consumer_receiptrequires a destinationdestination_idmust exist after the same request applies, including a request that removes a destination still named by an active subscriptionpath_templatehas no./..segments and only the four placeholders;{account_id}is required, and the template too forfile_transfer, when the scope can cover several accounts{account_id}templatessubscription_idvalues are rejecteddelivery_config_idvalues is rejected. The reverse direction is exposed asreportingSubscriptionIds(scope)for the host'ssync_accounts.feed_purposeVersioning:
accepted_at; earlier versions stay inprior_versionswithsuperseded_at.activeandreplace_account_configschange in place.active: falsedeactivates, and the subscription stays readable asinactive.replace_account_configsis applied once per version, or when the flag turns true, through a post-commit host hook. Without the hook it is refused withUNSUPPORTED_FEATURE.dry_runandexpected_configuration_versionwork as they do for the other sections, because the same CAS transaction applies them.Readback is
core/reporting-subscription-state.json. The state isactive,inactive, oraction_requiredwithissues(when the destination is suspended, removed,action_requiredorrejected). A destination transition that moves a subscription toaction_requiredfiresprincipal.changed(reporting_subscription_changed; the reason was added to the union). The engine maintainsaccounts_with_exceptionsandaccounts_unauthorizedthroughrecordReportingSubscriptionCoverage, which notifies on every change.The runtime also exposes
capabilities, theadcp.principalblock withsupported_sectionsderived from its options, andlistReportingSubscriptions(readback plus retained version history).2 and 3. Coverage engine (issue items 2 and 3)
createReportingSubscriptionEngineV1(from@adcp/sdk/reporting/service). The host supplies only facts it alone knows:listAuthorizedAccounts(principal, { cursor, signal }), paged. It may carryauthorized_at(the chain start),granted_at(required for a re-grant inside the chain),timezonewithtimezone_changed_at(required on a zone change),operator,source_timezone, andpending_actionwithpending_action_since.listAccountBuys(principal, accountId): the buy facts (confirmed_at,start_time,end_time,canceled_at,completed_at, each package'sreporting_delivery_offering_ids, and optional prior flights)resolveRevocationInstant(principal, accountId, { lastObservedAt, observedAt }): when authority was actually lost. The result is clamped to that window.isPeriodDelivered(principal, period), forwithheld_period_countlistAccountDeliveryConfigs, the caller's own account-level configurations, for precedenceThe host can push an authority change at its real instant with
authorizationChanged(principal, accountId, { authorized, at }), or callreconcile(principal)to trigger a reconcile.The engine maintains:
unauthorized_atand re-grant timesmax_accounts, applied deterministically: served accounts keep their slots, then byte order. The rest getcapacity.sync_accountsconfiguration for the account andfeed_purposebeats every subscription, then itsaccount_idssubscription beats its all-accounts one (overriddenwithoverridden_by)offering_unsupportedper buy, or per package withpackage_idsexcluded_by_buyeraction_requiredwithpending_actiontimezone_cutovers, andgaps[]after a re-grant without backfillEvery exception opens and closes at an E instant (cutover rule). Exceptions are retained for
status_retention_daysafteruntil.State is sharded: one CAS document per principal (the
max_accountsslots) and one per (principal, account). Owed periods are frozen as they elapse (see the review round below).What it serves:
subscription_coverage[].createReliableReportingService({ reportingSubscriptions: { engine, resolvePrincipal } })adds it to summary and periods views. It is empty for an account the caller cannot read, so caller isolation is unchanged.accounts_unauthorized[], withunauthorized_at,last_owed_period_endandwithheld_period_count, forstatus_retention_daysor until re-grant.owedPeriods/owedObligations. These are computed with the sameexpectedSubscriptionPeriodsrule as feat(reporting): buyer-computable coverage for reporting subscriptions (#3504, 1/3) #3505, so the seller owes exactly what the rule implies minus its declared exceptions. Obligations carry frozen, byte-sortedmedia_buy_ids, the resolvedoffering_id, andreport_definition_id.receiptRoster(tenantId, accountId): which principals of a tenant owe receipts for an account, derived from active subscriptions.5. Capabilities
The service advertises
configuration_tasks: ['sync_principal', 'sync_accounts'], and['sync_principal']alone withaccountConfigurations: false. In that caseconfiguration_taskis omitted andinstall()no longer requiresaccounts.upsert. It also advertisessubscriptions: { supported, max_accounts?, backfill_on_regrant, dimensions? }from the engine's own settings.createAdcpServerFromPlatform:capabilities.principal(adcp.principal) andcapabilities.capability_changesthroughprotocol.principalandmedia_buy.reporting_subscriptionstoexperimental_featuressubscriptionsis advertised withoutreporting_subscriptionsin the principal'ssupported_sections. This addresses the review on feat(reporting): buyer-computable coverage for reporting subscriptions (#3504, 1/3) #3505.Callers pinned to 3.2 get neither
configuration_tasks/subscriptionsnor thereporting_subscriptionssection or feature id. Async_principal-only seller dropsreporting_deliveryfor them, as the spec requires. The end-to-end test runs with strict response validation on both 3.3 and 3.2.1.Conformance
test/lib/reporting-subscription-engine.test.jspits a seller (principal runtime, engine and host facts) against a buyer that keeps only what it can observe: its own requests,get_principalreadback (validated against the 3.3 schema),list_accountsauthorized_at,get_media_buys, andsubscription_coverage. Buyer periods must equal seller-owed periods, period by period. The cases follow the issue:confirmed_at, cancellation mid-flight, and early completionmatchchange, includingall)backfill_on_regrant, includinglast_owed_period_end,withheld_period_count, and caller isolationmax_accountsaccount_idsover all-accountsexactagainstfinestagainstallover package, package × creative and package × countryoffering_unsupported(buy and package) andexcluded_by_buyeraction_requiredDeviations and decisions
Obligations are computed but not yet materialized into the ledger.
owedObligations()is the seller's authoritative owed set. Writing those into the reporting ledger, so the existing producer executes them, conflicts with three ledger assumptions:(account, delivery_config_id, delivery_config_version)(configurationIddigest)evaluateReportingLedgerCoverageV1)That needs a ledger change in step with the PostgreSQL store, so it moves to slice 3 with storage (see Remaining).
The engine store is in-memory here.
ReportingSubscriptionEngineStoreV1is the sharded port (get/putper key,listKeys,listPrincipals(tenantId)), and production refuses a process-local store. The durable implementation overAdcpStateStore/PostgreSQL is in feat(reporting): buyer runtime and durable storage for reporting subscriptions (#3504, 3/3) #3509.withheld_period_countcounts the periods owed beforeunauthorized_atthat the host's requiredisPeriodDeliveredreports as undelivered.action_requiredcovers destination problems only. A subscription moves to it when its destination is suspended, removed,action_requiredorrejected. Avalidatingdestination is the seller's to finish, so it staysactive.Conformance to adcontextprotocol/adcp#8585 (commit
b7268ff7)inactive_intervals({deactivated_at, reactivated_at?}, oldest first, bounded at 64), and the engine feeds them to the coverage function in wire shape.previous_timezone. Cutovers and re-grant gaps are recorded peroffering_idwhen the covered offerings' boundaries differ, and as one account-wide entry when they agree.authorized_atnow means the start of the authorization chain, so a host keeps reporting the original value across a re-grant inside retention. The engine used to read a re-grant's time fromauthorized_at; it now stamps the re-grant when authority is observed again (an explicitauthorizationChangedstill carries its own instant). The conformance suite models a buyer that sees only the unchangedauthorized_atand the seller'sgaps[].createReliableReportingService({ reportingSubscriptions })refuses to construct unless every delivery offering publishesstandard_dimensions.inactive_intervals, and an account timezone change withprevious_timezone.Wire gating (please review). The SDK pins 3.3.0-beta.1, and that release's offering, subscription-state and coverage schemas are closed (
additionalProperties: false). Emitting a post-beta.1 field therefore fails the SDK's own response validation, which is strict by default outside production. The fields arestandard_dimensions,unmapped_dimensions,inactive_intervals,previous_timezone, a per-offeringoffering_id, andprior_versions[].configuration.So the seller stores and computes everything, and each response emits a field only when the schema bundle served to the caller defines it. The gate is
reportingSubscriptionReleaseFieldsV1insrc/lib/server/reporting-subscription-release.ts, which reads the served bundle's schemas. An unknown or absent served version emits none of these fields, so the gate fails closed. It opens on its own once the SDK syncs beta.2.Until then:
The new fields are hand-modeled until beta.2; #3505's body explains why I didn't sync the unreleased bundle.
Expert review round (2026-10-29), commits
4323ee42,36e2e4ccandf103466aCritical and high:
Withdrawn offering history erasure: fixed.
offering_unsupportedfor a withdrawal or product edit opens at the E of the withdrawal or observation, using the per-buy resolution history. A test covers a product dropping the offering mid-flight; the elapsed history is kept.Retroactive history: fixed. Owed periods are frozen as they elapse (
frozen.periods,frozen.through) and never recomputed.owedPeriodsreturns frozen periods plus computed ones ending afterthrough. Pruning paststatus_retention_daysremoves periods from the queryable set and never re-owes them.Reconcile-time stamping: fixed. Change instants come from source data:
granted_aton a re-grant inside the chain (required, or the reconcile fails)timezone_changed_aton a zone change (required)resolveRevocationInstantfor a revocationactive_untilforoverriddenCapacity and
action_requiredopen at the coverage start when first observed. Reconcile time is used only where the change itself is first observed (product edits,pending_actionwithoutpending_action_since).Medium and low:
configuration_versionbump: fixed.recordReportingSubscriptionCoverageno longer bumpsconfiguration_version.replace_account_configshook: fixed. It runs after the section commits. A failed hook stays pending and is retried on the next sync, including an unchanged one.receiptRoster/listPrincipalscrossing tenants: fixed. Both take a tenant id.authorized_atfollows an unseen revocation closes the old chain at the resolved revocation instant and opens a gap.listDeliveryConfigIdsis required unlessaccountConfigurations: false.inactive_intervalsrejects the sync instead of truncating. Coverage recording rejects lists over 1000. Per the follow-up bot review,engine.reconcileisolates a subscription over that limit instead of failing the whole principal: it keeps its last lists and is reported inreadback_overflowand throughonReadbackOverflow.path_templates that can expand to the same path for some subscription are rejected.withheld_period_countdefault: fixed.isPeriodDeliveredis required.from: partially fixed. Each exception opens at the E of its own offerings' calendars.excluded[]carries nooffering_id, so with several calendars the publishedfromis still one account-wide instant, the latest E. This is flagged on feat(reporting): buyer-computable coverage for reporting subscriptions (#3504, 1/3) #3505 as a spec gap.prior_versions[].configurationis stored and emitted, release-gated.Remaining (slice 3)
owedObligations(per-period constituents, multi-offering generations, sparse coverage completeness), PostgreSQL migrations in@adcp/reporting-storagefor the engine store and subscription history, and maintenance retention (item 6).expandReportingSubscriptionPathTemplateships here), andaccount_idon every row (item 4).principal.changed, and a client helper forsync_principalsubscriptions (item 8).Review follow-ups
3073c31c.readFactsfails closed when the roster walk doesn't finish, so a partial roster never reads as revocations.protocol.principalis stripped together withadcp.principalfor callers pinned below 3.2.subscription_coverageis added only when the served release is 3.3 or later. Tests cover a 3.1 capabilities pin and a 3.2.1get_reporting_status, both under strict validation.cb2c8a35.path_templateplaceholders are scanned in one linear pass (CodeQL polynomial-regex alert), and nested or unbalanced braces are rejected.Follow-ups (2026-10-30)
a1c4fbb3.subscription_coverage[]lists a deactivated subscription withstatus: "inactive"anddeactivated_at, and only while it still has retained obligations for the account. That means an owed period, frozen or still running, ending withinstatus_retention_days; after that the entry is omitted.inactiveCoverageStatus). A caller served a release without it sees the account-level status as before, withoutdeactivated_at. This matches adcp-client-python#1730.c5bc42f8.engine.acceptsReceiptand its post-loss receipt window are removed. Once authority is lost, the account is unknown to every task. feat(reporting): owner-scoped subscription receipts and statuses; coverage follows the grant #3528 implements the reduced-grant alternative.replace_account_configsonce per version,c5bc42f8.truereran the host hook. I could not reproduce it. The existing logic already applies once persubscription_version, or when the flag changes to true.activetoggle never reapply. An omitted flag (its default, false) followed bytrueapplies again, as the spec says.73120d5a. Omission now refuses to open a 65th interval, likeactive: falsedoes.authorized_atcomes from (not redesigned yet; a DX plan to make the SDK the single source is pending approval). There are two independent sources today:authorized_atfrom the host'slistAuthorizedAccountsfacts (ReportingSubscriptionAccountFactV1.authorized_at, the authorization chain start).granted_atmarks a re-grant inside the chain.authorization.authorized_atfromlist_accounts, which the host'saccounts.listproduces.Local checks
npm run typecheck,build:lib,lint:boundariesandcheck:package-sizepass. The file budget is +4 for the release gate module.npm run test:lib:fastpasses.reporting-subscription-engine.test.js(26),principal-reporting-subscriptions.test.js,reliable-reporting-service.test.jsandreporting-subscription-coverage.test.js(262 total).Refs #3504
🤖 Generated with Claude Code