fix: type server handlers as payloads#1983
Conversation
There was a problem hiding this comment.
LGTM. Clean separation of payload from envelope, with the v3 split (status: TaskStatus at envelope, domain status at media_buy_status / verdict / rights_status) finally locked at the type level. ServerPayload<T> distributes correctly over unions and the ProtocolStatusKey<T> guard preserves domain status types — verified by the @ts-expect-error lock in decisioning.type-checks.ts:386-402.
Things I checked
ServerPayload<T>distribution.T extends unknown ? Omit<T, ProtocolEnvelopeKeys<T>> : neverdistributes over theAcquireRightsAcquired | AcquireRightsPendingApproval | AcquireRightsRejectedunion without flattening — discriminants (rights_status) are preserved because they aren't namedstatus. Confirmed.ProtocolStatusKey<T>only stripsstatuswhenNonNullable<T['status']> extends TaskStatus.MediaBuyStatusincludes'pending_creatives' | 'pending_start' | 'active' | 'paused'which aren't inTaskStatus, so the conditional resolves toneverandstatussurvives onCreateMediaBuySuccess/UpdateMediaBuySuccess. The dual lock atdecisioning.type-checks.ts:386-402(positive +@ts-expect-errornegative) is the right shape.AdcpToolMapcoverage increate-adcp-server.ts. 51 entries, allServerPayload<*>. Nothing missed.cancelMediaBuyResponsebehavior alignment. Previously returnedstructuredContent.status === 'canceled'. Now splits tomedia_buy_status: 'canceled'+ envelopestatus: 'completed'. This bringscancelMediaBuyResponsein line withmediaBuyResponseandupdateMediaBuyResponse, which already split. Test updates attest/server-responses.test.js:440-449cover it.governance_contextpreservation.test/server-create-adcp-server.test.js:476-492asserts the new positive —check_governancehandlers can returngovernance_contextand the SDK passes it through unchanged. Intentional, not a strip-list miss.- The
splitMediaBuyStatusvalue-set heuristic inresponses.ts:122-134.MEDIA_BUY_STATUS_VALUESandTaskStatuscollide on'completed','canceled','rejected', but the three call sites passServerPayload<CreateMediaBuySuccess>/ServerPayload<UpdateMediaBuySuccess>whosestatusis typed asMediaBuyStatus, notTaskStatus. By type, the value is the domain status. Adopters whoas anypast this aren't a guarantee the SDK owes. - Changeset present (
.changeset/server-handler-payload-types.md),patch, scoped to type-level narrowing. Runtime behavior unchanged on the 49 non-cancel response builders; envelope stamping was already happening at dispatch.
Follow-ups (non-blocking — file as issues)
- Re-export prefix scheme is inconsistent in
decisioning/index.ts.PreviewCreativePayloadandListCreativeFormatsPayloadfromspecialisms/creative(line 160) andspecialisms/creative-ad-server(lines 172-173) are collision-prefixed (CreativePreviewCreativePayload,CreativeAdServerListCreativeFormatsPayload). But the same names fromspecialisms/sales(lines 219-220) —ListCreativeFormatsPayload,ListCreativesPayload— re-export bare. All three currently resolve to the sameServerPayload<*Response>, so it compiles, but the bare-name slot is now owned by the sales re-export by accident of declaration order. If the three definitions ever diverge, adopters who imported the bare name will silently get the sales shape. Either prefix the sales re-exports too (SalesListCreativeFormatsPayload), or add a type-equality assertion that the three resolve identically. - Changeset prose understates the wire-shape change in
cancelMediaBuyResponse. The body reads as a pure typing change ("The SDK continues to stamp envelope fields such asstatus: 'completed'at dispatch time"). Mostly true, except thatcancelMediaBuyResponseused to stampstructuredContent.status === 'canceled'directly and now stamps'completed'+ splits tomedia_buy_status. Any adopter readingstructuredContent.statuson cancel responses sees a behavior change. The alignment is correct; the changeset should name it explicitly so adopters mid-v3-envelope migration aren't surprised. scripts/check-adopter-types.tsmodule-resolution switch covers one shape. Going fromcommonjs/nodetoESNext/bundlervalidates Vite/Next/esbuild adopters but loses CJS/node16/nodenextadopters. The packed-tarball type check probably wants a matrix (bundler + nodenext, at minimum) to catch resolution-mode-sensitive type leaks. Non-blocking — the new surfaces being exercised here are worth the trade.
Minor nits (non-blocking)
completedStructuredContentearly-return ordering.responses.ts:122-134first deletes asplitMediaBuyStatus-matchedstatus, then stamps'completed'if undefined. Correct for the three call sites that passsplitMediaBuyStatus: true, but a future caller stamping a non-terminal envelope status (e.g.'working') would lose it. One-line comment naming the assumed call shape would protect the invariant.SponsoredIntelligencePlatformre-exports fourSI*Payloadaliases but the type-check atdecisioning.type-checks.ts:425-432only exercisesgetOffering's domainavailable: trueslot. The other three (initiate/send/terminate) get only a structural check via the platform shape, not a payload-distinction check. Not a defect — just notable that the_server_payload_preserves_domain_status_fieldslock only fires forCreateMediaBuySuccess. Worth a parallel lock forSISendMessageResponse.session_statusif you want belt-and-suspenders on the SI flow.
Approving. The @ts-expect-error negative lock at decisioning.type-checks.ts:399-402 is the kind of test that catches the bug a year from now when someone tries to "clean up" the ServerPayload definition.
There was a problem hiding this comment.
LGTM. Clean cut between domain payload and protocol envelope. ServerPayload<T> = Omit<T, ProtocolEnvelopeKeys<T>> with the conditional ProtocolStatusKey<T> discriminator is the right primitive — domain MediaBuyStatus / CreativeStatus / AccountStatus survive because each union has at least one non-TaskStatus member, pure-envelope status: TaskStatus fields strip. Threading it through the 5 specialism interfaces, OperationalPlatform, AdcpToolMap.result, and the v5-deprecated helpers in a single mechanical pass is the right shape.
Things I checked
- Envelope key list at
src/lib/types/server-payload.ts:20-31againstcore.generated.tsand the AdCP 3.0/3.1 task envelope. Matches.errors[]correctly NOT stripped — it's a payload field per spec.adcp_minor_version/webhook_urlaren't envelope fields.push_notification_config(singular) is the wire name. ProtocolStatusKey<T>discrimination:NonNullable<MediaBuyStatus> extends TaskStatusis false (pending_creatives,pending_start,pausedaren't inTaskStatus), so domainstatusis preserved. Async-submitted envelopes likeCreateMediaBuySubmittedwherestatus: 'submitted'extendsTaskStatusget stripped and re-stamped at dispatch. Right semantics.AdcpToolMap[K]['result']narrowing fromTResponse→ServerPayload<TResponse>is structurally safe — the union atcreate-adcp-server.ts:832also acceptsAdcpToolMap[K]['response'], so adopters who still return the full wire shape continue to typecheck. Function return-type covariance covers the rest.completedStructuredContent(data, { splitMediaBuyStatus })inresponses.ts:120-133mirrors the dispatch-timenormalizeMediaBuyStatusCollisionatcreate-adcp-server.ts:2734-2746. v5-deprecated helpers now behave identically to the v6 dispatch path. New governance test attest/server-create-adcp-server.test.js:476-494locks thegovernance_contextpass-through.- Negative test at
scripts/check-adopter-types.ts:146-147(@ts-expect-error ServerPayload must preserve required domain fields) actually exercises required-field preservation against a packed tarball.
Follow-ups (non-blocking — file as issues)
- Changeset should be
minor, notpatch. Adds ~30 new public type exports (ServerPayload<T>plus the per-specialism*Payloadaliases). CLAUDE.md rule: "new export → minor." Repo is in8.0.0-betapre-mode so the rolled version doesn't shift today, but the migration narrative is wrong. MEDIA_BUY_STATUS_VALUESduplicated betweensrc/lib/server/responses.ts:112-120andsrc/lib/server/create-adcp-server.ts:2724-2732. Two copies of the same enum is a drift footgun — when the spec adds a value, one site updates and the other won't. Pull intomedia-buy-helpers.ts(already imported byresponses.ts).- Barrel name collision in
@adcp/sdk/server.src/lib/server/decisioning/index.ts:159-160, 171-173aliases the creative-builder and creative-ad-server variants asCreative*/CreativeAdServer*prefixed names, but the sales-specialismListCreativeFormatsPayload/ListCreativesPayload/PreviewCreativePayloadat lines 219-220 export under the bare name. Two conventions in the same barrel. Either rename toSales*prefix or commit to the bare name and drop the qualified aliases. - Changeset prose undercounts the surprise. Adopters who explicitly annotated handlers as
: Promise<*Response>will get a TS error on the interface assignment after upgrade; the changeset doesn't say to switch to*Payloador unannotate. Adopters mid-migration who use the v5mediaBuyResponse/updateMediaBuyResponse/cancelMediaBuyResponsehelpers AND readstructuredContent.statusdownstream now see'completed'instead of'canceled'/'active'/ etc. Worth one extra sentence. @exampleblock onServerPayload<T>atsrc/lib/types/server-payload.ts:33-39. The discriminator-narrowing behavior (preserving domainstatuswhose type isn'tTaskStatus) is exactly the kind of subtle contract LLMs benefit from anchoring on. Six lines, one preserved-domain-field case, one stripped-envelope-field case.
Approved.
Summary
Types server and platform handler returns as domain payloads instead of generated protocol wire envelopes. The SDK still stamps envelope fields such as
status: "completed"during dispatch, while generated wire response types remain intact for transport/client validation.This also exports
ServerPayload<T>plus payload aliases for the server specialism surfaces, preserves seller-suppliedgovernance_context, updates the flagship seller examples to the payload-return style, and widens the packed-adopter type check to cover public server payload imports.Root Cause
The v8 beta generated
*Responsetypes model the full AdCP task envelope, including required protocol fields likestatus. Server adopters return domain payloads and rely on the SDK server wrapper to add protocol metadata, so typing handlers directly as generated wire responses forced every adapter to hand-author framework-owned fields.Validation
npm run typechecknpm run build:libnpm run typecheck:examplesnpm run check:adopter-typesnpm run check:adopter-types-narrowNODE_ENV=test node --test --test-timeout=60000 test/server-create-adcp-server.test.jsnpx prettier --check ...git diff --checkNote:
build:libstill prints existing per-tool export-name collision warnings, but exits successfully.