feat: support AdCP 3.1.0-rc.7#2167
Conversation
There was a problem hiding this comment.
LGTM. Clean rc.7 bump. Tenant isolation on the new task-status surface is fail-closed at the right seam, and AUTHORIZATION_REQUIRED detail sanitization routes the wire-strip through a named allowlist plus depth/size caps instead of inventing a new envelope.
Things I checked
ADCP_VERSION,package.jsonadcp_version,src/lib/version.tsADCP_VERSION, andscripts/sync-version.tsCOMPATIBLE_PREFIXall carry3.1.0-rc.7.COMPATIBLE_ADCP_VERSIONSlists both3.1.0-rc.7and the release-precision3.1-rc.7.- Changeset present (
.changeset/rc-seven-task-auth.md).patchis consistent with the project's beta-cycle convention on9.0.0-beta.*— flagged below in case the team wants to revisit. - Tenant boundary on
get_task_status(src/lib/server/create-adcp-server.ts:5416+) is fail-closed:taskId && accountId !== undefined && taskRegistry !== undefined ? await taskRegistry.getTask(taskId) : null— no resolvedaccountId→ null → uniformREFERENCE_NOT_FOUND. The subsequenttask.accountId !== accountIdonly runs whenaccountIdis defined, so theundefined !== undefinedtruthiness trap doesn't apply.list_tasksis similarly gated (accountId !== undefined && taskRegistry?.list !== undefined ? ... : { tasks: [] }). sanitizeAdcpErrorDetailsinsrc/lib/server/errors.tsis allowlist-projection (AUTHORIZATION_REQUIRED_DETAIL_KEYS, 18 entries), routed throughpickSafeDetailswithmaxDepth: 4/maxSizeBytes: 4096. The keys cover the spec-named recovery hints (missing_connections,authorization_url,reference_authorization, etc.) without inviting tokens or internal bookkeeping through.applyAdcpErrorAllowlistruns sanitization before the per-code field filter, so the sanitization is unconditional on everyAUTHORIZATION_REQUIREDenvelope.- Payload-side sanitization is wired symmetrically:
wrapErrorArm(handler-returned error arms) andenrichErrorTwoLayer(framework-synthesized two-layer envelopes) both runsanitizePayloadErroragainstsc.errors[*], and the latter callssyncContentJsonTextafter sanitization so thecontent[0].textmirror stays consistent withstructuredContent. creative-transformersadded toschemas/registry/registry.yamlenum ANDCREATIVE-domain specialisms increateAdcpServerAND theCREATIVE_ENTRIEShandler-entry list. No drift.- New error codes in
docs/llms.txt(AUTHORIZATION_REQUIRED,BUDGET_CAP_REACHED,CATALOG_LIMIT_EXCEEDED,CREATIVE_INACCESSIBLE,EVALUATOR_AGENT_NOT_ACCEPTED,FEED_FETCH_FAILED,INVALID_FEED_FORMAT,ITEM_VALIDATION_FAILED,SIGNAL_TARGETING_INCOMPATIBLE,UNPRICEABLE_OUTPUT) tracked in the registry table —buyer-retry-policy.tsretry coverage updated for the new codes per the PR summary. - New test files cover the two security-critical changes:
test/server-authorization-required-sanitization.test.js(144 LoC, new) exercises the sanitizer;test/server-account-store-ctx-forwarding.test.js(+184) covers the account-store ctx forwarding tied to the authorization metadata projection.
Follow-ups (non-blocking — file as issues)
-
domainvsprotocolconsistency betweenlist_tasksandget_task_status.toProtocolTaskStatusreports the full mapped protocol (media-buy,creative,signals,measurement,brand,governance) viaTASK_TYPE_TO_PROTOCOL[taskType].toProtocolTaskListItemcollapses everything exceptsignalstomedia-buy:domain: protocol === 'signals' ? 'signals' : 'media-buy'. Async_creativestask would render asdomain: 'media-buy'inlist_tasksandprotocol: 'creative'inget_task_status. If the rc.7ListTasksResponse.tasks[].domainenum is narrower thanAdCPProtocolby design, this is correct and worth a code comment naming that. If not, the list view is lossy. Verify against the rc.7list_tasksschema. -
Changeset type vs surface area. The diff adds three new public
Agentmethods (listTransformers,getTaskStatus,listTasks) and four new typed-handler keys onCreativeHandlers/ the framework. On a stable release that's aminor; in9.0.0-beta.22the pre-release tag absorbs the precision sopatchis harmless. Worth a project-level convention note (in CLAUDE.md or the changeset README) so future bumps don't accidentally drop aminorinto a stable release. -
mcpTaskStore = taskStore ?? new InMemoryTaskStore()increateAdcpServer. PreviouslytaskStorepassed through asundefinedandcreateTaskCapableServerowned the default. Now the outer scope always materializes anInMemoryTaskStorewhen none is supplied, which is also what's referenced for task lookups. Behavior change for adopters who relied on the inner default; align the JSDoc onAdcpServerConfig.taskStoreto call this out. -
ListTasksResponse.status: 'completed'at the envelope top. Inherited from the async-response base, butlist_tasksis a synchronous read —'completed'on a sync read reads odd. If the schema makes it optional, drop it; if required, add a code comment naming the base-type contract so the next reader doesn't grep for the task lifecycle that doesn't exist.
Minor nits (non-blocking)
taskMatchesFiltersbuilds the full haystack forcontext_containson every filtered task.JSON.stringify({ task_id, task_type, result, error, progress })per call. Adopters with largetask.resultpayloads and many tasks may notice. Not a problem at the current defaultpageSize: 50ceiling, but if the registry grows, this is the cost site. File as a perf follow-up if list_tasks ever returns paginated counts on large fleets.
gh api rate budget was exhausted partway through, so I did not run the subagent stack (code-reviewer, ad-tech-protocol-expert, security-reviewer) — reviewed from the diff and the on-PR file contents directly. Approving on the strength of the fail-closed account gating, the allowlist-driven sanitization, and the symmetric payload/envelope coverage in wrapErrorArm + enrichErrorTwoLayer. Worth a second pass from the protocol-expert and security-reviewer once the API quota recovers, especially on the domain-flattening question.
Safe to merge.
There was a problem hiding this comment.
LGTM. Follow-ups below. Clean rc.7 sync — version pin, generated types, account-authorization projection with a compile-time exhaustion guard, AUTHORIZATION_REQUIRED.details sanitizer layered before the per-code allowlist, and the new framework-owned get_task_status / list_tasks wired to the scoped registry rather than the raw MCP task store. The cross-tenant guard at src/lib/server/create-adcp-server.ts (task.accountId !== accountId) is the right shape — fail-closed on accountId === undefined.
Things I checked
- Changeset present at
.changeset/rc-seven-task-auth.md(patch) — correct for a pre-release additive schema bump in changesets prerelease mode. ADCP_VERSION/package.json#adcp_version/src/lib/version.tsall move3.1.0-rc.6→3.1.0-rc.7in lockstep;COMPATIBLE_ADCP_VERSIONSextends additively, preserving every prior pin.ACCOUNT_AUTHORIZATION_WIRE_KEYS(allowed_tasks, field_scopes, scope_name, read_only)exact-matchesAccountAuthorizationSchema.shapevia the round-trip assertion attest/server-decisioning-to-wire-account.test.js:22;MissingAccountAuthorizationWireKey = Exclude<keyof AccountAuthorization, …>plusRecord<…, never>fails compile if the generated type widens. Both guards intact.projectAccountAuthorizationstrips adopter-suppliedaccess_token/refresh_token/internal_role_bindingsbeforetoWireAccount/toWireSyncAccountRow— confirmed by the new test cases attest/server-decisioning-to-wire-account.test.js.AUTHORIZATION_REQUIRED.detailssanitization runs insideapplyAdcpErrorAllowlistbefore the per-code allowlist step, so every call site (adcpError(),wrapErrorArm,enrichErrorTwoLayerviasanitizePayloadErrors, thecontent[0].textJSON fallback viasyncContentJsonText) filters once. Idempotent across passes —pickSafeDetailson already-filtered objects is a no-op; the equality check insanitizePayloadErrorsskips the L2 resync.get_task_status/list_tasksregister on the AdCP protocol namespace, distinct from MCPtasks/get—test/server-create-adcp-server.test.js:1437proves raw MCP task-store records don't leak through.- Cross-tenant assertions at
test/server-create-adcp-server.test.js(crossTenant) andtest/server-decisioning-from-platform.test.js(acc_attacker) — both exercise thetask.accountId !== accountIdguard explicitly and assertREFERENCE_NOT_FOUNDrather than leaking. parseTaskListCursor/^(0|[1-9]\d*)$/rejects negative/scientific/float/leading-zero cursors;INVALID_PARAMSreturned beforeArray.slice. Solid.creative-transformersspecialism legitimately added toschemas/registry/registry.yaml:1629,AdCPSpecialismValues,SPECIALISM_REQUIREMENTS, theRequiredPlatformsForconditional, and the creative-domain specialisms array — wire is consistent.list_tasks.tasks[].domain: 'media-buy' | 'signals'collapse intoProtocolTaskListItemis spec-conformant — the rc.7 wire shape pins that field to the two-value literal (src/lib/types/tools.generated.tsListTasksResponse), whileget_task_status.protocolkeeps the full 7-value enum. Asymmetry is upstream-shape, not SDK invention.
Follow-ups (non-blocking — file as issues)
-
Top-level
errors[i]fields are not allowlisted forAUTHORIZATION_REQUIRED. The sanitizer coversdetailsonly. An adopter handler that returns{ errors: [{ code: 'AUTHORIZATION_REQUIRED', message: '…', refresh_token: 'rt_…', upstream_request_id: 'tenant-acme-…' }] }ships the top-level extras to the wire viastructuredContent.errors[0]and thecontent[0].textfallback. Path B'sprojectPayloadErrorToEnvelopekeeps the envelope clean, but the payloaderrors[]is not. Either registerAUTHORIZATION_REQUIREDinADCP_ERROR_FIELD_ALLOWLISTwith the canonicalPAYLOAD_ERROR_FIELDSset or filter top-level keys inapplyAdcpErrorAllowlistfor any recovery-detail code. Add a test intest/server-authorization-required-sanitization.test.jswith a top-levelrefresh_tokento lock the contract. The new sanitization is defense-in-depth, not a complete top-level fix — worth tightening before adopters lean on it in production. -
TASK_TYPE_TO_PROTOCOLmapping drift vs the SDK's capability-tools grouping.sync_event_sources,sync_audiences,sync_catalogs,log_eventget'measurement'insrc/lib/server/create-adcp-server.ts:~923;src/lib/utils/capabilities.ts:237-247and:306group them underMEDIA_BUY_TOOLS/EVENT_TRACKING_TOOLSwith['media_buy', 'conversion_tracking']capability buckets. The rc.7AdCPProtocolenum legitimately includes'measurement', so the choice is defensible — but the two source-of-truth tables disagree, and the protocol-namespacing decision deserves an upstream cross-check. Same forsync_accounts/get_account_financials→'media-buy'(no'account'value in the enum, so it's a forced choice). Document the chosen mapping next to the table, or thread the manifest's protocol tag through instead of hardcoding. -
creative-transformerslacks adopter prose. The wire types, registry enum, and platform binding all land cleanly, butskills/build-creative-agent/SKILL.mdhas nocreative-transformers§ and the CLAUDE.md specialism index isn't extended. Adopters who claim the specialism viaget_adcp_capabilitieshave nothing to read. Fold the transformer story intobuild-creative-agent(per theRequiredPlatformsForshape —CreativeBuilderPlatformcovers both) and add the row to the CLAUDE.md table. -
taskRegistry.listordering contract is backend-dependent. Postgres returnsORDER BY created_at DESC, task_id DESC(src/lib/server/decisioning/runtime/postgres-task-registry.ts:284); the in-memory impl returnsArray.from(tasks.values()).filter(…)unsorted (task-registry.ts:197). Thelist_taskshandler re-sorts viacompareProtocolTaskItems, so end users see consistent pagination — but adopters callingregistry.list()directly will see different orderings in tests vs prod. Either sort the in-memory impl on the same key or document explicitly thatlist()returns unsorted records and callers must sort. -
mcpTaskStore = taskStore ?? new InMemoryTaskStore()is a silent behavior shift. Adopters who previously lefttaskStoreunset gotundefinedpropagated intocreateTaskCapableServer; now an in-memory store is always materialized, which means every server advertises MCP task support and the store grows unbounded for adopters who never intended async tasks. Worth a one-line callout in the changeset and either ataskStore: nullopt-out or a gate on whether a task tool actually needs it.
Minor nits (non-blocking)
-
pickSafeDetails(maxDepth: 4, maxSizeBytes: 4096)doubles the defaults (2 / 2048) atsrc/lib/server/errors.ts:217-220. Plausibly justified by the rc.7missing_connections[].resource_refnesting + free-form URLs/instructions, but a one-line comment naming the justification would stop the next person widening it on reflex. -
addCanonicalToolTypeAliasessilently skips on ambiguous candidates atscripts/generate-types.ts:1407-1415. Whencandidates.length !== 1, the alias just doesn't emit. Log a warning on> 1so a future schema change that introduces a second domain-qualified variant (e.g.GetTaskStatusProtocolRequest+GetTaskStatusMediaBuyRequest) doesn't quietly stop emitting the canonical name. -
FEED_FETCH_FAILEDandINVALID_FEED_FORMATretry policies atsrc/lib/utils/buyer-retry-policy.ts:336-337aremutate-and-retrywithreason: 'validation'. Feed-fetch is upstream-transport; resubmitting the same URL without mutation won't help.INVALID_FEED_FORMATis a fixed defect, retrying won't help either. Eitherretry(transient backoff) for fetch orescalatefor both — match the spec's recovery semantics. -
completed_at = task.updatedAtintoProtocolTaskStatus/toProtocolTaskListItemis a synthesis, not a registry-tracked value. Fine if the registry contract guarantees no further mutation after terminal status, but if the rc.7 spec intendscompleted_atto be the exact terminal-transition timestamp, consider tracking a discretecompletedAtonTaskRecordrather than reusingupdatedAt. Notable.
Safe to merge.
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Allowlist projection on AccountAuthorization, depth-and-size-capped AUTHORIZATION_REQUIRED.details sanitization, and account-scoped task-registry guards are the right shape — fail-closed at every seam, and the test at test/server-authorization-required-sanitization.test.js:139 asserts no secret string crosses to structuredContent.
Things I checked
- Changeset / version mechanics.
.changeset/rc-seven-task-auth.mdispatchagainst9.0.0-beta.22.package.jsonversionis untouched (line 4); onlyadcp_version: "3.1.0-rc.7"(line 463) and thesync-schemas:allscript flip.LIBRARY_VERSIONinsrc/lib/version.ts:9is unchanged. The bumps are confined toADCP_VERSIONandCOMPATIBLE_ADCP_VERSIONS. Right shape. - Protocol coherence (rc.7).
ad-tech-protocol-expert: sound. The 22-valueTaskTypeenum is fully covered byADCP_TASK_TYPESandTASK_TYPE_TO_PROTOCOLinsrc/lib/server/create-adcp-server.ts:2148-2199— the?? 'media-buy'fallback never fires for in-scope tasks. Thedomain: 'media-buy' | 'signals'collapse onListTasksResponse.tasks[].domainis the spec's restricted enum attools.generated.ts:24210, not a SDK truncation.protocolcarries the full enum on the siblingGetTaskStatusResponse.creative-transformersis inAdCPSpecialismValues; all 10 new error codes are inErrorCodeValues. - Account-scoped task reads.
if (task == null || task.accountId !== accountId)in theget_task_statushandler. Pre-check refuses lookup whenaccountId === undefined, so theundefined !== undefined === falsecollision path is unreachable.list_tasksreturns{ tasks: [] }whenaccountIdis missing ortaskRegistry.listis unwired. Both fail closed. AUTHORIZATION_REQUIREDsanitization coverage.sanitizePayloadErrorsruns inenrichErrorTwoLayerbefore Path A/B/C branching (create-adcp-server.ts:2604).wrapErrorArmsanitizes the typederrors[]arm before envelope synthesis.authorizationRequiredEscalationContextinbuyer-retry-policy.ts:382runsapplyAdcpErrorAllowlistfirst, then extracts convenience fields — order is right.pickSafeDetailscaps.maxDepth: 4coversdetails.missing_connections[].resource_ref.identity_id(depth-3 leaf).maxSizeBytes: 4096is reasonable for an auth-handoff envelope and fails closed (detailsomitted entirely) when exceeded.- SQL safety.
${table}interpolation inpostgres-task-registry.ts:276is gated by the constructor's identifier check;account_idis parameterized via$1. - Custom transport / mock data: none. All new handlers go through
server.registerToolandgenericResponse.
Follow-ups (non-blocking — file as issues)
taskListPageSizeNaN handling.src/lib/server/create-adcp-server.ts:2342—NaNpasses thetypeof === 'number'guard, thenMath.max(NaN, 1)returnsNaN, propagating throughMath.min/Math.floor. Result:tasks.slice(0, NaN)yields[]andpagination.has_moreis misreported. AddNumber.isFinite(pagination.max_results)to the guard.ACCOUNT_AUTHORIZATION_WIRE_KEYSdoesn't drive the projector.src/lib/server/decisioning/account.ts:43defines thesatisfies-checked constant;projectAccountAuthorizationat line 894 hard-codes the same four fields by hand. If the spec adds a fifthAccountAuthorizationfield, the compile-timeExcludecheck fires, the dev updates the constant — but the projector keeps dropping the new field silently. Iterate the constant in the projector, or rename it to reflect documentation-only intent.- Redundant
InMemoryTaskStoreconstruction.create-adcp-server.ts:3697—taskStore ?? new InMemoryTaskStore()is double work;createTaskCapableServeralready applies the same fallback attasks.ts. Drop the local??and passtaskStorethrough. ADCP_ERROR_FIELD_ALLOWLIST.AUTHORIZATION_REQUIREDis unset.src/lib/server/envelope-allowlist.tshas no entry for the code.applyAdcpErrorAllowlistsanitizesdetailsbut returns the rest of the payload unchanged. The builder is safe because it accepts only known keys, butsanitizePayloadErrorruns on hand-rolled typederrors[]arms — a handler that returnserrors: [{code: 'AUTHORIZATION_REQUIRED', tenant_id, refresh_token, details: {...}}]will getdetailssanitized but the top-level secrets pass through. Add the entry; add a top-level secret-key test.- ISO-8601 lexical comparison on buyer-supplied bounds.
create-adcp-server.ts:2206-2210—localeCompare/<=oncreated_after/updated_beforeis sound forZ-normalized records but breaks if a buyer sends+02:00-offset bounds. Normalize viaDate.parsebefore comparison. field_scopeskeys forwarded verbatim.decisioning/account.ts:891-895allowlists values (must bestring[]) but not key names — an adopter who emitsinternal_admin_override: ['*']ships that key on the wire. Constrain to canonical AdCP task names.
Minor nits (non-blocking)
compareProtocolTaskItemstiebreak.create-adcp-server.ts:2271—localeCompareis spec-permitted to return any integer; theresult === 0check is correct only by coincidence on V8. Today's behavior is fine; worth a one-line comment.
The third drift-cleanup commit message on this branch ("fix request/response schema registration, creative-transformer specialism wiring, protocol capabilities drift") is the sort of thing that argues for a registry-driven generation pass on the next rc bump — a follow-up topic, not this PR.
Approving on the strength of the sanitizer coverage at every emit point plus the protocol-expert read on rc.7 schema alignment.
| import type { ResolvedAuthInfo } from './decisioning/account'; | ||
| import type { TaskRegistry } from './decisioning/runtime/task-registry'; | ||
| import type { TaskRecord, TaskRegistry } from './decisioning/runtime/task-registry'; | ||
| import { protocolForTool } from './decisioning/runtime/protocol-for-tool'; |
There was a problem hiding this comment.
Approving. The rc.7 sync is mechanically right against the generated schemas, and the security work around AUTHORIZATION_REQUIRED sanitization plus ownerScope isolation is the actual deliverable hiding behind the version bump.
Things I checked
- Version gate is correct.
supportsProtocolTaskTools = protocolBundleKey === '3.1.0-rc.7' || protocolBundleKey === '3.1-rc.7'(src/lib/server/create-adcp-server.ts:3478) covers both full-semver and release-precision outputs ofresolveBundleKey. Both forms reach this code in production. The 3.0.12-pin test pins this down. ADCP_TASK_TYPEScovers the rc.7 enum. All 22 values inTaskTypeSchema(schemas.generated.ts:1522) are in the Set; nothing dropped silently fromlist_tasks.list_tasks.tasks[].domaincollapse is spec-compliant, not a fidelity loss.ListTasksResponseSchema.tasks[].domainin rc.7 is literallymedia-buy | signals(schemas.generated.ts:4258). Theprotocol === 'signals' ? 'signals' : 'media-buy'projection atcreate-adcp-server.ts:2244is what the spec asks for.AccountAuthorizationprojection is exact.AccountAuthorizationSchemais exactly{allowed_tasks, field_scopes, scope_name, read_only}. The[...ACCOUNT_AUTHORIZATION_WIRE_KEYS].sort() === Object.keys(AccountAuthorizationSchema.shape).sort()test (test/server-decisioning-to-wire-account.test.js:22) is the right drift guard.taskOwnerScopeForContextandtaskOwnerScopeForagree byte-for-byte. Same priority ladder (session → agent_url → http_sig → oauth → api_key → clientId → account) in bothcreate-adcp-server.ts:2329andfrom-platform.ts:2695. No cross-scope read possible today.- Cross-tenant isolation holds. Test
does not leak tasks between credentials that resolve to the same account(create-adcp-server test:14998) confirms anapi_key:buyer-2caller can't see aapi_key:buyer-1task on the same shared account. - Validation order on the new tools fails closed. Credential-policy → request validation → bounds → version → account resolution runs before any registry call (
create-adcp-server.ts:5786-5800). No unsanitized payload reachestaskRegistry.getTask/list. pickSafeDetailsrecursesresource_refwith the same allowlist. Theprivate_notestrip in the sanitization test is real —filterByAllowlistdrops any key not in the 19-key set at every depth ≤4.'Task failed'fallback atfrom-platform.ts:2950is deliberate hardening. PreventstaskFnError.message(upstream HTTP bodies, DB error strings, stack traces) from being persisted into task error envelopes when the throw isn't anAdcpError.
Follow-ups (non-blocking — file as issues)
- Changeset understates the behavior changes. The prose calls out
ownerScopepersistence. It does more than that: (a)tasks_getregistry-resolution failures now returnSERVICE_UNAVAILABLEinstead of warn-and-fall-through (from-platform.ts:2057-2061); (b) malformed handler-returnederrors[]arms now fail closed asVALIDATION_ERRORinstead of warn-and-wrap (create-adcp-server.ts:4907-4933); (c)sanitizePayloadErrorprojects standard-code envelopes through an allowlist, dropping adopter-supplied top-level fields (create-adcp-server.ts:2677). All three are directionally right; adopters who relied on either pre-upgrade surface deserve a paragraph in the changeset. - Legacy NULL
owner_scoperows + shared-account pre-upgrade tenants.postgres-task-registry.ts:264exposes pre-upgrade rows to any caller whose post-upgrade scope resolves toaccount:${accountId}. The changeset disclaims this. For deployments that previously multiplexed principals through a shared bearer with noclientIdattribution, a backfill migration (owner_scope = 'legacy:pre-3.1', treated as non-listable) would close the window. Worth a note indocs/migration-*.mdfor shared-account adopters. taskOwnerScopeForContextandtaskOwnerScopeForare byte-identical twins. They agree today. Extract to a shared util insrc/lib/server/decisioning/runtime/task-ownership.tsand import from both call sites — drift between these would silently flip cross-tenant isolation.projectAccountAuthorizationis shallow.[...authorization.allowed_tasks]/[...fields](account.ts:889) trust the TypeScript element type. An adopter mistakenly populatingallowed_tasks: [{token: '...'}]would echo the object untouched. Filter each array totypeof item === 'string'.accountResolutionAttemptedis plumbed but never destructured. Five return branches inresolveTaskQueryAccountIdcompute the flag; the two call sites only destructure{accountId, ownerScope, error}(create-adcp-server.ts:5799, 5874). Drop the field from the return type or wire it into dispatcher diagnostics.- Verify
error.detailsflatten against the rc.7 spec.from-platform.ts:2213spreads{recovery, field, suggestion, retry_after}and adopterdetailsinto the top-levelerror.details. Inline comment claims rc.7 conformance; worth confirming againstadcontextprotocol/adcp@v3.1.0-rc.7:tasks-get-response.jsonbefore GA. mcpTaskStorepre-default is a no-op.createTaskCapableServeralready doesoptions?.taskStore ?? new _InMemoryTaskStore()(src/lib/server/tasks.ts:143). The new local binding atcreate-adcp-server.ts:3777is observably equivalent — fold back totaskStore.
Minor nits (non-blocking)
taskTimestampMatchesis half-open.create-adcp-server.ts:2314uses strict>/<on both*_afterand*_before. Spec convention is typically inclusive (>=/<=) — confirm against rc.7list-tasks-request.jsonor drop a one-line comment justifying half-open.compareProtocolTaskItemssilently treats unknownsort.fieldascreated_at.create-adcp-server.ts:2350. Request-schema validation catches this when on, but defaults to'off'in production. Alogger.warnwould surface drift in adopter logs.
The changeset is minor, which matches the wire impact (additive tools, new optional fields, new error codes). No breaking-shape changes on stable surfaces.
Ship it once you've extended the changeset prose to cover the three behavior changes in follow-up #1 — that paragraph is the only thing standing between this PR and a quiet upgrade for adopters who haven't been reading line diffs.
There was a problem hiding this comment.
LGTM. Tight schema bump with the right shape: AdCP ownerScope lives on the application-layer registry (not the MCP transport TaskStore), the AccountAuthorization projection has a compile-time coverage check, AUTHORIZATION_REQUIRED.details runs through a key-allowlist + size/depth budget, and the dispatcher now fails closed on malformed Error arms instead of warn-and-passthrough.
Things I checked
- Version gate.
src/lib/version.tsalready at3.1.0-rc.7with'3.1.0-rc.7'inCOMPATIBLE_ADCP_VERSIONS;scripts/sync-version.tsextended; no drift against rootADCP_VERSION. Premise thatversion.tswas un-regenerated is wrong — sync ran. - Type coverage.
ADCP_TASK_TYPES(create-adcp-server.ts:2156) matches the rc.7TaskTypeenum 1:1.MUTATING_TASKSauto-derives from requiredidempotency_keyfields so the three new RO tools correctly omit themselves andupdate_rightscorrectly lands. AccountAuthorizationprojection.ACCOUNT_AUTHORIZATION_WIRE_KEYS+MissingAccountAuthorizationWireKey: Record<…, never>(src/lib/server/decisioning/account.ts:36-46) catches drift at compile time. Allowlist is exactly the four rc.7 wire fields;field_scopesper-entryArray.isArrayfilter drops shape-shifted adopter values.- Owner-scope ladder fails closed.
taskBelongsToCallerreturns false whenownerScope === undefined; in-memorylist()returns{ tasks: [] }on undefined scope; Postgreslist()mirrors. Legacy ownerless rows readable only via theaccount:${accountId}fallback owner scope, exactly as the changeset documents. - Dispatcher fail-closed.
create-adcp-server.ts:4901-4934— malformed Error arms now emit a hardcodedVALIDATION_ERRORpayload; the malformedresult.errorsis never re-serialized into the response. No leak path. - MCP wiring. Both new tools register with
PASSTHROUGH_INPUT_SCHEMA, omitoutputSchema(per the existing banner at L103-115 about the SDK client-side validator bug), and route throughfinalizeProtocolTaskToolResponse. Coexists with thetasks_getMCP-safe alias without collision. - Protocol coherence.
list_tasks.tasks[].domaincorrectly narrows to'media-buy' | 'signals'per the rc.7 schema'squery_summary.domain_breakdownshape (governance/creative/brand collapse to media-buy is spec-mandated, not SDK choice). AUTHORIZATION_REQUIREDsanitizer. 19-key allowlist +pickSafeDetails(maxDepth: 4, maxSizeBytes: 4096); reused verbatim inbuyer-retry-policy.tsauthorizationRequiredEscalationContextso the UX-handoff path applies the same filter.- No SQL/replay/idempotency regressions. Postgres
listis parameterized; HTTP-sigagent_urlserver-derived from JWKS; no manualpackage.jsonversion edit; changeset present withminorbump (correct — additive + behavior-change call-out for custom registries).
Follow-ups (non-blocking — file as issues)
protocol-for-tool.tsmissingupdate_rightsandlist_transformers.src/lib/server/decisioning/runtime/protocol-for-tool.ts:68-89. This PR addssearch_brands: 'brand'but skipsupdate_rights: 'brand'andlist_transformers: 'creative'. Both fall through to the'media-buy'default inprotocolForTool().update_rightsis a brand-domain mutating tool added toMUTATING_TASKSandBRAND_RIGHTS_TOOLSin this PR — its lifecycle webhook payload'sprotocolfield will read'media-buy', silently misrouting brand-rights subscribers that filter by protocol. Two-line fix.supportsProtocolTaskToolshard-equality version gate.create-adcp-server.ts:3482—protocolBundleKey === '3.1.0-rc.7' || protocolBundleKey === '3.1-rc.7'. Silently disablesget_task_status/list_tasksregistration on rc.8 / GA / any later pin. Replace withcompareAdcpVersion(protocolBundleKey, '3.1.0-rc.7') >= 0so future bumps inherit the surface.AUTHORIZATION_REQUIREDnot inADCP_ERROR_FIELD_ALLOWLIST.src/lib/server/envelope-allowlist.ts—sanitizeAdcpErrorDetailsfilterspayload.details, butapplyAdcpErrorAllowlistthen has no entry forAUTHORIZATION_REQUIREDand lets every top-level field pass through. Hand-rolled adopter envelopes ({ code: 'AUTHORIZATION_REQUIRED', refresh_token: '…', tenant_id: '…' }) bypass the dispatcher's defense-in-depth re-sanitizer. Register the standard{ code, message, recovery, field, suggestion, retry_after, issues, details }allowlist so the doctring's intent matches the code.tasks_getownership skip when bothresolvedAccountIdandrecord.accountIdare falsy.src/lib/server/decisioning/runtime/from-platform.ts:2148-2179— the two boundary checks gate on truthiness; if a customTaskRegistryever returnsaccountId: '', ownership skips. Built-in registries (in-memory + PostgresNOT NULL) can't hit this, but the defense-in-depth fix isif (resolvedAccountId === undefined) return REFERENCE_NOT_FOUNDregardless of the task's recorded accountId.- Owner-scope
clientId === 'unknown'collapse.create-adcp-server.ts:2341-2342andfrom-platform.ts:2702-2706.auth.ts:511setsauthInfo.clientId = 'unknown'when neitherclient_idnorsubis extractable from an OAuth token. Non-empty string → derivesclient:unknownfor every such caller. Multiple distinct buyers whose JWTs lack both fields collapse onto the same owner scope. Treat the'unknown'sentinel as missing in the owner-scope derivation, or dropauthInfo.clientIdwhenprincipal === 'unknown'. - Duplicated owner-scope helpers.
taskOwnerScopeForContext(create-adcp-server.ts:2287-2301) andtaskOwnerScopeFor(from-platform.ts:2692-2706) are byte-identical in logic and priority. Currently in sync; future edits will silently desync and the symptom is buyers 404-ing on their own tasks. Centralize into one export intask-registry.ts. - Changeset undersells two behavior changes.
.changeset/rc-seven-task-auth.mdcovers theownerScopemigration but skips: (a)tasks_get/get_task_status/list_tasksnow surfaceSERVICE_UNAVAILABLEon buyer-agent-registry resolution failures (was: swallow + fall through), (b) dispatcher returnsVALIDATION_ERRORfor handler-returned Error arms with raw-string / missing-code-or-messageitems (was: warn + passthrough). Both are correct improvements; adopters running with raw-string error returns or relying on the silent-degrade poll path need the heads-up. include_history: true → INVALID_REQUEST.create-adcp-server.ts:3953— spec markshistoryoptional on the response andinclude_historyfreely settable on the request. Buyers passing it defensively to "ask politely" get a hard fail. Silent no-op (omit history) is the buyer-friendly read.context_containshaystack overreach.create-adcp-server.ts:2272-2275— seed array includesitem.task_type,item.status,item.domain, socontext_contains: 'completed'matches every completed task. Spec text frames the field as identifier search; drop the non-id fields from the seed and lean on the existing(_)?(id|ids|ref|refs)$walker.measurementprotocol gating.create-adcp-server.ts:6143-6152auto-adds'measurement'tosupported_protocolsonly whenexperimental_featuresincludes'measurement.core'. Worth checking whetherschemas/cache/3.1.0-rc.7/enums/adcp-protocol.jsonactually marks themeasurementmemberx-status: experimental. If stable, drop the experimental gate.- Postgres
ALTER TABLEruns every boot.postgres-task-registry.tsschema migration around L175 —ADD COLUMN IF NOT EXISTS owner_scoperuns insideensureSchemaon every server start. Fine for Postgres ≥9.6; one-line comment justifying the runtime migration vs a one-shot script would prevent the next reviewer from flagging it as a smell.
Minor nits (non-blocking)
- Dead
accountResolutionAttemptedfield.create-adcp-server.tsresolveTaskQueryAccountIdreturns it; neitherget_task_statusnorlist_tasksdestructures it. Either wire it into unauthenticated-vs-anonymous diagnostics or drop it. PROTOCOL_TASK_TYPE_TO_PROTOCOLtypedPartial<Record<TaskType, ...>>.create-adcp-server.ts:2181. A futureTaskTypeaddition silently maps to the?? 'media-buy'fallback. Tighten to totalRecord<TaskType, WireAdcpProtocol>so the next rc fails the build instead.sanitizePayloadErroroverwrites seller-suppliedrecovery.create-adcp-server.ts:2680— for standard codes the function forcesrecovery = STANDARD_ERROR_CODES[code].recovery. Adopters who legitimately mark a standard-code error asrecovery: 'transient'will see it rewritten. Skip the rewrite when the seller's value is a validErrorRecoverymember.extra.authInfo.credentialin-place hoist.create-adcp-server.ts:4049-4051andfrom-platform.ts:2014-2016. The shared-session sticky-credential mutation is intentional (idempotent guard, sticks for downstream tools in the same session), but it reads as a clone-bug at first glance. One-line comment at both sites prevents a drive-by "should be a clone" PR.
Safe to merge.
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
There was a problem hiding this comment.
LGTM. Real hardening — owner-scoped task polling, AUTHORIZATION_REQUIRED detail allowlist, fail-closed on malformed errors[] arms — backed by tests that actually exercise cross-tenant isolation (test/server-create-adcp-server.test.js:14843-14900 runs two distinct api_key: buyers against the same registry and asserts the second buyer gets REFERENCE_NOT_FOUND on the first's task). Follow-ups below; none block.
Triage: source-code PR → ran code-reviewer + ad-tech-protocol-expert + security-reviewer in parallel. No expert returned a High / Critical / unsound verdict.
Things I checked
- Largest-file rule: read
src/lib/server/create-adcp-server.ts(+895/-16) andsrc/lib/server/decisioning/runtime/from-platform.ts(+186/-27) end-to-end.taskBelongsToCaller(create-adcp-server.ts:2316-2321) enforces bothaccountIdmatch ANDownerScopematch, with the documented legacy-row fallback (record.ownerScope === undefined → expectedOwnerScope === 'account:${accountId}'). Same logic infrom-platform.ts:2162-2179fortasks_get. Read/write halves are byte-identical — see follow-ups. - Changeset audit:
.changeset/rc-seven-task-auth.mdexists, typedminor. SDK is on9.0.0-beta.22so prerelease semver applies. Narrative is honest about the three behavior reversals (registry-resolution → SERVICE_UNAVAILABLE, malformederrors[]→ VALIDATION_ERROR, allowlist projection of standard-code envelopes). On a GA train I'd push formajor; on a beta train with this much disclosure, fine. - Protocol coherence:
ad-tech-protocol-expertreturned sound-with-caveats. rc.7 tool wiring matches generatedPROTOCOL_TOOLSandWIRE_SPEC_FIELDS.AccountAuthorizationprojection allowlist (allowed_tasks,field_scopes,scope_name,read_only) matches the rc.7 schema. Nine new standard codes (BUDGET_CAP_REACHED, CREATIVE_INACCESSIBLE, EVALUATOR_AGENT_NOT_ACCEPTED, UNPRICEABLE_OUTPUT, SIGNAL_TARGETING_INCOMPATIBLE, FEED_FETCH_FAILED, INVALID_FEED_FORMAT, ITEM_VALIDATION_FAILED, CATALOG_LIMIT_EXCEEDED) land inenums.generated.tswith consistent recovery classifications. - Security:
security-reviewerreturned Medium/Low only — no High. Real cross-tenant isolation tests attest/server-create-adcp-server.test.js:14843-14900.AUTHORIZATION_REQUIREDsanitization test (test/server-authorization-required-sanitization.test.js) assertsaccess_token,refresh_token,tenant_id,private_noteare dropped at every nesting level, with a finaldoesNotMatch(/secret_should_not_cross_wire|internal_tenant_1/)belt-and-suspenders check. - Witness invariant:
toProtocolTaskStatus/toProtocolTaskListItem/projectAccountAuthorizationfaithfully map registry/adapter state without fabricated upstream fields. The synthesizedcompleted_at: task.updatedAtand the synthesized error shape fromstatusMessageare framework-owned fields, not buyer-supplied — acceptable, but see follow-up onTASK_STATUS_MESSAGE. - Test-plan honesty: every checklist item in the PR body's Validation section is marked done. No unchecked manual-verify path for the primary user-facing change.
- Postgres migration:
ADD COLUMN IF NOT EXISTS owner_scope TEXTis metadata-only (no rewrite). Follow-up below on the non-concurrent index.
Follow-ups (non-blocking — file as issues)
- Sanitize
detailson more than justAUTHORIZATION_REQUIRED.errors.ts:217gates the sanitizer oncode === 'AUTHORIZATION_REQUIRED'.code-reviewerandsecurity-reviewerboth flagged this. Adopters who put credential-shaped data in aSERVICE_UNAVAILABLE/UPSTREAM_REJECTED/ custom-codedetailsblock — including thedispatchHitlpath that persiststask.errorfor replay throughget_task_status— get verbatim passthrough. ExtendsanitizeAdcpErrorDetailsto apply a conservative default budget (depth + size + redact-pattern-scrub onmessage) to all standard codes. - Custom-code
detailspasses through unfiltered.sanitizePayloadErrorincreate-adcp-server.ts:2680-2700returns the input unchanged for codes that are neither inSTANDARD_ERROR_CODESnorADCP_ERROR_FIELD_ALLOWLIST. A handler returning{code:'VENDOR_X_FAIL', details:{access_token:'…'}}round-trips the secret. Default to the same minimal{code, message, recovery, field, suggestion, retry_after}projection for unrecognized codes. - Two byte-identical implementations of
taskOwnerScopeFor*.from-platform.ts:2685andcreate-adcp-server.ts:2329are the write and read halves of the same security primitive. Drift between them silently breaks polls. Extract to a single helper, import in both places, add a unit test that asserts equivalence acrosshttp_sig/oauth/api_key/clientId/ no-credential matrix. ownerScopecollision on empty credential subfields. Neither helper checks thatcredential.agent_url/credential.client_id/credential.key_idis a non-empty string. An adopter authenticator that produces{kind:'http_sig', agent_url:''}collapses every caller tohttp_sig:. ReuseisVerifiedHttpSigPayload-style guards (already present atsrc/lib/server/decisioning/buyer-agent.ts:377).TASK_STATUS_MESSAGEis an invented code.create-adcp-server.ts:2231synthesizeserror.code: 'TASK_STATUS_MESSAGE'for non-failed tasks carrying astatusMessage.ad-tech-protocol-expertcouldn't verify this in the rc.7StandardErrorCodeunion. If it isn't there, strict response validation will catch it (and downgrade to VALIDATION_ERROR), but loose mode ships a non-spec code on the wire. Either drop the field whentask.erroris absent, or land the code in upstreamerror-codes.jsonfirst.- Non-CONCURRENT CREATE INDEX on populated task tables.
postgres-task-registry.ts:184-186runsCREATE INDEX IF NOT EXISTS idx_${table}_owner_accountwithoutCONCURRENTLY. Holds ACCESS EXCLUSIVE on the table for the duration of the build — minutes on a multi-tenant SaaS with millions of historical task rows. Existing indexes in the same migration follow this same convention, so this PR isn't introducing the pattern, but it's worth agetDecisioningTaskRegistryMigrationJSDoc warning and an opt-inCONCURRENTLYhelper for adopters with large task tables. - Legacy ownerless rows readable through
account:${accountId}fallback.code-reviewerflagged this as a Major,security-reviewercharacterized it as pre-PR baseline (no isolation existed before). Both are right. The narrative in the changeset already covers it. Worth a follow-up to consider denying the fallback when ownerless rows exist in the registry — fail-closed beats fail-open. - Version gate
=== '3.1.0-rc.7' || '3.1-rc.7'is intentionally narrow. Correct for an RC, but at 3.1.0 GA the new task tools will silently disappear from the wire until the gate widens. File the follow-up now so it's not forgotten.
Minor nits (non-blocking)
'Task failed'is unhelpful for ops.from-platform.ts:2956flattens non-AdcpErrorthrows to the bare string. Logerr.messageat the same call site (opts.logger.error) so adopters can correlate the persisted task error against ops logs even though the buyer never sees the raw message.list_tasksregistration asymmetry.create-adcp-server.ts:5894requirestaskRegistry.list !== undefined;get_task_statusdoesn't. Buyer callinglist_tasksagainst a custom registry that omitslistgets MCP "method not found" instead of anadcpError('NOT_SUPPORTED', …). Inconsistent with how the rest of the framework signals capability gaps.
Safe to merge.
Summary
3.1.0-rc.7.get_task_status,list_tasks, andlist_transformers.list_accountsandsync_accounts, including the TikTok-style split between advertiser account access and publisher identity/post authorization.AUTHORIZATION_REQUIRED.detailsso downstream authorization hints survive while accidental tokens/internal fields are stripped.Expert Review
sync_accounts.accounts[].authorizationprojection and rc.7 tool registration gaps.AUTHORIZATION_REQUIREDdetail envelopes.checked_atpreservation.Validation
npm run sync-schemasnpm run generate-typesnpm run generate-registry-typesnpm run build:libnpm run generate-agent-docsnpm run typecheckNODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/lib/x-entity-hydration.test.js test/lib/zod-schemas.test.js test/lib/request-builder-schema-roundtrip.test.js test/lib/capabilities-tools-drift.test.js test/lib/response-schema-validation.test.js test/lib/storyboard-completeness.test.js test/lib/storyboard-drift.test.js test/server-create-adcp-server.test.js test/server-decisioning-to-wire-account.test.js test/server-account-store-ctx-forwarding.test.js test/server-authorization-required-sanitization.test.jsnpm test