feat!: align media-buy responses with AdCP 3.1#842
Conversation
cb1f48a to
e82197a
Compare
d067104 to
50d130a
Compare
50d130a to
63e0999
Compare
There was a problem hiding this comment.
Request changes. Wire shape is right; the semver signal is wrong. Read-shape breaks on the adcp.* public surface but the squash will land under fix: — dependents on ^6 pick it up automatically.
Block (must fix before merge)
Conventional-commit prefix. src/adcp/types/generated_poc/media_buy/create_media_buy_response.py:46 and update_media_buy_response.py:48 flip status from MediaBuyStatus | None = None to Literal[\"completed\"] (required). _normalize_legacy_status is load-bearing for construction — CreateMediaBuySuccessResponse(status=\"active\") still works — but the read side breaks: if resp.status == MediaBuyStatus.active is silent False, resp.status.value raises AttributeError. The PR-internal diff at examples/v3_reference_seller/tests/test_smoke_broadening.py:730,789,831 is the proof — every existing call site moves from result.status.value to result.media_buy_status.value. Land as feat!: or add a BREAKING CHANGE: footer with a one-line migration note ("read media_buy_status for lifecycle; status is now the task envelope") so release-please gates this behind a major.
Things I checked
- Wire shape vs AdCP 3.1:
ad-tech-protocol-expertverdict sound. Three-armResponse1|Response2|Response3matchesmedia-buy/create-media-buy-response.jsonandupdate-media-buy-response.jsononeOf.cache_scope=\"public\"on the anonymous wholesale feed is mandated byget-products-response.json:315-321. Strippedcategoriesongovernance_agentsecho is mandated bysync-governance-response.json:38-58(additionalProperties: false). Impairment shape matchescore/impairment.jsonrequired-field set. - Generated-type audit:
create_media_buy_response.pyandupdate_media_buy_response.pyare output ofscripts/post_generate_fixes.py:1919-2020. Not hand-edited. - Type-layering:
aliases.py/_generated.pyare the only new importers — within the allowlist. _normalize_response_envelope(src/adcp/server/mcp_tools.py:53-87) gates on sync media-buy success (media_buy_idpresent;task_id/errorsabsent). Submitted-envelope path bypassed correctly.- New
is_update_media_buy_submitted(src/adcp/types/guards.py:165-173) and the early-return tweaks at:179/:188correctly short-circuit success/error guards on the submitted arm. open_impairments(examples/seller_agent.py:83) survives status flaps with stable IDs; the lifecycle test (tests/test_seller_agent_storyboard.py:529-562) covers reopen → new ID.
Follow-ups (non-blocking — file as issues once the block clears)
_MEDIA_BUY_STATUS_VALUESdiverges between MCP boundary and generated models.src/adcp/server/mcp_tools.py:36-46includes\"draft\",\"completed\",\"cancelled\"(double-l).src/adcp/types/generated_poc/media_buy/create_media_buy_response.py:26-33does not. A handler returning{\"media_buy_id\": \"X\", \"status\": \"draft\"}gets MCP-normalized tomedia_buy_status=\"draft\"— andMediaBuyStatus(src/adcp/types/generated_poc/enums/media_buy_status.py) doesn't havedraft. The boundary manufactures a wire payload that fails buyer-side enum parse. Extract a single shared constant; drop\"draft\"and the double-l\"cancelled\".cancel_media_buy_responsestill writes envelopestatus=\"canceled\"; A2A doesn't normalize.src/adcp/server/helpers.pywasn't moved to the new contract, andsrc/adcp/server/a2a_server.py:452-471doesn't call_normalize_response_envelope. MCP catches it viaupdate_media_buynormalization; A2A ships envelopestatus=\"canceled\"un-normalized. Move_normalize_response_envelopeto a transport-agnostic spot and call it from both sides._normalize_legacy_statusis asymmetric on mismatch.create_media_buy_response.py:48-65:{status=\"pending_start\", media_buy_status=\"active\"}errors loudly;{status=\"completed\", media_buy_status=\"active\"}passes silently. Pick one — raise on any genuine mismatch, or always prefer the new shape.- Docs drift on the new shape.
skills/adcp-media-buy/SKILL.md:151-153documentsstatusas "Current lifecycle state — pending_creatives, pending_start, or active" — after this PR that'smedia_buy_status.skills/call-adcp-agent/SKILL.md:191shows the sync arm without the new dual-status field.AGENTS.mdType Guards section doesn't listis_create_media_buy_submitted/is_update_media_buy_submitted.llms.txt:41-42doesn't surface the*SubmittedResponsealiases. Buyer-side LLMs reading the skills will mis-parse. media_buy_response(..., status=...)kwarg is now misnamed.src/adcp/server/responses.py:342. The kwarg maps to lifecycle, not envelopestatus. Rename tomedia_buy_status=with a deprecation alias for one release.- Three copies of the
_value/_enum_valuehelper + lifecycle set.mcp_tools.py, both generated files, with the codegen template atscripts/post_generate_fixes.py:1925-2020inlining two of them. Shared module needed before this drifts further. open_impairmentsmodule-global isn't account-partitioned.examples/seller_agent.py:83. Reference seller is single-tenant-demo per the_DEFAULT_ACCOUNT_IDcomment at:85butopen_impairmentsdoesn't carry the same caveat. Add the comment or key by(account_id, media_buy_id, creative_id).
Minor nits (non-blocking)
_valuehelper name is too generic.src/adcp/types/generated_poc/media_buy/create_media_buy_response.py:36-37. Rename to_unwrap_enumso future codegen helpers don't collide._normalize_submitted_statusis redundant underuse_enum_values=True.create_media_buy_response.py:80-86,update_media_buy_response.py:82-88. Pydantic already accepts the string\"submitted\"and the enum interchangeably. Drop the validator.- Validation list omits
test_smoke_broadening.py. That's the file demonstrating the read-side migration — the contract change this PR ships. Worth listing explicitly.
Unblock by amending the squash subject to feat!: or appending a BREAKING CHANGE: footer.
There was a problem hiding this comment.
LGTM. Right factoring: payload helpers carry lifecycle, MCP boundary owns the task envelope. Adopters keep status="active" ergonomics through model_validator(mode='before'), on-the-wire status reads "completed" per 3.1.
Things I checked
feat!:title +BREAKING CHANGE:footer + migration note in body — semver signal correct for release-please.CreateMediaBuyResponse1.status: Literal["completed"]with_normalize_legacy_statusvalidator preservesstatus="active"construction by remapping tomedia_buy_status. Conflict case (raw_status != media_buy_status) falls through to a PydanticValidationError— not silent.- Boundary at
src/adcp/server/mcp_tools.py:73-85and model-level validator agree on the migration direction.status="completed"+media_buy_status=Nonecorrectly does not infermedia_buy_status="completed"—"completed"is the task envelope marker, not a lifecycle value. _looks_like_sync_media_buy_successexcludes payloads carryingtask_idso the submitted envelope isn't rewritten as sync at the boundary.is_update_media_buy_submittedshort-circuits both success and error guards — parity with the existing create-sideis_create_media_buy_submittedpattern.examples/seller_agent.py::_health_fields_for_media_buyimpairment shape (impairment_id,resource_type,resource_id,package_ids,transition,reason_code,reason,observed_at,remediation) matchesschemas/cache/3.1.0-beta.3/core/impairment.json; lifecycle test (reopen gets freshimpairment_id) passes.supports_proposals=trueis the canonical 3.1 discovery flag perbundled/protocol/get-adcp-capabilities-response.json.governance_agentsstrip ofcategoriesmatchessync-governance-response.json(additionalProperties: false,required: [url]).cache_scopeonproposal_finalizeprojection — perget-products-response.jsonthe field is REQUIRED on the projection;account if params.account else publicmatches the schema directive.- Generated files match the
scripts/post_generate_fixes.py:1520+post-processor injection — not hand-edited. - Public API snapshot updated for both
UpdateMediaBuyResponse3andUpdateMediaBuySubmittedResponse;is_update_media_buy_submittedlisted in guards__all__.
Follow-ups (non-blocking — file as issues)
- Export asymmetry:
UpdateMediaBuyResponse3is publicly exported by numbered name;CreateMediaBuyResponse3is not (only the semanticCreateMediaBuySubmittedResponsealias is). Per CLAUDE.md numbered names are unstable — pick one direction. Either dropUpdateMediaBuyResponse3from__all__and route adopters throughUpdateMediaBuySubmittedResponse, or addCreateMediaBuyResponse3for symmetry. _MEDIA_BUY_STATUS_VALUESdivergence between the boundary set (mcp_tools.py:36-46: includesdraft,cancelled,completed) and the model-level set (generated_poc/.../create_media_buy_response.py:20-27: six core lifecycle values). Handler returningstatus="draft"gets normalized at boundary, then trips Pydantic on the enum. Narrow the boundary set or document the legacy aliases the boundary intentionally translates (cancelled→canceled).result.setdefault("status", "completed")atmcp_tools.py:87runs unconditionally; a handler that returns{"task_id": ...}withoutstatus(partial migration) gets stamped"completed"and looks malformed. Gate on"task_id" not in resultfor media-buy methods or log a one-time deprecation when this fires.- Conflict case in
_normalize_legacy_status(raw_status != media_buy_status, both set) currently falls through to aValidationErroron theLiteral["completed"]mismatch. AValueErrorfrom the validator with both observed values would land faster for adopters who hit it.
Minor nits (non-blocking)
- Shared
_value/_MEDIA_BUY_STATUS_VALUEShelpers. Identical bodies appear ingenerated_poc/media_buy/create_media_buy_response.py:20-32,update_media_buy_response.py:20-32, and a third copy inmcp_tools.py:36-50. Extract togenerated_poc/core/so they don't drift on the next regeneration. "__anonymous__"sentinel.examples/seller_agent.py:131uses a string sentinel for the no-media_buy_idcase; collides with any real media-buy ID that happens to match. Use an object-identity sentinel or namespace it.- Pre-1.0 release-please semver.
feat!:on a beta line withbump-minor-pre-major: trueproduces a minor bump, not a major. The!is still the right changelog signal — just confirm the version delta matches release-manager intent.
Safe to merge.
Summary
Fixes the reference seller and Python SDK response surfaces for AdCP 3.1 storyboard compatibility around issue #825.
cache_scope="public"for cacheable reference-seller product responsescategoriesand emits the current account-governance URL shapestatusfrom media-buy lifecyclemedia_buy_statusfor create/update responsesstatusat the MCP boundary, including enum-valued dict payloadstools/listoutput schemas with the wire shape, while preserving legacy typed construction likestatus="active"@adcp/sdk@8.1.0-beta.7Validation
PYTHONPATH=src python3 -m ruff check src/-> passedPYTHONPATH=src python3 -m mypy src/adcp/-> passedPYTHONPATH=src python3 -m mypy --strict tests/type_checks/-> passedPYTHONPATH=src python3 -m pytest tests/ -q->5109 passed, 30 skipped, 10 deselected, 1 xfailedPYTHONPATH=src python3 -m pytest tests/test_code_generation.py tests/test_public_api.py -q->25 passedPYTHONPATH=src python3 -m pytest tests/test_proposal_lifecycle.py tests/test_proposal_auto_commit.py tests/test_proposal_lifecycle_e2e.py -q->56 passed@adcp/sdk@8.1.0-beta.7reference seller storyboard ->passing,88 passed, 0 failed, 2 skipped@adcp/sdk@8.1.0-beta.7sales-proposal-modeproposal_finalizestoryboard ->passing,5 passed, 0 failed, 0 skippedblack,ruff,mypy,bandit, whitespace/EOF/YAML/JSON/large-file/conflict/key checks all passedBreaking Change
CreateMediaBuySuccessResponse.statusandUpdateMediaBuySuccessResponse.statusnow represent the task-envelope status and read as"completed". Readmedia_buy_statusfor the media-buy lifecycle state.BREAKING CHANGE: read
media_buy_statusfor lifecycle;statusis now the task envelope.