[bokelley]

Summary

Gate slice P7 of the reporting-subscriptions DX plan (§2.4, §4 item 11, §6.2), stacked on #1740. Review the commit feat(reporting-storage): one storage decision with versioned migrations. The branch also merges eab57f98a from next (#1724 and #1726, the PostgreSQL principal and engine stores) as real commits, so folding them in creates no conflict when the stack lands on next.

from adcp.storage import memory_storage
from adcp.storage.postgres import postgres_storage

storage = postgres_storage(dsn=os.environ["DATABASE_URL"])  # or pool=...; memory_storage() in development
reporting = ReliableReportingService(
    storage=storage,
    account_context=resolve_account_context,
    subscriptions=SubscriptionOptions(max_accounts=500),
)

One object

ReportingStorage (from memory_storage() or postgres_storage(pool= | dsn=)) holds:

• the reporting ledger;
• staged source objects and seals;
• sync_principal principal records;
• the subscription engine state: grant journal, exceptions and version snapshots.

The service takes the ledger and default source stores from it, and the managed path takes the principal and state stores. Durability is a property of the storage, never a flag. With dsn=, the storage opens the pool at startup and the service closes it. postgres_storage lives in adcp-reporting-storage and is re-exported from adcp.storage and adcp.storage.postgres. #1724's PgReportingSubscriptionStateStore gains list_principals(), and both its stores declare is_durable.

Refusals

• REPORTING_STORAGE_NOT_DURABLE: reporting subscriptions on process-local storage outside development. pytest, and ADCP_ENV=development or test, allow it. The message names both fixes: storage=postgres_storage(dsn=...), or ADCP_ENV=development. Without storage=, every store the managed path uses must declare is_durable = True; the message lists the ones that don't.
• REPORTING_SCHEMA_BEHIND: durable storage checks the schema at startup instead of changing it. The message names python -m adcp migrate, lists the pending versions, and flags any stopped-worker upgrade among them.

Versioned migrations

• Numbered files in adcp_reporting_storage/migrations/ (0001–0005: ledger, subscription generations, ledger extensions, subscription state, inline storage). They are applied in order and recorded in adcp_schema_migrations.
• Each file's header gives its version. -- include: lines apply the package's existing idempotent DDL files, so each statement still lives in one place and hand-wired create_schema() keeps working.
• Each migration commits on its own under the reporting schema lock, after the owned-schema precondition the stores already enforce.
• python -m adcp migrate [--dsn URL] [--status | --dry-run]. --dsn defaults to $ADCP_DATABASE_URL, then $DATABASE_URL.

The stopped-worker note

0002_reporting_subscription_generations.sql, #1729's widened generation key, declares -- adcp-upgrade: stopped-worker, and its header says to stop every reporting worker first. --status prints it on that line. The startup refusal repeats it while 0002 is pending.

Deviations from the plan

• Migrations include existing DDL rather than copying it. Moving every DDL file into numbered files would rename paths that create_schema() and the packaging tests read. Expand/contract migrations, which remove the stopped-worker requirement, remain P17.
• Production managed delivery (PgReportingProductionStore) and the buyer consumer stores are outside the facade for now. Production keeps postgres_reliable_reporting_service(production=...). Python has no buyer consumer store yet; it arrives with P10/P13.
• No serve(storage=...). Storage is passed to the service, which already owns installation.
• The CLI is a migrate subcommand of the existing python -m adcp entry point. An agent alias named migrate would now be shadowed.

Tests

• tests/test_reporting_storage_facade.py (no database):
  • one object backing every store;
  • both durability refusals, and the ADCP_ENV rule;
  • storage/store exclusivity;
  • the migration manifest and its --status lines;
  • the CLI argument check.
• tests/conformance/reporting/test_reporting_storage_migrations_pg.py, against PostgreSQL 16:
  • migrate from empty, dry run, idempotence and status;
  • the startup refusal, then success after migrating;
  • list_principals and durability flags;
  • the CLI end to end;
  • a managed service running sync, sweep, planning and obligations on postgres_storage.
• The subscription, reliable-reporting, PostgreSQL subscription-store and ledger, packaging and public-API suites pass, as do ruff and mypy.

Refs #1695

🤖 Generated with Claude Code

[aao-secretariat]

Ladon verdict: Approve

Approve — clean diff, no blocking or medium findings.

What I checked:

• No findings of any severity from the reviewer; decision table rows 1–8 do not fire, so this falls through to row 9 (approve).
• No author team gate (no_auto_approve_teams empty).
• gated_paths: false — row 2 does not apply.
• high_risk: false — rows 3 and 5 do not apply.
• First review of this PR — no prior-escalation stickiness (row 6).

Reviewer-verified substance: Adds the adcp.storage facade (memory_storage()/postgres_storage(), ReportingStorage), versioned migrations, python -m adcp migrate, and PG subscription/principal stores. Semver signal correct (additive store widening + new storage= kwarg + new adcp.storage exports = feat). Migration concurrency is safe (re-checks applied_migrations inside pg_advisory_xact_lock, double-apply impossible). CAS parity with InMemory holds. Durability gate runs after validate(). Pool released on REPORTING_SCHEMA_BEHIND via registered resource — no leak. No credentials path, no generated-code hand-edits, no import-layering violations.

[aao-secretariat]

Ladon verdict: Approve

Approve — P7 reporting storage changes are clean.

Checked:

• Migration loop (migrations/init.py:336-349) takes a per-migration advisory lock, re-checks applied_migrations under it, and fail-closes on a missing migrations table.
• check_schema (storage.py:448-465) refuses REPORTING_SCHEMA_BEHIND without mutating the schema.
• storage/store exclusivity and the durability guard in service.py are sound.
• Additive public surface shipped under feat:, public_api_snapshot.json updated, PG conformance tests cover migrate/idempotence/CAS/restart.

No critical, high, or medium findings. gated_paths is false; review_decision is APPROVED; high_risk is false; no no-auto-approve team match. Prior decision was also approve. No decision-table trigger fires — falls through to row 9.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean additive PR on an experimental AdCP 3.3 surface.

What I checked:

• No critical/high/medium findings from the reviewer.
• Author team gates: no no-auto-approve match.
• Gated paths: false; review_decision is APPROVED regardless.
• High-risk: false.
• Prior decision was approve with no findings — this run remains clean.

Semver/public-API signal: The storage facade (adcp.storage), versioned migrations, PG subscription stores, and ReliableReportingService(storage=...) are additive. The store required→optional relaxation is a widening, not a break, and ships under a correct feat: prefix — consistent with release-please cutting a minor.

Durability & concurrency: Migration apply loop re-checks applied_migrations inside each pg_advisory_xact_lock transaction, so concurrent runners serialize and idempotently skip (confirmed by conformance tests). Durability is fail-closed (require_durable / REPORTING_STORAGE_NOT_DURABLE); startup refuses a behind schema (REPORTING_SCHEMA_BEHIND) rather than silently mutating; pool open(wait=True) is bounded by psycopg's default timeout.

No decision-table trigger fired (rows 1–8 all clear) → row 9 approve.

[aao-secretariat]

Ladon verdict: Approve

Approve — P7 reporting storage facade, clean pass.

This PR adds the ReportingStorage facade (memory_storage()/postgres_storage()), versioned SQL migrations, and the python -m adcp migrate CLI. No critical/high/medium findings were surfaced.

Checks confirmed:

• Migration loop (migrations/__init__.py L336-349) is concurrency-safe: pg_advisory_xact_lock serializes runners and the applied-version re-check at L340 absorbs the race — no double-apply.
• CAS semantics in subscriptions/pg.py (rowcount==1) are correct; _require_durable_subscriptions resolves to an existing PrincipalService._store.
• store going required→optional is additive — feat: is the correct prefix; no public-API/semver break since adcp.storage is not re-exported from adcp.__init__, so no snapshot entry is owed. memory_storage/postgres_storage are storage factories (not protocol handlers), so the AGENTS.md handler table does not apply.
• Operational readiness: pool open/borrow carry psycopg_pool default timeouts; the opened guard prevents pool accumulation on a failed check_schema.

