Skip to content

feat(media-buy): split product discovery tools for AdCP 3.2 - #6115

Open
bokelley wants to merge 18 commits into
mainfrom
security-wave-12-get-products-idempotency
Open

feat(media-buy): split product discovery tools for AdCP 3.2#6115
bokelley wants to merge 18 commits into
mainfrom
security-wave-12-get-products-idempotency

Conversation

@bokelley

@bokelley bokelley commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What changed

  • add the AdCP 3.2 list_products, recommend_products, refine_proposal, and finalize_proposals tools with narrow request/response schemas and task documentation
  • retain get_products as a key-optional compatibility facade throughout AdCP 3.x; new integrations should use the split tools
  • make the stateful split calls retry-safe by requiring caller-owned idempotency keys, while list_products remains a synchronous key-optional read
  • canonicalize equivalent legacy and split requests into one get_products operation family before authorization, idempotency, task persistence, and webhook delivery
  • advertise the split tools in capabilities and manifest metadata, with source-schema-driven MCP tools/list projection

Compatibility and idempotency

This is an additive AdCP 3.2 minor change, not the AdCP 4.0 tightening.

Existing 3.x get_products payloads remain valid without an idempotency key. When a key is supplied, exact retries replay across the legacy and equivalent split tool names; key reuse with a different canonical request returns a conflict. Explicit version pins participate in the canonical hash, and unpinned requests are isolated by their effective served release so behavior cannot replay across release boundaries.

recommend_products, refine_proposal, and finalize_proposals require idempotency keys because they can create tasks or mutate proposal state. list_products accepts an optional key and accepts the universal callback wrapper field for portability, but remains synchronous and emits no callbacks. Stateful aliases emit the canonical get_products task/webhook identity exactly once.

The stricter removal/tightening of the polymorphic legacy facade remains a separate AdCP 4.0 concern.

Validation

  • three independent final reviews: protocol/schema, split-contract parity, and implementation/runtime
  • TypeScript no-emit check
  • schema validation: 28/28; deprecation metadata: 4/4; composed examples: 302/302
  • MCP schema projection: 9/9; docs navigation: 20/20
  • focused server unit coverage: 692/692
  • sales tenant routing and shared task-store coverage: 22/22
  • webhook integration coverage: 11/11
  • compliance build and source-authority lints
  • Changesets resolves adcontextprotocol as a minor release
  • git diff --check

@bokelley
bokelley force-pushed the security-wave-12-get-products-idempotency branch from f460c71 to 60068ad Compare August 9, 2026 06:38
@bokelley
bokelley marked this pull request as ready for review August 9, 2026 15:09
Comment thread server/src/training-agent/task-handlers.ts
@aao-secretariat aao-secretariat Bot added the ladon/needs-human-review Ladon has escalated this PR for human review. label Aug 9, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Escalate to human review

Escalate — gated schema path change requires human/CODEOWNERS review, and this is a Breaking-class protocol change.

This PR classifies get_products as state-mutating and adds a required idempotency_key to the published request schema — an optional→required addition on a published wire surface, i.e. a Breaking-class change. It correctly carries a major changeset and schema↔docs coherence checks out (idempotency_key row: Yes, 16-255, matching pattern; manifest, examples, compliance fixtures, MCP/v5/v6 routes, and Addie tools all coherent). The idempotency.ts fencing and task-handlers recovery/natural-key reuse logic were reviewed and are sound.

Two escalation triggers apply and neither is satisfiable by the arbiter:

  • Gated paths: static/schemas/source/manifest.schema.json and static/schemas/source/media-buy/get-products-request.json are under the hard approval gate (gated_paths: true), and review_decision is REVIEW_REQUIRED (not APPROVED). Decision-table row 2 forces escalate.
  • Repo hard gate: a Breaking-class protocol change is never auto-approved — ratification is a human act — unless review_decision is APPROVED. It is not.

No critical/high findings were reported (a correct major changeset accompanies the breaking change, so no spec-drift/changeset finding). One Medium remains: the new per-session get_products mutex CONFLICTs concurrent pure reads.

