feat(media-buy): split product discovery tools for AdCP 3.2 - #6115
feat(media-buy): split product discovery tools for AdCP 3.2#6115bokelley wants to merge 18 commits into
Conversation
f460c71 to
60068ad
Compare
There was a problem hiding this comment.
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.jsonandstatic/schemas/source/media-buy/get-products-request.jsonare under the hard approval gate (gated_paths: true), andreview_decisionisREVIEW_REQUIRED(notAPPROVED). 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_decisionisAPPROVED. 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) matchesstatic/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.
There was a problem hiding this comment.
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.jsonandstatic/schemas/source/media-buy/get-products-request.jsonunder the hard, non-overridable approval gate;review_decisionisREVIEW_REQUIRED, notAPPROVED. Human/CODEOWNERS approval is required before merge. - Breaking-class protocol change (optional→required
idempotency_keyon the publishedget_productsrequest schema;get_productsreclassified as state-mutating) is never auto-approved — ratification is a human act unlessreview_decisionisAPPROVED.
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) matchesstatic/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.
983fced to
d5b0a59
Compare
There was a problem hiding this comment.
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.jsonandstatic/schemas/source/media-buy/get-products-request.jsonare under the hard approval gate andreview_decisionisREVIEW_REQUIRED, notAPPROVED— human/CODEOWNERS review required. - Breaking-class protocol change: optional→required
idempotency_keyflip on published get_products request schema; get_products reclassified state-mutating. Never auto-approved unlessreview_decisionisAPPROVED— 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) matchesstatic/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.
There was a problem hiding this comment.
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.jsonandstatic/schemas/source/media-buy/get-products-request.jsonare under the repo's hard, non-overridable approval gate andreview_decisionisREVIEW_REQUIRED, notAPPROVED. Human/CODEOWNERS review is required before merge (decision-table row 2). - Breaking-class protocol change: an optional→required
idempotency_keyflip on the publishedget_productsrequest schema, reclassifyingget_productsas state-mutating (x-mutates-state: true). Per repo governance, Breaking-class changes are never auto-approved even with a correctmajorchangeset — ratification is a human act. Escalate unlessreview_decisionisAPPROVED.
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) matchesstatic/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.
There was a problem hiding this comment.
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, andreview_decisionisREVIEW_REQUIRED, notAPPROVED. Human/CODEOWNERS approval is required before merge (decision-table row 2). - Breaking-class protocol change: the get_products request schema flips
idempotency_keyoptional→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 unlessreview_decisionis 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) matchesstatic/schemas/source/**; static/schemas/source/index.json (modified) matchesstatic/schemas/source/**; static/schemas/source/manifest.schema.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-input-required.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-submitted.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-working.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/get-products-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/list-products-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/list-products-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/product-fields.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/product-refinement.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/recommend-products-async-response-input-required.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/recommend-products-async-response-submitted.json
…(truncated)…
There was a problem hiding this comment.
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) matchesstatic/schemas/source/**; static/schemas/source/index.json (modified) matchesstatic/schemas/source/**; static/schemas/source/manifest.schema.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-input-required.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-submitted.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-async-response-working.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/finalize-proposals-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/get-products-request.json (modified) matchesstatic/schemas/source/**; static/schemas/source/media-buy/list-products-request.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/list-products-response.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/product-fields.json (added) matchesstatic/schemas/source/**; static/schemas/source/media-buy/product-refinement.json (added) matchesstatic/schemas/source/**; static/schemas/source/med
…(truncated)…
What changed
list_products,recommend_products,refine_proposal, andfinalize_proposalstools with narrow request/response schemas and task documentationget_productsas a key-optional compatibility facade throughout AdCP 3.x; new integrations should use the split toolslist_productsremains a synchronous key-optional readget_productsoperation family before authorization, idempotency, task persistence, and webhook deliverytools/listprojectionCompatibility and idempotency
This is an additive AdCP 3.2 minor change, not the AdCP 4.0 tightening.
Existing 3.x
get_productspayloads 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, andfinalize_proposalsrequire idempotency keys because they can create tasks or mutate proposal state.list_productsaccepts an optional key and accepts the universal callback wrapper field for portability, but remains synchronous and emits no callbacks. Stateful aliases emit the canonicalget_productstask/webhook identity exactly once.The stricter removal/tightening of the polymorphic legacy facade remains a separate AdCP 4.0 concern.
Validation
adcontextprotocolas a minor releasegit diff --check