Skip to content

feat(media-buy): add opportunity and proposal decline flow#5523

Open
bokelley wants to merge 4 commits into
mainfrom
feature-feedback
Open

feat(media-buy): add opportunity and proposal decline flow#5523
bokelley wants to merge 4 commits into
mainfrom
feature-feedback

Conversation

@bokelley

@bokelley bokelley commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Adds optional opportunity context across get_products and create_media_buy so buyers can identify a planning cycle, carry phase/horizon/deadline context, and close the opportunity without conflating it with idempotency or proposal versioning. Adds proposal-scoped action: "decline" to get_products refinement with required proposal_version, reason feedback, idempotent acknowledgement semantics, and privacy guidance. Carries proposal versions into create_media_buy so sellers can verify the accepted offer version and reject omitted, stale, unresolved, or previously declined proposal versions with the documented errors. Updates docs, source schemas, error-code guidance, changeset metadata, and regression tests for the combined opportunity/proposal-resolution flow.

Validation: npm run build:schemas, npm run test:schemas, npm run test:examples, npm run test:json-schema, npm run test:oneof-discriminators, git diff --check, and precommit hook.

@mintlify

mintlify Bot commented Jun 14, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
adcp 🟢 Ready View Preview Jun 14, 2026, 2:27 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@bokelley bokelley marked this pull request as ready for review June 14, 2026 13:26
aao-release-bot[bot]
aao-release-bot Bot previously approved these changes Jun 14, 2026

@aao-release-bot aao-release-bot 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.

Clean additive change. New optional fields, one new enum value (decline), and conditional required that only fires on the new action — textbook minor, and every conformant 3.0 payload validates unchanged.

