Skip to content

Conversation

@bokelley
Copy link
Contributor

Problem

The AdCP JSON schema does not allow null values for optional fields. According to the spec, optional fields should be omitted entirely, not set to null or empty values.

Issues:

  1. ListAuthorizedPropertiesResponse was explicitly setting errors=[], causing it to be included in responses
  2. AdCPBaseModel.model_dump() wasn't using exclude_none=True by default
  3. Tests were incorrectly validating that ALL optional fields must be present

Example of incorrect behavior:

{
  "publisher_domains": ["example.com"],
  "errors": [],           // ❌ Should be omitted
  "primary_channels": null // ❌ Should be omitted
}

Correct behavior:

{
  "publisher_domains": ["example.com"]
  // ✅ Optional fields omitted
}

Root Causes

1. Missing exclude_none in AdCPBaseModel.model_dump()

AdCPBaseModel didn't set exclude_none=True by default, so None-valued fields were included in serialization.

2. Incorrect override in ListAuthorizedPropertiesResponse.model_dump()

Lines 3444-3446 in schemas.py were converting errors=None to errors=[]:

def model_dump(self, **kwargs) -> dict[str, Any]:
    data = super().model_dump(**kwargs)
    if data.get("errors") is None:
        data["errors"] = []  # ❌ Violates AdCP spec
    return data

3. Explicit errors=[] in tool code

src/core/tools/properties.py line 202 was setting errors=[] explicitly, causing it to be included even though it had no errors.

Solution

1. Added model_dump() to AdCPBaseModel

def model_dump(self, **kwargs):
    """Dump model with AdCP-compliant defaults."""
    if "exclude_none" not in kwargs:
        kwargs["exclude_none"] = True
    return super().model_dump(**kwargs)

Now all AdCP response models automatically omit None-valued optional fields.

2. Fixed ListAuthorizedPropertiesResponse.model_dump()

Removed the override that was converting None to []. Now delegates to parent:

def model_dump(self, **kwargs) -> dict[str, Any]:
    """Return AdCP-compliant response."""
    return super().model_dump(**kwargs)

3. Removed explicit errors=[] from tool code

Properties tool no longer sets errors=[] when there are no errors.

Testing

48/48 AdCP contract tests passing
829/829 unit tests passing
Updated 4 unit tests to verify omission behavior
Updated 1 integration test to verify MCP/A2A parity with new behavior

Tests now verify:

  • Required fields are always present
  • Optional fields with None values are omitted
  • Optional fields with actual values are included
  • Both MCP and A2A return identical response data

Impact

Breaking Change: ⚠️ Clients expecting all optional fields to be present (even with null/empty values) will need to handle omitted fields.

Benefits:

  • ✅ Full compliance with AdCP JSON schema spec
  • ✅ Prevents validation errors from strict AdCP consumers
  • ✅ Reduces payload size (no unnecessary null/empty fields)
  • ✅ More idiomatic JSON (optional fields are truly optional)

Scope:

  • Affects all AdCP response models that inherit from AdCPBaseModel
  • Most visible in ListAuthorizedPropertiesResponse responses
  • No changes to request handling or validation

Files Changed

  1. src/core/schemas.py (2 changes)

    • Added model_dump() override to AdCPBaseModel with exclude_none=True
    • Fixed ListAuthorizedPropertiesResponse.model_dump() to delegate to parent
  2. src/core/tools/properties.py (1 change)

    • Removed explicit errors=[] assignment
  3. tests/unit/test_adcp_contract.py (4 test updates)

    • Updated assertions to verify omission instead of presence
  4. tests/unit/test_authorized_properties.py (2 test updates)

    • Updated to verify AdCP-compliant omission behavior
  5. tests/integration/test_a2a_response_compliance.py (1 test update)

    • Updated MCP/A2A parity test to expect omitted fields

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com

@bokelley bokelley force-pushed the fix-null-validation-adcp branch 2 times, most recently from ad1bab6 to cea4798 Compare October 27, 2025 13:06
Problem: AdCP spec requires optional fields to be omitted (not present)
rather than set to null or empty values.

Root Causes:
1. AdCPBaseModel lacked model_dump() override with exclude_none=True
2. ListAuthorizedPropertiesResponse.model_dump() was forcing errors=[]
3. list_authorized_properties tool set errors=[] explicitly
4. Test suite incorrectly required all optional fields to be present

Fixes:
1. Added model_dump() to AdCPBaseModel with exclude_none=True
2. Fixed ListAuthorizedPropertiesResponse to delegate to parent
3. Fixed list_authorized_properties tool to not set errors=[]
4. Fixed 4 AdCP contract tests + field count assertions

Testing:
All 48 AdCP contract tests pass
Pre-commit hooks pass (schema sync skipped - in different PR)

Pattern:
✅ response = Response(required='value')  # Omit optional
❌ response = Response(required='value', optional=[])  # Don't set empty
@bokelley bokelley force-pushed the fix-null-validation-adcp branch from cea4798 to b2bdc29 Compare October 27, 2025 15:21
@bokelley bokelley merged commit ab7c4cd into main Oct 27, 2025
9 checks passed
danf-newton pushed a commit to Newton-Research-Inc/salesagent that referenced this pull request Nov 24, 2025
…ontextprotocol#638)

Problem: AdCP spec requires optional fields to be omitted (not present)
rather than set to null or empty values.

Root Causes:
1. AdCPBaseModel lacked model_dump() override with exclude_none=True
2. ListAuthorizedPropertiesResponse.model_dump() was forcing errors=[]
3. list_authorized_properties tool set errors=[] explicitly
4. Test suite incorrectly required all optional fields to be present

Fixes:
1. Added model_dump() to AdCPBaseModel with exclude_none=True
2. Fixed ListAuthorizedPropertiesResponse to delegate to parent
3. Fixed list_authorized_properties tool to not set errors=[]
4. Fixed 4 AdCP contract tests + field count assertions

Testing:
All 48 AdCP contract tests pass
Pre-commit hooks pass (schema sync skipped - in different PR)

Pattern:
✅ response = Response(required='value')  # Omit optional
❌ response = Response(required='value', optional=[])  # Don't set empty
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.

2 participants