Repository navigation
Conversation
…ng ledger A principal's reporting subscription is now executable by the ledger. The engine's frozen owed sets become ordinary obligations of one configuration generation per account, owning principal, subscription_id, version and resolved offering_id, aligned with the TypeScript SDK. - ReportingConfigurationGenerationKey gains an optional offering_id, set only for subscription generations. ledger_asdict omits it when unset, so every digest, work key and persisted scope of an account generation is byte-identical to before; a guard test forbids bare asdict in reporting. - SubscriptionObligationPlanner writes sparse, immutable obligations for periods ending by the engine's horizon, then advances the owed set's completeness horizon. Stores opt in through ReportingSubscriptionLedger; the in-memory store implements it, and the planner refuses other stores. - The producer never grid-fills an owed set and reads an owed obligation only once its period has ended. Owed sets are not leased; the service runs them through run_configuration. - get_reporting_status reports offering_id per period, keeps one wire generation per id, version and purpose, and leaves scope_closed false while the scope ends after the owed set's horizon. - sync_reporting_status resolves a statement through the caller's owed obligation for its period and report definition; a period the owed set does not hold fails with INVALID_STATUS_PERIOD. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…pshot The closed-payload allowlist in reporting_reconciliation_validate now names offering_id, so the required-schema manifest carries the function's new fingerprint, computed from a clean migration chain. The public API snapshot records ReportingSubscriptionGeneration and ReportingSubscriptionLedger. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The engine now persists its state through a compare-and-swap store, so frozen_owed_sets reads one snapshot of the principal's state, as owed_periods and subscription_coverage do, instead of the removed process-local lock. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…Service ReliableReportingService(reporting_subscriptions=...) now owes and serves a principal's reporting subscriptions. Without the option, capabilities, responses and worker turns are unchanged. - Each worker turn plans every listed principal's frozen owed set, then executes it with the account configurations. A subscription generation is installed through the same trusted configure path: account_context receives it with configuration.subscription set, and the service validates the source route, currency and advertised offering as for an account configuration. The horizon is the earlier of the engine's and the turn clock. - A subscription that cannot be planned is isolated, recorded in turn.subscription_errors and reported to planning_error_handler or the worker error handler; it never stops another subscription or an account configuration. - Capabilities gain configuration_tasks, the subscriptions block, standard_dimensions, the feature id and the principal section at the served release; callers below 3.3 see the pre-3.3 block, and adcp#8585 offering fields reach only 3.3.0-beta.2 callers. - get_reporting_status summary and periods views gain subscription_coverage. Ledger reads stay scoped to the caller, and resolve_principal is checked against the caller resolver. - The planner takes an install hook and resolves currency per generation; the engine exposes its offerings and status_retention_days. Production options, managed or receipt-reconciled subscription offerings and PostgreSQL owed sets are refused or not yet served. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…on grant Access means access. A loss of authority opens no window: the account is unknown to every task, sync_reporting_receipts and sync_reporting_status included. A seller that wants a buyer to finish reconciliation keeps a reduced grant instead of revoking. - AuthorizedAccount carries the grant's allowed_tasks, persisted in the engine state document only when set, so unscoped documents are unchanged. - Coverage continues only while the grant lists get_reporting_status; a grant reduced below that ends coverage as a revocation does. - ReportingSubscriptionEngine.grants_task answers from the last reconcile. - For a caller holding subscription generations on the account, ReliableReportingService checks get_reporting_status, exact revision reads, sync_reporting_status and sync_reporting_receipts against the grant, and refuses an absent task with ACCOUNT_NOT_FOUND. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…SQL ledger The PostgreSQL generation key widens to account, consumer, delivery_config_id, delivery_config_version and offering_id, matching the TypeScript ledger, so several offerings of one subscription version coexist with account generations, which keep offering_id ''. This is a stopped-worker maintenance upgrade. Stop every reporting worker, run create_schema once with this release, then restart: workers of earlier releases refuse the widened schema. Under the schema lock the upgrade adds offering_id to configurations, obligations and consumer statuses, widens the primary key and the obligation period index, and re-points the SDK's own references at the widened key. Those referencing tables hold account-level generations only, enforced by a CHECK. A foreign key outside the SDK refuses the upgrade instead of being dropped. The upgrade is lossless and idempotent, and a fresh schema ends identical to an upgraded one. - PgReportingLedgerStore implements ReportingSubscriptionLedger: owed-set obligations, a monotonic completeness horizon in the private adcp_reporting_owed_set_progress table, and horizons in status snapshots. - Every configuration, obligation and status query carries offering_id; an owed set is never leased; production, delivery and materializer lookups stay on account generations. - A store with status notifications enabled refuses an owed set until the status checkpoints hold one per offering. - The required-schema manifests and the pinned object counts are regenerated from a clean migration chain. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The materializer manifest now lists the discovery table's offering_id column, CHECK and foreign key, so the installed and exact catalog counts grow by those objects. The PostgreSQL owed-set test imports psycopg only after the pool fixture, so it skips cleanly where psycopg is absent. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A deactivated subscription stays in subscription_coverage only while it still has retained obligations for the account. It reads status inactive with its deactivated_at, which takes precedence over the account-level status; once no owed period is within status retention its entry is omitted. The inactive status and deactivated_at are drafted for 3.3.0-beta.3 and hand-modelled on the pinned coverage model. They are served only to callers at 3.3.0-beta.3 or later; earlier callers keep the account-level status their schema defines. The omission rule applies at every release. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — scoped review of feat(reporting): read deactivated subscriptions as inactive in coverage.
The inactive-coverage logic is correct and well-covered: the retention-horizon omission (_retains_obligations), the inactive/deactivated_at model preview, the 3.3.0-beta.3 release gate (serves_inactive_coverage/_at_least_beta), and the earlier-caller downgrade to the account-level status all line up with the beta.3 draft and are backed by tests (beta.3 inactive, beta.1/beta.2/unversioned downgrade, omission past retention, gate). The DR-0032/0033 pathway-back is satisfied — earlier callers get a status move plus a dropped read-only deactivated_at, with tests.
Medium findings
- src/adcp/reporting/subscriptions/engine.py:1280 — Silent fail-open in
_retains_obligationsswallows owed-set errors with no log (inconsistent with the module's two other warn-logging isolation handlers)
high_risk is true only because src/adcp/decisioning/handler.py was modified, but the reviewer surfaced no medium-or-higher finding on that file, so the modification is presumed safe (row 5 does not fire). No blocking findings; a single medium with no escalation trigger and no team gate falls through to row 9.
Medium findings
- src/adcp/reporting/subscriptions/engine.py:1280 — Silent fail-open in _retains_obligations swallows owed-set errors with no log
The retention check that decides whether a deactivated subscription stays listed keeps the entry when its owed periods cannot be computed, and now logs a warning, as the module's other isolated evaluations do. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
This is a subsequent review on a prior approve decision. The delta is a single 7-line hunk in src/adcp/reporting/subscriptions/engine.py that resolves the prior-pass medium finding: _retains_obligations now logs a warning (subscription_id + exception class name, no payload) before the fail-open return True. The fix is the right shape and leaks no account data — logger is defined at engine.py:70.
Prior finding resolved:
- [RESOLVED] src/adcp/reporting/subscriptions/engine.py:1280 — Silent fail-open now logs a warning before returning True.
No new findings in this run. No critical/high findings (row 1 N/A). gated_paths is false and review_decision is APPROVED (rows 2 N/A). high_risk is true only via src/adcp/decisioning/handler.py (modified), but the reviewer surfaced no medium-or-higher concern on it, so rows 3–5 do not fire. Prior decision was approve, not escalate, so row 6 is N/A. No no-auto-approve team match (row 7 N/A). Zero medium findings, so row 8 N/A. Falls through to row 9 → approve. Net improvement over the prior pass.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #1695. Applies the coverage change drafted for 3.3.0-beta.3, which the owner flagged. It stacks on #1729, #1728, #1725 and #1723, so it contains their commits. Review only
feat(reporting): read deactivated subscriptions as inactive in coverage.The rule (beta.3 draft)
status: "inactive"withdeactivated_at. Inactive takes precedence over the account-level status.What changes
ReportingSubscriptionEngine.subscription_coverage:status_retention_daysof now. Such an entry readsinactivewith the state'sdeactivated_at.active, and never omitted them.inactiveanddeactivated_atare not in the pinned 3.3.0-beta.1 schema, so they are hand-modelled inadcp.types._reporting_subscription_previewwith aTODO(3.3.0-beta.3).serves_inactive_coverage). Earlier callers keep the account-level status their closed schema defines, anddeactivated_atis dropped.Until the SDK supports a beta.3 bundle, no caller is served at that release, so the visible effect today is the omission rule. The engine is ready for the bundle.
Tests
tests/test_reporting_subscription_inactive_coverage.pycovers:inactivewithdeactivated_atat beta.3;deactivated_at;This does not adopt beta.2.
🤖 Generated with Claude Code