Things I checked

  • Conditional logic is sound. The three allOf if/then blocks in get-products-request.json:483-549 are correct. Block (b) is the load-bearing one: if: { not: { properties:{action:{const:"decline"}}, required:["action"] } } correctly fires when action is absent (defaults to include) as well as for omit/include/finalize, forbidding reason/detail there — and correctly does NOT fire for decline. The naive action:{not:{const:"decline"}} would have wrongly passed when action is omitted. This avoids that footgun.
  • oneOf-of-const reason follows the established AdCP idiom, not a divergence — same inline scalar pattern as core/start-timing.json:6-17, used across ~88 source files. The audit walker classifies it as scalar (scripts/audit-oneof.mjs:145), so it's excluded from the baseline and --check. No undiscriminated-oneOf regression. The parent refine-entry oneOf stays discriminated on scope.
  • enumMetadata parity for INVALID_STATE. Both halves updated — the description map (error-code.json:138) and the metadata block (error-code.json:341-344) — and recovery stays "correctable" on both sides, honoring the metadata invariant.
  • Test assertions match AJV v8 output. ['proposal_version'], ['reason'], ['must NOT be valid'] (the not keyword's exact message), ['detail'], and ['proposal_id'] (the new dependencies.proposal_version:["proposal_id"]) all map to the right failing keyword. The seven cases cover the right invariants.
  • additionalProperties:false interaction is safe. The new properties are declared in the variant's properties block, so false sees them; the allOf adds only required, never new properties. A decline payload matches only the proposal variant. No regression.
  • Changeset present (proposal-decline-refinement.md), minor, correct for additive optional fields. No generated files touched.

Follow-ups (non-blocking — file as issues)

  • Docs don't state the non-decline rejection rule. Both code-reviewer and docs-expert flagged it: schema block (b) hard-rejects reason/detail on any non-decline proposal action, and there's a test for it (tests/example-validation-simple.test.cjs:628), but no prose says so. An agent attaching reason to an include entry hits a schema rejection the docs never warned about. One line in specification.mdx or the get_products.mdx reason row closes it.
  • Runtime obligations the schema can't express. The reject-omitted-version→INVALID_REQUEST, stale→CONFLICT, declined→INVALID_STATE MUSTs in specification.mdx/create_media_buy.mdx are seller-side runtime checks (correctly — JSON Schema only enforces proposal_versionproposal_id co-presence). Worth confirming the conformance storyboards cover these rejections, since the schema gate can't.

Minor nits (non-blocking)

  1. proposal_version length bounds are consistent. core/proposal.json (minLength:1), create-media-buy-request.json (minLength:1,maxLength:255), and the request-side field all agree. Good.

ad-tech-protocol-expert: sound-with-caveats (caveats are docs/runtime division-of-labor, not schema defects). code-reviewer: clean. docs-expert: clean, one optional polish. Note main is in pre-mode (rc tag) so this ships as 3.1.0-rc.Nminor is still the right declaration.

LGTM. Follow-ups noted below.

@bokelley bokelley changed the title feat(media-buy): add proposal decline refinement feat(media-buy): add opportunity and proposal decline flow Jun 15, 2026
aao-release-bot[bot]
aao-release-bot Bot previously approved these changes Jun 15, 2026

@aao-release-bot aao-release-bot 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.

Additive across the board — new optional opportunity, proposal_version, and a fourth proposal action — so minor is the right changeset and nothing on the wire flips from valid to invalid. The decline/version lifecycle is the right shape: proposal_id + proposal_version identifies one offer state, opportunity_id spans the planning cycle, and the schema is explicit that neither is an idempotency key.

Things I checked

  • Conditional logic, three blocks in get-products-request.json proposal entry. action: "decline" requires proposal_version + reason; non-decline (including omitted action) forbids reason/detail; reason: "other" requires detail. All three compose correctly — a valid decline passes blocks 2 and 3 vacuously. Confirmed against code-reviewer's case-by-case ajv v8 trace.
  • opportunity-context.json close gate (L466-497). if not(status===closed present) then not(close_reason or close_detail) correctly forbids close fields on absent/open status and allows them only on closed. Test close_reason requires closed status exercises it.
  • additionalProperties: false is safe. proposal_version/reason/detail are declared in the variant's own properties; the new allOf if/then blocks add only required/not, no new property names — no additionalProperties strip footgun.
  • oneOf audit. The new reason field is oneOf of pure consts → classified scalar by audit-oneof.mjs:144 and filtered from the baseline at L307/L322. No discriminator regression. The proposal oneOf stays discriminated on scope.
  • Error codes all pre-exist. INVALID_STATE (broadened, both enumDescriptions and enumMetadata.suggestion updated in parallel — parity held), CONFLICT, PROPOSAL_NOT_FOUND, INVALID_REQUEST. No new code, no error-code lint surface.
  • Changeset present (.changeset/proposal-decline-refinement.md, minor), wire-touching PR, correct type. ad-tech-protocol-expert: sound-with-caveats; docs-expert: no enum/phase/close_reason value drift between schema and the five .mdx files.
  • Dependency proposal_version → proposal_id (one-directional) — correct; unversioned proposal_id still allowed.

Follow-ups (non-blocking — file as issues)

  • reason modeling breaks enumDescriptions parity. It's type: string + oneOf of {const, description}, which the build does not harvest into enumDescriptions/enumMetadata — so SDK and doc generators get nothing for the decline taxonomy. The wire shape is identical (same 10 strings), so this is internal polish, not a block. The convention is a shared enums/*-reason.json with enum + enumDescriptions (action-not-allowed-reason.json, makegood-remedy.json). Worth extracting enums/proposal-decline-reason.json and $refing it. Notable that close_reason/phase/status in this same PR use plain enumreason is the one outlier.
  • CONFLICT is recovery: transient (concurrent-write). The PR overloads it for a stale proposal_version (optimistic-version mismatch). Defensible — re-read-and-retry is the transient contract — but extend the CONFLICT enumDescriptions prose to name the stale-version case so SDKs don't treat it as a pure race.
  • opportunity-context.json not registered in static/schemas/source/index.json. Won't fail CI (registry check doesn't enforce completeness), but every sibling core schema is listed. Add the entry.

Minor nits (non-blocking)

  1. refinement.mdx proposal-action table omits a proposal_version column. get_products.mdx lists it as a first-class conditional field; refinement.mdx folds it into the decline row's free text. Not a contradiction, just less scannable.
  2. The central proposal_version MUST is runtime-only. specification.mdx and create_media_buy.mdx require sellers to reject a versioned-proposal execution that omits proposal_version (INVALID_REQUEST), but the request schema can't know whether the referenced proposal was versioned, so it stays optional. Docs frame this correctly as a seller-side rule — flagging so reviewers know it's not schema-caught.

LGTM. Follow-ups noted below.

aao-release-bot[bot]
aao-release-bot Bot previously approved these changes Jun 15, 2026

@aao-release-bot aao-release-bot 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.

Clean additive change. The opportunity/proposal-decline split is the right shape: opportunity_id spans the planning cycle, proposal_id + proposal_version pins one offer state, and neither is conflated with idempotency_key. The conditional logic is load-bearing and correct.

Things I checked

  • oneOf audit is clean. The new reason taxonomy in get-products-request.json is a oneOf of const branches — scripts/audit-oneof.mjs classifies all-const oneOf as scalar (variantInfo L111), so it's neither flagged nor baselined. No discriminator regression.
  • Decline gating fires correctly. additionalProperties: false on the proposal refine branch recognizes the new proposal_version/reason/detail because they're declared in that branch's own properties; the allOf only adds required/not(required). Traced all four branches: decline requires version+reason (cond 1), non-decline forbids reason/detail even when action is omitted (cond 2, not-wrapped — no leak), reason: other requires detail (cond 3). code-reviewer: no blockers, manual semantic trace passes on every branch.
  • Close-won asymmetry is deliberate. opportunity-context.json permits all 7 close_reason values when status: closed; create-media-buy-request.json narrows to accepted_with_seller only on create. Loss reasons belong on a get_products refine close. Consistent with docs.
  • error-code parity holds. INVALID_STATE updated in both enumDescriptions (L885) and enumMetadata suggestion (L894), recovery: correctable unchanged. Reusing INVALID_STATE for declined-then-executed is the right call — it already means "operation not permitted for terminal lifecycle state."
  • Changeset is right. minor, purely additive: decline is an added enum value, every new field is optional or conditionally-required on a brand-new branch. No required↔optional flip, no removal. ad-tech-protocol-expert: sound-with-caveats, nothing here is a breaking wire change.
  • No schema-vs-docs drift. reason (10 values) and close_reason (7) match their schema enums exactly across get_products.mdx, specification.mdx, refinement.mdx; the two taxonomies are intentionally distinct (proposal-version-level vs opportunity-level), not drift. Changeset present.
  • Compliance scenario. proposal_decline.yaml covers acknowledge → idempotent duplicate → reject-create-after-decline with INVALID_STATE (L685-688), wired into both sales-guaranteed and sales-proposal-mode. end_time: 2027-... dodges the concrete-date lint.
  • Tests. 13 new assertions in example-validation-simple.test.cjs (L1188-1400) exercise every positive and negative branch including proposal_version requires proposal_id and closed opportunity rejects non-accepted close_reason.

Follow-ups (non-blocking — file as issues)

  • create-media-buy-response.json doesn't echo proposal_version. The request-side contract is fully specified, but the response is silent on which offer version the seller executed. When versions race, the buyer can't reconcile what it bought from the response alone — it has to re-read via get_media_buys. Adding an optional echo stays minor. (ad-tech-protocol-expert.)
  • CONFLICT recovery class is transient but a stale proposal_version is correctable-by-re-read. This PR is the first to route a value-mismatch to CONFLICT. The docs route it correctly ("re-read or refine"), but an SDK trusting enumMetadata.CONFLICT.recovery = transient could auto-retry the stale version and loop. Worth a one-line spec note that this CONFLICT is correctable, not blindly retryable.
  • Response-side "MUST echo proposal_version on decline ack" is prose-only. The response schema can't see the request action, so validation alone won't catch a seller that drops the echo — only the compliance scenario does. Expected, just don't assume the schema guarantees it.

Minor nits (non-blocking)

  1. reason uses oneOf of const; the parallel close_reason uses a plain enum. Both validate identically. The per-const form buys per-value descriptions — fine — but the inconsistency between two sibling taxonomies in the same PR is gratuitous.

Approving on the strength of two clean expert verdicts plus a full trace of the conditional gates. Ship it once CI confirms test:examples / test:json-schema / test:oneof-discriminators green.

aao-release-bot[bot]
aao-release-bot Bot previously approved these changes Jun 17, 2026

@aao-release-bot aao-release-bot 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.

Clean, additive feature. The architecture is right: three distinct identifiers — idempotency_key (one mutating attempt), proposal_id + proposal_version (one offer state), opportunity_id (the planning cycle) — kept orthogonal instead of overloading one field to do three jobs.

ad-tech-protocol-expert: sound-with-caveats. code-reviewer: sound-with-caveats. No MUST-FIX in the diff.

Things I checked

  • Semver is minor, and that's correct. Every wire change is additive: open added to the buying_mode request enum, decline to refine[].action, open to the buying_modes items enum in get-adcp-capabilities-response.json:184, and the optional proposal_version / opportunity fields. Adding an enum member only breaks a consumer doing closed-world validation that rejects unknowns — a posture this protocol explicitly forbids. No major needed.
  • oneOf gate passes without a baseline change. get-products-request.json refine[].reason is 10 bare {const, description} branches. scripts/audit-oneof.mjs classifies const-only variants as scalar and excludes them from diff()/writeBaseline(), so this never lands in the baseline. scripts/oneof-discriminators.baseline.json correctly stays untouched — the standard enum-with-descriptions idiom, same as core/start-timing.json.
  • The decline conditional has no escape hatch. get-products-request.json per-entry allOf: the not-decline branch is if not({action: const decline}) then not(reason or detail). Draft-07 does not apply default: "include" before evaluating if, so an absent action is treated as not-decline and reason/detail are correctly forbidden. The decline branch requires proposal_version + reason; reason: "other" requires detail. Three blocks compose without conflict.
  • opportunity-context.json:965 close-reason guard is correct. if not(status == closed) then not(close_reason or close_detail) — closure metadata is forbidden unless the opportunity is actually closed. additionalProperties: false, required: [opportunity_id], and the ^[A-Za-z0-9_.:-]{1,255}$ pattern are consistent with the docs table.
  • create-media-buy-request.json close-won rule is enforced on the wire. The top-level allOf if opportunity.status == closed then close_reason ∈ [accepted_with_seller] uses an inline subschema in then, so it does not re-impose additionalProperties: false, and a closed opportunity with no close_reason stays valid. dependencies: { proposal_version: [proposal_id] } correctly forces proposal_id.
  • proposal_decline.yaml:844 closes the loop. The compliance scenario asserts INVALID_STATE on create_media_buy against a declined proposal_id + proposal_version, plus duplicate-decline idempotency and the refinement_applied echo — the cross-request state that JSON Schema can't model.
  • example-validation-simple.test.cjs:1676 and neighbors assert on the right substrings: expectInvalid matches against message + JSON.stringify(params), so ['accepted_with_seller'], ['proposal_version'], ['detail'] land on enum allowedValues / missingProperty. No false-green.
  • error-code parity holds. INVALID_STATE updated in both the enum description (error-code.json:138) and the enumMetadata.suggestion (error-code.json:341). No new error code — CONFLICT, PROPOSAL_NOT_FOUND, INVALID_REQUEST all pre-exist.
  • Schema-vs-docs: no drift. Action enums, the four buying modes, the opportunity field table, and the proposal_version "required when versioned" rule all match across get_products.mdx, create_media_buy.mdx, specification.mdx, refinement.mdx, and the source schemas.

Ship condition (blocking on the queue, not on the diff)

  • Rebase the branch before merge. CI shows TypeScript Build, Server integration tests, and No duplicate migration numbers red. This diff contains zero migration files, zero .ts, zero server/ changes — it cannot introduce a duplicate migration number. PR #5587 merged into main at 21:16 with all three green; this branch ran at 21:23 against a stale base. The failures are inherited, not yours. Merge is already BLOCKED until checks pass, so this can't land red — but rebase on current main to clear them rather than waiting.

Follow-ups (non-blocking — file as issues)

  • The response-side proposal_version echo on refinement_applied is a prose MUST that JSON Schema can't enforce (it conditions a response field on a separate request payload). Conformance rests entirely on proposal_decline.yaml — confirm that scenario keeps asserting the echo as the spec evolves.
  • The "reject omitted proposal_version when the proposal is versioned → INVALID_REQUEST" rule is likewise seller-behavior-only; the request alone can't reveal whether the referenced proposal carried a version. Consistent with how PROPOSAL_NOT_COMMITTED is already handled, but it's untestable at the schema layer.

Approving on the strength of two independent sound verdicts plus a clean schema/docs/compliance coherence pass. Ship it once the branch is rebased and CI is green.

aao-release-bot[bot]
aao-release-bot Bot previously approved these changes Jun 17, 2026

@aao-release-bot aao-release-bot 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.

Additive schema change, done right. New optional fields, two new enum values, one new optional core/ object — nothing flips required↔optional, nothing is removed or renamed, so minor is the correct changeset and the if/then blocks only constrain fields this PR introduces.

Things I checked

  • Decline conditional logic (get-products-request.json, refine proposal allOf). Three composed if/then members: action: "decline" ⇒ require proposal_version+reason; not(decline) ⇒ forbid reason/detail; reason: "other" ⇒ require detail. The not{properties+required} idiom correctly folds "action absent" and "action present non-decline" into the prohibited-reason case. Matches the prose in refinement.mdx and specification.mdx.
  • The reason oneOf does not trip the discriminator gate. Every branch is {const, description}scripts/audit-oneof.mjs classifies that as scalar (const !== undefined), and writeBaseline/diff skip scalar rows. get-products-request.json has zero baseline entries; the existing refine.items.oneOf already classifies as discriminated on scope. No baseline ratchet needed. This is the established enum-with-descriptions idiom, not a new undiscriminated oneOf.
  • create-media-buy-request.json: closed-opportunity ⇒ close_reason ∈ {accepted_with_seller} constrains the value only when present (doesn't force it to exist) — matches "the only valid close_reason is accepted_with_seller." dependencies.proposal_version: [proposal_id] enforces the version⇒id requirement.
  • opportunity-context.json:60-92: close_reason/close_detail forbidden unless status: "closed" via if:not{status==closed} then:not{anyOf required}. Correct.
  • Test honesty on the one suspicious assertion. 'open mode rejects wholesale conditional version' expects a 'wholesale' error string — that fires off the pre-existing if_wholesale_feed_versionbuying_mode: "wholesale" rule, where AJV emits {"allowedValue":"wholesale"}. The rule exists; the assertion is real, not a tautology.
  • INVALID_STATE reuse is sound. A declined proposal version is a terminal lifecycle state — parallel to "updating a canceled media buy." No new error code warranted. CONFLICT (stale version) and PROPOSAL_NOT_FOUND (unresolvable) reuses are consistent with the existing refinement error table.
  • Migration renames + CI gate. 517_*519/520_* resolves the pre-existing 517 collision on main; the new prefix_dupes() logic in check-migration-numbers.yml suppresses the main-only 517 dupe so the fixing PR can merge, while still catching a PR that adds a fresh collision via local_dupes. Right shape.
  • Changeset present (.changeset/proposal-decline-refinement.md, minor). Docs match schema across get_products.mdx, create_media_buy.mdx, refinement.mdx, specification.mdx. ad-tech-protocol-expert: sound-with-caveats. code-reviewer: sound-with-caveats.

Caveat on validation

Neither I nor the subagents could execute the suites — this sandbox gates git fetch/npm/node. The conclusions above are static traces of the schemas, tests, and audit classifier against the on-disk main tree, plus the PR's own claim that build:schemas, test:schemas, test:examples, test:json-schema, and test:oneof-discriminators were run. CI is the empirical gate. The newly schema-enforced brief-required / refine-forbidden / brief-forbidden rules mean any legacy get-products-request example that violated them only in prose would now fail test:examples — my scan of $schema-tagged doc blocks and the request-validating test files found none, but I did not run them. Watch test:json-schema and test:examples go green before merge.

Follow-ups (non-blocking — file as issues)

  • The privacy guidance on opportunity_id, close_detail, and decline detail ("MUST NOT include competitor identity / clearing prices / campaign IDs") is enforced only by maxLength — the substance is out-of-band. Consider a $comment noting enforcement is seller-side, so nobody mistakes the prose for a schema guarantee.
  • "MUST NOT decline an unversioned proposal" and "reject create after decline with INVALID_STATE" are stateful seller rules the schema can't express. Correctly pushed to proposal_decline.yaml compliance coverage — just noting the schema alone doesn't guard them.

Minor nits (non-blocking)

  1. Stale comment in the migration workflow. .github/workflows/check-migration-numbers.yml:47-50 still describes the old "union, hunt dupes in that union" approach; the new logic suppresses main-only dupes. Update the comment to match.
  2. Decline scenario read steps. proposal_decline.yaml omits idempotency_key on the two get_products steps — correct because the reads are non-mutating, but confirm the scenario runner doesn't require it for stateful: true read steps.

The migration-readiness docs had been pointing sellers at a browse buying mode that never shipped; this PR quietly retires it for the real brief/wholesale/refine/open set. Good cleanup to fold in.

Approving on the strength of the additive-only schema delta plus two independent sound-with-caveats verdicts. Safe to merge once CI is green.

@aao-release-bot aao-release-bot 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.

One unregistered x-entity value breaks npm test. Everything else is clean, additive, and correctly versioned — fix the registry and this ships.

MUST FIX (blocking)

x-entity: "proposal" is not registered — fails test:storyboard-context-entity. create-media-buy-response.json adds the proposal_id echo field with "x-entity": "proposal" (the success-result block, ~L29). But proposal is absent from the enum in static/schemas/source/core/x-entity-types.json — this PR registers only opportunity (L13/L52). lint() in scripts/lint-storyboard-context-entity.cjs:616 walks [...lintRegistry(registry)] over every source schema and emits unknown_entity for any x-entity not in the registry (L336). tests/lint-storyboard-context-entity.test.cjs:35 asserts deepEqual(violations, []), and that test is wired into npm test (test:storyboard-context-entity). So the suite goes red on merge.

What breaks for adopters: the PR cannot land green — CI fails on the context-entity gate.

Fix is two lines: add \"proposal\" to the enum array and a matching x-entity-definitions entry in x-entity-types.json. Worth a glance at the request side too — create-media-buy-request.json's proposal_id carries no x-entity while the response echo now does; register proposal and the asymmetry at least resolves to a registered type.

Your validation list (build:schemas, test:schemas, test:examples, test:json-schema, test:oneof-discriminators) didn't include test:storyboard-context-entity — that's the gap that let this through. Run the full npm test before re-pushing.

Things I checked

  • oneOf on refine[].reason — scalar const variants, classified kind: scalar (STATUS_RANK 0) by audit-oneof.mjs:111. Not baselined, not flagged, no --accept-new. The enum-with-descriptions idiom, not an undiscriminated-union regression.
  • open is additive[brief, wholesale, refine]+open in both get-products-request.json buying_mode and get-adcp-capabilities-response.json buying_modes. No enum values removed. action likewise +decline. No wire break.
  • proposal_version cross-schema paritystring / minLength 1 / maxLength 255 identical across proposal.json, get-products-request refine entries, get-products-response refinement_applied, create-media-buy req + resp. Optional at property level; conditionally required via if/then (decline) and dependencies (proposal_version → proposal_id).
  • Schema conditionalsget-products-request.json brief/refine if/then blocks partition the enum with no overlap, and buying_mode being in top-level required defuses the draft-07 vacuous-if trap. opportunity-context.json allOf[0] if: { not: { ... status const closed } } correctly reads as "status absent or ≠ closed." Both sound.
  • Changesetminor is right. New optional fields, two additively-extended enums, one new optional schema. The request-validation tightening (brief-required / refine-required, previously prose-only) only rejects payloads the docs already declared invalid — no conforming client relied on them. minor holds.
  • error-code.json parity — INVALID_STATE broadened in both enumDescriptions (L138) and the enumMetadata suggestion (L341). No new code minted; INVALID_STATE / CONFLICT / PROPOSAL_NOT_FOUND / INVALID_REQUEST all pre-exist.
  • Schema-vs-docs — get_products.mdx, create_media_buy.mdx, specification.mdx, refinement.mdx track the schema: field names, required/optional, the reason and close_reason enum lists, and the create→error mapping all match.
  • check-migration-numbers.ymlprefix_dupes() + union/main/local dedup fails on local dupes, skips pre-existing main-only dupes, and catches stale-merge collisions. The 519/520 renames produce a clean contiguous sequence.

Minor nits (non-blocking)

  1. Thin negative assertions. Several expectInvalid cases in tests/example-validation-simple.test.cjs assert only ['must NOT be valid'] (open-rejects-brief, omit-rejects-reason, create-closed-rejects-non-accepted). Asserting the offending field/keyword instead would stop an unrelated schema change from making them pass for the wrong reason.

Fix the registry, run npm test, and I'll approve.

@aao-release-bot aao-release-bot 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.

Clean, fully-additive extension to the media-buy flow. Right shape: opportunity is advisory buyer-reported context fenced off from idempotency, pricing, and buyability, while proposal_version does the load-bearing work of pinning the exact offer state a buyer accepts or declines.

code-reviewer, ad-tech-protocol-expert (sound-with-caveats), and security-reviewer (no High/Medium) all came back without a blocker.

Things I checked

  • Changeset type is correct. minor holds: every wire change is additive — new optional fields (proposal_version, opportunity, decline reason/detail), enum values appended to request/response/capability enums (open on buying_mode + capabilities buying_modes, decline on proposal action), and reused error codes. No removed/renamed fields, no required↔optional flips, no removed enum values. proposal_version stays globally optional on proposal.json and is only conditionally required via dependencies.proposal_version: [proposal_id] plus the per-call if/then — the legacy unversioned-proposal path is preserved.
  • The conditional logic is right. opportunity-context.json:74-105if: not(status present AND status===closed) → then: not(required close_reason OR close_detail) correctly gates the close fields to status: "closed" only, and additionalProperties: false coexists cleanly since both are declared properties. create-media-buy-request.json:191-223 — the then only constrains opportunity.close_reason to ["accepted_with_seller"], it does not require it, so a closed-on-create with no reason still validates; no contradiction with the context schema's own conditional (strict intersection, not conflict).
  • open mode is fenced everywhere it needs to be. get-products-request.json forbids brief (brief-forbidden conditional includes open) and refine (the refine conditional's else fires for any non-refine mode), and get-products-async-response-submitted.json excludes open from the Submitted arm — open is a synchronous state read, matching async-operations.mdx.
  • The reason oneOf is the accepted enum-with-descriptions idiom, not a discriminator regression. Every variant is {const, description}; audit-oneof.mjs classifies all-const variants as scalar and excludes them from the baseline and diff, so --check does not regress.
  • error-code parity. INVALID_STATE updated in both enumDescriptions and enumMetadata.suggestion, recovery: "correctable" unchanged and still matching the prose. No new enum values — CONFLICT/PROPOSAL_NOT_FOUND/INVALID_REQUEST are reused.
  • x-entity registration is complete. Both opportunity and proposal registered in x-entity-types.json, proposal_id → proposal added to scripts/x-entity-field-map.json, the doc table updated, and annotations placed on the request/response/proposal id sites. The PATCH-4 "register proposal entity annotation" commit is exactly the internal-consistency cleanup that would otherwise have shipped a half-wired entity.
  • Migration renames are safe. main currently carries a triple collision at 517; this PR moves two to free 519/520 (highest prior was 518), and both renamed files are idempotent (ON CONFLICT DO UPDATE, IF NOT EXISTS, guarded ADD CONSTRAINT). The new check-migration-numbers.yml main-only-duplicate reconciliation is what lets this fixing PR pass its own gate.
  • Compliance coverage is real. proposal_decline.yaml exercises the full loop — versioned-proposal brief, decline-with-echo, idempotent duplicate decline, and the INVALID_STATE create-reject at the declined version — and the new negative tests in example-validation-simple.test.cjs cover open-mode-rejects-brief/refine and decline-required-fields.

Follow-ups (non-blocking — file as issues)

  • Stale error row in refinement.mdx. The INVALID_REQUEST row still reads "refine provided in brief or wholesale mode" — not updated to add open, even though the same PR adds open to the forbidden-refine set in specification.mdx and get_products.mdx and the schema enforces it. Doc-only drift; tighten when convenient.
  • Receiver-side untrusted-input framing (security-reviewer L1). The privacy prose on opportunity_id/detail/close_detail is thorough but entirely buyer-obligation-facing. For an agent-to-agent protocol where these free-text fields are explicitly routed into "CRM labels," "human-review prioritization," and "win-rate analytics," one normative sentence telling sellers to treat them as untrusted before placing them in LLM context would close the prompt-injection gap. Spec text, not code — hence a follow-up.
  • CONFLICT is slightly off-label. Its canonical recovery is transient (retry as-is), but stale-proposal_version recovery is correctable (re-read for a new version). Defensible as the established re-read-then-retry model; a one-line note in the CONFLICT description would remove the ambiguity.

Minor nits (non-blocking)

  1. opportunity_id per-seller scoping is SHOULD, and every example reuses the same literal. opp_01HX7W8S3Q9K2M4N6P0R5T1V appears identically across the doc examples and proposal_decline.yaml, modeling the cross-seller-reuse anti-pattern the schema warns against. Vary the example IDs so the spec doesn't demonstrate the thing it tells buyers not to do.
  2. for prefix in $union_dupes is unquoted (check-migration-numbers.yml). Safe for numeric prefixes from sed 's/_.*//', but grep -Fqx over a quoted iterator would harden the dup verdict against an oddly-named .sql file. CI hygiene, not a bug.

Approving on the strength of the additive-only wire surface plus the in-schema enforcement of every new invariant. Follow-ups noted above.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant