[bokelley]

Summary

Gate slice P8 of the reporting-subscriptions DX plan (§2.11, §4 item 13, §6.2), stacked on #1742. Review only the commit feat(accounts)!: ensure returns an AccountOutcome and routes to request_account. Breaking: ensure's return type changes, which the beta allows.

outcome = await client.accounts.ensure(key, currency="EUR", timezone="Europe/Paris", billing="operator")
match outcome:
    case AccountReady(account_id=aid): use(aid)
    case AccountPending(action=action) if action.waiting_on == "buyer": show_to_human(format_pending_action(action))
    case AccountAmbiguous(candidates=c): outcome = await outcome.choose(c[0]["account_id"])
    case AccountRejected(errors=e): report(e)

Routing

• account.requests advertised: request_account, with the natural key and any billing, payment_terms and billing_entity.
• Otherwise: sync_accounts, as today, which needs billing=.
  • A pending_approval row with setup is AccountPending(via="sync_accounts").
  • A failed or rejected row is AccountRejected.

Outcomes (adcp.account_outcomes, exported from adcp)

• AccountReady(account_id, status, via, request_id, account).
• AccountPending(action, via, request_id, account_id, status), with PendingAction(waiting_on, kind, message, url, expires_at). The docstrings say url is for a person, not a bearer link, never fetched.
• AccountAmbiguous(candidates, request_id), with await outcome.choose(account_id), which continues the request with request_id plus account_id.
• AccountRejected(errors, via, request_id).

Only a failure to process the call raises AccountRequestError, a new ADCPTaskError subclass carrying the wire codes.

Idempotency, refresh, pending

• ensure is idempotent per (seller, buyer identity, natural key):
  • a known account is returned without a call;
  • an open request is read back through list_accounts(request_ids=[...]), so it is never duplicated;
  • request_account idempotency keys are deterministic per seller, request body and prior request, so registry instances sharing one buyer and durable storage send one key and the seller replays one request;
  • a rejected request may be asked again.
• accounts.refresh() re-reads every open request in one list_accounts call (request_ids), re-reads accounts pending approval with a second call, and returns the outcomes whose state moved.
• accounts.pending() lists what is still waiting, with no network call.
• Both find records across restarts when the storage implements items(seller) (ListableAccountStorage, which InMemoryAccountStorage implements). Otherwise they see the keys this process touched.
• AccountRecord gains request_id, request_state and pending_action. Registry observation of sync_accounts/list_accounts results preserves them.

Agent guidance

• next_step(outcome) returns none, wait_for_seller, send_human_to_url, pick_candidate or contact_seller. Agents switch on it, not on the extensible kind.
• format_pending_action(action) renders safe text for a CLI or chat. It strips control and bidirectional characters from the untrusted message, collapses whitespace, quotes it, and caps it at 1000 characters.

Deviations from the plan

• AccountRef(...)/BrandRef(...) constructors are not added. ensure still takes the existing AccountReference or a mapping.
• outcome.wait(timeout=...), change() and use() are P15.
• refresh() on the sync path reads only the first page of accounts.

Tests

• tests/test_account_outcomes.py (12 tests):
  • request_account routing to ready;
  • pending as a value, plus no duplicate on a repeat ensure;
  • refresh reporting a moved request after a restart, through listable storage;
  • ambiguous with choose;
  • rejected, then a new request;
  • AccountRequestError;
  • sync-path pending_approval and rejected rows;
  • billing required on the sync path;
  • currency/timezone joining the key;
  • format_pending_action escaping and next_step.
• tests/test_account_lifecycle.py is updated for the new return type. The account, client, exception, public-API and import suites pass, as do ruff and mypy.

BREAKING CHANGE: AccountRegistry.ensure returns an AccountOutcome instead of an AccountRecord; billing= is required only on the sync_accounts path; a failed or rejected row is returned as AccountRejected instead of raised.

Refs #1695

🤖 Generated with Claude Code

[aao-secretariat]

Ladon verdict: Approve

Approve — breaking AccountOutcome reshape is correctly signaled and no blocking issues remain.

Checked: the breaking return-type change on client.accounts.ensure carries feat(accounts)! + a BREAKING CHANGE: footer + migration prose in docs/account-lifecycle.md, satisfying the mandatory semver-signal gate on the public surface. Capability routing (request_account vs sync_accounts) is per-seller so the two state families can't mix. No generated-code hand edits, no _generated.py import-layering break, no numbered-class imports, no credentials in ctx_metadata, bounded pagination in _read_requests. CI gates untouched.

Medium findings

• src/adcp/accounts.py:429 — request_account idempotency_key is random (uuid4); dedup is process-local only, so two instances on a shared durable store could open duplicate requests.

