feat: createAdcpServer with domain-grouped handlers and governance helper#541
feat: createAdcpServer with domain-grouped handlers and governance helper#541
Conversation
…ed handlers Domain registration that wires handler types → input schemas → Zod validation → response formatting. Auto-generates get_adcp_capabilities from registered tools. - Domain-grouped handlers: mediaBuy, signals, creative, governance, accounts, eventTracking, sponsoredIntelligence - resolveAccount middleware: auto-resolves AccountReference on tools with account field, returns ACCOUNT_NOT_FOUND on failure - Response builder auto-application: productsResponse, mediaBuyResponse (with revision/confirmed_at defaults), getSignalsResponse, etc. - Tool annotations: readOnlyHint, destructiveHint, idempotentHint set per tool - Tool coherence warnings: create_media_buy without get_products, etc. - Unknown handler key warnings: catches typos like getProduct vs getProducts - Handler error catching: try/catch returns SERVICE_UNAVAILABLE on unhandled errors - checkGovernance composable helper: seller calls explicitly in financial handlers - governanceDeniedError convenience: converts denial to COMPLIANCE_UNSATISFIED Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Session state persistence needed in
|
AdcpStateStore interface with InMemoryStateStore (default) and PostgresStateStore (production) implementations. Handlers receive ctx.store for persisting media buys, accounts, creatives, etc. - Generic document store: get/put/delete/list by collection + id - InMemoryStateStore: nested Maps, filtering, pagination - PostgresStateStore: JSONB table, containment queries, keyset pagination - Shared across all domain handlers via ctx.store - getAdcpStateMigration() for postgres table setup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…mit cap - Fix InMemoryStateStore cursor tracking (was broken after filtering) - Add patch() to interface and both implementations (JSONB || merge for postgres) - get() returns a shallow copy (prevents mutation of store internals) - list() returns copies and caps limit at 500 - Validate cursor format in PostgresStateStore - Add tests for cursor roundtrip, patch, copy semantics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bokelley
left a comment
There was a problem hiding this comment.
Code Review: createAdcpServer
Overall design is solid — domain grouping is clean, handler signature (params, ctx) is simple, governance as composable helpers (not middleware) matches the design spec, and auto-generated capabilities from registered tools is the right pattern. Coherence warnings are a nice touch.
Must Fix
1. Four hasAccount flags are wrong — account resolution silently skipped
get_creative_delivery, get_signals, si_initiate_session, si_terminate_session are marked hasAccount: false in TOOL_META but their Zod schemas include account fields. si_initiate_session has account as required (not nullish). Account resolution and ACCOUNT_NOT_FOUND errors will never fire for these tools.
Better fix: derive hasAccount from the Zod schema at init time ('account' in schema.shape) instead of hand-coding flags. Eliminates this entire class of drift bug.
2. InMemoryStateStore cursor pagination broken with filters
state-store.ts cursor logic indexes into unfiltered Map keys using filtered array indices:
const cursorIndex = items.findIndex((_, i) => {
const keys = [...col.keys()]; // unfiltered keys
return keys[i] === options.cursor; // i is index in filtered items
});After filtering, items[i] no longer corresponds to col.keys()[i]. Will skip items or return duplicates. Also O(n²) from reconstructing [...col.keys()] on each iteration.
Fix: maintain (id, data) pairs through the filter/pagination pipeline.
Should Fix
3. Module JSDoc still shows resolveGovernance — the top-of-file example references a config property that doesn't exist. Contradicts the actual design (governance is composable helper, not middleware).
4. checkGovernance does unsafe cast — raw as CheckGovernanceResponse with no Zod validation. Malformed governance agent responses will produce confusing property-access errors instead of clear "invalid response" errors. Use CheckGovernanceResponseSchema.safeParse(raw).
5. TOOL_REQUEST_SCHEMAS and TOOL_META are two sources of truth for tool→schema mappings. Drift risk — if a tool is added to one but not the other, they'll silently disagree. Should be a single source.
6. Tests use server._registeredTools — MCP SDK private API. Will break if SDK changes internals. Isolate the access or use a public test transport.
7. No test for required account field — account resolution tests use get_products where account is optional. No test for tools where account is required (e.g., create_media_buy).
Consider
genericResponsealways returns "OK" — not helpful for agents. At minimum include the tool name.- Capabilities building uses
as anycasting extensively — won't catch type changes. governanceDeniedErroralso acceptsGovernanceConditions— name implies denial-only.- State store
listordering is undocumented (relies on Map insertion order).
Systemic concern
Items 1 and 5 are symptoms of the same problem: hand-coded metadata that should be derived from schemas. TOOL_META maintains hasAccount flags, annotations, and schema references that could be generated. Once #538 lands the full schema pipeline, these should come from a single generated source, not hand-maintained tables.
Updated 8 SKILL.md files, BUILD-AN-AGENT.md, and llms.txt to reference createAdcpServer with domain-grouped handlers, ctx.store for state persistence, and auto-applied response builders. - Seller, signals, creative, governance, SI, brand rights, retail media, generative seller skills all show createAdcpServer patterns - BUILD-AN-AGENT.md leads with createAdcpServer, demotes createTaskCapableServer to advanced/escape-hatch section - llms.txt Quick Start updated with createAdcpServer example - Common Mistakes tables updated across all skills - SDK Quick Reference tables updated across all skills Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
update_media_buy now has a required account field per adcp#2174. This enables automated account resolution on the most financially sensitive mutation tool. preview_creative flattening (adcp#2175) will be picked up when schemas are regenerated from the updated spec. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Already implemented in this PR!
Interface: // Dev (default — zero config)
createAdcpServer({ name: 'Agent', version: '1.0.0', mediaBuy: { ... } })
// Production
const stateStore = new PostgresStateStore(pool);
createAdcpServer({ name: 'Agent', version: '1.0.0', stateStore, mediaBuy: { ... } })Migration: The training agent can swap |
… JSDoc - hasAccount no longer hand-coded in TOOL_META — derived from Zod schema at init time via 'account' in schema.shape. Eliminates drift bugs (get_creative_delivery and get_signals were wrong). - checkGovernance validates required fields before casting response - Module JSDoc updated to remove resolveGovernance (now composable helper) - genericResponse includes tool name instead of "OK" - Add test for required account field on create_media_buy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… schemas Replace 40+ individual schema imports and hand-coded TOOL_META.schema with TOOL_REQUEST_SCHEMAS from #540. Schema and hasAccount are now derived from the canonical map at registration time. - Eliminates dual source of truth (review item #5) - hasAccount derived from schema ('account' in schema.shape) - Tools not in TOOL_META still register if present in TOOL_REQUEST_SCHEMAS - TOOL_META now only contains response builders and annotations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Handler wrapper auto-injects params.context into response data before
response builder wraps it. Handlers don't need to echo context manually.
- get_adcp_capabilities registered with actual schema (not empty {}) so
context from request is received and echoed.
- Storyboard results: 5/6 signal_marketplace steps pass (remaining failure
is signal_id null/undefined schema mismatch tracked in #542).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The #1 DX blocker: Zod schemas use .nullish() (null | undefined) but TypeScript types used ? (undefined only). Every handler echoing params.context hit type errors. Fix: generate-types.ts now adds | null to optional TypeScript properties, matching Zod .nullish() behavior. 25 internal call sites updated with ?? undefined to strip null where needed. Also fixes ZodIntersection schemas (get_signals, etc.) that lost .shape after regeneration — extractShape() traverses intersection _def.right. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Root cause fixes for the two schema generation issues: 1. **Nullish removed**: postProcessForNullish was converting every .optional() to .nullish(), making Zod produce null|undefined while TypeScript types only had undefined. Removed the post-processor — Zod schemas now use .optional() matching TypeScript. Also removed alignOptionalWithNullish (TypeScript workaround) and postProcessLazyTypeAnnotations (cascading nullish fix). 2. **Intersections fixed**: postProcessRecordIntersections already handles most cases. The 5 remaining schemas (get_signals, etc.) are now clean z.object() with .shape. Only comply_test_controller remains a union (by design — multiple scenario types). Reverted extractShape() workaround in createAdcpServer — schemas have .shape directly now. 0 nullish, 2877 optional. Types and schemas match. 2972 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GOVERNANCE_DENIED (adcp#2194) is specific to governance agent denials. COMPLIANCE_UNSATISFIED is broader (content standards, brand safety). governanceDeniedError() now uses the correct code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…pServer Pull in adcp#2194: GOVERNANCE_DENIED error code, flatten comply_test_controller, context/ext on all schemas, require signal_id. - preview_creative now flat z.object() — added to CreativeHandlers, TOOL_META, TOOL_REQUEST_SCHEMAS, and CREATIVE_ENTRIES - Removed old PreviewCreative variant exports (no longer needed) - Updated discriminated union tests for flat schema behavior - signal_id now required on signals (should fix storyboard provenance check) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
resolveAccount callback referenced ctx.store which is not in scope. Fixed to use stateStore (declared outside createAgent factory). Added InMemoryStateStore import and declaration to make example copy-paste correct. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
createAdcpServer— declarative server builder that wires domain-grouped handlers to input schemas, Zod validation, response builders, and auto-generated capabilitiescheckGovernance+governanceDeniedError— composable helpers for governance checks in financial handlers (create_media_buy, update_media_buy, activate_signal)resolveAccountmiddleware — auto-resolves AccountReference on every tool with anaccountfieldDesign decisions
update_media_buydoesn't have an account field. Sellers callcheckGovernance()explicitly — 3 visible lines beats invisible middleware.Related protocol issues
update_media_buyneeds requiredaccountfield for governance paritypreview_creativeunion schema should be flattened for server.tool() compatibilityTest plan
🤖 Generated with Claude Code