Human/CODEOWNERS review of the gated schema files is required before this can merge.

Medium findings:

  • server/src/training-agent/task-handlers.ts:3912 — get_products session mutex CONFLICTs concurrent pure reads

Medium findings

  • server/src/training-agent/task-handlers.ts:3912 — get_products session mutex CONFLICTs concurrent pure reads

Why human review

  • Modifies gated schema files static/schemas/source/manifest.schema.json and static/schemas/source/media-buy/get-products-request.json under the hard approval gate; review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • Breaking-class protocol change (optional→required idempotency_key on published get_products request schema, get_products reclassified as state-mutating) — never auto-approved; ratification is a human act unless review_decision is APPROVED.
  • Medium: server/src/training-agent/task-handlers.ts:3912 — get_products session mutex CONFLICTs concurrent pure reads.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/manifest.schema.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/get-products-request.json (modified) matches static/schemas/source/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Escalate to human review

Escalate — gated schema files require human/CODEOWNERS review; a Breaking-class protocol change is present.

Escalation reasons

  • Gated path not satisfied: modifies static/schemas/source/manifest.schema.json and static/schemas/source/media-buy/get-products-request.json under the hard, non-overridable approval gate; review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS approval is required before merge.
  • Breaking-class protocol change (optional→required idempotency_key on the published get_products request schema; get_products reclassified as state-mutating) is never auto-approved — ratification is a human act unless review_decision is APPROVED.

Iteration note
The prior medium finding (get_products session mutex CONFLICTing concurrent pure reads at server/src/training-agent/task-handlers.ts:3912) is resolved: brief/wholesale now take the mutex only for a bounded (~1s, backoff+jitter) directive-consumption preflight, release before read-only work, and no longer write lastGetProductsContext. Directive consumption is threaded through readDirectives so exactly one of N parallel reads consumes a one-shot rejection. Four concurrency tests added. No new blocking or medium findings in the delta.

The escalation persists solely because the gated-path approval gate remains unsatisfied (REVIEW_REQUIRED) and the underlying change is Breaking-class — not because of any unresolved reviewer finding. The gate lifts the moment a human/CODEOWNERS approval is recorded.

Why human review

  • Gated paths not satisfied: static/schemas/source/manifest.schema.json and static/schemas/source/media-buy/get-products-request.json are under the hard approval gate and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • Breaking-class protocol change (optional→required idempotency_key on published get_products request schema; get_products reclassified as state-mutating) is never auto-approved unless review_decision is APPROVED — human ratification required.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/manifest.schema.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/get-products-request.json (modified) matches static/schemas/source/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

@bokelley
bokelley force-pushed the security-wave-12-get-products-idempotency branch from 983fced to d5b0a59 Compare August 9, 2026 20:42

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Escalate to human review

Escalate — hard approval gate unsatisfied plus breaking-class protocol change.

