Skip to content

feat(media-buy): add compact AdCP 3.2 lifecycle - #6115

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

feat(media-buy): add compact AdCP 3.2 lifecycle#6115
bokelley wants to merge 49 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 compact AdCP 3.2 lifecycle: list_products, request_proposals, refine_proposals, decline_proposals, buy_products, accept_proposal, and control_media_buy
  • make accept_proposal the single commitment boundary for creating, amending, or canceling a MediaBuy from a committed proposal
  • support direct product purchase through buy_products and operational controls through control_media_buy
  • route commercial changes through refine_proposals(accepted_proposal_id) followed by accept_proposal; accepted snapshots remain immutable
  • keep creatives on the separate sync_creatives path: the new purchase/control inputs do not accept inline creatives, legacy creative shapes, BrandRef payloads, or transitive provenance graphs
  • keep get_products, create_media_buy, and update_media_buy as 3.x compatibility facades, with machine-readable fallback metadata for SDK adapters
  • use canonical compact account, brand, product, proposal, targeting, optimization, measurement, and reporting views
  • add durable wholesale product-feed webhook/mirror semantics; list_products is the synchronous bootstrap and repair read
  • incorporate the reusable opportunity lifecycle from feat(media-buy): add opportunity and proposal decline flow #5523 and keep targeting-aware product-filter work composable with feat(media-buy): add targeting-aware product discovery #6203

Lifecycle boundaries

  • list_products reads offers only and never creates proposals.
  • request_proposals produces immutable committed snapshots; refine_proposals forks new snapshots.
  • buy_products commits listed products without a proposal workflow.
  • accept_proposal creates a buy or applies an accepted amendment/cancellation proposal.
  • control_media_buy handles operational actions within the accepted envelope. Changes to product, flight, budget, targeting, billing/measurement, performance, or reporting terms require proposal refinement and acceptance.
  • sync_creatives remains the only creative-management surface for this lifecycle.
  • get_media_buys returns the accepted proposal and canonical routed actions so clients can recover the correct next step after restart.

Versioning and compatibility

This is additive in AdCP 3.2. Legacy tools remain supported throughout 3.x and map to the canonical lifecycle through declared fallback grades and operation families. Stateful calls replay only under their own canonical tool identity; exact retries converge, while reuse with different logical input conflicts.

The stricter removal of compatibility surfaces, inline creatives, and transitively inlined creative/provenance data remains 4.0 work.

Surface-area result

  • full MCP projection: 71 tools, 142 schemas, 12.32 MiB
  • production 3.2 profile: 66 tools, 132 schemas, 3.23 MiB structural projection
  • the production profile excludes compliance-only and deprecated compatibility tools and strips presentation annotations without changing validation semantics
  • hosts still select only their implemented protocol/capability subset; the profile is a clean catalog and validation source, not a recommendation to load all tools into one model context

Additional review fixes

  • bind accepted proposal MediaBuy IDs and canonical available actions in commitment/recovery responses
  • require the appropriate wholesale product payload view and prohibit it for non-product webhook events
  • document PROPOSAL_UNAVAILABLE for committed offers without reserved inventory
  • add countries and property_list offer attributes without turning discovery into delivery targeting
  • publish compact task-specific async envelopes for proposal planning and re-underwriting
  • support account IDs and natural-key accounts consistently in schemas and the training runtime

Validation

  • independent code and protocol expert reviews: no merge blockers
  • build and TypeScript no-emit: clean
  • focused lifecycle/idempotency/runtime suites: 709/709
  • composed schema validation: 344/344
  • MCP schema projection/profile validation: 14/14
  • schema registry, docs navigation, schema links/UTF-8, compliance generation, error-code drift, current storyboard matrix, and released 3.0 compatibility storyboard matrix: clean
  • merged with current main and the latest remote PR branch; conflicts resolved

Human/CODEOWNERS approval is still required because this PR changes gated protocol schema paths.

@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-path hard gate unsatisfied + breaking-class protocol change requires human ratification.