Gating: high_risk false, gated_paths false, review_decision APPROVED, no no-auto-approve team match. No rows 1–8 fire → row 9 approve. Consistent with the prior approve.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean additive storage facade with no blocking or medium findings.

Checked:

• All changes are additive: new adcp.storage facade (memory_storage/postgres_storage/ReportingStorage), versioned migrations, python -m adcp migrate CLI, and storage= wiring into ReliableReportingService. store went required→optional (keyword-only, backward compatible) — no public export removed or repointed, only two names added to the snapshot. feat: is the correct semver signal.
• Migration runner (migrations/__init__.py L336-L350) is race-safe: pg_advisory_xact_lock serializes concurrent runners and the if migration.version in applied_migrations(...) recheck (L340) makes a loser continue rather than double-applying.
• Owned-pool (dsn=) cleanup verified: a REPORTING_SCHEMA_BEHIND startup refusal triggers close() which fires the registered close_pool resource — no leak.
• CAS logic in subscriptions/pg.py and fail-closed _require_durable_subscriptions are correct.

high_risk false, gated_paths false (and review_decision APPROVED regardless), no no-auto-approve team match, no prior blocking decision. No critical/high/medium findings. Falls through to row 9.

Non-blocking follow-up: adcp.storage is now public but scripts/export_resolution.snapshot_modules() does not enumerate it, so the snapshot won't catch a future removal/repoint of a storage export — worth adding when the surface leaves experimental.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean pass, no blocking or medium findings.

This PR decouples adcp from the adcp-reporting-storage distribution via entry points: _plugin() resolves adcp.storage:postgres and adcp.commands:migrate, the migrate CLI moves into the companion dist, and python -m adcp migrate dispatches through the entry point. The reviewer verified: exception identity preserved (ReliableReportingConfigurationError is the same class across modules), entry_points(group=...) is 3.10-safe, the missing-dist path fails closed with ImportError→exit 2, no stale adcp.storage._cli references remain, and both the fail-closed and resolution paths are tested. Public postgres_storage signature and its ImportError contract are unchanged, so the fix: prefix is correct — no semver-signal concern.

Decision path: no critical/high/medium findings (rows 1, 4, 5, 6, 8 don't fire); gated_paths is false with review_decision: APPROVED (row 2 n/a); high_risk is false (rows 3, 5 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve. Prior decision was also approve; this is a consistent clean pass.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean additive PR with no blocking findings.

This PR adds the adcp.storage facade (memory_storage/postgres_storage/ReportingStorage), versioned migrations under adcp_reporting_storage.migrations with python -m adcp migrate, and the PG subscription stores. All changes are additive, non-breaking public surface, carried on feat: commits — correct semver signal.

Verified by the reviewer:

• Per-migration transactions acquire pg_advisory_xact_lock and re-check applied_migrations inside the lock before applying (correct against concurrent runners, idempotent).
• require_owned_schema_or_empty only rejects a legacy reporting_configurations lacking consumer_id — safe after creating adcp_schema_migrations.
• _require_durable_subscriptions fails closed (getattr(..., "is_durable", False)).
• ledger/*.sql include files are in package-data so wheels resolve them.
• Owned dsn pools open in check_schema and close via the registered resource.
• Real PostgreSQL conformance tests cover migrate/CAS/restart/concurrency.

One non-blocking follow-up noted by the reviewer: the new adcp.storage surface is not added to snapshot_modules(), so its exports aren't guarded by the public-API snapshot (behavioral facade tests do cover existence). This is a low-severity suggestion, not a finding.

Gate checks: no critical/high/medium findings; gated_paths false; review_decision APPROVED; high_risk false; no no-auto-approve team match. Prior decision was also approve. Falls through to row 9 → approve.