Conversation
a4410f4 to
7718f44
Compare
f1c2f22 to
675ff33
Compare
…builder output Optional ImageAsset / VideoAsset / UrlAsset fields (format, alt_text, provenance, container_format, etc.) default to None in the Pydantic models but the bundled JSON schemas declare them as non-nullable (\"type\": \"string\", not [\"string\", \"null\"]). When adopters build dict-based creative responses with those fields explicitly set to None, the null values reach the wire and cause the storyboard schema validator's oneOf branch to reject the asset. Add _strip_none_values() — a recursive helper that removes None-valued keys from dicts before wire serialisation — and apply it in _serialize() (for dict items after the write-only-field strip), sync_creatives_response(), preview_creative_response(), and build_creative_response(). The Pydantic model path was already safe via model_dump(exclude_none=True). Closes #622 https://claude.ai/code/session_01AkZRebtF7xsZJZ2FPnQmNL
675ff33 to
a36923f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #622
Summary
ImageAsset,VideoAsset, andUrlAssethave optional Python fields (format: str | None = None,alt_text,provenance,container_format, etc.) that default toNone. The bundled JSON schemas declare these fields as non-nullable ("type": "string", not["string", "null"]). When adopters build dict-based creative responses with those fields set toNone, the null values reach the wire and cause the storyboard schema validator to reject the asset — theoneOfdiscriminated union branch fails becausenullis not a string per JSON Schema Draft 7.The fix adds
_strip_none_values()— a recursive helper that removesNone-valued keys from dicts before wire serialisation — and applies it in:_serialize()— for dict items, after the existing_strip_write_only_fields()passsync_creatives_response()— now routes through_serialize()(was returning raw list)preview_creative_response()— now routes through_serialize()(was unguarded)build_creative_response()— for both single and multi-manifest variantsThe Pydantic model path was already safe:
_serialize()callsmodel_dump(mode="json", exclude_none=True)for model items, and all SDK dispatch sites (serve.py,a2a_server.py,mcp_tools.py) also passexclude_none=True. This PR closes the gap in the dict (loose-dict / hand-built payload) path, which is the most common pattern in adopter code.What tested
ruff check src/adcp/server/responses.py tests/test_server_dx.py— passesuv run mypy src/adcp/server/responses.py— passes (no issues in 1 source file)uv run --all-extras pytest tests/test_server_dx.py -v— 56/56 passedpytest tests/— 128 passed, 1 pre-existing failure (test_real_tls_handshake_still_validates_hostname— network env issue, unrelated to this change)_strip_none_valuesunit behaviour andlist_creatives_response,preview_creative_response,build_creative_response,sync_creatives_responsewith explicitNone-valuedImageAssetandVideoAssetfieldsNit noted (not fixed):
sync_creatives_response()wiring through_serialize()also adds the write-only-field strip to sync records — this is safe but slightly broader than needed; flagging for reviewer awareness.Upstream note: Widening the JSON schema to
["string", "null"]for these asset fields (so that nullable-in-Python == nullable-on-wire) is the cleaner long-term fix and would require a spec-level change inadcontextprotocol/adcp. That is tracked separately.Pre-PR review
Session: https://claude.ai/code/session_01AkZRebtF7xsZJZ2FPnQmNL
Generated by Claude Code