-
Notifications
You must be signed in to change notification settings - Fork 13
feat: enforce strict AdCP v1 spec compliance for Creative model (BREAKING CHANGE) #706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
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
The test agent was incorrectly rejecting inline creative objects in create_media_buy, requiring response-only fields (content_uri, principal_id, created_at, updated_at). Per AdCP v2.2.0 spec, CreativeAsset only requires: creative_id, name, format_id, and assets. Changes to schemas.py: - Made Creative.url (alias content_uri) optional (None default) - Made Creative.principal_id optional (sales agent adds this) - Made Creative.created_at optional (sales agent adds this) - Made Creative.updated_at optional (sales agent adds this) - Updated content_uri property to return str | None - Updated get_primary_content_url() to return str | None - Added null checks in get_creative_type() and get_snippet_content() Changes to creative_helpers.py: - Added null check in _convert_creative_to_adapter_asset() for creative.url This allows buyers to send inline creatives in create_media_buy requests without having to include internal/response-only fields. Tests: - All 860 unit tests pass (102 creative tests pass) - AdCP contract tests pass - Creative validation tests pass - Format ID tests pass - mypy type checking passes (0 errors) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added comprehensive documentation explaining why the Creative class has more fields than the AdCP spec and why many fields are optional. Context: The Creative class is a "hybrid model" that serves three purposes: 1. AdCP Input - Accepts spec-compliant CreativeAsset (4 required fields) 2. Internal Storage - Adds metadata (principal_id, timestamps, workflow) 3. Response Output - Filters internal fields for AdCP compliance Why fields are optional: - AdCP spec only requires: creative_id, name, format_id, assets - Internal fields (principal_id, created_at, status) are added by sales agent - Buyers should NOT have to provide internal sales-agent fields - Making them optional allows accepting pure AdCP CreativeAsset objects Documentation added: - docs/architecture/creative-model-architecture.md - Full architecture explanation - Updated Creative class docstring with architecture note This explains the design behind the fix for GitHub issue #703 and inline creatives validation. The hybrid model approach is intentional and pragmatic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
EmmaLouise2018
approved these changes
Nov 5, 2025
…KING CHANGE) Removes all non-spec fields from Creative model and refactors creative conversion to extract data from the AdCP-compliant assets dict. Fixes #703 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixes 12 test failures across integration and E2E tests: **Integration Tests (10 failures fixed):** - test_impression_tracker_flow.py: Converted 8 Creative instantiations to AdCP v1 format - Removed non-spec fields (content_uri, media_url, width, height, duration, snippet, template_variables, delivery_settings) - Moved all content to assets dict with proper structure - Updated tracking URLs to use url_type="tracker_pixel" - Updated assertions to match new delivery_settings structure (nested dict with url_type) - test_schema_contract_validation.py: Fixed 2 Creative contract tests - Changed from format_id (alias) to format (actual field name) for validator - Converted to assets dict format per AdCP v1 spec **E2E Tests (2 failures fixed):** - src/core/tools/creatives.py: Added None safety checks in list_creatives - Fixed "NoneType * int" error at line 1756 (duration conversion) - Added None checks before arithmetic operations on width, height, duration - Prevents errors when legacy database records have None values All tests now use strict AdCP v1 compliant Creative format with assets dict. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Removed AUDIT_non_spec_fields.md - this was a working document for analyzing non-spec fields during the Creative model refactoring. The analysis is complete and the findings are documented in the PR description and ASSET_TYPE_MAPPING.md. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ristic Addresses user feedback on PR #706: **Comment #1 (line 133)**: Replace heuristic role-based detection with declarative format-id based detection - Now uses format_id prefix (display_, video_, native_) to determine expected asset type - Declarative role mapping based on format type instead of trying all possible roles - Clearer separation of concerns: video formats look for video assets first, display looks for images/banners **Comment #2 (line 214)**: Remove redundant clickthrough URL handling in tracking URLs - Clickthrough URLs are already extracted to asset["click_url"] (lines 213-228) - Removed redundant code that tried to add clickthrough to tracking_urls["click"] - Clickthrough URLs are for landing pages, not tracking pixels **Key changes:** 1. Extract format type from format_id: `format_str.split("_")[0]` 2. Use declarative role lists per format type (video/native/display) 3. Fallback skips tracking pixels when looking for primary asset 4. Fixed FormatId string conversion: use `str(creative.format_id)` 5. Added comment explaining clickthrough vs tracking URL distinction **Tests:** - ✅ All 8 impression tracker flow tests passing - ✅ All 8 creative conversion asset tests passing - ✅ All 15 schema contract validation tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
EmmaLouise2018
approved these changes
Nov 5, 2025
Moved asset type mapping documentation from root to docs/ for better organization. This document describes: - AdCP v1 asset types (image, video, HTML, JavaScript, VAST, URL) - Conversion logic for ad server adapters (GAM) - Asset role naming conventions - Format type mappings 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Merged standalone ASSET_TYPE_MAPPING.md into creative-model-architecture.md for better organization and context. **Changes:** - Added "AdCP v1 Asset Types and Adapter Mappings" section to creative-model-architecture.md - Documents all 6 AdCP v1 asset types (Image, Video, HTML, JavaScript, VAST, URL) - Explains conversion logic for GAM adapter - Documents asset role naming conventions and format type detection - Removed standalone docs/ASSET_TYPE_MAPPING.md **Benefits:** - Single source of truth for Creative model documentation - Asset type reference is now contextualized within the hybrid model design - Easier to maintain - one doc instead of two - Related file references now include creative_helpers.py and conversion tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
danf-newton
pushed a commit
to Newton-Research-Inc/salesagent
that referenced
this pull request
Nov 24, 2025
…KING CHANGE) (adcontextprotocol#706) * fix: make Creative response-only fields optional for inline creatives The test agent was incorrectly rejecting inline creative objects in create_media_buy, requiring response-only fields (content_uri, principal_id, created_at, updated_at). Per AdCP v2.2.0 spec, CreativeAsset only requires: creative_id, name, format_id, and assets. Changes to schemas.py: - Made Creative.url (alias content_uri) optional (None default) - Made Creative.principal_id optional (sales agent adds this) - Made Creative.created_at optional (sales agent adds this) - Made Creative.updated_at optional (sales agent adds this) - Updated content_uri property to return str | None - Updated get_primary_content_url() to return str | None - Added null checks in get_creative_type() and get_snippet_content() Changes to creative_helpers.py: - Added null check in _convert_creative_to_adapter_asset() for creative.url This allows buyers to send inline creatives in create_media_buy requests without having to include internal/response-only fields. Tests: - All 860 unit tests pass (102 creative tests pass) - AdCP contract tests pass - Creative validation tests pass - Format ID tests pass - mypy type checking passes (0 errors) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: add architecture documentation for Creative hybrid model Added comprehensive documentation explaining why the Creative class has more fields than the AdCP spec and why many fields are optional. Context: The Creative class is a "hybrid model" that serves three purposes: 1. AdCP Input - Accepts spec-compliant CreativeAsset (4 required fields) 2. Internal Storage - Adds metadata (principal_id, timestamps, workflow) 3. Response Output - Filters internal fields for AdCP compliance Why fields are optional: - AdCP spec only requires: creative_id, name, format_id, assets - Internal fields (principal_id, created_at, status) are added by sales agent - Buyers should NOT have to provide internal sales-agent fields - Making them optional allows accepting pure AdCP CreativeAsset objects Documentation added: - docs/architecture/creative-model-architecture.md - Full architecture explanation - Updated Creative class docstring with architecture note This explains the design behind the fix for GitHub issue adcontextprotocol#703 and inline creatives validation. The hybrid model approach is intentional and pragmatic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: enforce strict AdCP v1 spec compliance for Creative model (BREAKING CHANGE) Removes all non-spec fields from Creative model and refactors creative conversion to extract data from the AdCP-compliant assets dict. Fixes adcontextprotocol#703 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: resolve CI test failures with AdCP v1 Creative format Fixes 12 test failures across integration and E2E tests: **Integration Tests (10 failures fixed):** - test_impression_tracker_flow.py: Converted 8 Creative instantiations to AdCP v1 format - Removed non-spec fields (content_uri, media_url, width, height, duration, snippet, template_variables, delivery_settings) - Moved all content to assets dict with proper structure - Updated tracking URLs to use url_type="tracker_pixel" - Updated assertions to match new delivery_settings structure (nested dict with url_type) - test_schema_contract_validation.py: Fixed 2 Creative contract tests - Changed from format_id (alias) to format (actual field name) for validator - Converted to assets dict format per AdCP v1 spec **E2E Tests (2 failures fixed):** - src/core/tools/creatives.py: Added None safety checks in list_creatives - Fixed "NoneType * int" error at line 1756 (duration conversion) - Added None checks before arithmetic operations on width, height, duration - Prevents errors when legacy database records have None values All tests now use strict AdCP v1 compliant Creative format with assets dict. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: remove temporary audit document Removed AUDIT_non_spec_fields.md - this was a working document for analyzing non-spec fields during the Creative model refactoring. The analysis is complete and the findings are documented in the PR description and ASSET_TYPE_MAPPING.md. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor: improve creative asset detection to be declarative, not heuristic Addresses user feedback on PR adcontextprotocol#706: **Comment adcontextprotocol#1 (line 133)**: Replace heuristic role-based detection with declarative format-id based detection - Now uses format_id prefix (display_, video_, native_) to determine expected asset type - Declarative role mapping based on format type instead of trying all possible roles - Clearer separation of concerns: video formats look for video assets first, display looks for images/banners **Comment adcontextprotocol#2 (line 214)**: Remove redundant clickthrough URL handling in tracking URLs - Clickthrough URLs are already extracted to asset["click_url"] (lines 213-228) - Removed redundant code that tried to add clickthrough to tracking_urls["click"] - Clickthrough URLs are for landing pages, not tracking pixels **Key changes:** 1. Extract format type from format_id: `format_str.split("_")[0]` 2. Use declarative role lists per format type (video/native/display) 3. Fallback skips tracking pixels when looking for primary asset 4. Fixed FormatId string conversion: use `str(creative.format_id)` 5. Added comment explaining clickthrough vs tracking URL distinction **Tests:** - ✅ All 8 impression tracker flow tests passing - ✅ All 8 creative conversion asset tests passing - ✅ All 15 schema contract validation tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: move ASSET_TYPE_MAPPING.md to docs/ folder Moved asset type mapping documentation from root to docs/ for better organization. This document describes: - AdCP v1 asset types (image, video, HTML, JavaScript, VAST, URL) - Conversion logic for ad server adapters (GAM) - Asset role naming conventions - Format type mappings 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: integrate asset type mappings into creative architecture doc Merged standalone ASSET_TYPE_MAPPING.md into creative-model-architecture.md for better organization and context. **Changes:** - Added "AdCP v1 Asset Types and Adapter Mappings" section to creative-model-architecture.md - Documents all 6 AdCP v1 asset types (Image, Video, HTML, JavaScript, VAST, URL) - Explains conversion logic for GAM adapter - Documents asset role naming conventions and format type detection - Removed standalone docs/ASSET_TYPE_MAPPING.md **Benefits:** - Single source of truth for Creative model documentation - Asset type reference is now contextualized within the hybrid model design - Easier to maintain - one doc instead of two - Related file references now include creative_helpers.py and conversion tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
Summary
This PR removes all non-spec fields from the Creative model and refactors creative conversion to extract data from the AdCP-compliant assets dict instead of top-level fields. This resolves issue #703 where inline creatives were incorrectly required to have response-only fields.
Breaking Change
Key Changes
1. Creative Model (src/core/schemas.py)
REMOVED 20+ non-spec fields:
KEPT AdCP v1 spec fields only:
2. Creative Conversion (src/core/helpers/creative_helpers.py)
Complete rewrite of
_convert_creative_to_adapter_asset():3. sync_creatives & list_creatives Tools
4. Documentation
AUDIT_non_spec_fields.md- Analysis of removed fieldsASSET_TYPE_MAPPING.md- Spec-compliant asset mappings for all typesdocs/architecture/creative-model-architecture.md- Architecture explanation5. Tests
test_creative_conversion_assets.pywith 8 tests for all asset typesMigration Guide
Before (non-compliant):
After (AdCP v1 compliant):
Fixes
Test Plan
🤖 Generated with Claude Code