Why not approve: This PR touches 30+ files under static/schemas/source/**, which is a hard, deterministic approval gate (gated_paths: true). The current GitHub review_decision is REVIEW_REQUIRED, not APPROVED. Per decision-table row 2, the gate is NOT satisfied and the outcome must be escalate — regardless of how clean the diff is.

Reviewer findings: None — no critical/high/medium findings. Relationship validation (legacy_fallback/superseded_by with cycle detection), the enum-completeness ratchet, schema-path/identity checks, and the documented, changeset-backed non-breaking result loosening all pass cleanly.

Escalation reasons:

  • Gated-path hard gate (row 2): 30+ files under static/schemas/source/** modified/added while review_decision is REVIEW_REQUIRED, not APPROVED. Human/CODEOWNERS approval required before merge.
  • Breaking-class protocol surface change (new proposal-workflow tools/schemas: decline-proposals, refine-proposals, request-proposals; new core schemas brand-key, catalog-selection, opportunity-context, product-offer-filters; new enum values in proposal-status, proposal-decline-reason, task-type, error-code). Per the constitution's hard gate, breaking-class ratification is a human act — never auto-approved absent review_decision: APPROVED.

The prior decision was also escalate; the gate condition still holds this run, so escalation stands. No blocking findings exist, so this is not request-changes — a human/CODEOWNERS approval simply needs to be recorded.

Why human review

  • Gated-path hard gate (row 2): 30+ files under static/schemas/source/** modified/added while review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS approval required before merge.
  • Breaking-class protocol change (new proposal-workflow tools/schemas, new core schemas, new enum values in proposal-status/proposal-decline-reason/task-type/error-code) — ratification is a human act and is never auto-approved absent review_decision=APPROVED.
  • 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/async-response-data.json (modified) matches static/schemas/source/**; static/schemas/source/core/brand-key.json (added) matches static/schemas/source/**; static/schemas/source/core/catalog-selection.json (added) matches static/schemas/source/**; static/schemas/source/core/opportunity-context.json (added) matches static/schemas/source/**; static/schemas/source/core/product-offer-filters.json (added) matches static/schemas/source/**; static/schemas/source/core/proposal.json (modified) matches static/schemas/source/**; static/schemas/source/core/tasks-get-response.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/creative/list-creative-formats-request.json (modified) matches static/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matches static/schemas/source/**; static/schemas/source/enums/proposal-decline-reason.json (added) matches static/schemas/source/**; static/schemas/source/enums/proposal-status.json (modified) matches static/schemas/source/**; static/schemas/source/enums/task-type.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/create-media-buy-request.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/create-media-buy-response.json (modified) matches static/schemas/source/**; static/schemas/

…(truncated)…

@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-path hard gate not satisfied.

This PR modifies/adds 30+ files under static/schemas/source/**, which are 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: outcome must be escalate until a human/CODEOWNERS approval is recorded.

Beyond the deterministic path gate, this is a breaking-class protocol change (new proposal-workflow tools/schemas, new core schemas, new enum values in proposal-status / proposal-decline-reason / task-type / error-code). Per the repo's hard gate, breaking-class changes are never auto-approved — ratification is a human act — even with a correct major changeset.

The reviewer surfaced no critical/high/medium findings (delta since the prior run is a single editorial link-to-plain-text change in docs/building/by-layer/L3/task-lifecycle.mdx; substance unchanged). This is a clean pass on the diff itself, but the gate stands independent of diff quality.

Escalation reasons

  • Gated-path hard gate (row 2): 30+ files under static/schemas/source/** modified/added while review_decision is REVIEW_REQUIRED — human/CODEOWNERS approval required before merge.
  • Breaking-class protocol change (new proposal-workflow tools/schemas, new core schemas, new enum values) — ratification is a human act, never auto-approved absent review_decision=APPROVED.

No blocking correctness findings; escalation is purely to require the human approval the gate mandates.

Why human review

  • Gated-path hard gate (row 2): 30+ files under static/schemas/source/** modified/added while review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS approval required before merge.
  • Breaking-class protocol change (new proposal-workflow tools/schemas, new core schemas, new enum values in proposal-status/proposal-decline-reason/task-type/error-code) — ratification is a human act and is never auto-approved absent review_decision=APPROVED.
  • 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/async-response-data.json (modified) matches static/schemas/source/**; static/schemas/source/core/brand-key.json (added) matches static/schemas/source/**; static/schemas/source/core/catalog-selection.json (added) matches static/schemas/source/**; static/schemas/source/core/opportunity-context.json (added) matches static/schemas/source/**; static/schemas/source/core/product-offer-filters.json (added) matches static/schemas/source/**; static/schemas/source/core/proposal.json (modified) matches static/schemas/source/**; static/schemas/source/core/tasks-get-response.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/creative/list-creative-formats-request.json (modified) matches static/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matches static/schemas/source/**; static/schemas/source/enums/proposal-decline-reason.json (added) matches static/schemas/source/**; static/schemas/source/enums/proposal-status.json (modified) matches static/schemas/source/**; static/schemas/source/enums/task-type.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/create-media-buy-request.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/create-media-buy-response.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/decline-proposals-request.json (added) matches static/schemas/source/**; static/schema

…(truncated)…

@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-path hard gate unsatisfied plus breaking-class protocol change awaiting human ratification.

This PR touches 34 files under static/schemas/source/**, which are under a hard, non-overridable approval gate (gated_paths: true), and the current GitHub review_decision is REVIEW_REQUIRED — not APPROVED. Under decision-table row 2, this alone forces escalation: Ladon cannot auto-approve until a human/CODEOWNERS approval is recorded, regardless of how clean the diff is.

Independently, this is a Breaking-class protocol change: new proposal-workflow tools/schemas (decline-proposals, refine-proposals, request-proposals, product-discovery-criteria, catalog-selection, etc.), new enum values (proposal-status, proposal-decline-reason, task-type, error-code), and new core schemas. Per the repo's "Breaking-class changes are never auto-approved" hard gate, ratification is a human act and must not be auto-approved absent review_decision: APPROVED.

The reviewer surfaced no critical/high/medium findings in this delta — the schema↔docs coherence, changeset scope/type, oneOf-discriminator, and released-artifact-immutability checks all passed, and the delta's fallback-grade/manifest changes are fail-closed and unit-tested. This is a net-clean diff. But both hard gates stand until a human/CODEOWNERS approval lands.

Blocking findings: none.

Escalation reasons:

  • Gated path hard gate (row 2): 34 files under static/schemas/source/** modified/added while review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS approval required before merge.
  • Breaking-class protocol change (new proposal-workflow tools/schemas, new enum values across proposal-status/proposal-decline-reason/task-type/error-code, new core schemas) — ratification is a human act; never auto-approved absent review_decision: APPROVED.

Why human review

  • Gated path hard gate (row 2): 34 files under static/schemas/source/** modified/added while review_decision is REVIEW_REQUIRED, not APPROVED — human/CODEOWNERS approval required before merge.
  • Breaking-class protocol change (new proposal-workflow tools/schemas, new enum values in proposal-status/proposal-decline-reason/task-type/error-code, new core schemas) — ratification is a human act; never auto-approved absent review_decision=APPROVED.
  • 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/async-response-data.json (modified) matches static/schemas/source/**; static/schemas/source/core/brand-key.json (added) matches static/schemas/source/**; static/schemas/source/core/catalog-selection.json (added) matches static/schemas/source/**; static/schemas/source/core/opportunity-context.json (added) matches static/schemas/source/**; static/schemas/source/core/product-offer-filters.json (added) matches static/schemas/source/**; static/schemas/source/core/proposal.json (modified) matches static/schemas/source/**; static/schemas/source/core/tasks-get-response.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/creative/list-creative-formats-request.json (modified) matches static/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matches static/schemas/source/**; static/schemas/source/enums/proposal-decline-reason.json (added) matches static/schemas/source/**; static/schemas/source/enums/proposal-status.json (modified) matches static/schemas/source/**; static/schemas/source/enums/task-type.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/

…(truncated)…

Comment thread scripts/build-schemas.cjs Fixed

@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-path hard gate unsatisfied (breaking-class protocol change awaiting human/CODEOWNERS approval).

Escalation reasons

  • 34 files under static/schemas/source/** are added/modified (new proposal-workflow tools & schemas: request/refine/decline-proposals, product-discovery-criteria, product-fields, brand-key, catalog-selection, opportunity-context, product-offer-filters; new enum values in proposal-status, proposal-decline-reason, task-type, error-code). gated_paths: true and review_decision is not APPROVED (unknown/none) → decision-table row 2 forces escalate. Human/CODEOWNERS approval is required before merge.
  • Breaking-class protocol change per repo hard gate: ratification is a human act and is never auto-approved absent review_decision=APPROVED, regardless of how clean the diff is.

Findings
The reviewer delta since the prior escalate is two files with no actionable findings: scripts/build-schemas.cjs (L1078-1084) replaces a TOCTOU existsSync+statSync/readFileSync with a single try/readFileSync/catch that still handles ENOENT/EISDIR — no behavior regression; and docs/building/by-layer/L3/task-lifecycle.mdx (L197) is an editorial hyperlink→plain-text swap on a non-normative page. No new critical/high/medium findings. The gated wire-schema changes belong to prior commits already under this same escalation.

This remains an escalate on the gated-path gate, not on any code defect. The gate lifts the moment a human/CODEOWNERS records review_decision=APPROVED.

Why human review

  • Gated path hard gate (row 2): 34 files under static/schemas/source/** modified/added while review_decision is not APPROVED (unknown/none) — human/CODEOWNERS approval required before merge.
  • Breaking-class protocol change (new proposal-workflow tools/schemas, new enum values in proposal-status/proposal-decline-reason/task-type/error-code, new core schemas) — ratification is a human act; never auto-approved absent review_decision=APPROVED.
  • 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/async-response-data.json (modified) matches static/schemas/source/**; static/schemas/source/core/brand-key.json (added) matches static/schemas/source/**; static/schemas/source/core/catalog-selection.json (added) matches static/schemas/source/**; static/schemas/source/core/opportunity-context.json (added) matches static/schemas/source/**; static/schemas/source/core/product-offer-filters.json (added) matches static/schemas/source/**; static/schemas/source/core/proposal.json (modified) matches static/schemas/source/**; static/schemas/source/core/tasks-get-response.json (modified) matches static/schemas/source/**; static/schemas/source/core/x-entity-types.json (modified) matches static/schemas/source/**; static/schemas/source/creative/list-creative-formats-request.json (modified) matches static/schemas/source/**; static/schemas/source/enums/error-code.json (modified) matches static/schemas/source/**; static/schemas/source/enums/proposal-decline-reason.json (added) matches static/schemas/source/**; static/schemas/source/enums/proposal-status.json (modified) matches static/schemas/source/**; static/schemas/source/enums/task-type.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/create-media-buy-request.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/create-media-buy-response.json (modified) matches static/schemas/source/**; static/schemas/source/media-buy/decline-proposals-request.json (added) matches static/schemas/source/**; static/schemas/source/media-buy/decl

…(truncated)…

@lukasz-pubx

Copy link
Copy Markdown

Reviewed at 58bf79abf. Directionally I support this change — the progressive-disclosure split is a real improvement over the get_products mega-tool, and it reduces tool bloat exactly where it matters. Particular strengths worth keeping as-is: the conditional-get design on list_products (feed_version/pricing_version, cache_scope), immutable snapshots with new-ID-on-changed-terms (which cleanly kills proposal-version ambiguity), the declared deferral of targeting-aware discovery to #6203, and the idempotency hardening of the legacy scenarios. The findings below are asks against that otherwise sound design.


1. Blocker — committed without a hold surface leaves the spend path undefined; "lock the deal" needs a primitive.

The baseline enum ratified committed as "Firm pricing with inventory reserved … After expires_at, the hold lapses." This PR rewrites it to "does not by itself promise that inventory is reserved; a seller that provides a hold must state it in the proposal terms" — but no field exists to state a hold in; proposal.json only alludes to it ("when the proposal terms include a hold, it lasts no later than this timestamp"). Legacy finalize let a buyer request a hold; the split lifecycle removes that capability without replacement.

Concrete failure: a buyer sits on an unexpired committed proposal through two days of internal approval; inventory sells out; seller A rejects create_media_buy, seller B accepts and underdelivers — both conformant, divergent on the path that commits spend. Real-world deals have a "lock" step for a reason.

Ask, any of:

  • (a) add a machine-readable hold surface — e.g. optional hold: { held, held_until } on proposal, requestable on request_proposals/refine_proposals — restoring what legacy finalize provided without an extra round-trip;
  • (b) define the no-hold disposition normatively (seller MAY reject an unexpired committed proposal with a named error; availability risk pre-execution is carried by the buyer) in both the enum and the create_media_buy page;
  • (c) restore reservation as part of split-lifecycle committed.

As written, the redefinition of a shared ratified enum value also strains the "intentionally additive" claim and should be called out in the changeset regardless of which resolution lands.

2. Concern — list_products critically needs an async mode, or a scoping surface that makes sync viable; as specified it is impractical at multi-tenant scale.

list_products is specified as always-synchronous with a 100-item page cap, but for a multi-tenant sales agent fronting many publishers — catalogs in the tens of thousands of products — a full-feed read by default is not a viable primary discovery path. The compact request makes this worse in three compounding ways: (1) the filter surface lost the narrowing tools legacy wholesale had — countries, regions, metros, keywords exist in product-filters.json but not in product-offer-filters.json, and top-level property_list and time_budget didn't carry over — some of that is the declared #6203 targeting deferral, but keyword/country/property narrowing is discovery scoping, not delivery targeting; (2) the request schema has no required fields, so an empty {} is a conformant request for the entire cross-publisher feed; (3) incomplete is carried over by $ref but without time_budget its trigger semantics are unspecified for this task.

Concrete failure: a buyer agent scoping "US CTV" against an aggregator must page the whole catalog — hundreds of sequential synchronous calls — or the seller must block long enough to materialize filtered pages across upstream publisher systems with no task handoff available. We consider async support here critical for aggregator-scale sellers.

Ask, in preference order:

  • (a) give list_products an async arm (submitted + webhook/poll), explicitly carving it out of the legacy wholesale MUST-NOT-async rule it otherwise inherits; and/or
  • (b) restore a scoping surface adequate to make sync pages cheap — countries, keywords, and a property_list equivalent in ProductOfferFilters — plus time_budget with defined incomplete semantics; or
  • (c) state normatively that sellers MUST serve list_products from a pre-materialized indexed feed, making the sync guarantee an obligation on the seller rather than an assumption.

Silent inheritance of "synchronous, unscoped, 100 per page" is the worst of the three.

3. Warning — async support at request_proposals/refine_proposals is critically required and currently unstated.

These are the consultative arms — the successors of exactly the legacy modes (brief, refine) that MAY return a Submitted envelope, and the task index itself estimates ~60s. Seller-authored planning routinely involves upstream systems or a human sales desk; without async these tasks cap out at transport timeouts and exclude proposal-mode sellers. The wiring half-exists: async-response-data.json adds their terminal responses to the webhook union, task-type.json makes them pollable, and the requests accept push_notification_config — but no submitted-ack schemas exist (the get-products-async-response-submitted.json convention), the response outcome enums have no submitted arm, and the doc pages are silent. Two conformant sellers diverge today: one blocks, one improvises a submitted envelope that validates against nothing.

Ask: state the disposition on each task page ("MAY return submitted when planning requires upstream queries or human review", mirroring the legacy brief-mode rule) and add the per-task submitted-ack schemas or explicitly designate a generic ack envelope.

4. Warning — four new normative task surfaces, zero behavioral compliance coverage.

All four doc pages carry testable: true, and the tasks define MUST-grade behavior (decline terminality, INVALID_STATE on declined/executed snapshots, new-ID-on-changed-terms, expiry enforcement, the all-or-nothing opportunity update on decline_proposals) — but no scenario under static/compliance/source/ exercises any of them; the compliance edits in this PR are idempotency-key plumbing for legacy get_products scenarios. "The spec permits it but nothing tests it" is exactly how the conformant-divergence cases above ship.

Ask: add at least a split-lifecycle happy-path storyboard (list → request → refine → buy), a decline-terminality probe (decline, then refine_proposals/create_media_buyINVALID_STATE), and an expiry probe — or mark the pages non-testable and declare scenario work as an explicit follow-up.

5. Notes.

  • push_notification_config on list_products is dead surface while the task is specified as always-synchronous (moot if finding 2's async arm lands — then it becomes load-bearing and should stay).
  • proposal-refinement.json's instructions description says "changes that are not represented by structured criteria," but the object offers no structured criteria — presumably reserved for feat(media-buy): add targeting-aware product discovery #6203 composition; worth saying so.
  • declines caps at maxItems: 25 while refinements is unbounded — asymmetric batch limits worth a rationale.
  • The changeset should acknowledge the committed enum-semantics revision and the supports_proposals description rewrite, not only the new tasks, even under a minor bump.

@bokelley

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review @lukasz-pubx. I ran protocol and product analysis on these; here is the consolidated synthesis.


Finding 1 — committed semantic revision: blocker confirmed

The "Firm pricing with inventory reserved … After expires_at, the hold lapses" wording is verbatim in dist/schemas/3.0.0 through 3.1.13 — it is a GA normative guarantee, not a draft artifact. Under AdCP's own versioning rules, weakening a published normative guarantee on a shared enum value is a semantic breaking change regardless of beta status on main, because the contract is inherited by any 3.x implementation. The changeset should call this out explicitly; leaving it implicit strains the "intentionally additive" claim.

Minimum viable resolution in this PR is option (b): a normative rejection path in the committed enumDescription — a seller without a hold MAY reject an unexpired committed proposal via create_media_buy with a named error (e.g., PROPOSAL_UNAVAILABLE) — plus a matching note on the create_media_buy page stating that availability risk pre-execution is on the buyer in the no-hold case. This requires no new schema fields and no new round-trip. Option (a) (hold: { held, held_until }) is the richer long-term shape and a natural follow-up. Option (c) contradicts the PR's design intent.


Finding 3 — async at request_proposals/refine_proposals: well-founded, shares the blocker framing

The "wiring half-exists" observation is verifiable: these tasks appear in task-type.json as pollable and in async-response-data.json terminal responses, and requests accept push_notification_config — but no request-proposals-async-response-submitted.json or refine-proposals-async-response-submitted.json exists in static/schemas/source/, and neither has an anyOf slot in the webhook union for a submitted envelope. A seller emitting one today either validates against nothing or silently structurally matches get-products-async-response-submitted.json — both outcomes are worse than an explicit schema.

The precedent is unambiguous: every async-capable task in the repo has a per-task submitted schema (7 tasks: get_products, create_media_buy, update_media_buy, sync_catalogs, build_creative, get_signals, sync_creatives). No generic ack envelope exists. The MVR here is mechanical — two schemas following the established template, registered in the union, plus a one-line MAY-async statement on each doc page. If the author wants to treat this as a follow-up, the minimum gate is a normative placeholder ("submitted envelope schema forthcoming; sellers MUST NOT block beyond transport timeout") and a linked tracking issue before merge.


Finding 2 — list_products scoping: well-founded

The keyword/country/property distinction from delivery targeting is correct. In the existing product-filters.json, countries describes geographic coverage of the product, keywords describes keyword targeting the product supports, and property_list narrows which publisher inventory is returned — all product attributes, not delivery instructions. The #6203 deferral is for delivery targeting (audience segments, frequency overlays), not discovery scoping.

Option (a) — async arm — is the wrong direction. list_products is the synchronous structured catalog path; adding an async arm would duplicate what buying_mode: brief already covers and undermine the PR's design rationale. Option (b) is the right resolution and is in scope: add countries, channels, and a property_list equivalent to ProductOfferFilters, and define incomplete trigger semantics explicitly (fires on result-count truncation at the page cap, not time limit — time_budget has no meaning for a sync task). The empty-{} problem follows from the absent scoping fields. Option (c) (normative seller obligation for pre-materialized indexed feeds) is implementation guidance, not spec language, and would not belong here.


Finding 4 — compliance coverage: well-founded

testable: true validates schema conformance of code-block examples in CI; it does not probe MUST-grade behavioral requirements. Decline terminality, INVALID_STATE on declined/executed snapshots, new-ID-on-changed-terms, and expiry enforcement are state-machine behaviors that require storyboard scenarios in static/compliance/source/. The existing proposal_finalize.yaml and proposal_not_found_errors.yaml storyboards illustrate the expected pattern for the legacy lifecycle; neither covers any step in the new one.

Preferred resolution: add a happy-path storyboard covering list_products → request_proposals → create_media_buy before merge (low effort, sets the floor for conformance, keeps testable: true honest on those pages), and file explicit follow-up issues for the decline-terminality and expiry probes. If that is out of scope for this PR, flip all four pages to testable: false and file the issues before merge — the coverage gap is better surfaced explicitly than left silent behind a testable: true flag.


Finding 5 — notes

All four land correctly:

  • push_notification_config on always-sync list_products is dead surface in its current form; it becomes load-bearing if a future async arm lands (which finding 2 argues against), so either remove it or explicitly document it as reserved for a future amendment
  • The instructions description in proposal-refinement.json ("changes that are not represented by structured criteria") is correct but would benefit from a note that structured criteria are reserved for feat(media-buy): add targeting-aware product discovery #6203 composition
  • The declines maxItems:25 / unbounded refinements asymmetry warrants a rationale comment in the schema (e.g., "decline reasons are auditable terminal state; batch cap prevents unbounded audit records")
  • The changeset should acknowledge the committed enum-semantics revision and the supports_proposals description rewrite alongside the new tasks

@bokelley: findings 1, 3, and 4 have clear MVRs and are actionable before merge; finding 2's fix is bounded and in scope. Happy to dig further on any of them.


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Issue #6432 proposes behavioral compliance storyboards for list_products, request_proposals, refine_proposals, and decline_proposals — same surface as this PR. The four task pages were flipped to testable: false as an interim measure; folding the storyboards before merge would restore testable: true rather than shipping the gap into 3.2. If scope expansion would delay WG review, close the fold window here and the issue will resurface as a tracked follow-up on merge.


Generated by Claude Code

Finding 1 (blocker): Revise `committed` enumDescription to document the
normative rejection path — a seller that did not reserve inventory MAY
reject an unexpired committed proposal at create_media_buy via
PROPOSAL_UNAVAILABLE; availability risk pre-execution is on the buyer
absent a stated hold. Add PROPOSAL_UNAVAILABLE error code with correctable
recovery guidance and regenerate compliance-error-codes.mdx. Add matching
PROPOSAL_UNAVAILABLE note to the create_media_buy doc page.

Finding 2 (well-founded): Add `countries` and `property_list`
product-attribute filters to ProductOfferFilters (channels was already
present). Override `incomplete` description on list-products-response to
clarify it fires on result-count truncation at the page cap, not
time_budget (which has no meaning for a sync task). Add dead-surface note
to push_notification_config on list-products-request.

Finding 3 (blocker): Create per-task async submitted schemas for
request_proposals and refine_proposals, register both in the webhook union
anyOf. Add one-line MAY-async statement to each doc page.

Finding 4 (well-founded): Flip all four new task pages (list_products,
request_proposals, refine_proposals, decline_proposals) to testable: false
pending behavioral compliance storyboard work (tracked in #6432).
@bokelley

Copy link
Copy Markdown
Contributor Author

All four findings addressed in commit 6c033bab4. Summary:

Finding 1 (blocker) — committed semantics + PROPOSAL_UNAVAILABLE

  • proposal-status.json: Added normative sentence — a seller that did not reserve inventory MAY reject an unexpired committed proposal at create_media_buy via PROPOSAL_UNAVAILABLE; availability risk pre-execution is on the buyer absent a stated hold.
  • error-code.json: Added PROPOSAL_UNAVAILABLE with correctable recovery guidance (call request_proposals for a fresh executable proposal; consider requesting a hold in the brief if inventory reservation is required). Compliance snippet regenerated.
  • create_media_buy.mdx: Added paragraph explaining the no-hold rejection path and directing buyers to request a hold in the request_proposals brief.

Finding 2 (well-founded) — list_products scoping

  • product-offer-filters.json: Added countries (ISO 3166-1 alpha-2 array) and property_list ($ref to property-list-ref.json). channels was already present.
  • list-products-response.json: Overrode the $ref-inherited incomplete description — clarified it fires on result-count truncation at the page cap; time_budget has no meaning for a synchronous task and MUST NOT be used as a trigger condition.
  • list-products-request.json: Added description to push_notification_config marking it a reserved dead surface for this sync task.

Finding 3 (blocker) — async at request_proposals / refine_proposals

  • Created request-proposals-async-response-submitted.json and refine-proposals-async-response-submitted.json following the established per-task template (status: "submitted", task_id required, not constraint blocking result fields).
  • async-response-data.json: Both schemas registered in the webhook union anyOf.
  • request_proposals.mdx / refine_proposals.mdx: One-line MAY-async statement added to each.

Finding 4 (well-founded) — compliance coverage


Generated by Claude Code

…empotency' into clean-up-pr-6115

# Conflicts:
#	.changeset/secure-get-products-idempotency.md
#	docs/media-buy/task-reference/refine_proposals.mdx
#	static/schemas/source/enums/proposal-status.json
#	static/schemas/source/media-buy/list-products-request.json
#	static/schemas/source/media-buy/list-products-response.json
#	static/schemas/source/media-buy/refine-proposals-async-response-submitted.json
#	static/schemas/source/media-buy/request-proposals-async-response-submitted.json
@bokelley bokelley changed the title feat(media-buy): split product discovery tools for AdCP 3.2 feat(media-buy): add compact AdCP 3.2 lifecycle Aug 12, 2026
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.

4 participants