feat: add RC2 governance support and full spec coverage#161
Conversation
Add governance protocol (sync_plans, check_governance, report_plan_outcome, get_plan_audit_logs, get_creative_features) with GovernanceHandler, wire RC2 tasks through client/server/adapters/CLI/MCP, add spec coverage tests, and integrate PR #160's oneOf flattening. Remove stale SubAsset types (schema removed in RC2), fix list field shadowing in GetPropertyListResponse, add path-traversal validation to registry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a53bfc1 to
096f693
Compare
Review: AdCP 3.0 RC2 — Feedback from Salesagent ConsumerWe reviewed this from the perspective of a downstream consumer (Prebid Sales Agent) that extends library types via inheritance and uses the client/MCP surface extensively. Overall this is solid work — Must Fix1. On main, Any consumer doing 2.
3. The field went from required ( Should Fix4. ~1500 lines of duplicated not-supported boilerplate across handlers
Fix: Override class GovernanceHandler(ADCPHandler):
_agent_type = "Governance"
# Base class already returns not_supported() for unimplemented methodsThen in 5. MCP tool schemas are hand-crafted and can drift from generated types
Fix: Either generate 6. 35 tools on a single MCP server — context window cost All 35 tools are registered regardless of handler type. A 7. Five types removed without deprecation aliases
8. Inconsistent validation: TypeAdapter vs model_validate
9. ADCPClient method boilerplate Every client method follows the identical pattern: create operation_id → emit activity → call adapter → emit activity → parse response. This could be extracted to Consider10. Tool descriptions are too terse for agent consumption Descriptions like 11. No timestamp validation in webhook signature verification
12. Every method has a default implementation ( Impact on Salesagent (our upgrade plan)When we upgrade to this version, we'll need to:
The structural changes (brand submodule split, SyncCreatives move to creative/) don't affect us since we import from Overall: good progress toward RC2. The must-fix items (#1-3) are the main blockers from our perspective. The handler boilerplate (#4) and MCP schema drift (#5) are significant tech debt that will compound as the spec grows. Happy to discuss any of these. |
…plate, deprecation aliases) - Fix FieldModel export collision: add GetProductsField/GetBrandIdentityField semantic aliases, restore backward-compat FieldModel → get_products version - Remove ~1100 lines of duplicated not-supported boilerplate from handlers by using _agent_type class attribute and base class _not_supported() helper - Add deprecation aliases for removed types (Disclaimer, Performance, ProductCatalog, MediaSubAsset, TextSubAsset) - Add MCP schema drift detection test comparing inputSchema vs Pydantic models - Document TypeAdapter usage for Union type aliases, fix TypeAdapter[Any] annotation - Fix TypeAdapter[Any] → TypeAdapter[SiSendMessageRequest] in SI handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Response to ReviewThanks for the thorough review. Pushed fixes in b9eac61 — net reduction of 741 lines. Must Fix1. FieldModel silently rebound — Fixed. Added semantic aliases 2. Snapshot gains required fields — Investigated: 3. Product.delivery_measurement → optional — This is an RC2 spec change (removed from Should Fix4. ~1500 lines of duplicated not-supported boilerplate — Fixed. Added 5. MCP schema drift — Added 6. 35 tools on a single MCP server — Agree this is worth doing. Deferring to a follow-up since it requires changes to the MCP adapter's tool registration flow, and this PR is already large. 7. Five types removed without deprecation aliases — Fixed. Added backward-compat aliases:
8. TypeAdapter vs model_validate — Documented with inline comments. Both 9. ADCPClient method boilerplate — Deferring to follow-up. Agree the pattern could be extracted, but it's a refactor of the entire client surface that should be its own PR. Consider10. Tool descriptions — Agree, tracking for follow-up. 11. Webhook timestamp validation — Good catch, tracking for follow-up (security hardening). 12. ABC without abstract methods — Intentional design: |
KonstantinMirin
left a comment
There was a problem hiding this comment.
Great work here — the spec coverage test is a particularly smart addition, makes it impossible to forget wiring a new task through all five layers. The handler boilerplate consolidation with _not_supported() is clean too.
Two small things I'd want to tighten before merge:
TypeAdapter comments are stale — governance.py:39 and sponsored_intelligence.py:27 say GetPlanAuditLogsRequest and SiSendMessageRequest are Union type aliases, but after the oneOf flattening in c242a7d they're regular classes with model_validate(). The TypeAdapter works, but the comments will mislead the next person adding a handler. Quick swap to model_validate() and delete the comments.
Registry slug validation — the get_member() check blocks / and \ but an allowlist regex ([a-zA-Z0-9_-]+) would be tighter. Minor but it's the kind of thing that's easier to get right now than patch later.
Optional: the 5 governance campaign methods in client.py use single-line docstrings while the property list methods right below them use multi-line with Args/Returns — might want to match them up.
Rest looks solid. The full-stack wiring across client/handlers/adapters/CLI/MCP is consistent and complete.
- After oneOf flattening (c242a7d), GetPlanAuditLogsRequest and SiSendMessageRequest are regular Pydantic classes. Replace TypeAdapter with model_validate() for consistency across all handlers. - Tighten registry slug validation from blocklist (/ and \) to allowlist regex [a-zA-Z0-9_-]+. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bokelley
left a comment
There was a problem hiding this comment.
Thanks @KonstantinMirin — both fixed in 103d616:
-
TypeAdapter → model_validate() — You're right, after the oneOf flattening both
GetPlanAuditLogsRequestandSiSendMessageRequestare regular Pydantic classes now. ReplacedTypeAdapter.validate_python()withmodel_validate()and removed the stale comments. -
Slug validation — Tightened from blocklist (
/and\) to allowlist regex[a-zA-Z0-9_-]+.
Skipping the docstring alignment for now — can do in a follow-up if you'd like.
- MCP tool filtering: specialized handlers (Governance, Content Standards, Sponsored Intelligence) now only register their relevant tools plus get_adcp_capabilities, reducing context window cost for agent consumers. ADCPHandler (sales agents) still gets all tools. - Webhook replay protection: reject timestamps older than 5 minutes (configurable via webhook_timestamp_tolerance). Also rejects future timestamps for clock skew protection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… handler tools Security fixes from review: - Webhook auth bypass: when webhook_secret is configured, require both signature and timestamp headers. Previously, omitting headers bypassed verification entirely. - MCP tool filtering: unknown handler types now get protocol-only tools (minimum privilege) instead of all tools. ADCPHandler is explicitly listed with all tools. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- get_tools_for_handler now accepts instance or class, walks MRO to find the matching handler base class. Fixes filtering for user subclasses (e.g. MyGovernanceAgent(GovernanceHandler) now correctly gets only governance tools). - Add module-level assertion that all tool names in _HANDLER_TOOLS reference real tools in ADCP_TOOL_DEFINITIONS. - Add test for subclass MRO-based filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
adbf51c to
39985e5
Compare
Summary
sync_plans,check_governance,report_plan_outcome,get_plan_audit_logs,get_creative_featuresget_media_buys,get_account_financials,report_usage,sync_audiences,sync_catalogs) through client, adapters, server handlers, MCP tools, and CLIGovernanceHandlerwith input validation, delegation, and domain-specific not-supported stubsContentStandardsHandlerandSponsoredIntelligenceHandlerTypeAdapterusage forGetPlanAuditLogsRequestunion validation$refresolution in extra policy test for versioned schema pathsget_property_list_response.pytest_spec_coverage.pyregression test ensuring every task inschemas/cache/index.jsonis wired into client, handler, adapters, CLI, and MCP toolsTest plan
ruff check src/ tests/— all checks passedmypy src/adcp/— 0 errors (453 files)pytest tests/— 720 passed, 0 failed🤖 Generated with Claude Code