This PR modifies published protocol source under static/schemas/source/** (manifest.schema.json, media-buy/get-products-request.json), which is a deterministic hard approval gate: gated_paths: true and review_decision: REVIEW_REQUIRED (not APPROVED). Row 2 of the decision table fires — cannot auto-approve until a human/CODEOWNERS approval is recorded.

Independently, the reviewer confirms this delta reframes the get_products idempotency contract as a 4.0 major boundary: idempotency_key is in required[] with x-mutates-state:true, an optional→required flip on a published field that reclassifies get_products as state-mutating. That is a Breaking-class protocol change under the constitution and the repo's "Breaking-class changes are never auto-approved" hard gate — ratification is a human act, escalate unless review_decision is APPROVED. It is not.

Reviewer found no Critical/High/Medium correctness findings in the delta; schema↔docs coherence holds (security.mdx, specification.mdx, get_adcp_capabilities.mdx align on the 4.0 cut), oneOf/canonical coverage preserved, deploy wiring safe. The only finding is one low-severity legacy-wire projection note, which does not block on its own. But the two hard gates above are dispositive.

Escalation reasons

  • Gated paths not satisfied: static/schemas/source/manifest.schema.json and static/schemas/source/media-buy/get-products-request.json are under the hard approval gate and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • Breaking-class protocol change: optional→required idempotency_key flip on published get_products request schema; get_products reclassified state-mutating. Never auto-approved unless review_decision is APPROVED — human ratification required.

Why human review

  • Gated paths not satisfied: static/schemas/source/manifest.schema.json and static/schemas/source/media-buy/get-products-request.json are under the hard approval gate and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • Breaking-class protocol change: optional→required idempotency_key flip on the published get_products request schema, reclassifying get_products as state-mutating. Ratification is a human act — never auto-approved unless review_decision is APPROVED.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/manifest.schema.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/get-products-request.json (modified) matches static/schemas/source/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

@bokelley bokelley added the needs-wg-review Blocked on a working-group decision — surface in WG meeting agendas label Aug 10, 2026
@bokelley bokelley added this to the 4.0 milestone Aug 10, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Escalate to human review

Escalate — hard approval gate not satisfied + Breaking-class protocol change requires human ratification.

Escalation reasons

  • Gated paths not satisfied: static/schemas/source/manifest.schema.json and static/schemas/source/media-buy/get-products-request.json are under the repo's hard, non-overridable approval gate and review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS review is required before merge (decision-table row 2).
  • Breaking-class protocol change: an optional→required idempotency_key flip on the published get_products request schema, reclassifying get_products as state-mutating (x-mutates-state: true). Per repo governance, Breaking-class changes are never auto-approved even with a correct major changeset — ratification is a human act. Escalate unless review_decision is APPROVED.

Findings
No critical/high/medium code findings surfaced this run. The reviewer verified schema↔docs coherence (schema requires the key + x-mutates-state:true; get_products.mdx marks Required:Yes), a correct major changeset, no oneOf/additionalProperties regressions, and confirmed the session-mutex, hash-aware fenced idempotency claim/save/release, and durable-state flush paths are safe.

This is the same posture as the prior escalate decision: the diff is clean, but the gated path + Breaking-class governance gate hold until a human/CODEOWNERS approval is recorded. Once review_decision becomes APPROVED, this PR falls through to a normal approve.

Why human review

  • Gated paths not satisfied: static/schemas/source/manifest.schema.json and static/schemas/source/media-buy/get-products-request.json are under the hard approval gate and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • Breaking-class protocol change: optional→required idempotency_key flip on the published get_products request schema, reclassifying get_products as state-mutating (x-mutates-state:true). Ratification is a human act — never auto-approved unless review_decision is APPROVED, even with a correct major changeset.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/manifest.schema.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/get-products-request.json (modified) matches static/schemas/source/**) and the current GitHub review decision is 'REVIEW_REQUIRED', not APPROVED. This is a hard gate enforced in code — Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

@bokelley bokelley modified the milestones: 4.0, 3.2.0 Aug 10, 2026
@bokelley bokelley changed the title fix(security): make get_products retry-safe feat(media-buy): split product discovery tools for AdCP 3.2 Aug 10, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Escalate to human review

Escalate — gated paths under a hard approval gate with review_decision: REVIEW_REQUIRED, plus a breaking-class protocol change.

Escalation reasons

  • Gated paths not satisfied: this PR modifies files under static/schemas/source/** (e.g. static/schemas/source/manifest.schema.json, static/schemas/source/media-buy/get-products-request.json, static/schemas/source/index.json, plus 20+ new media-buy schema files) which are under the hard, non-overridable approval gate, and review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS approval is required before merge (decision-table row 2).
  • Breaking-class protocol change: the get_products request schema flips idempotency_key optional→required, reclassifying get_products as state-mutating (x-mutates-state:true). Per the constitution's Breaking-class hard gate, ratification is a human act and is never auto-approved unless review_decision is APPROVED — even with a correctly scoped major changeset.

Findings
No critical/high/medium findings. Clean subsequent pass: schema↔docs coherence verified, changesets minor and correctly scoped, product-refinement.json oneOf is discriminated, no dist/<semver> mutations, keyless read path and webhook fail-close logic sound. The absence of blocking findings does not lift the gate — the gate lifts only on a recorded human/CODEOWNERS approval.

Prior decision was also escalate for the same gate reasons; the gate condition still holds (review_decision unchanged), so escalation persists.

Why human review

  • Gated paths not satisfied: static/schemas/source/manifest.schema.json, static/schemas/source/media-buy/get-products-request.json, static/schemas/source/index.json and 20+ other files under static/schemas/source/** are under the hard approval gate and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • Breaking-class protocol change: get-products-request.json flips idempotency_key optional→required and reclassifies get_products as state-mutating (x-mutates-state:true). Ratification is a human act and is never auto-approved unless review_decision is APPROVED, even with a correct major changeset.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/account-authorization.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/index.json (modified) matches static/schemas/source/**; static/schemas/source/manifest.schema.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-input-required.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-submitted.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-working.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-request.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-response.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/get-products-request.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/list-products-request.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/list-products-response.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/product-fields.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/product-refinement.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/recommend-products-async-response-input-required.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/recommend-products-async-response-submitted.json

…(truncated)…

Comment thread server/tests/integration/training-agent-tool-catalog-drift.test.ts Outdated

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ladon verdict: Escalate to human review

Escalate — gated-paths hard gate unsatisfied + breaking-class protocol change.

This PR touches 25 files under static/schemas/source/**, which sit under a hard, non-overridable approval gate (gated_paths: true). The current GitHub review_decision is REVIEW_REQUIRED, not APPROVED, so decision-table row 2 fires: Ladon cannot auto-approve regardless of how clean the diff is until a human/CODEOWNERS approval is recorded.

Additionally, this is a Breaking-class protocol change per the constitution's hard gate: get-products-request.json flips idempotency_key optional→required and reclassifies get_products as state-mutating (x-mutates-state: true). Ratification of a breaking change is a human act and is never auto-approved absent review_decision: APPROVED — even with a correct major changeset (.changeset/secure-get-products-idempotency.md).

Prior decision was escalate; the gated-paths and breaking-class conditions still hold on this head, so the escalation persists.

Medium findings

  • server/tests/integration/training-agent-tool-catalog-drift.test.ts:148 — 3.0-compat guard test only fails when all four tools leak, not on a per-tool regression. Non-blocking; worth tightening but does not change the outcome.

Escalation reasons

  • Gated files under static/schemas/source/** (e.g. get-products-request.json, manifest.schema.json, index.json + 22 others) require human/CODEOWNERS approval; review_decision is REVIEW_REQUIRED.
  • Breaking-class change: get_products idempotency_key optional→required and x-mutates-state:true — human ratification required.

Medium findings

  • server/tests/integration/training-agent-tool-catalog-drift.test.ts:148 — 3.0-compat guard test only fails when all four tools leak, not on a per-tool regression

Why human review

  • Gated paths not satisfied: 25 files under static/schemas/source/** (get-products-request.json, manifest.schema.json, index.json, and 22 others) are under the hard approval gate and review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS review required.
  • Breaking-class protocol change: get-products-request.json flips idempotency_key optional→required and reclassifies get_products as state-mutating (x-mutates-state:true). Ratification is a human act and is never auto-approved unless review_decision is APPROVED, even with a correct major changeset.
  • This PR touches a path under a hard, non-overridable approval gate (static/schemas/source/core/account-authorization.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/index.json (modified) matches static/schemas/source/**; static/schemas/source/manifest.schema.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-input-required.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-submitted.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-working.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-request.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-response.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/get-products-request.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/list-products-request.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/list-products-response.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/product-fields.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/product-refinement.json (added) matches static/schemas/source/**; static/schemas/source/med

…(truncated)…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ladon/needs-human-review Ladon has escalated this PR for human review. needs-wg-review Blocked on a working-group decision — surface in WG meeting agendas

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant