Repository navigation
feat(reporting): buyer-computable coverage for reporting subscriptions (#3504, 1/3) - #3505
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>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — buyer-computable reporting-subscription coverage (slice 1/3).
Checked: five new source modules read in full (coverage.ts, calendar.ts, selection.ts, capabilities.ts, index.ts), from-platform.ts, changeset, package-size guard, and both test files. No no-auto-approve team match; gated_paths false; high_risk false; no prior decision.
Verified the unbounded ordinal loop at coverage.ts:794 is bounded by start >= latestStop and fails closed on max_periods (L814-816); authority/grant chain, cutover handoff, flight-at-resolution, timezone/exception-window logic are consistent and byte-order-stable; the 32-entry experimental_features guard rewrite is logically equivalent; changeset correctly minor for purely additive exports; no witness/fabrication issues (buyer-side pure computation). 67-case parity table plus invariant/calendar/selection tests are thorough.
Medium findings
- from-platform.ts:3169 —
media_buy.reporting_subscriptionsadvertised before the requiredadcp.principalblock (allOf[11]) is emitted, so an opt-in adopter could emit a 3.3-invalid capabilities response (documented ambiguity #11).
Single medium finding, no blocking issues, no escalation trigger — falls through to row 9.
Medium findings
- src/lib/server/decisioning/runtime/from-platform.ts:3169 — reporting_subscriptions advertised before required adcp.principal block exists
…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 — prior Medium resolved, no current findings.
This is a subsequent push responding to the earlier review. The sole prior Medium (from-platform.ts:3169 — media_buy.reporting_subscriptions advertised before its required adcp.principal companion block existed) has been fixed: from-platform.ts now advertises only media_buy.reporting_delivery in experimental_features and drops the premature id, which is fail-closed. The 32-cap guard recomputes correctly for the single-id case, and the changeset prose and test were updated to match.
Current run surfaces no findings. Decision table: rows 1–8 do not fire (no critical/high, gated_paths false, high_risk false, no medium findings, no team gate, <3 mediums), so this falls through to row 9 → approve. Note: gated_paths is false, so the REVIEW_REQUIRED status does not force a gate here.
… SDK does Align expected_subscription_periods with adcontextprotocol/adcp-client#3505 so that both SDKs owe identical periods. The spec leaves each of these choices open: - A dedicated period calendar. Years, months and days are local civil arithmetic with the day clamped from the origin. Hours, minutes and seconds are exact elapsed time. Calendar months (P1M billing cycles) are supported, and numeric UTC offsets are refused. - Cutovers are paired per buy over the union of its packages. A continuing offering hands off at E of its own outgoing calendar. A newly resolved one starts at the latest E of everything the previous regime resolved. - A period_timezone override moves account_timezone and account-resolved schedules into its zone, while utc and fixed-zone schedules match only their own zone. - Versions may carry in-place inactive intervals and their destination method. The coverage entry's timezone_cutovers are applied. - A re-grant continues authority only before unauthorized_at plus status_retention_days, and a re-grant without retention is an error. The TypeScript SDK's 46-case table is vendored and run as a parity test. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Checked:
- Wire-shape fidelity: new
wire.tsaligns hand-modeled 3.3 reporting-subscription types to settled spec #8585 (per-offeringstandard_dimensions/unmapped_dimensions, readbackinactive_intervals, per-offeringtimezone_cutovers/gaps,previous_timezone). Witness-not-translator holds — no fabrication or silent normalization. coverage.ts:timezoneFor(L234-248) fail-closes on missingprevious_timezone;resolveForBuy(L1235-1237) routes selector override vs account zone correctly; globalinactive_intervalsmerge (L939) sound givenbuildRegimesselects one version per boundary.selection.tsstandard()throws whenstandard_dimensionsabsent — no guessing. Every new error branch has a fixture case (L2079/L2158/L2246).- Changeset present and
minor-correct: theexpectedSubscriptionPeriodssurface is new in this unmerged PR, so reshaping its input breaks no published API.
Decision table: 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 (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve. (review_decision REVIEW_REQUIRED does not force escalate because gated_paths is false.)
bokelley
left a comment
There was a problem hiding this comment.
Review: buyer-computable coverage (#3505), checked against adcontextprotocol/adcp main (42dc2467, includes #8585)
I reviewed calendar.ts, selection.ts, coverage.ts, wire.ts, capabilities.ts and the parity table against core/reporting-subscription.json x-adcp-validation, core/reporting-schedule.json period_generation, core/reporting-subscription-{state,coverage}.json, core/reporting-schedule-offering.json, and the get_reporting_status / sync_principal docs. I also ran the calendar and coverage function directly against cases I worked out from the spec.
Summary. The pure function matches the spec on every rule I checked:
- Start rule. Coverage starts at
ceilof the latest ofaccepted_at,confirmed_atandauthorized_at. The code takes the max of the three ceilings, which is equivalent becauseceilis monotone. - Flight. The flight is evaluated at the period end. Periods are never clipped.
- Cutover. E is computed per incoming offering. I verified a
match: allv1→v2 change where B (New York) is added beside A (UTC): A hands off at 11-05T00Z and B starts at 11-05T05Z. - Reactivation, chains and gaps. Reactivation is a fresh start. Authorization chains and retention work, and gaps follow
floor(U)/ceil(R). Exceptions only ever remove periods. - Selection.
exact,finestandallwork, and ties go to the lowestoffering_idin byte order (Bbeatsa). Unmapped dimensions are handled, as are theperiod_timezonematrix and the configurablebilling_cycleexclusion. - Period arithmetic. Civil and elapsed arithmetic are correct. I checked midnight DST gaps and folds (Havana, Santiago, Beirut), month-end clamping, and Apia's zero-length day.
The problems are at the edges: inputs the buyer cannot get from the wire, a few untested spec corners, and parity-table claims that go further than the table does.
Confirmed issues
1. Medium: "computable from the wire" does not hold for three inputs, and the function silently assumes the favorable default when they are missing.
selection.ts:24-26,124-127andcoverage.ts:625-628:available_fromandwithdrawn_atare SDK-invented offering fields. No AdCP schema carries them, and a withdrawn offering simply disappears fromreporting_delivery.offerings. Failure scenario:finest [package], and the seller addspackage x creativeon 11-04T12. A buyer passes the offerings exactly as published today, with noavailable_from. It expectsoff_bfrom 11-03 and never expectsoff_a(11-03, 11-04), while the seller owesoff_afor 11-03 and 11-04, thenoff_bfrom 11-05. That is the table case "resolution change is a cutover" with the non-wire field removed.coverage.ts:94-95:prior_flightsis not on the wire either, becauseget_media_buysshows only the current flight. Failure scenario: a buy ends 11-06 and is extended to 11-08 at 11-07T12. A buyer without history expects 11-06, which the seller never owes (table case "flight extension adds only unresolved periods"). Reinstatement is the same.coverage.ts:117-121:source_timezoneforaccount_resolvedofferings can only be learned from an obligation'speriod.source_timezone, so the buyer cannot know it before the first obligation exists.
Fix. Make these inputs explicit rather than optional-with-a-default. For example, require prior_flights: [] and an offering_history acknowledgment, or document that the buyer MUST retain them. Raise a spec issue so that offering availability and flight history can be recovered from the wire. Otherwise "the seller owes exactly what the buyer computes" is true only for buyers that kept local history.
2. Medium: the parity table is narrower than the PR body says, and the cross-SDK corners are not in it. The cases I checked match the spec; I spot-checked the misaligned cutover, DST, timezone cutover, reactivation, resolution change and billing cycle. The gaps are these:
- "a monthly billing cycle with month-end clamping": the table's billing case is anchored on the 1st (
2026-01-01T05:00:00Z), so nothing clamps. Clamping is tested only in the TypeScript unit test (reporting-subscription-coverage.test.js:230), and the Python copy never sees it. - Several calendar edge cases are TypeScript-only or missing entirely:
- a DST gap or fold at a midnight boundary (Havana, Santiago, Beirut); the table's New York cases never put midnight in a gap
- a zero-length period (unit test only)
PT24HagainstP1Dacross DST (unit test only, and it uses an elapsed duration without comparing it to the civil one)
- Missing cases I would add, all derived from the normative text:
- (a)
match: allwith a new offering on a different alignment (per-incoming-offering E for a non-continuing offering, with a stretch owed by neither) - (b) a version change with T exactly on a boundary
- (c) reactivation exactly on a boundary, and an inactive interval spanning a version change
- (d) two authorization chains (expired re-grant, then a second revocation)
- (e) several interruptions inside one chain, with and without backfill
- (f) exception
from/untilvalues that fall between boundaries - (g) a
finesttie betweenBanda(byte order rather than locale order) - (h) a billing anchor on the 31st and on the 29th of February
- (i) a stop time equal to the flight start, and a buy canceled before
start_time
- (a)
- The table cases for resolution change and flight extension depend on the non-wire fields in item 1. Label them as such so the Python implementation does not treat them as wire-derivable.
3. Low: a billing_cycle anchor inside a DST fold does not reproduce the anchor. calendar.ts:201-202,238: the anchor is converted to wall time and back, and the fold rule picks the earlier offset. Verified: period_anchor: 2026-11-01T06:30:00Z (01:30 EST, the second 01:30) in America/New_York gives boundary(0) = 05:30Z, and ceil(anchor) jumps to December. The spec makes interval zero the anchor instant. Fix: return the anchor itself for ordinal 0. More generally, when a computed wall time equals the origin's wall time, resolve the fold with the anchor's own offset.
4. Low: building grants from wire values throws on a continued chain. The doc comment says authorized_at is "when this grant began" (coverage.ts:99-104). Under #8585, authorization.authorized_at stays at the chain's first grant after a re-grant within retention. A buyer that appends the re-grant using the wire value gets [{A, U}, {A}] and TypeError: Grants must not overlap (coverage.ts:439). Fix: document that the field is the re-grant instant the buyer observed, or treat authorized_at equal to the chain start after a revocation as a continuation with an unknown re-grant instant, relying on the seller's gaps[].
5. Low: the iteration count is unbounded even though output is capped. max_periods caps pushed rows, but the ordinal loop (coverage.ts:264-287) still walks every boundary up to min(regime.until, latestStop, now), including removed or unauthorized ones. Each non-UTC boundary costs two to four Intl.formatToParts calls, and the cache is cleared every 4096 entries. A PT1S or PT1M timezone-aligned offering over a long flight with an open exception does millions of Intl calls. Fix: skip ahead with ceilOrdinal past an exception's or gap's until, or add an iteration budget.
Questions (spec ambiguities both SDKs need to pin)
- Stop time on a boundary.
coverage.ts:271uses half-open overlap, socompleted_at = 11-04T00:00Zowes nothing for the 11-04 period. The normative text also says "the period containing the stop time is owed in full", and the instant 11-04T00:00 is contained in[11-04, 11-05). Your reading is the sensible one, since anend_timeat midnight is usually exclusive. A zero-length flight (start == stopat 11-03T12) owes the 11-03 period under the current code, which is consistent with "containing" but not with "overlap". Worth a one-line spec clarification and a table case for each. - E when the outgoing side resolved something but owed nothing. For example, its flight had not started, or it was accepted mid-period. The code uses the schedule-based
ceil(T), which can create a stretch owed by neither. The spec says "the end of the last of them", meaning the owed periods. Please confirm the Python SDK reads it the same way. - ICU tzdata drift. Boundaries come from the runtime's ICU tz database, and the buyer and seller may run Node versions with different tzdata, for example after a zone's rules change. Is that acceptable, or should the obligation's echoed boundaries take precedence when they disagree?
I have nothing to report on capabilities.ts or ReportingConfigurationImmutableError.
- 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>
|
Parity table update for the Python SDK (
|
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass on a previously clean-approved PR, no blocking or medium findings.
Checked (decision table, top-down):
- Row 1 (critical/high): no critical/high findings.
- Row 2 (gated_paths): gated_paths is false — does not fire despite review_decision=REVIEW_REQUIRED.
- Row 3 (deleted high-risk): high_risk is false; no deletions.
- Row 4/5 (medium + category/high-risk): no medium findings.
- 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 reviewed: 7 delta files on the reporting-subscriptions coverage work. calendar.ts sub-daily walk (instantsForWall, next()=ceil(instant+1), ordinal-0→anchorInstant override) preserves boundary monotonicity and guarantees walk progress. coverage.ts next()-based walk is equivalent to the old ordinal stepping, coveringRemoval skip-ahead skips only in-window starts, the authorizedAt<=previousEnd continuation defers to seller gaps[] (not an auth gate), and the iteration budget bounds sub-daily walks. schema-loader.ts enum opening is idempotent and intentional per the changeset with a mirroring parity test. Changeset is minor — correct for additive exports (satisfies changeset-vs-wire-impact). No witness-not-translator violations and no version-pathway gaps surfaced.
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>
|
Parity update for adcp#8593 (owner decision on the start rule), pushed as f8b48c0:
Changed expectations (the first period containing
New cases:
|
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean, surgical start-rule refinement in reporting subscription coverage.
What I checked:
- The adcp#8593 change:
firstOwedBoundarynow returnsmax(ceil(handoff), floor(confirmedAt))instead ofmax(ceil(handoff), ceil(confirmedAt)), correctly making the period in which a buy is confirmed owed in full. The handoff floor and downstream flight-overlap filter are preserved, and the on-boundary case is unchanged. - Changeset present and correctly typed as
minorwith updated prose — satisfies changeset-vs-wire-impact for thesrc/lib/**touch. - Extensive new data-driven fixture coverage (ASAP, monthly billing, account-tz, hourly, sub-daily local) — new branch is tested, no silent regression risk.
- No witness-not-translator concerns, no version-translation pathway gap, no official-transport-client issues.
No critical/high/medium findings. Prior pass was also clean. gated_paths is false, high_risk is false, no author team gate. review_decision is REVIEW_REQUIRED but gated_paths is false, so row 2 does not fire. Falls through to row 9 → approve.
…r capabilities (#1696) * feat(reporting): compute owed reporting-subscription periods and their capabilities Add adcp.reporting.subscriptions, the shared half of AdCP 3.3 principal-level reporting subscriptions (media_buy.reporting_subscriptions): - expected_subscription_periods(): the coverage rule from core/reporting-subscription.json as a pure function over plain data. It owes each confirmed buy and resolved offering the complete periods that start at or after the latest of the version's accepted_at, the buy's confirmed_at and the account's authorized_at, and that overlap the flight as of each period's scope_resolved_at. It applies cutovers for versions, deactivation, resolution changes and account timezone changes, removes exceptions and gaps over [from, until), and handles revocation and re-grant with and without backfill_on_regrant. Results are ExpectedReportingPeriod values, so they feed evaluate_reporting_ledger and the consumer-status planner unchanged. - resolve_subscription_offerings(): exact / finest / all selection with ties to the lowest offering_id in byte order. - subscription_capabilities() and advertise_reporting_subscriptions(): configuration_tasks, the deprecated configuration_task only when sync_accounts is listed, the subscriptions block, the feature id and the principal section. Civil-time schedules carry the normative local-midnight origin explicitly, so they do not depend on the ledger's legacy anchor default. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(reporting): derive subscription periods exactly as the TypeScript SDK does Align expected_subscription_periods with adcontextprotocol/adcp-client#3505 so that both SDKs owe identical periods. The spec leaves each of these choices open: - A dedicated period calendar. Years, months and days are local civil arithmetic with the day clamped from the origin. Hours, minutes and seconds are exact elapsed time. Calendar months (P1M billing cycles) are supported, and numeric UTC offsets are refused. - Cutovers are paired per buy over the union of its packages. A continuing offering hands off at E of its own outgoing calendar. A newly resolved one starts at the latest E of everything the previous regime resolved. - A period_timezone override moves account_timezone and account-resolved schedules into its zone, while utc and fixed-zone schedules match only their own zone. - Versions may carry in-place inactive intervals and their destination method. The coverage entry's timezone_cutovers are applied. - A re-grant continues authority only before unauthorized_at plus status_retention_days, and a re-grant without retention is an error. The TypeScript SDK's 46-case table is vendored and run as a parity test. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(reporting): adopt the reporting-subscription rules settled in adcp#8585 adcontextprotocol/adcp#8585 settles the ambiguities both SDKs hit. - Offerings publish standard_dimensions and unmapped_dimensions. SubscriptionOffering.from_offering reads them from the wire, and sellers use with_subscription_dimensions() to publish them. subscription_capabilities() refuses an offering without them. - Coverage consumes the readback's inactive_intervals. - timezone_cutovers[] carry previous_timezone and an optional offering_id, and gaps[] carry an optional offering_id. An entry with an offering_id applies to that offering only, and previous_timezone supplies the zone before the first cutover. - period_timezone follows the normative text: a utc offering matches only UTC or Etc/UTC, a source_timezone offering only its published zone (never an account-resolved one), and an account_timezone offering takes the override. The beta.1 models close these objects, so _reporting_subscription_preview adds the fields to the generated classes until a 3.3.0-beta.2 bundle carries them. The vendored TypeScript parity table keeps a scope valid under maxProperties 1 in three cases, matching the fix pending upstream. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(reporting): take each offering's prior zone from its own timezone cutover Follow adcp#8585: the account's timezone is its current zone, as list_accounts shows it. Before a timezone change, an offering's account-timezone periods use that change's previous_timezone, considering only the changes that apply to the offering. An offering with no applicable change uses the current zone throughout. A timezone change without previous_timezone is now refused, and so is a wire offering without standard_dimensions. Previously the first cutover's previous_timezone became the base zone for every offering, so a cutover scoped to one offering could shift another offering's boundaries. Re-vendor the TypeScript parity table at f8b2fddd (54 cases, with the scope fix now upstream). Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * feat(reporting): serve adcp#8585 subscription fields only to 3.3.0-beta.2 callers The server validates responses against the release it serves, and the pinned 3.3.0-beta.1 schemas close the objects adcp#8585 extends. The new wire module gates the new fields on 3.3.0-beta.2 or later, as the TypeScript SDK does: - standard_dimensions and unmapped_dimensions on offerings; - inactive_intervals on the readback; - previous_timezone and offering_id on timezone_cutovers[] and gaps[]. For older callers, per-offering gaps collapse into one account-wide gap spanning them, and per-offering cutovers into one entry at the latest effective_at. The parity test now also validates every subscription in the TypeScript table against core/reporting-subscription.json. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(reporting): address the coverage-rule review and adopt adcp#8588 periods Fixes from the expert review of the coverage rule: - An empty flight (stopped at or before it started) overlaps no period, so a buy canceled before its flight owes nothing. - Each regime carries its per-offering handoff. E for a continuing or new offering is the latest of the outgoing calendar's boundary and the outgoing regime's own handoff, so two cutovers inside one period never owe a period twice. A timezone change passed at T or at its E now gives the same result. - The extensible reporting-dimension vocabulary parses on selectors, offerings and the capability block; an unknown selector value matches nothing. - Zone names must be IANA zones (posixrules and right/... are refused), and offerings with a year or month delivery_sla are refused at construction. - The beta.1 projection keeps distinct timezone changes distinct and drops prior_versions[].configuration. The feature id is refused without the principal section. Stale docstrings are corrected. adcp#8588 (pending for 3.3.0-beta.2): - Sub-daily periods under a local alignment restart at each local midnight on wall-clock multiples, so a DST day has 23 or 25 hourly periods. The duration must divide 24 hours and cannot mix date and time components. - prior_versions[].configuration is consumed, and require_prior_configurations makes a missing one an error. A version accepted while inactive has an empty window. The parity runner now checks row order, expected_obligations and the error class. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(reporting): accept undated continuation re-grants and re-vendor the parity table Re-vendor the TypeScript coverage table at a99597a4: 79 cases, adding - empty flights; - two cutovers inside one period; - sub-daily local periods; - DST gaps and folds at midnight; - a skipped civil date; - PT24H versus P1D; - month-end clamps; - three-grant chains; - simultaneous changes; - unknown dimensions. All pass under the strict runner. Match TS on one rule the table pins: a grant whose authorized_at is at or before the previous loss continues the chain at an unknown instant. This happens because adcp#8585 keeps authorized_at at the chain start after a re-grant inside retention. The buyer removes no gap of its own; the seller's gaps[] decide. Grants are read in input order. The schema check accepts dimension values outside the beta.1 enum, because the vocabulary is x-extensible. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(reporting): owe the period containing confirmed_at in full, per adcp#8593 Owner decision for 3.3.0-beta.2: - 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 then owed every period that ends after its confirmed_at, so the confirmation period is owed in full. - An empty flight still owes nothing. The rule lives in the one function that holds it, _first_owed_start. Re-vendor the TypeScript parity table at f8b48c0c. It has 81 cases, conformed to the same decision, and adds an ASAP buy confirmed mid-day and a monthly billing buy confirmed mid-month. All pass under the strict runner. Refs #1695 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…bscription-coverage # Conflicts: # scripts/check-package-size.mjs
…dules after merging main Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass, clean.
Prior decision was approve (clean, no findings). This pass surfaces no critical/high/medium findings. The large diff is explained by main advancing between the prior approve and now: the envelope-status, pricing-currency, product-page-collection, and Zod-generator changes already exist at base SHA 8eddda7 (ENVELOPE_FIELD_SCHEMAS present at schema-loader.ts:693 in base), so they are already-merged, already-reviewed content rather than this PR's authored work. The PR's own content (reporting-subscription coverage feature) was reviewed on the prior pass.
Genuinely new since prior: version bump beta.5→beta.7, changelog/doc stamps, and a documented check-package-size.mjs budget raise (26.36M→26.41M) — mechanical and sound. A .changeset/reporting-subscription-coverage.md is present, satisfying changeset-vs-wire-impact. Version bumps are via Changesets tooling, not hand-edited.
No no-auto-approve team match; gated_paths is false; high_risk is false. Decision table rows 1–8 do not fire → row 9 approve.
…bscription-coverage
…e budget 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:
- MCP shared/cached schema validator + ≥64 KiB schema-factoring: fully fail-safe (returns original schema on every unsupported-graph condition, operates on an acyclic cloned tree) and validation-only — no witness/wire-shape violation.
- Forked/watchdogged performance regression harness.
- Mechanical 3.1.29→3.1.31 maintained-schema bump: consistent Changesets output, not a hand-edit.
- Two guarded storyboard refinements.
- Changesets present and correctly patch-typed.
Decision table: Rows 1–8 do not fire. No critical/high findings (row 1), gated_paths is false (row 2), high_risk is false (rows 3/5), no medium data-loss/schema/infra findings (row 4), prior decision was approve so no sticky escalation (row 6), no no-auto-approve team match (row 7), zero medium findings (row 8). Falls through to row 9 → approve. Note: review_decision is REVIEW_REQUIRED but gated_paths is false, so row 2 does not apply.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking findings.
Checked: delta is compliance-pin/version alignment (3.1.29→3.1.31, beta.7→beta.9) plus additive, fail-closed work — a content-keyed LRU shared MCP schema-validator (dual byte+entry budget, per-entry Ajv), MCP schema factoring in compact-schema.ts that returns the original document on every unsupported graph (duplicate IDs, dynamic scopes, unvisited ref targets, URI-resolver throws), an isolated performance-regression harness with out-of-loop watchdog, and compliance-runner behavior fixes. Witness-not-translator holds — factoring alters only reference codegen; constraints preserved. Changesets present and correctly scoped.
Decision trace: 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 the hard gate does not apply.
…bscription-coverage
…ndle 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.
The reviewer confirmed the delta is a mechanical beta.1→beta.2 bump across generated types/docs/version constants plus three well-tested source changes (legacy ref constraint reconciliation, typed RequestValidationError marker, and the ESM envelope-status loader fix). Changesets are present and correctly typed; pathway-back/negotiation tests updated; witness-not-translator and doc-link integrity hold.
Decision-table walk: no critical/high findings (row 1 n/a); gated_paths is false (row 2 n/a); high_risk is false, no deletions/modifications triggers (rows 3–5 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 (row 8 n/a). Falls through to row 9 → approve.
Note: review_decision is REVIEW_REQUIRED, but row 2 only applies when gated_paths is true — which it is not here — so no escalation is triggered.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
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>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass, clean.
This is a follow-up pass (prior decision: approve, no findings). The delta is 66 lines across 4 CI/test-only files: an empty-frontmatter CI changeset, a packed-tarball budget bump (26.41M→26.42M), a test swap from the retired '3.3.0-beta.1' pin to live ADCP_VERSION (behavior matches the exact-preview negotiation comment), and a one-line reformat. No src/lib/** wire changes, so no changeset obligation triggers.
Reviewer surfaced no findings. Decision table: 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 so sticky escalation 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 the row 2 hard gate does not apply.
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>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — delta pass, no new findings.
This is a subsequent push on top of a prior clean approve (head 62473e9 → 3f5bf10). The reviewer examined a 41-line delta across calendar.ts and its test: a new guard that refuses sub-daily billing_cycle alignment (adcp#8622). It is correctly placed after the mixed date/time refusal and before the sub-daily local-calendar return, so a PT6H billing_cycle now fails closed instead of silently misaligning to local midnight and ignoring the anchor's time-of-day — consistent with the witness-not-translator posture (refuse rather than silently re-shape). Unreachable cases are excluded by the duration regex, and a matching test was added. The changeset is present and correctly typed (minor).
No critical/high/medium findings in this run. high_risk is false; gated_paths is false; no no-auto-approve team match. Although review_decision is REVIEW_REQUIRED, row 2 only applies when gated_paths is true — it is not here — so no hard gate fires. None of rows 1–8 match, so the outcome falls through to row 9: approve.
First of a short series for #3504 (AdCP 3.3 reporting subscriptions in the reliable-reporting runtime). This slice is the buyer-side pure function, the capability builder, and the typed immutable-configuration error. The seller principal runtime and obligation engine follow in a stacked PR.
What ships
expectedSubscriptionPeriods(input)(exported from@adcp/sdk/reporting/consumerand@adcp/sdk/reporting/ledger). A pure function of plain JSON the buyer already holds. It returns every owed(subscription_version, media_buy_id, offering_id, period).groupExpectedSubscriptionPeriodsV1groups those rows into the obligations that must carry them, with frozen, byte-sortedmedia_buy_ids.createReportingPeriodCalendarV1implements the normativeperiod_generationforutc,account_timezone,source_timezoneandbilling_cycle.resolveReportingSubscriptionOfferingsV1handlesexact,finestandall, with ties going to the lowestoffering_idin byte order.reportingSubscriptionScopeIncludesV1decides scope membership.reportingConfigurationCapabilitiesV1buildsconfiguration_tasks, the deprecatedconfiguration_task(emitted only whensync_accountsis listed) and thesubscriptionsblock. It is a pure builder: nothing in this PR changes what the SDK advertises.ReportingConfigurationImmutableError(code: 'REPORTING_CONFIGURATION_IMMUTABLE', withaccountId,deliveryConfigIdanddeliveryConfigVersion) plusisReportingConfigurationImmutableError. The message text is unchanged, so hosts that match on it keep working.Cross-SDK parity table
test/fixtures/reporting-subscriptions/coverage-cases.jsonholds 81 cases. Each has aninputand eitherexpected_error, or the exactexpectedrows plusexpected_obligations(rows grouped by version, offering and period, with byte-sortedmedia_buy_ids). A case whose input needs data no AdCP response carries lists that data innon_wire_inputs. The expected rows were derived by hand from the spec rule, not by running this code. Compare instants, not strings: TS emitstoISOString()form. Rows are ordered byperiod_start,subscription_version,offering_idandmedia_buy_id(byte order). Expected rows leave outsubscription_id,account_idandreport_definition_id, which come straight from the input.test/lib/reporting-subscription-coverage.test.jsruns the table strictly: it checks rows and obligations, and every case must have one or the other. It also adds invariant, calendar and selection tests.The table covers:
confirmed_at, and a buy confirmed long before its flightnowcut-off and an unconfirmed buyauthorized_atrefinementexclude_account_idsandoperator_domainsgaps, and exceptions that only remove periodsmax_accountscapacity, and precedence (overridden)exact/finest/allover package, package × creative, package × country and package × unmapped deviceperiod_timezoneoverrideThe rule as implemented (for parity)
Per buy (confirmed only), split time at every version
accepted_at/superseded_at, deactivation and reactivation instant, offeringavailable_from/withdrawn_at, and timezone cutover. Each segment that has an active, in-scope version and an unchanged resolution forms a regime. Resolution is the union, over the buy's packages, of the selector resolution against that package'sreporting_delivery_offering_ids.For each regime and resolved offering
owith calendarc:handoff. With no adjacent predecessor regime, or a predecessor that resolved nothing, it is the regime start. Otherwise it is the cutover endE:c_o_old.ceil(T)whenocontinues, else the max ofceil(T)over the predecessor's calendars. When two cutovers fall inside one period, the incoming handoff ismax(E, the predecessor's own handoff), so the owing start carries forward.[s, e)ofcwhere all of the following hold:s ≥ max(c.ceil(handoff), c.floor(confirmed_at)). Under adcp#8593 every period ending afterconfirmed_atis owed, so the period in which the buy is confirmed is owed in full. This isfirstOwedBoundary, the one place the start rule lives.s < regime.untile ≤ nowe > se(s < stop,e > start_time, wherestop = min(end_time, canceled_at, completed_at)). A prior flight applies whilee < replaced_at. A flight whose stop is at or before its start overlaps nothing.excluded[]entry withoutpackage_ids, and nogaps[]entry, hasfrom ≤ s < until. An exception withoutmedia_buy_idis account-wide.Authority comes from the ordered grants. A re-grant before
unauthorized_at + status_retention_dayscontinues the chain; a later one starts a fresh chain. A period is authorized when, for some chain:s ≥ c.ceil(chain.first_authorized_at)s < c.floor(final_unauthorized_at), when the chain has endedsis not in[c.floor(unauthorized_at), c.ceil(regranted_at))for any interruptionUnder
utcalignment, and for any duration with a date component, years, months and days are local civil arithmetic: the day is clamped from the origin's day, a gap advances, and a fold takes the earlier instant. Underutc, hours, minutes and seconds are exact elapsed time. Under a local alignment, a sub-daily duration restarts at each local midnight on wall-clock multiples (adcp#8588). A DST gap yields no boundary, and a fold yields one boundary per offset. The duration must divide 24 hours, and mixing date and time components is refused. Interval zero of abilling_cycleis the anchor itself. Zero-length periods (a skipped civil date) are never owed.Deviations from the issue
authorized_at. This follows the spec as amended by adcp#8593, the owner decision for beta.2. The start bound is the first boundary at or after the later ofaccepted_atandauthorized_at, and a buy is owed every period that ends after itsconfirmed_at.ExpectedReportingPeriodneeds destination, verification and schema pins that a subscription doesn't carry. So it returns a narrower JSON row type plus the obligation grouping; wiring into the consumer runtime is in the third slice.createReliableReportingServiceand addsmedia_buy.reporting_subscriptionstoexperimental_features, together with the runtime and theadcp.principalblock, so the SDK never advertises subscriptions it cannot serve or a 3.3-invalid capabilities document. This follows review: the first push added the feature id here.Conformance to adcontextprotocol/adcp#8585 (commit
f8b2fddd)#8585 settled the ambiguities this PR first listed. Changes:
standard_dimensionsandunmapped_dimensions; the host-supplieddimensionsfield is gone. An offering withoutstandard_dimensionsfails closed (TypeError).period_timezone. Autcoffering matches onlyUTCorEtc/UTC(the earlier alias list was broader). Asource_timezoneoffering matches only its own published zone, so an account-resolved one never matches (it used to). Anaccount_timezoneoffering uses the override; a buyer-configuredbilling_cyclenever matches.inactive_intervalsin wire shape ({deactivated_at, reactivated_at?}) in place of the per-version, buyer-recorded intervals.previous_timezone(required;TypeErrorwithout it) and an optionaloffering_id.account.timezoneis now the account's current zone, aslist_accountsshows it. Gaps may carryoffering_id. An entry withoutoffering_idapplies to every offering.Rules re-confirmed against the normative text, unchanged:
match: exactwithout dimensions (lowestoffering_idwins), years/months/days civil and hours/minutes/seconds elapsed, zero-length periods never owed, per-offering E (including "a predecessor that resolved nothing starts at T"), authorization chains, andscope_resolved_at= period end.Parity table: the Python copy must be re-vendored.
coverage-cases.jsonnow has 54 cases:period_timezoneoverride, and "account new in a later version") had ascopewith both keys, whichmaxProperties: 1forbids. A merge in my generator caused it;scopeis now replaced, never merged.standard_dimensions. Reactivation uses top-levelinactive_intervals. The timezone-cutover case usesprevious_timezone, withaccount.timezoneas the current zone.period_timezoneagainst account-resolved and fixedsource_timezoneofferings; autcoffering withGMT; and error cases for a missingprevious_timezoneand a missingstandard_dimensions.Generated types. The new fields are hand-modeled in
src/lib/reporting/subscriptions/wire.ts, withTODO(3.3.0-beta.2)to regenerate. I tried the repo's existing unreleased-bundle mechanism: a commit-addressed PR bundle plusschemas/codegen-provenance.json, which works with the #8585 head commit's bundle. I backed it out for two reasons:release.ymlsyncs withADCP_REQUIRE_SIGNATURE=1, and PR bundles are unsigned, so merging it would block the beta release until someone re-syncs.ACCOUNT_CHANGE_REQUIRES_NEW_ACCOUNT,allow_separate_account) into the published SDK.Expert review round (2026-10-29), commits
a99597a4andf8b48c0cFindings fixed in this PR:
max(E, carried)), so the second cutover never re-owes an overlapping period.reporting-dimensionisx-extensible, so strict request validation no longer rejects a dimension the bundle doesn't list. The schema loader relaxes that one enum to itsx-pattern; otherx-extensibleenums are unchanged (flagged below). A selector value an offering doesn't publish matches nothing.utcmatches onlyUTCandEtc/UTC. Asource_timezoneoffering matches aperiod_timezoneonly when its own published zone equals it.calendar.next, skips whole removed stretches, and stops atmax_iterations(default 2,000,000) andmax_periods.firstOwedBoundary.Parity table, from 54 to 81 cases:
expected_obligationsas well as rows.PT24HvsP1DSpec ambiguities left open (please confirm):
PT24Hwith a local alignment equalsP1D. The table encodes that.reporting-dimensiongets thex-extensiblerelaxation. Validatingx-extensibleenums in general is an SDK-wide question.non_wire_inputs:prior_flights, offeringavailable_from/withdrawn_at, andsource_timezone. Buyers must retain them.excluded[]has nooffering_id, so with several calendars one exception'sfromis account-wide.Local checks
npm run typecheck,build:lib,lint:boundariesandcheck:package-sizeall pass. The file budget is +20, for five dual-format module sets.reporting-subscription-coverage.test.jspasses 106/106, andreliable-reporting-service.test.jspasses.Refs #3504
🤖 Generated with Claude Code