[sujanchalla0510]

Fixes #547 bugs 1 + 3 together (per @bokelley's 2026-10-19 security-hold decisions on this PR), plus the v3 ACCOUNT_NOT_FOUND recovery correction from bug 2.

Bug 1 — account_id-only references resolve. In resolveAccount, an AccountReference carrying only account_id — { "account_id": "acc_123" }, the schema's own first example in core/account-ref.json — hit the Brand == nil && Operator == "" early return and was treated as no account, so the handler ran with public pricing silently. Now any reference with a non-empty account_id is handed to ResolveAccount, and an unknown ID surfaces the standard ACCOUNT_NOT_FOUND path.

Hold decision 2 — schema-invalid references rejected before the resolver (both packages). resolveAccount in adcp and adcp/v3 validates a supplied reference against the pinned core/account-ref.json forms (validAccountReference): {account_id} alone, or the natural key {brand, operator} with both domains matching the schemas' domain pattern (brand-ref requires domain); a present brand_id must match core/brand-id.json. Anything else — in particular an operator-only reference — returns INVALID_REQUEST (recovery: correctable) and never reaches a resolver with a nil Brand. Deliberate decoded-value handling, documented in the Config.ResolveAccount contract: an omitted account (including a decoded reference carrying no identifier, a supplied-but-empty {}, and account_id: "" — indistinguishable from omission in the flattened Go struct) keeps the no-account path and does not invoke the resolver; with no account supplied, handlers run with a nil account (public discovery). Sandbox may accompany only the natural-key form, where it passes through as a modifier for the resolver to honor; {account_id, sandbox: true} is schema-invalid (the account_id arm is additionalProperties: false) and is rejected with INVALID_REQUEST — pinned in TestResolveAccountSchemaEdgeForms in both packages.

Hold decision 1 — bug 3: reference resolvers are lookup-only. reference/seller-agent's resolver (extracted to backend.resolveAccount, matching the build-seller-agent skill example) and reference/hello-seller's resolver return (nil, nil) for unknown references — by account_id or natural key — and store nothing; the SDK answers ACCOUNT_NOT_FOUND. Provisioning happens only through the provisioning path: seller-agent's existing SyncAccounts, and a new SyncAccounts handler for hello-seller (created/updated semantics mirroring seller-agent). seller-agent's main() pre-provisions exactly the two fixture accounts the storyboard names (its Acme test-kit account and the runner default), since fabrication no longer resolves them. sync_accounts handlers in both reference sellers (and the skill example) skip entries without a natural key instead of minting acct-unknown- accounts, and AccountInput (both packages) now decodes the settings-update account key shape.

Register examples + authorization. Both Register doc examples branch on ref.AccountID before dereferencing ref.Brand (the previous example nil-derefed — a crash for anyone copying it) and now perform the principal check in code (PERMISSION_DENIED when the principal may not act for the account); v1's Config.ResolveAccount doc gains the MUST-check-the-principal paragraph and names that v1 adds no principal to ctx (PrincipalFromContext is v3-only — the old v1 example cited it anyway). Reference resolvers carry SWAP notes for the principal check and ref.Sandbox.

Wire change: ACCOUNT_NOT_FOUND message/suggestion branch on the reference form (unknown account_id → "Verify the account_id via list_accounts, or contact the seller."), and v3 defaultRecovery classifies ACCOUNT_NOT_FOUND as terminal (was correctable), matching enums/error-code.json enumMetadata and the v1 module.

Release note: the action-required note on #540 was refreshed 2026-10-25 with the full behavior list; the suggested BREAKING CHANGE: footer for the squash commit is reproduced at the end of this description so the merge commit carries it.