Single medium finding, not in {data-loss, schema, infra}. high_risk is true only because src/adcp/__init__.py was modified, with no medium-or-higher finding on that file, so row 5 does not fire. gated_paths is false and no author team gate applies. One medium finding falls through to row 9 → approve. Worth a follow-up on making the idempotency key deterministic for shared-store deployments.

Medium findings

• src/adcp/accounts.py:429 — request_account idempotency_key is random; dedup is process-local only

[aao-secretariat]

Ladon verdict: Request changes

Request changes — 1 blocking finding.

Blocking findings

• src/adcp/accounts.py:529 — ensure sends non-existent list_accounts field account_request_ids (should be request_ids). extra='allow' lets it validate locally, but a conformant seller returns an empty account_requests[], so read-back finds nothing: a repeat ensure opens a duplicate request (defeating the exact idempotency contract this PR claims) and refresh() never reports a move. All tests mask this by mocking list_accounts regardless of payload — this is AdCP wire-shape drift on a load-bearing path, graded high.

Medium findings

• src/adcp/accounts.py:617 — refresh() skips sync-path pending accounts whenever any request is also open.

Rationale: Decision table row 1 fires on the high finding → request-changes. The medium finding should also be addressed. Note: the reshape to AccountOutcome correctly carries the feat!:/BREAKING CHANGE signal with a migration note, and the account_outcomes.py sanitization / next_step routing check out — the wire-field bug is the blocker.

Blocking findings

• src/adcp/accounts.py:529 — ensure sends non-existent list_accounts field account_request_ids (should be request_ids); conformant sellers return empty account_requests[], breaking read-back, duplicate-request prevention, and refresh() move reporting (AdCP wire-shape drift)

Medium findings

• src/adcp/accounts.py:617 — refresh() skips sync-path pending accounts whenever any request is open

Superseded by a later Ladon review that rechecked and resolved its blocking findings.

[aao-secretariat]

Ladon verdict: Approve

Approve — subsequent pass, both prior blockers resolved, no current findings.

This is a follow-up review on PR #1743. The prior run (request-changes) flagged two issues, both now fixed:

• [high, resolved] src/adcp/accounts.py:529 — ensure/refresh previously sent a non-existent account_request_ids field to list_accounts. Now _read_requests sends {"request_ids": request_ids}, matching ListAccountsRequest.request_ids (confirmed against the generated schema). Covered by test_pending_..._never_opens_a_duplicate.
• [medium, resolved] src/adcp/accounts.py:617 — refresh() previously skipped sync-path pending accounts when any request was open. Now open_requests and pending_accounts are processed independently, covered by test_refresh_rereads_pending_accounts_beside_open_requests.

The reviewer also verified the semver signal: the feat(accounts)!: breaking prefix plus migration note correctly accompanies the AccountRecord→AccountOutcome return-shape change (satisfies the public-surface semver mandate), and the format_pending_action untrusted-text/bidi sanitization is in place.

No new high/critical or medium findings. Non-blocking, documented follow-ups: refresh() re-reads only the first page of pending-approval accounts, and _read_requests sends all open request_ids in one payload where request_ids caps at 100 (>100 open requests would hit a validation error) — both low-likelihood and noted, not blocking.

Decision table: no critical/high (row 1 n/a); gated_paths false (row 2 n/a); no deletions (row 3 n/a); no medium findings at all (rows 4/5/8 n/a); prior decision was request-changes, not escalate (row 6 n/a); no no-auto-approve team match (row 7 n/a). high_risk is true only via src/adcp/__init__.py (modified) but with zero medium-or-higher findings, the modification is presumed safe. Falls through to row 9 — approve.

[aao-secretariat]

Ladon verdict: Approve

Approve — breaking return-type reshape correctly signaled and gated.

The PR reshapes client.accounts.ensure to return a discriminated AccountOutcome, routing by capability (request_account vs sync_accounts). The breaking return-type change carries the required semver signal (feat(accounts)!:, a BREAKING CHANGE: footer, and a migration note in the PR body), and the DR-0032/0033 backward pathway is capability-gated for 3.2 sellers lacking account.requests, with both paths tested. list_accounts field names match the schema, AccountRequestError matches ADCPTaskError.__init__, the idempotency key satisfies the wire pattern, pagination is bounded, and format_pending_action/_untrusted sanitization has the right security shape.

Medium findings

• src/adcp/accounts.py:640 — refresh() compares raw wire pending_action to normalized stored form, so an extra='allow' seller field can make unchanged requests report as moved.

A single medium finding with no critical/high. high_risk is true only for src/adcp/__init__.py (modified) but the reviewer found no concern there. gated_paths is false; review_decision is APPROVED. No no-auto-approve team match. Prior decision was approve. No escalation trigger (rows 1-8 don't fire — 1 medium, not data-loss/schema/infra category, no modified-high-risk-file finding) — falls through to row 9.

Medium findings

• src/adcp/accounts.py:640 — refresh() compares raw wire pending_action to normalized stored form

[aao-secretariat]

Ladon verdict: Approve

Approve.

This is a subsequent review of PR #1743 (prior outcome: approve). The breaking return-type change (ensure() → AccountOutcome) correctly carries the feat(accounts)! marker with a migration note in the docstring and docs, and the public-surface snapshot was updated — the semver signal on the public surface is satisfied. Capability routing falls back to sync_accounts, so the DR-0032/0033 pathway holds and the required pathway/test is present.

Medium findings

• src/adcp/accounts.py:640 — refresh() compares raw wire pending_action to the normalized stored form, so a seller that omits pending_action makes every refresh() report an unchanged in_review request as moved.

No critical/high findings. One medium finding (re-surfaced from the prior run, still unresolved). Decision table: Row 1 (no critical/high) does not fire; gated_paths is false (row 2 n/a); high_risk is true but the only high-risk reason is src/adcp/__init__.py (modified) — row 5 requires a modified high-risk file AND a medium finding, but the medium is in accounts.py, not the modified high-risk file, so row 5 does not fire; the medium is not data-loss/schema/infra (row 4 n/a); prior decision was approve (row 6 n/a); no team gate (row 7 n/a); only 1 medium finding (row 8 requires ≥3). Falls through to row 9 → approve.

Medium findings

• src/adcp/accounts.py:640 — refresh() compares raw wire pending_action to normalized stored form

[aao-secretariat]

Ladon verdict: Approve

Approve — subsequent pass, prior Medium resolved, no new findings.

The prior Medium (src/adcp/accounts.py:640 — refresh() compared raw wire pending_action to normalized stored form) is resolved: refresh() now normalizes the current wire action via PendingAction.from_wire(...).to_wire() before comparing against the stored to_wire() form, covered by test_refresh_ignores_unknown_action_fields_and_an_absent_action. The breaking return-type change ships under feat(accounts)! with a migration note, satisfying the semver signal on the public surface. The largest-delta files (account_outcomes.py, accounts.py) were reviewed: untrusted-message handling, deterministic idempotency keys, rejected-reask, and the four outcome arms are sound.

Decision-table walk: no critical/high findings (row 1 n/a); gated_paths false and review_decision APPROVED (row 2 n/a); high_risk is true but the only reason is src/adcp/init.py (modified) with no medium-or-higher finding on it, so rows 3–5 do not fire; prior decision was approve so sticky escalation (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.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean correctness fix with no blocking or medium findings.

What I checked:

• The delta is a single correctness fix in refresh() (src/adcp/accounts.py:908-915): the incoming wire pending_action is normalized through from_wire().to_wire() before being compared to the already-normalized stored record.pending_action. This stops unknown wire fields and an absent action from spuriously registering as a state change on every poll. Normalization is symmetric with how _record_request persists the action, and the else-None branch matches how non-in_review records store None.
• New tests (tests/test_account_outcomes.py, tests/test_account_lifecycle.py) cover both the absent-action default and the unknown-field case — the exact branches this PR added.
• high_risk is true only because src/adcp/__init__.py is (modified); reviewer found no medium-or-higher concerns on it, so the modification is presumed safe (not an escalation trigger). public_api_snapshot.json tracks the public surface and no breaking export change was flagged.
• gated_paths: false; review_decision: APPROVED; no no-auto-approve team match.
• Prior decision was approve with no findings — this run continues clean.

Decision table: rows 1–8 do not fire (no critical/high/medium findings, gated_paths false, no deletions, no team gate, fewer than three mediums). Falls through to row 9 → approve.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean pass, no blocking or medium findings.

Checked the decision table top-down:

• Rows 1 (critical/high) — no findings of any severity.
• Row 2 (gated paths) — gated_paths is false; also review_decision is APPROVED.
• Row 3 (deleted high-risk path) — the only high-risk match is src/adcp/__init__.py (modified), no deletions.
• Row 4 (medium data-loss/schema/infra) — no medium findings.
• Row 5 (high-risk modified + medium) — high_risk is true but there are zero medium findings, so this does not fire.
• 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.

Reviewer verified the substantive risk areas: ensure() reshaped to return AccountOutcome with capability-based routing (request_account vs sync_accounts), tested both ways per DR-0032/0033 backward-compat path. The breaking return-type change ships correctly under feat(accounts)! with a BREAKING CHANGE footer, migration prose, and an updated public_api_snapshot.json — the semver signal on the public surface is satisfied. Idempotency key hashing confirmed safe (datetimes normalized via _payload json mode), pagination bounded at 100 pages, and format_pending_action sanitizes untrusted seller text. This is a net-improving subsequent push over a prior clean approve.