Skip to content

fix(server): strip None-valued asset fields from dict-based response builders#631

Merged
bokelley merged 1 commit intomainfrom
claude/issue-622-asset-null-strip
May 10, 2026
Merged

fix(server): strip None-valued asset fields from dict-based response builders#631
bokelley merged 1 commit intomainfrom
claude/issue-622-asset-null-strip

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #622

Summary

ImageAsset, VideoAsset, and UrlAsset have optional Python fields (format: str | None = None, alt_text, provenance, container_format, etc.) that default to None. 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 to None, the null values reach the wire and cause the storyboard schema validator to reject the asset — the oneOf discriminated union branch fails because null is not a string per JSON Schema Draft 7.

The fix adds _strip_none_values() — a recursive helper that removes None-valued keys from dicts before wire serialisation — and applies it in:

  • _serialize() — for dict items, after the existing _strip_write_only_fields() pass
  • sync_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 variants

The Pydantic model path was already safe: _serialize() calls model_dump(mode="json", exclude_none=True) for model items, and all SDK dispatch sites (serve.py, a2a_server.py, mcp_tools.py) also pass exclude_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 — passes
  • uv 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 passed
  • Full suite pytest tests/ — 128 passed, 1 pre-existing failure (test_real_tls_handshake_still_validates_hostname — network env issue, unrelated to this change)
  • 12 new tests added covering _strip_none_values unit behaviour and list_creatives_response, preview_creative_response, build_creative_response, sync_creatives_response with explicit None-valued ImageAsset and VideoAsset fields

Nit 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 in adcontextprotocol/adcp. That is tracked separately.

Pre-PR review

  • code-reviewer: approved — blockers (sync_creatives, preview Pydantic path) addressed in final diff
  • ad-tech-protocol-expert: approved — strip-rather-than-null matches AdCP/OpenRTB "omit rather than null" convention; no spec-defined sentinel use case for null on these fields

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout <num>
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See adcp#3121
for context.

Session: https://claude.ai/code/session_01AkZRebtF7xsZJZ2FPnQmNL


Generated by Claude Code

@bokelley bokelley force-pushed the claude/issue-622-asset-null-strip branch from a4410f4 to 7718f44 Compare May 10, 2026 13:28
@bokelley bokelley marked this pull request as ready for review May 10, 2026 13:28
@bokelley bokelley force-pushed the claude/issue-622-asset-null-strip branch 3 times, most recently from f1c2f22 to 675ff33 Compare May 10, 2026 15:15
…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
@bokelley bokelley force-pushed the claude/issue-622-asset-null-strip branch from 675ff33 to a36923f Compare May 10, 2026 15:15
@bokelley bokelley merged commit c02ea84 into main May 10, 2026
16 checks passed
@bokelley bokelley deleted the claude/issue-622-asset-null-strip branch May 10, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ImageAsset.format / alt_text / provenance — Pydantic type allows null but bundled schema does not

2 participants