How it was tested: TestRegisteredAccountLifecycle in both SDK packages drives the registered tools with a reference-style lookup-only resolver (unknown account_id → ACCOUNT_NOT_FOUND, unknown natural key → ACCOUNT_NOT_FOUND with nothing provisioned, operator-only → INVALID_REQUEST, then sync_accounts provisions and get_products succeeds by natural key and by returned account_id; fixtures carry billing and buying_mode per the pinned schemas). Schema-edge tests pin brand_id valid/invalid and the sandbox forms in both packages. New hello-seller unit test + seller-agent MCP test pin the sync_accounts skip. Reference sellers keep their pinned adcp/v3 v3.0.0 (they don't compile against in-repo v3 for unrelated 3.3-dev reasons — tracked in #597), so their committed tests pin the resolver/skip contracts directly. Local: go test ./... + go vet green in adcp, adcp/v3, reference/hello-seller, reference/seller-agent.

Not in this PR: the rest of #547 — bug 2's remaining per-code defaultRecovery divergence from enumMetadata (owned by @torbenbrodt per his 2026-10-20 comment on #547) and the AccountReference round-trip field drops tracked by #564.

Suggested squash-commit footer (from the #540 release note):

BREAKING CHANGE: account_id-only AccountReferences now resolve via Config.ResolveAccount (unknown IDs return ACCOUNT_NOT_FOUND instead of silently running with no account); schema-invalid supplied references are rejected with INVALID_REQUEST before the resolver; reference seller resolvers are lookup-only and no longer provision accounts on discovery; v3 classifies ACCOUNT_NOT_FOUND recovery as terminal.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean fail-closed fix with full test coverage.

seller.go:149 adds && acctRef.AccountID == "" so an account_id-only AccountReference now reaches the resolver instead of silently falling through to the public-pricing no-account path. This replaces fail-open behavior with fail-closed (unknown IDs yield ACCOUNT_NOT_FOUND), and the doc comment was updated to match. Seven table/unit tests cover value, pointer, unknown→NOT_FOUND, empty/nil/sandbox-only, brand-only, resolver-error, and nil-resolver paths.

No critical/high/medium findings. No schema/generated-type, TMP signing, TEE, or protocol-skill files touched; no exported-symbol/wire-shape changes. gated_paths is false and high_risk is false. No no-auto-approve team match. Rows 1–8 do not fire; falls through to row 9.

[torbenbrodt]

Thanks @sujanchalla0510, the guard change in resolveAccount is right and I'd like to see it merged. core/account-ref.json (3.2.2) is a oneOf whose first branch is account_id alone, so {"account_id": "acc_acme_001"}, the schema's own first example, names an account and must not fall through to the no-account path. On a local merge with today's main, four of the seven new tests fail without the change and all seven pass with it, and go vet / go test ./... in adcp/v3 are green.

The triage on #547 asked for the resolver contract to be reviewed alongside this guard change. Doing that turns up two things I'd fix in the same PR:

1. The documented resolver now takes the server down. The Register doc comment (adcp/v3/seller.go:37) shows return db.FindAccount(ref.Brand.Domain, ref.Operator). With this PR, get_products with "account": {"account_id": "acc_acme_001"} calls it with ref.Brand == nil. go-sdk v1.5.0 has no recover(), and it runs the tool handler on its own goroutine, so the nil dereference ends the process. I reproduced that with callRegisteredTool over the in-memory transport. An operator-only reference could already reach the resolver with a nil brand, but that payload is schema-invalid. Suggest changing the example to branch on ref.AccountID, and adding to Config.ResolveAccount: "Exactly one form is set: AccountID, or Brand and Operator; Brand is nil for an account_id reference." A release-note line would help anyone who copied the example.

2. The ACCOUNT_NOT_FOUND an unknown account_id now gets points the buyer at the wrong fix. Today it is:

   {"adcp_error":{"code":"ACCOUNT_NOT_FOUND","field":"account","message":"Account not found. Call sync_accounts first to register this brand and operator.","recovery":"correctable","suggestion":"Call sync_accounts with the brand and operator before making this request."}}

   enums/error-code.json gives ACCOUNT_NOT_FOUND "recovery": "terminal" and the suggestion "for a natural key, provision through sync_accounts or omit account for public discovery; for an account_id, verify via list_accounts or contact seller". The message and suggestion could branch on acctRef.AccountID != "". The correctable is #547's second bug, which #543 fixed in adcp/errors.go but which is still in adcp/v3/errors.go's defaultRecovery.

Smaller, not blocking:

• The repo's own resolvers don't read AccountID. skills/build-seller-agent/SKILL.md, reference/seller-agent and reference/hello-seller key accounts as acct-<brand.domain>-<operator>, which becomes acct-- for an account_id reference:

  • reference/seller-agent returns an active account for any account_id;
  • hello-seller stores one acct-- that every account_id caller shares;
  • the skill returns ACCOUNT_NOT_FOUND even for the account_id its own sync_accounts returned.

  In the skill and reference/seller-agent, the map is keyed by that same id, so a lookup on ref.AccountID first would fix them. The reference-seller part overlaps #547's third bug.

• adcp/seller.go:407 has the identical guard and is unchanged. #547's triage said any fix must touch both packages. If v1 is left out on purpose, a line in the description would settle it.

• Three test payloads are schema-invalid. {} and {"sandbox": true} match neither branch, and a brand without operator fails the second branch's required. The natural-key case would read better with the schema's own {"brand": {"domain": "acme-corp.com"}, "operator": "acme-corp.com"}. Also, {"account_id": ""} is schema-valid and still takes the no-account path.

Out of scope here: AccountReference still drops currency, operator_unit, timezone and brand.countries from the schema's fourth example on a round trip, before and after this PR. #564 tracks that.

Our interest: we're building a multi-tenant seller on adcp/v3 and refer to accounts by account_id, so we'd like this one in.

[sujanchalla0510]

Thanks for the thorough review, @torbenbrodt — addressed in 42fd0b6:

1. Doc example crash: both Register examples (v3 and v1) now branch on ref.AccountID before touching ref.Brand, and the Config.ResolveAccount docs in both packages state the contract: exactly one form is set — AccountID, or Brand and Operator together; Brand is nil for an account_id reference.
2. ACCOUNT_NOT_FOUND guidance: message/suggestion now branch on the reference form in both packages — an unknown account_id gets 'Verify the account_id via list_accounts, or contact the seller.' Also fixed the recovery you flagged: v3 defaultRecovery classified ACCOUNT_NOT_FOUND as correctable; it is now terminal, matching enums/error-code.json and the v1 module.
3. v1 guard: adcp/seller.go resolveAccount now has the same AccountID == \"\" condition as v3, so account_id-only refs resolve in v1 too (per the Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547 triage: both packages).
4. Reference resolvers: reference/hello-seller, reference/seller-agent and the build-seller-agent skill now look up ref.AccountID first (the id sync_accounts returned) instead of deriving acct-- for account_id references.
5. Tests: the natural-key case now uses the schema's own {brand: {domain: acme-corp.com}, operator: acme-corp.com} example; the empty-ref cases are commented as decoded representations of an omitted account field, with an explicit {account_id: \"\"} case pinned to the no-account path; new tests pin the branched NOT_FOUND guidance and terminal recovery in v3, plus v1 resolver tests.

Left alone as you noted: the AccountReference round-trip field drops — tracked by #564.

Local verification: go test ./... + go vet green in adcp and adcp/v3; hello-seller and seller-agent tests green.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean pass, no blocking findings.

Delta commit 42fd0b6 ports the previously-approved v3 account_id-only resolveAccount fix into the v1 adcp package, adds ACCOUNT_NOT_FOUND guidance that branches on reference form (account_id→list_accounts, natural key→sync_accounts), and corrects the v3 recovery classification (correctable→terminal) to match the pinned enums/error-code.json and existing v1 behavior (adcp/errors.go:131). Reference resolvers now look up ref.AccountID before deriving the natural key.

Checked: no schema/types_gen/TMP/identity-agent/protocol-skill surface touched; no breaking wire change; changes consistent across packages and well-tested. No Critical/High/Medium findings.

Gate review: gated_paths is false and high_risk is false, so no hard path gate applies. No no-auto-approve team match. review_decision is REVIEW_REQUIRED, but with gated_paths false that does not force escalate. Decision table rows 1–8 do not fire → row 9 approve.

[torbenbrodt]

Thanks @sujanchalla0510, 42fd0b6 does what you describe. On a fresh merge with main I checked:

• Both Register examples now branch on ref.AccountID first. Run end to end, the v3 example answers get_products with "account": {"account_id": "acc_acme_001"} with ACCOUNT_NOT_FOUND instead of panicking: "message":"Account not found.","recovery":"terminal","suggestion":"Verify the account_id via list_accounts, or contact the seller.". That matches enums/error-code.json.
• adcp/seller.go now has the same guard. The new tests fail on main (5 of 8 in adcp/v3, both in adcp) and pass on the branch. The v3 CI checks (regeneration, any-coverage, lint.py --strict, Python tests, vet, test) and the adcp, seller-agent and hello-seller tests are green.

From our side this is good to merge. Three small things, none blocking:

• The description still describes the first commit: 7 tests, adcp/v3 only. It's worth a refresh, since ACCOUNT_NOT_FOUND's recovery changes on the wire.
• reference/seller-agent and reference/hello-seller build against the pinned adcp/v3 v3.0.0, whose guard still drops account_id references, so their new AccountID lookup isn't exercised yet. Also, seller-agent still returns an active account for an unknown id (acc_nope resolves to an account named acc_nope). That's Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547's third bug, not this PR.
• As a follow-up: adcp/v3's defaultRecovery still differs from enumMetadata for 104 of the enum's 120 codes. For example, ACCOUNT_SETUP_REQUIRED should be correctable and CONFLICT should be transient. That's the rest of Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547's second bug.

[sujanchalla0510]

Thanks @torbenbrodt — glad this is good to merge from your side. Two follow-ups from your notes:

• Description refreshed to cover the full 42fd0b6 delta (both packages, the Register examples, the form-branched ACCOUNT_NOT_FOUND guidance, and the v3 recovery correction on the wire) instead of only the first commit.
• The defaultRecovery remainder is now noted on Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547: your count (104 of 120 codes differing from enumMetadata, e.g. ACCOUNT_SETUP_REQUIRED → correctable, CONFLICT → transient) is recorded there as the open remainder of bug 2, along with the seller-agent unknown-id behavior as bug 3 (including the pinned adcp/v3 v3.0.0 caveat).

[torbenbrodt]

Approving the code. 42fd0b6 resolves my review, and merged onto today's main (188cdcf) CI's core checks pass locally: generator tests, regeneration with no diff, lint.py --strict and the any-coverage gate in both modules, and go vet with go test -race for root, adcp, adcp/v3 and the reference agents. #547 tracks the remaining defaultRecovery mismatch and bug 3.

Not tracked yet:

• The new doc text says "Exactly one form is set", but the SDK doesn't enforce it. A reference with only operator still reaches the resolver with Brand nil, and the documented example then dereferences ref.Brand.Domain. That was already true on main.
• The example's account_id branch looks the account up without checking that the caller may act for it. It also ignores sandbox, which an account_id reference now carries through to the resolver.

[torbenbrodt]

@bokelley, two decisions for you before this merges:

1. Bug 1 before bug 3? Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547's triage asks for them to land together. This PR fixes bug 1's guard. Bug 3, the reference seller returning an account for any id, stays open, and becomes reachable through account_id once the reference agents move off the pinned adcp/v3 v3.0.0. Is landing bug 1 first fine with you?
2. Should the SDK reject the reference forms the schema doesn't allow? A reference with only operator reaches the resolver with Brand nil, and the documented resolver then panics. fix(adcp): reject nil handler results instead of panicking the dispatcher #544 shows a panic inside an MCP tool call is not recovered and takes the process down. Rejecting such forms in resolveAccount with INVALID_REQUEST would close it, but that changes the resolver contract, so it's your call.

Separately, adopters who copied the old example will crash on {"account_id": "..."} after upgrading, so the release notes for this change (#540) need an action-required line.

[sujanchalla0510]

Thanks @torbenbrodt — approving review actioned. The 'not tracked yet' items are now recorded on #547 so they survive this PR:

• the unenforced 'exactly one form is set' contract (an operator-only reference reaches the resolver with Brand nil and the example would deref it — pre-existing on main);
• the example account_id branch's missing caller-authorization check and its ignoring sandbox;
• the release-notes action-required line for adopters of the old example (chore(main): release adcp 3.3.0-beta.0 #540).

Your two questions for @bokelley — landing bug 1 before bug 3, and whether resolveAccount should reject schema-invalid reference forms with INVALID_REQUEST — are contract decisions for him; I've noted the second on #547 as pending his call rather than changing the resolver contract unilaterally.

[bokelley]

@torbenbrodt @sujanchalla0510 — maintainer decisions, posted by Brian's coding agent at his direction:

1. Hold this PR until bug 3 in Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547 is fixed alongside bug 1. The security triage on Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547 explicitly called for coordinated changes. Both reference sellers still accept unknown accounts, and the pinned dependency currently masks the newly reachable account_id path. Fix the reference resolvers to return no account for an unknown reference, provision only through the provisioning path, and exercise the combined behavior against the updated SDK before merging. The remaining per-code recovery alignment can stay in Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547.
2. Yes: reject schema-invalid supplied account references with INVALID_REQUEST before invoking ResolveAccount, in both SDK packages. In particular, an operator-only reference must never reach a resolver with a nil Brand. Validate against the pinned schema rather than adding stricter rules of our own; preserve omitted-account behavior and handle decoded zero values deliberately. The documented resolver contract must match what the SDK actually enforces.
3. Include an action-required release note in chore(main): release adcp 3.3.0-beta.0 #540 telling adopters of the old resolver example to branch on AccountID before dereferencing Brand. Examples must also preserve caller authorization and the applicable sandbox semantics.

This is an explicit security-related hold; a lazy-consensus timeout does not clear it. These decisions supersede the earlier “none of these block #554” tracking note on #547.

[sujanchalla0510]

Thanks @bokelley — decisions noted and accepted; recording them here so the hold state is unambiguous:

• This PR is on explicit security hold. No merge and no lazy-consensus reliance until bug 3 lands alongside bug 1, per your direction. I'm not treating any timeout as clearing it.
• Implementation plan (next adcp-go work item, in this order):
  1. resolveAccount in both packages (adcp, adcp/v3): validate a supplied AccountReference against the pinned account-ref schema before ResolveAccount is invoked; schema-invalid forms (in particular an operator-only reference) return INVALID_REQUEST and never reach a resolver with a nil Brand. Omitted-account behavior is preserved; decoded zero values are handled deliberately, and the documented ResolveAccount contract will be updated to state exactly what the SDK enforces.
  2. Reference resolvers (hello-seller, seller-agent): return no account for an unknown reference — provisioning only through the provisioning path — and exercise the combined bug 1 + bug 3 behavior against the updated SDK (the reference agents' pinned v3.0.0 dependency currently masks the account_id path, so that gets verified explicitly rather than assumed).
  3. Action-required release note posted on chore(main): release adcp 3.3.0-beta.0 #540 for adopters of the old resolver example (done — see that issue), and the Register examples keep caller authorization and sandbox semantics intact.
• The remaining per-code recovery alignment stays in Seller accounts: account_id-only refs silently dropped, recovery values outside the spec, reference seller provisions on discovery #547, as you directed.

I'll implement on this branch and re-request review here when the combined behavior is in and tested; the hold stays until you or @torbenbrodt release it.

[sujanchalla0510]

Implemented per your decisions, @bokelley — new head 07b40058, on top of 42fd0b6:

1. Validation before ResolveAccount (both adcp and adcp/v3): resolveAccount validates a supplied reference against the pinned core/account-ref.json forms — {account_id} alone, or {brand, operator} with both domains matching the schemas' domain pattern. An operator-only reference (and every other non-form) returns INVALID_REQUEST and never reaches a resolver with a nil Brand. No stricter rules than the schema. Two deliberate decoded-value choices, documented in the code and in the Config.ResolveAccount contract so the contract matches enforcement: an omitted/no-identifier reference keeps the no-account path without invoking the resolver, and sandbox is passed through as a modifier on either form for the resolver to honor (the flattened Go type cannot express the wire schema's per-branch property sets). Flagging the sandbox choice explicitly in case you want it stricter.
2. Bug 3: both reference resolvers are lookup-only now — unknown account_id or natural key returns (nil, nil) and stores nothing (ACCOUNT_NOT_FOUND from the SDK). Provisioning only via sync_accounts: seller-agent's existing handler, plus a new SyncAccounts handler for hello-seller, which previously had no provisioning path at all (its resolver was the only way an account came to exist).
3. Combined behavior against the updated SDK: new TestRegisteredAccountLifecycle in both SDK packages drives the registered tools with a reference-style resolver — unknown id/key → ACCOUNT_NOT_FOUND with nothing created, operator-only → INVALID_REQUEST, sync_accounts → get_products succeeds by natural key and by returned account_id. The reference modules still pin adcp/v3 v3.0.0, and they don't compile against the in-repo v3 for unrelated 3.3-dev reasons (ProductAllowedAction, PackageInput.Budget), so the account_id half of the combined exercise lives in the SDK tests, where CI runs it against this code; the reference modules' own new tests pin the lookup-only contract directly and, for seller-agent, at MCP level on the natural-key path. Full detail in the refreshed PR description.

Local: go test ./... + go vet green in adcp, adcp/v3, reference/hello-seller, reference/seller-agent.

@bokelley @torbenbrodt — re-requesting review; the hold stays until you release it.

[aao-secretariat]

Ladon verdict: Approve

Approve.

Verified against the decision table top-down:

• No critical/high/medium findings (reviewer reports a clean pass).
• gated_paths: false — row 2 does not apply.
• high_risk: false — rows 3 and 5 do not apply.
• No no-auto-approve team match — row 7 does not apply.
• Fewer than three medium findings (zero) — row 8 does not apply.
• Prior decision was approve (not escalate) — row 6 does not apply.

Falls through to row 9 → approve.

What was checked: the PR implements the #554 security-hold items — a fail-closed validAccountReference guard in adcp/seller.go and adcp/v3/seller.go (returns INVALID_REQUEST before the resolver for references matching neither pinned account-ref form), reference sellers (hello-seller, seller-agent) converted to lookup-only with provisioning routed through sync_accounts, plus new unit/MCP tests across both SDK packages and both reference modules. Guard placement (after the empty-reference early return) preserves the no-account path for omitted/sandbox-only refs; oneOf branching and required BrandReference.Domain match the documented contract; the bug-3 fabrication-on-miss path is removed and pinned by tests. No schema/types_gen coherence, TMP signing, TEE boundary, protocol-skill, or wire-break concerns triggered.

Note (non-blocking, not a finding): the reviewer could not diff accountDomainPattern against the live schema because schemas are gitignored and network fetch was blocked. Worth a human eye if brand-ref's domain is unconstrained upstream, but this is speculative and not actionable here.

[sujanchalla0510]

Follow-up on 07b40058's CI: everything passed except Reference Seller Storyboard (6 create_media_buy steps → ACCOUNT_NOT_FOUND). Reproduced locally with the CI harness (@adcp/sdk@13.0.0-rc.26) and traced it:

• The storyboard names two buyer accounts that no step ever provisions via sync_accounts: the Acme Outdoor test-kit account (acmeoutdoor.example / pinnacle-agency.example, the fixtures' declared buyer) and the runner's own default account for enricher-built requests (test.example / test.example — resolveAccount's default in the runner). Both previously resolved only because the reference resolver fabricated an account for any name — bug 3 itself.
• Fix in 9a05946: main() now pre-provisions exactly those two fixture accounts at startup, alongside the seeded product catalog — operator provisioning at boot, not resolution-time creation. The resolver stays lookup-only: any other unknown account_id or natural key still answers ACCOUNT_NOT_FOUND and stores nothing (pinned by the new unit + MCP tests).

Local verification with the exact CI script (scripts/ci/run_storyboard_reference_seller.sh): 83 steps passed, 0 failed, clean partial (only fixture-unavailable / not-applicable skips — the state the harness accepts). go test ./... in reference/seller-agent still green.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean delta pass, no blocking findings.

This is a subsequent review; the prior decision was approve and the developer pushed additional commits. The reviewer's delta pass covered the single meaningful change: ~24 net added lines in reference/seller-agent/cmd/seller-agent/main.go that pre-provision two sandbox accounts at startup (the acme-outdoor storyboard buyer and the test.example fixture) so the media_buy_seller storyboard runs without a sync_accounts step.

Checked: both seeded keys match the new resolveAccount natural-key format (acct-{brand.domain}-{operator}) and are reachable via the account_id path; the #547 bug-3 fix is preserved (resolution fabricates nothing — only sync_accounts and this explicit startup seed create accounts). No schema/types_gen coherence concerns, no TMP signing changes, no identity-agent TEE changes, no protocol-managed skill edits, no wire-shape breaks.

No Critical/High/Medium findings. Decision table: rows 1–8 do not fire — no critical/high findings, gated_paths is false, high_risk is false, no medium findings, no no-auto-approve team match, fewer than three mediums. Note review_decision is REVIEW_REQUIRED, but with gated_paths false that imposes no approval gate here. Falls through to row 9 → approve.

[torbenbrodt]

Thanks @sujanchalla0510, this is a good implementation of @bokelley's 2026-10-19 decisions. On a fresh merge with today's main I checked:

• Regeneration (no diff), lint.py --strict, the generator tests, go vet and go test -race in adcp and adcp/v3, and reference/seller-agent and reference/hello-seller as CI runs them, all pass.
• Decision 2: I ran about 30 reference shapes through resolveAccount and through Register over an in-memory MCP client in both packages, and compared each to core/account-ref.json (Draft 7).
  • Operator-only, brand-only and mixed account_id plus natural key get INVALID_REQUEST (field:"account", correctable, matching enums/error-code.json) and never reach a resolver. The only nil-Brand call is the documented account_id form.
  • Nothing is stricter than the schema, and accountDomainPattern is identical to the pinned pattern.
• Decision 1: both reference resolvers are lookup-only. I ran hello-seller end to end against the updated SDK (patched only for its unrelated Budget compile error). Unknown id and key give ACCOUNT_NOT_FOUND with nothing created, and sync_accounts then makes both forms resolve.
• The storyboard: I reproduced 6 ACCOUNT_NOT_FOUND failures at 07b4005 and 83 steps passing at 9a05946. Dropping only the test.example seed brings the failures back. Your explanation is right: the resolver stopped fabricating, and the SDK did not regress.

Findings, with a fix each:

1. Authorization in the examples is a comment only. The code is db.FindAccountByID(ref.AccountID). The v1 example names adcp.PrincipalFromContext, which does not exist in package adcp, and v1's Config.ResolveAccount doc has no "MUST check the principal" paragraph. Please pass the principal in the example code, use the right symbol per package, and add SWAP notes to the reference resolvers about principal and ref.Sandbox.
2. The #540 note is a comment on the release PR, so it will not reach the changelog, and it predates the implementation. It does not mention the new INVALID_REQUEST, that account_id references now resolve, the v3 recovery change on ACCOUNT_NOT_FOUND, or the reference sellers no longer auto-creating accounts. Please add a BREAKING CHANGE: footer or release-please override to the merge commit and refresh the note.
3. Title and description are stale. The top still says bug 3 stays open, and 9a05946 is not mentioned. The squash message inherits this.
4. A few schema-invalid references still pass. {account_id, sandbox:true} reaches the resolver (schema: additionalProperties:false; rejecting it is not a stricter rule). A supplied {} or {"sandbox":true} on the pointer-typed requests takes the public path, because nil and a pointer to a zero value are collapsed. {"account_id":""} is schema-valid but gets public discovery instead of ACCOUNT_NOT_FOUND. brand_id is not validated against ^[a-z0-9_]+$. Please reject the first, treat a non-nil empty pointer deliberately, and say in the contract what is intentionally left out.
5. create_media_buy with no account or account:{} still runs the handler with a nil account. hello-seller created a buy for both. That sidesteps the unknown-account refusal. This looks like a question for @bokelley; at minimum the contract should say so.
6. hello-seller's new SyncAccounts creates acct-unknown- for a schema-valid entry that has only account. seller-agent's handler does the same. Please skip or reject entries without brand and operator.
7. The hello-seller README sample create_media_buy now returns ACCOUNT_NOT_FOUND. It needs a sync_accounts step first.
8. Tests: the new lifecycle fixtures lack billing and buying_mode. Please use assertSchemaValid from #587, and add a test that ties the regexp and the case table to the bundle.

Smaller points: the reference modules do not compile against the in-repo v3 (ProductAllowedAction, Budget), so the real reference code is never exercised against the tree. That is worth its own issue. The seed ties seller-agent to the runner's default account, so it may be worth asking in adcp whether sellers must resolve unprovisioned accounts.

Our interest is unchanged: we call adcp/v3 by account_id and want this one in.

From the original reviewer's side, the substance of the hold looks met: bug 3 and bug 1 land together, and schema-invalid forms are rejected before the resolver. What I would still want is 1 and 2 (the authorization example and a real release note), and a decision on 4 and 5. I am leaving the release of the hold to @bokelley.

[aao-secretariat]

Ladon verdict: Approve

Approve — refines prior-approved account-ref work with no blocking issues.

What I checked:

• validAccountReference now correctly rejects account_id+sandbox (schema account_id arm is additionalProperties:false) and enforces core/brand-id.json (^[a-z0-9_]+$).
• AccountInput gains the account settings-update key shape matching the SyncAccountsRequest two-key-shape schema. AccountInput is a hand-written KNOWN_TYPES escape hatch, so no types_gen.go regen is required — schema↔generated-type coherence preserved.
• Reference sync_accounts handlers now skip entries without a natural key instead of fabricating acct-unknown- (fixes adcp-go#547 bug 3). Well-tested.
• No TMP signing/verification, identity-agent TEE, or protocol-managed-skill changes. No wire-breaking exported-symbol changes requiring a conventional-commit marker.

Gates: No no-auto-approve team match; gated_paths: false; high_risk: false. Although review_decision is REVIEW_REQUIRED, row 2 only fires when gated_paths is true, which it is not here — so no escalation from that path.

Only one Low finding (dead/panic-prone defensive id check at hello-seller/main_test.go:256; count assertions carry the real coverage), which is omitted per severity rules. Falls through to row 9.

[aao-secretariat]

Ladon verdict: Approve

Approve — clean subsequent pass, no findings.

This is a re-review after a rebase onto newer main. The prior decision on head df7c5b3 was approve with no findings; none recur. The 160-file delta is overwhelmingly rebase-imported code, and the PR-surface touches (SyncAccountsAuthorizer confirmed already present in base via identity/grants.go, and the lint.py KNOWN_SCHEMA_GAPS 'account' removal now that AccountInput carries the Account field) were verified correct. The PR's own #547 fix (resolveAccount/validAccountReference in adcp/ and adcp/v3/, ACCOUNT_NOT_FOUND→terminal in errors.go, the Account wire field, lookup-only reference resolvers, and tests) is unchanged from the approved SHA and consistent with the pinned schema and new base tooling.

Gate checks: no no-auto-approve team match; gated_paths false; high_risk false; no deleted sensitive files. No critical/high/medium findings. Rows 1–8 do not fire; falls through to row 9 → approve.

[sujanchalla0510]

Thanks for the full verification pass, @torbenbrodt — all eight findings addressed in df7c5b3, plus a merge of current main in 2b5a4bf7 (final head; CI 16/16 green, Ladon approved):

1. Authorization in the examples — done. Both Register examples now perform the check in code: look up, return nil for unknown, then PERMISSION_DENIED when the principal may not act for the account. Per package: v3 uses adcp.PrincipalFromContext(ctx); v1 has no principal plumbing at all (no PrincipalFromContext, no ctx principal), so the v1 example uses the adopter's own middleware value and says so — and v1's Config.ResolveAccount doc gains the MUST paragraph, corrected to name that. SWAP notes covering the principal check and ref.Sandbox added to both reference resolvers (and hello-seller's existing SWAP note extended).

2. Release note — done. Refreshed note on chore(main): release adcp 3.3.0-beta.0 #540 (supersedes the 2026-10-19 one) covering INVALID_REQUEST rejection, account_id resolution, the v3 ACCOUNT_NOT_FOUND recovery change, and lookup-only reference resolvers, with a suggested BREAKING CHANGE: footer for the squash commit — the footer text is also in the PR description, since the merge commit itself is yours to write.

3. Title and description — done. Both rewritten for the full current scope (bugs 1+3 together, 2b5a4bf7 included).

4. Schema-invalid references — mostly done, one deliberate exception flagged for @bokelley. Added: a present brand_id must match core/brand-id.json (^[a-z0-9_]+$), both packages, with tests. Documented in the contract: a supplied-but-empty reference ({}, {sandbox:true}) and account_id: "" are indistinguishable from omission in the decoded Go struct and deliberately take the no-account path (not rejected). Exception: {account_id, sandbox: true} — you are right that the schema arm is additionalProperties:false, but this branch already pins the passthrough in TestResolveAccountSandboxFormsStillResolve as a deliberate decision from the hold implementation (sandbox as a flattened-type modifier on either form). I did not flip a pinned, previously-flagged behavior unilaterally; it is now called out in validAccountReference's comment as standing behavior awaiting a maintainer call. @bokelley — tighten it (reject) or keep the passthrough?

5. No-account create_media_buy — contract now says it, decision left with @bokelley. The Config.ResolveAccount contract in both packages now states: with no account supplied, handlers run with a nil account (public discovery), and sellers that require an account must enforce that in their handlers. Whether the SDK should instead refuse account-less calls on account-scoped tools is the open question.

6. sync_accounts acct-unknown- — fixed. Both reference handlers (and the build-seller-agent skill example) now skip entries without a natural key instead of minting from empty fields; pinned by a new hello-seller unit test and a seller-agent MCP test (response carries only provisioned entries). Root cause worth noting: the pinned SDK's AccountInput could not even decode the account key shape — in-repo AccountInput (both packages) now has the account field for the settings-update shape. Removing its stale KNOWN_SCHEMA_GAPS entry (CI: catch silent schema/Go type drift (round trips, null, fixtures, references) #564) is part of the merge commit, per lint.py's own rule.

7. hello-seller README — done. The sample flow now provisions via sync_accounts (with billing) before the create_media_buy sample.

8. Tests — done, one caveat. Lifecycle fixtures now carry billing (provisioning mode) and buying_mode (required by get-products-request) in both packages, plus new schema-edge tests (brand_id valid/invalid, sandbox forms). Caveat: assertSchemaValid from test(adcp/v3): validate wire fixtures against their JSON schema #587 exists on main but was not on this branch; the branch now merges main, so the helper is in the tree — wiring the lifecycle fixtures through it is a small follow-up I left rather than expanding this diff further. Filed Reference seller modules do not compile against the in-repo adcp/v3 module #597 for the reference-modules-vs-in-repo-v3 compile gap, as you suggested.

The merge also verified clean locally before pushing: lint.py --strict (both modules), generator unittests, and go test -race -count=1 ./... in adcp and adcp/v3 all pass in the merge tree.

[torbenbrodt]

Thanks @sujanchalla0510. On 2b5a4bf I re-ran regeneration, lint.py --strict, the generator tests, go test -race in adcp and adcp/v3, and both reference sellers, and all pass. Points 1, 2 and 5 to 8 hold as you describe. Point 4 is better than your reply says: df7c5b3 already rejects {account_id, sandbox:true} with INVALID_REQUEST in both packages (validAccountReference, pinned in TestResolveAccountSchemaEdgeForms), and my probe confirms it. So that question for @bokelley is already settled, and the description should say so. Please also put the BREAKING CHANGE: footer text into the description, so the squash commit carries it.

One thread is open: reference/hello-seller/main_test.go:256. Replace the inverted guard with strings.HasPrefix(id, "acct-unknown"). I'll re-approve once it's resolved. Releasing the hold and the remaining question on account-less calls stay with @bokelley.

[sujanchalla0510]

@torbenbrodt — the open thread is resolved in adf7e97 (on top of 2b5a4bf):

• reference/hello-seller/main_test.go: the inverted guard is replaced with exactly strings.HasPrefix(id, "acct-unknown") (plus the strings import) — no slice, so no panic on short ids, and it actually catches the acct-unknown-<operator> ids the old code minted. Local: go vet + go test ./... pass in reference/hello-seller, gofmt clean. FINAL CI on adf7e97: 16/16 completed (14 success, 2 skipped).
• Point 4: you're right, and the description now says so — validAccountReference in both packages rejects {account_id, sandbox: true} with INVALID_REQUEST (the account_id arm is additionalProperties: false), pinned in TestResolveAccountSchemaEdgeForms; sandbox remains a modifier on the natural-key form only. The stale "question left open" sentence is gone.
• The BREAKING CHANGE: footer text from the chore(main): release adcp 3.3.0-beta.0 #540 note is now in the PR description verbatim, so the squash commit carries it.

Re-approval is yours when you're ready; the hold release and the account-less-calls question stay with @bokelley, as you said.

[torbenbrodt]

Re-approving at adf7e97: the inverted guard is now strings.HasPrefix(id, "acct-unknown"), the description carries the BREAKING CHANGE: footer and no longer leaves the sandbox case open, and CI is green. From my side the hold's conditions are met; releasing it, and the account-less-calls question, stay with @bokelley.