Skip to content

feat(reporting): compute owed reporting-subscription periods and their capabilities - #1696

Merged
bokelley merged 8 commits into
nextfrom
claude/issue-1695-subscription-coverage
Oct 29, 2026
Merged

bokelley merged 8 commits into
nextfrom
claude/issue-1695-subscription-coverage

Conversation

@bokelley

@bokelley bokelley commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

First slice of #1695 (Python parity for adcontextprotocol/adcp-client#3504). It adds adcp.reporting.subscriptions, the part of AdCP 3.3 reporting subscriptions that buyer and seller share. The seller runtime (next PR) owes exactly what this function returns, so the two sides agree period by period by construction.

What it adds

  • expected_subscription_periods(...): the coverage rule from core/reporting-subscription.json (x-adcp-validation.coverage and cutover) as a pure function over plain data. Inputs: the get_principal subscription readback (or explicit version windows), the buys the caller can see, offerings with the seller's dimension mapping, the account timezone (plus timezone changes), the grant history, and the subscription_coverage exceptions and gaps. It returns ExpectedSubscriptionPeriod, which subclasses ExpectedReportingPeriod, so the result feeds evaluate_reporting_ledger and plan_consumer_statuses unchanged. There is one value per (version, offering, period), and media_buy_ids is the frozen denominator.
  • resolve_subscription_offerings(...): exact, finest and all matching. Unmapped native dimensions count as distinct. Ties go to the lowest offering_id in byte order. Candidates are filtered on purpose, duration, finality, reconciliation mode, alignment, destination method and period_timezone.
  • Capabilities: subscription_capabilities() emits configuration_tasks, the deprecated configuration_task only when sync_accounts is listed, and the subscriptions block. advertise_reporting_subscriptions() adds media_buy.reporting_subscriptions and the reporting_subscriptions principal section. With None, both return the block unchanged. Nothing is wired into the service yet; that happens in the runtime PR so the SDK never advertises a runtime it lacks.

Tests

tests/test_reporting_subscription_coverage.py has hand-written period tables for every rule:

  • mid-period confirmation and acceptance;
  • cancellation and early completion mid-flight;
  • extension and reinstatement as of scope_resolved_at;
  • version cutovers on and off a boundary, daily to hourly and back with the unowed stretch, and accounts added or dropped by a version;
  • deactivation;
  • resolution changes;
  • revocation still in effect, and re-grant with and without backfill and after retention;
  • a 200-case property test that exceptions only ever remove periods, each removal justified by an entry;
  • account timezone, DST fall-back and spring-forward, and the spec's UTC-5 to UTC-8 cutover example;
  • billing-cycle anchors and no clipping;
  • a 27-case grid over confirmed_at, accepted_at and authorized_at.

Capability tests validate each projected block against the pinned 3.3.0-beta.1 core/reporting-delivery-capabilities.json.

Where the issue and the spec differ (spec wins)

  • The issue says configuration_task appears "only when sync_accounts is listed". The schema also requires it when configuration_tasks is absent, so a seller without subscriptions keeps its existing block byte for byte.
  • The issue's start rule names the later of accepted_at and confirmed_at. The spec adds authorization.authorized_at (.changeset/reporting-subscriptions.md), and this implements the spec.
  • The changeset lives at .changeset/reporting-subscriptions.md, not under .changeset/pre/.

Settled by adcontextprotocol/adcp#8585

The spec ambiguities this PR first reported are now settled upstream (merged to the spec's main at 6deac5550, not yet in a signed bundle). This PR conforms:

  • Offering dimensions. Offerings publish standard_dimensions and unmapped_dimensions. SubscriptionOffering.from_offering reads them from the wire, and explicit arguments override them for sellers that haven't published yet. Sellers use with_subscription_dimensions() to publish them, and subscription_capabilities() refuses an offering without them.
  • Readback and coverage fields. The coverage function consumes the readback's inactive_intervals (reactivation is a fresh start). It also consumes previous_timezone and an optional offering_id on timezone_cutovers[], and an optional offering_id on gaps[]. An entry without offering_id applies to every offering, and previous_timezone supplies the zone before the first cutover.
  • period_timezone follows the normative text: an account_timezone offering matches and uses the override; a utc offering matches only UTC or Etc/UTC; a source_timezone or fixed billing_cycle offering matches only its published zone; a buyer-configured billing_cycle offering never matches.
  • Other rules, confirmed to match the normative text: exact without dimensions lets every candidate qualify, with the lowest offering_id winning. Authorization is evaluated as a chain. Reactivation is a fresh start. Years, months and days use civil arithmetic, while hours, minutes and seconds are exact elapsed time. Zero-length periods are never owed. E is computed per incoming offering. scope_resolved_at is the period end.

Timezones follow adcp#8585 and TS: account_timezone is the account's current zone. Before a change applicable to an offering (account-wide, or naming that offering), the zone is that change's previous_timezone. An offering with no applicable change uses the current zone throughout. A change without previous_timezone is refused, and so is a wire offering without standard_dimensions.

Release gating. The server validates responses against the release it serves, and the pinned beta.1 schemas close these objects. adcp.reporting.subscriptions.wire therefore emits the adcp#8585 fields only to callers served 3.3.0-beta.2 or later, matching TS: standard_dimensions, unmapped_dimensions, inactive_intervals, previous_timezone, and per-offering offering_id. For older callers, or when the release is unknown, the fields are dropped. Per-offering gaps become one account-wide gap spanning them, and per-offering cutovers of one change become one entry at the latest effective_at.

Unreleased schema fields. The pinned 3.3.0-beta.1 models close these objects, so adcp/types/_reporting_subscription_preview.py hand-models the adcp#8585 fields. It adds them to the generated classes from _forward_compat, the repo's existing pattern for forward-looking fields, with a TODO(3.3.0-beta.2) to remove it once a bundle carries them. Capability tests strip the two new offering fields before validating against the beta.1 schema, with the same TODO.

Cross-SDK parity with TypeScript (adcontextprotocol/adcp-client#3505)

tests/test_reporting_subscription_parity.py runs the TS SDK's coverage table, vendored from #3505 at a99597a4 (79 cases), strictly: exact row order, expected_obligations, and the error class. It also schema-validates every subscription in the table. All 79 cases pass, including empty flights, two cutovers inside one period, sub-daily local periods, midnight DST gaps and folds, skipped dates, month-end clamps and three-grant chains.

Start rule (adcp#8593, owner decision for 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 owed every period that ends after its confirmed_at, so the confirmation period is owed in full, and an empty flight owes nothing. The rule lives in _first_owed_start. Eight TS parity cases still carry pre-decision rows and are strict expected failures until TS #3505 is re-vendored.

Expert review and adcp#8588

The billing-grade review's findings are fixed, or deferred with a reason in the PR comment. adcp#8588 (pending for beta.2) is implemented: sub-daily local periods restart at each local midnight, and prior_versions[].configuration is read.

Remaining open points

  • Prior configurations. adcp#8588 adds prior_versions[].configuration. Until a seller sends it (beta.2), a buyer that lost local state passes version_configurations or opts into assuming the current configuration.
  • Spec questions. An anchor in a DST fold, civil delivery_sla arithmetic, and package history (see the review reply).

Related findings

  • Ledger origin default. The ledger's ReportingScheduleSpec defaults a civil-time schedule without an anchor to the UTC epoch in local time, so daily America/New_York boundaries fall at 19:00 local rather than local midnight. Production admission already refuses that phase.
  • Ledger calendar. The ledger treats every duration component as civil time, rejects months, and skips a civil period whose local start doesn't exist. Subscription coverage uses its own PeriodCalendar, which the ledger has to adopt before it can materialize subscription obligations.

🤖 Generated with Claude Code

…r 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>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean, additive PR with no blocking or medium findings.

This PR introduces a new experimental adcp.reporting.subscriptions subpackage (coverage rule, offering resolution, capabilities) across 8 files, all additions (3036 insertions, 0 deletions). The reviewer verified:

  • Zero deletions; no public export removed/renamed/retyped.
  • feat(reporting): is the correct semver signal for a non-breaking minor — no discriminated-union, _generated import-layering, or forward-compat surface touched.
  • coverage.py (804 lines, largest file) reviewed: cutover pairing, as-of flight resolution, loss/withheld gates, and owed-key dedup all consistent.
  • Cross-checked against the pinned 3.3.0-beta.1 reporting-subscription-coverage.json: _REASONS matches the exclusion enum, half-open [from, until) removal faithful, gaps' required until matches.
  • Pure functions, no I/O/workers/timeouts — operational-readiness N/A.

Decision-table walk: no critical/high findings (rows 1, 6 N/A); gated_paths false (row 2 N/A); high_risk false, no deletions/modifications triggers (rows 3-5 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.

… 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>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — TS-parity delta on the new reporting-subscriptions module is clean.

What I checked:

  • No findings (critical/high/medium) surfaced by the reviewer on the delta commit (39ca1b3).
  • Additive feat/fix on an unreleased module; no public-surface removal/rename/type-signature change, so no semver-signal gate triggered.
  • No touch to forward-compat unions (_forward_compat.py, aliases.py, _ergonomic.py), no generated-code hand-edit under types/domains/** or _generated.py, no import-layering violation, no credential-in-ctx_metadata.
  • The one behavior flip — subscription_covers_account returning False instead of raising for an unknown operator (coverage.py:414-449) — is fail-closed, matches the TS reference, and is pinned by test_scope_filters.
  • All paths exercised by the vendored 46-case TS parity table plus hand-written coverage tables, both CI-gating.

Gate checks:

  • gated_paths: false; review_decision: APPROVED — no hard path gate.
  • No no-auto-approve team match.
  • high_risk: false.
  • Prior decision was approve; this is a consistent clean subsequent pass.

No row 1–8 triggers fire → row 9 approve.

…cp#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>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve.

This PR adds reporting-subscription follow-ups (adcp#8585): capabilities, coverage, resolution logic plus fixtures and parity tests. Net additions, no deletions.

Checked against repo-specific gates:

  • _forward_compat.py is high-risk (modified) — but the reviewer found no medium-or-higher concern on it; no fallback arm removed, no discriminator change, no layering violation. Presumed safe.
  • _reporting_subscription_preview.py forward-compat field patching imports only domains/base, no _generated.py layering violation, idempotent.
  • No credentials-in-ctx_metadata, CI-gate, or semver-signal concerns raised.

Single Medium finding: coverage.py:688 — previous_timezone account-zone fallback keys on changes[0] without checking it is an account-wide (offering_id is None) cutover.

Decision table (top-down): no critical/high findings (row 1 no). gated_paths is false (row 2 no). No (deleted) high-risk reason — only (modified) (row 3 no). The single medium is not in {data-loss, schema, infra} (row 4 no). Row 5 requires high_risk (modified) AND a medium finding — high_risk is (modified) and there IS one medium finding on _forward_compat.py? No: the medium is on coverage.py:688, not on the modified high-risk file, but row 5 only requires ANY medium finding to coexist with a modified high-risk file. Re-reading row 5: "high_risk true AND any high_risk_reasons entry contains (modified) AND any finding has severity = medium" — all three are literally true here.

Correction: Row 5 fires → escalate.

Medium findings

  • src/adcp/reporting/subscriptions/coverage.py:688 — previous_timezone fallback takes from a per-offering cutover as the global account zone

… 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>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — prior Medium resolved, no current blocking or Medium findings.

What I checked:

  • Prior decision was approve with one Medium (coverage.py:688 global-zone fallback). The delta removes that fallback; _timezone_at (coverage.py:1103-1110) now scopes each change's previous_timezone to its offering, with account_timezone meaning the account's current zone. That prior Medium is resolved.
  • New previous_timezone-required validation, the standard_dimensions refusal (resolution.py:100-107), and reworked parity fixtures/tests are internally consistent and verified against representative expected rows.
  • The subscriptions module is net-new in this PR, so no published-contract break.

High-risk flag: _forward_compat.py is (modified) and matches a high-risk glob, but the reviewer surfaced no Medium-or-higher concern on it, so the modification is presumed safe (row 5 does not fire). No (deleted) reasons.

Gate status: gated_paths: false; review_decision: APPROVED; no no-auto-approve team match.

Decision table: rows 1–8 do not fire (no critical/high, no Medium findings, gated paths false, no deletions, no team gate, prior decision was approve not escalate). Falls through to row 9 → approve.

…ta.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>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean pass, no blocking or Medium findings.

What I checked:

  • The PR adds wire.py, a version-gated projection of the adcp#8585 reporting-subscription fields (standard_dimensions, unmapped_dimensions, inactive_intervals, per-offering gaps[]/timezone_cutovers[]) plus comprehensive tests (11,525 additions, 0 deletions — almost entirely new files + tests).
  • serves_settled_subscription_fields fail-closes on absent/unparseable versions and gates beta.N at N>=2; all 11 parametrized cases pass.
  • Gap/cutover collapse helpers degrade conservatively (interval-union, latest effective_at).
  • All five projections act on response/readback fields, so Droppable is the correct DR-0032/0033 pathway, and tests cover both directions — the mandatory minor-release wire-change pathway is satisfied with a test.
  • resolution.py tuple(... or ()) change hardens against a null standard_dimensions.

High-risk flag: _forward_compat.py is (modified) and matches a high-risk glob, but the reviewer found no Medium-or-higher concern on it, so row 5 does not fire. The change preserves the file's contract.

Gates: gated_paths: false; review_decision: APPROVED; no no-auto-approve team match. Prior decision was approve with no findings — this is a consistent net-clean result.

No decision-table rows 1–8 fired → row 9 approve.

@bokelley bokelley left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: coverage rule, period calendar, selection, wire gating (billing-grade pass)

I checked this against spec main after adcp#8585: reporting-subscription.json x-adcp-validation (coverage, cutover, precedence, selection, destination), reporting-subscription-state.json, reporting-subscription-coverage.json, reporting-delivery-offering.json, reporting-schedule.json period_generation, and account-authorization.json authorized_at. All 240 subscription tests pass locally, and the vendored parity table is byte-identical to the TS head (f8b2fddd).

Most of the rule is right:

  • The start rule. ceil is monotone, so applying it per input equals ceil(max(accepted_at, confirmed_at, authorized_at)).
  • No clipping, and the period containing the stop is owed.
  • Flight evaluated at scope_resolved_at (the period end).
  • Per-offering E for the simple case.
  • Fresh start on reactivation.
  • Authorization chains and retention.
  • Exceptions that only remove periods.
  • Byte-order ties.
  • exact/finest/all, including exact without dimensions.
  • period_timezone matching.
  • Civil Y/M/D followed by elapsed H/M/S.
  • DST gaps (fold=0 resolves to "advance by the gap") and folds (earlier instant).
  • Month-end clamping from the origin day.
  • Skipping zero-length periods. I checked Pacific/Apia 2011-12-30 and zones whose DST change falls at midnight (Havana, Santiago, Beirut).

Findings, ranked by severity. Line numbers are at the PR head.

Confirmed bugs

1. [High] A buy canceled (or completed) before its flight starts still owes a full period. coverage.py:783
if not (start < flight_stop and end > flight_start) treats [start_time, stop) as an interval even when stop < start_time. Any period that spans both instants is owed.

  • Repro: P1M utc offering, buy start_time 2026-11-15, canceled_at 2026-11-10, confirmed 2026-10-01. expected_subscription_periods returns 2026-11-01..2026-12-01, a month of billing for a buy that never ran.
  • P1D hits it too: a buy starting at 06:00 local and canceled at 02:00 the same day.
  • The TS SDK has the identical check (coverage.ts:271), so parity holds and both are wrong. The parity table has no case for it.
  • Fix: treat an empty flight as overlapping nothing.
    if flight_stop <= flight_start or not (start < flight_stop and end > flight_start):
        continue
    Add a shared parity case, and ask the spec to say so explicitly ("a flight whose stop is at or before its start overlaps no period").

2. [Medium-High] E for a continuing offering ignores where the predecessor regime itself started, so two cutovers inside one period owe overlapping periods. coverage.py:900-907 (used at :769)
_cutover_end returns predecessor.calendar.ceil(T). The spec's E is "the end of its own last outgoing period that starts before T". When the predecessor regime began with a handoff E_prev > T, it owed nothing before T. The last outgoing period then belongs to the regime before it, and ends at E_prev, not at ceil_pred(T).

  • Repro (runs through the #1698 engine): account timezone LA→NY at 10-03T12:00Z (old E = 10-04T07:00Z), then a new subscription_version at 10-03T13:00Z.
    • The seller owes v1 LA [10-03T07Z, 10-04T07Z) and also v2 NY [10-04T04Z, 10-05T04Z). The two overlap by 3 hours.
    • A buyer reading timezone_cutovers.effective_at computes v2 from 10-05T04Z.
  • The same happens for any two cutovers within one period, such as two version bumps a few hours apart with misaligned schedules.
  • Fix:
    • Carry each regime's per-offering handoff and use E = max(pred.calendar.ceil(T), pred_handoff[offering]). For a new offering, take the max of the same expression across the predecessor's offerings.
    • Normalize every AccountTimezoneChange to the old calendar's ceil(changed_at) per offering before splitting regimes. A change passed at T then gives the same result as one passed at E, which both the engine (#1698) and the tracker (#1699) need.
    • Add a parity case.

3. [Medium] Extensible dimensions are rejected, but receivers MUST accept unknown values. _reporting_subscription_preview.py:83
The preview patch types standard_dimensions as a closed list[ReportingDimension]. enums/reporting-dimension.json is x-extensible and says "Receivers MUST accept unknown values". A capabilities response from a newer seller that maps a device dimension would then fail to parse on the buyer. The same closed enum on selector.dimensions makes sync_principal reject ["device"] at schema validation. The spec wants that selector to match no offering, so STANDARD_REPORTING_DIMENSIONS' "matches nothing" branch is unreachable through sync_principal.

  • Fix: type both as list[str] constrained by ^[a-z][a-z0-9_]*$, or as ReportingDimension | str.

4. [Low] A billing anchor inside a DST fold does not produce boundary 0 at the anchor. _clock.py:80, :125
for_schedule converts the anchor to local wall time, and fold=1 survives on the second occurrence. _resolve then forces fold=0. With period_anchor 2026-11-01T06:30Z (01:30 EST, second occurrence), boundary(0) is 05:30Z. For a pure PT… duration every boundary is shifted 1h from the anchor. TS does the same (instantForWall prefers the earlier offset), so this is a spec question. I'd argue period_anchor is an instant and is by definition boundary 0. Fix: keep the origin as an aware instant for ordinal 0, and for the elapsed-only case.

5. [Low] delivery_sla with Y or M components raises. _clock.py:69-75
The offering schema's delivery_sla pattern allows P1M, which is plausible for monthly billing. parse_delivery_sla raises, and through item.calendar() this aborts expected_subscription_periods for every subscription that resolves the offering; see #1698 for the engine impact. Days are also 24h elapsed, while TS resolves calendar SLAs civilly. That matches Python's ledger, but the two SDKs can differ by 1h on expected_at across DST. The parity table compares neither expected_at nor withheld. Fix: reject such offerings at construction with a clear error, or support civil SLA. Raise the arithmetic with the spec.

6. [Low] require_zone accepts keys that are not IANA zone names. _clock.py:47-54
posixrules (a host-dependent copy), Factory, right/UTC and posix/America/New_York all load. zoneinfo.available_timezones() excludes them and TS's Intl rejects most of them, so a period_timezone of right/UTC is accepted by Python and refused by TS. Fix: check membership in a cached available_timezones().

7. [Low] The pre-beta.2 cutover projection merges distinct changes. wire.py:130-138
_account_wide_cutovers keys on (timezone, previous_timezone). NY→LA in January, LA→NY in March and NY→LA again in June collapse the two NY→LA changes into one entry at the latest instant, so beta.1 callers lose the January change. Fix: group by change (the per-offering entries of one change share timezone and previous_timezone and fall within one period), not by zone pair.

8. [Low] The advertise_reporting_subscriptions advertisement can be left non-conformant. capabilities.py:176-184
When adcp.principal is absent, it adds the feature id but not the reporting_subscriptions section. Advertising requires both. Fix: raise instead.

9. [Low] Stale docstrings. resolution.py:75-79 and :243-244 say an account_resolved source_timezone offering accepts a period_timezone override. The code, correctly per the spec, never matches it.

Questions and spec gaps

  • Package/product history. SubscriptionBuy.packages is static (coverage.py:868). A package added mid-flight whose product lists a new offering (under match: all, or one that changes finest) is treated as present for all time. The buy is then owed elapsed periods of that offering, which contradicts "a resolution change is a cutover" and "never retroactively". Should packages carry added_at and removed_at, mirroring superseded_flights?
  • beta.1 readers and reactivation. project_subscription_state drops inactive_intervals for beta.1. A beta.1 buyer then sees an active subscription with no deactivated_at and expects every period of the inactive interval. That is not fail-safe, though beta.1 cannot express it. Please document it as a known limitation. The gaps merge, by contrast, does fail safe: it only over-removes.
  • Zero-length flights (start_time == stop) currently owe the period containing that instant. Is that intended? Settle it together with finding 1.

Test quality

  • The parity harness compares rows only: start, end, version, offering, buy. expected_at, withheld and source_timezone are unconstrained across SDKs, as is subscription_version_windows (the harness builds windows itself, so readback derivation is never cross-checked).
  • expected_error cases accept any ValueError/TypeError.
  • Missing cases, none in either SDK's table:
    • an empty flight;
    • two cutovers inside one period;
    • an anchor in a fold;
    • PT24H vs P1D across DST;
    • a day-31 anchor through February;
    • a zone whose DST change falls at midnight;
    • a skipped civil date;
    • a re-grant at exactly unauthorized_at + retention (the < boundary);
    • a flight change effective exactly at a period end;
    • a delivery_sla with months;
    • an unknown dimension.

Merge readiness: not yet. Finding 1 creates billing obligations for buys that never ran, and finding 2 owes overlapping periods twice. Both need fixes in both SDKs and new shared parity cases.

…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>

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean subsequent pass, no blocking findings.

This is a follow-up to a prior clean approve on PR #1696. The delta refines the additive reporting/subscriptions module: DST-correct sub-daily local period derivation (_clock.py), per-offering cutover handoffs plus empty-flight/negative-window fixes (coverage.py), fail-closed capability advertising, extensible reporting-dimension forward-compat (_reporting_subscription_preview.py), and wire projection of prior_versions[].configuration — each paired with targeted tests.

Checks performed:

  • high_risk is true only because src/adcp/types/_forward_compat.py is (modified) — a forward-compat load-bearing file. Reviewer found no medium-or-higher concern on it; the change is extensible/additive forward-compat work, no fallback arm removed, no discriminator change. Presumed safe per the (modified) risk rule.
  • gated_paths: false and review_decision: APPROVED — no gate obstruction.
  • No no-auto-approve team match.
  • No Critical/High/Medium findings reported.

Decision table: No critical/high (row 1 n/a). gated_paths false (row 2 n/a). No (deleted) reasons (row 3 n/a). No medium findings, so rows 4, 5, 8 n/a. Prior decision was approve, so sticky row 6 n/a. No team gate (row 7 n/a). Falls through to row 9 → approve.

The module is additive under feat(reporting) with no public-surface break, so the semver signal is satisfied.

@bokelley
bokelley force-pushed the claude/issue-1695-subscription-coverage branch from 7a8f5d6 to 30b9031 Compare October 29, 2026 19:10

Copy link
Copy Markdown
Contributor Author

Thanks for the billing-grade pass. Here is each finding against the current head (30b903148).

Fixed

  1. Empty flight. A flight that stops at or before its start overlaps no period, so a buy canceled before its flight owes nothing (P1M and P1D repros). A zero-length flight also owes nothing, which settles that question the same way. TS added the shared cases ("empty flight: canceled before start_time", "empty flight: stop equal to start"), and the vendored table at a99597a4 covers them.
  2. Two cutovers inside one period. Each regime now carries its per-offering handoff (_Regime.handoffs). E is max(pred.calendar.ceil(T), pred.handoff), and for a new offering the max of that across the predecessor's offerings. A timezone change passed at T or at its E therefore gives identical periods; test_two_cutovers_inside_one_period_never_overlap asserts that equality and no overlap. I did not add separate normalization: carrying the handoff makes T and E equivalent. The shared case "two cutovers inside one period" passes.
  3. Extensible dimensions. Selector dimensions, offering standard_dimensions and the capability block's dimensions all accept any ^[a-z][a-z0-9_]*$ token, and an unknown selector value matches nothing.
  4. Lows
    • Month SLA: an offering with a year or month delivery_sla is refused at construction, with a clear error.
    • Zone names: must be in zoneinfo.available_timezones(), so posixrules, right/… and posix/… are refused.
    • beta.1 cutover projection: consecutive grouping keeps distinct changes distinct; the Jan/Mar/Jun case is a test.
    • Advertisement: advertise_reporting_subscriptions raises when adcp.principal is absent.
    • Docstrings: corrected.
  5. adcp#8588 (pending for beta.2), implemented:
    • Sub-daily local periods restart at each local midnight on wall-clock multiples, so a DST day has 23 or 25 hourly periods. Singapore and Kathmandu hours land on their local hour, PT6H in New York does not drift, a duration that doesn't divide 24h is refused, and so is one mixing date and time components.
    • prior_versions[].configuration is consumed, and require_prior_configurations turns a missing one into an error.
  6. Strict parity runner. It checks exact row order, expected_obligations, and the error class (TypeError maps to ValueError). It also schema-validates every subscription in the table. All 79 cases at a99597a4 pass. That head added one rule I matched: a grant whose authorized_at is at or before the previous loss continues the chain at an unknown instant, and the seller's gaps[] decide what it removed.
  7. confirmed_at start rule. The rule sits in _first_owed_start, so the pending owner decision on the period containing confirmed_at changes one function.

Deferred (reason)

  • chore(main): release 1.0.0 #4, anchor in a DST fold. This is a spec question and TS behaves identically (it prefers the earlier offset). Changing Python alone would break parity. I'd like the spec to say whether period_anchor is by definition boundary 0.
  • Civil delivery_sla arithmetic (finding 5's second half). Month SLAs are refused instead. Day SLAs stay 24h elapsed, matching the ledger. The spec should pin this arithmetic before either SDK supports calendar SLAs.
  • Package and product history (added_at/removed_at on packages). This is a real retroactivity gap, but it needs a spec decision on how package changes interact with "never retroactively". I'd rather not invent fields ahead of it.
  • Cross-SDK expected_at/withheld. TS returns neither, so the parity table can't constrain them yet.

Confirmed

  • beta.1 readers and reactivation: documented in wire.py as a known limitation.
  • The dispatcher never resolves a bare "3.3" while only the beta.1 bundle is pinned (resolve_adcp_version("3.3") raises ConfigurationError). test_a_bare_3_3_version_cannot_be_served_while_beta_1_is_pinned pins it.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean pass, no blocking or medium findings.

Checked:

  • No critical/high/medium findings reported. Decision table rows 1–8 do not fire; falls through to row 9.
  • gated_paths: false and review_decision: APPROVED — no hard path gate in play.
  • No no-auto-approve team match.
  • high_risk is true only because src/adcp/types/_forward_compat.py is (modified), but the reviewer found no medium-or-higher concern on it; a modified high-risk file with no findings is presumed safe (row 5 requires a medium finding, which is absent).
  • The real delta is two files: coverage.py (dropping sorted() to preserve adcp#8585 within-retention re-grant ordering, plus an undated-regrant branch recording (previous_end, None) and deferring removed periods to seller gaps[], with both interruptions consumers patched for the None arm) and the vendored TS parity fixture (coverage-cases.json adds expected_obligations, asserted row-by-row by the new parametrized parity test). The widened tuple type is on a private _Chain — no public/semver impact.
  • Prior decision was approve; this is a net-neutral/clean subsequent pass.

No escalation triggers present. Outcome: approve.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean pass, no blocking or medium findings.

This delta refines the previously-approved reporting subscription-coverage feature. Key checks:

  • _chains (coverage.py:1164) now reads authorized_at <= previous_end as an undated re-grant (previous_end, None) per adcp#8585, with _authorized/_expected_at correctly deferring the interruption's removal to the seller's gaps[]. Dropping the sorted()/overlap guard is justified since 8585 legitimizes that shape, and the "oldest first" contract is documented.
  • _first_owed_start is a behavior-identical extraction.
  • The parity fixture's new expected_obligations activates the strict obligation assertion (test ordering matches the output sort key).
  • The scoped dimensions-enum suppression is a correct x-extensible-vocabulary check, not a disabled test — CI gates stay armed.

High-risk flag is true only because _forward_compat.py is modified, but the reviewer surfaced no medium-or-higher concerns on it, so the modification is presumed contract-preserving. No discriminator/fallback-arm removal, no type-layering violation, no credential-in-metadata issue.

Gated paths false; review_decision APPROVED; no no-auto-approve team match. No decision-table rows 1–8 fire → row 9 approve. Prior decision was also approve; this is a net-neutral/positive iteration.

…he 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>
@bokelley
bokelley force-pushed the claude/issue-1695-subscription-coverage branch from 30b9031 to 2396b0b Compare October 29, 2026 19:19

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean pass, no blocking findings.

Checked:

  • _forward_compat.py is flagged high-risk (modified), but the reviewer found no medium-or-higher concerns on it; the change to coverage.py's _chains authority-chain handling is a narrow, correct refinement (drops sorted()/overlap ValueError in favor of a grant.authorized_at <= previous_end branch for the adcp#8585 chain-start-kept case), consistent with authorized_at no longer being monotonic.
  • _authorized/_expected_at correctly skip regranted is None so the seller's gaps[] remove the periods; _first_owed_start is guard-checked non-None at the call site.
  • Both the continuation and two-unrevoked-grants error paths are covered by a unit test plus a cross-SDK parity fixture row.
  • Import reorder in _reporting_subscription_preview.py is cosmetic; fixture additions are additive with no expected-array changes.

Decision trace: no critical/high/medium findings (row 1 n/a). gated_paths is false (row 2 n/a). high_risk reason is (modified) not (deleted), and no medium finding exists so rows 3/4/5 do not fire. Prior decision was approve, so row 6 (sticky escalate) does not apply. No no-auto-approve team match (row 7 n/a). Zero medium findings (<3, row 8 n/a). Falls through to row 9 — approve. review_decision is APPROVED.

@bokelley
bokelley force-pushed the claude/issue-1695-subscription-coverage branch from 09ec8ab to 02648a3 Compare October 29, 2026 19:48

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve.

Clean pass with no Critical/High/Medium findings. The delta since the prior approve is scoped to the adcp#8593 owner decision (3.3.0-beta.2): _first_owed_start changes from ceil(max(handoff, confirmed_at)) to max(ceil(handoff), floor(confirmed_at)), making the period containing confirmed_at owed in full. The reviewer verified this is correct — since candidate starts are period boundaries, floor(confirmed_at) is equivalent to "period ends after confirmed_at," and it composes correctly with the separate authorized_at ceil gate and floor(flights_start) bound. Tests were updated to match, and the TS-parity divergence is handled honestly via strict=True xfail so a stale entry cannot linger once the TS table is re-vendored.

High-risk flag is true only because src/adcp/types/_forward_compat.py is a load-bearing file (modified), but the reviewer surfaced no medium-or-higher concerns on it, so the modification is presumed safe (no deletion). gated_paths is false, review_decision is APPROVED, and no no-auto-approve team match. No decision-table trigger fires (rows 1–8 all pass through) → row 9 approve.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean pass, no blocking or medium findings.

This PR adds a new reporting-subscriptions module (coverage, resolution, capabilities, wire) with tests and fixtures. The reviewer found no critical/high/medium issues. The core delta implements the adcp#8593 owner decision (owe the period containing confirmed_at in full) in _first_owed_start via max(ceil(handoff), floor(confirmed_at)), which is correct, and the eight pre-decision parity cases are marked xfail(strict=True) with a re-vendor reason (fail-closed).

Gate checks:

  • high_risk is true only because src/adcp/types/_forward_compat.py was modified, but the reviewer confirmed no medium-or-higher concern on that file — presumed safe (row 5 does not fire without a medium finding).
  • gated_paths: false and review_decision: APPROVED — no gate obstruction.
  • No no-auto-approve team match.
  • Prior decision was approve; current run is consistent.

No findings → rows 1–8 do not fire → row 9 applies.

…dcp#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>
@bokelley
bokelley force-pushed the claude/issue-1695-subscription-coverage branch from 02648a3 to 4688e83 Compare October 29, 2026 19:58

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ladon verdict: Approve

Approve — clean diff, no blocking or medium findings.

This PR implements owner decision adcp#8593: a buy is owed every period ending after its confirmed_at, so the confirmation period is owed in full. The load-bearing change is coverage.py:1563, which replaces calendar.ceil(max(handoff, confirmed_at)) with max(calendar.ceil(handoff), calendar.floor(confirmed_at)) — correct per the reviewer: ceil(handoff) is the handoff boundary and floor(confirmed_at) starts the period that ends after confirmed_at. Old and new behavior agree except in the intended mid-period confirmed_at case; boundary-aligned confirmed_at still correctly excludes the period ending at it. Unit test and parity fixtures (new ASAP and monthly-billing cases plus floor-derived rows) are consistent.

High-risk flag is true only because src/adcp/types/_forward_compat.py is a (modified) match, but the reviewer surfaced no medium-or-higher concern there, so the modification is presumed safe (no deletion, no finding). gated_paths is false and review_decision is APPROVED, so no gate forces escalation. No no-auto-approve team match. Prior decision was approve.

Decision table: rows 1–8 do not fire (no findings, no deletion in high-risk reasons, no team gate, fewer than three medium findings). Falls through to row 9 → approve.

@bokelley
bokelley merged commit 58e825a into next Oct 29, 2026
41 checks passed
@bokelley
bokelley deleted the claude/issue-1695-subscription-coverage branch October 29, 2026 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant