Skip to content

fix: type server handlers as payloads#1983

Merged
bokelley merged 5 commits into
mainfrom
bokelley/sdk-v8-handler-types
May 24, 2026
Merged

fix: type server handlers as payloads#1983
bokelley merged 5 commits into
mainfrom
bokelley/sdk-v8-handler-types

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

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-supplied governance_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 *Response types model the full AdCP task envelope, including required protocol fields like status. 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 typecheck
  • npm run build:lib
  • npm run typecheck:examples
  • npm run check:adopter-types
  • npm run check:adopter-types-narrow
  • NODE_ENV=test node --test --test-timeout=60000 test/server-create-adcp-server.test.js
  • npx prettier --check ...
  • git diff --check

Note: build:lib still prints existing per-tool export-name collision warnings, but exits successfully.

@bokelley bokelley changed the title [codex] type server handlers as payloads fix: type server handlers as payloads May 24, 2026
@bokelley bokelley marked this pull request as ready for review May 24, 2026 15:15
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes May 24, 2026
Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>> : never distributes over the AcquireRightsAcquired | AcquireRightsPendingApproval | AcquireRightsRejected union without flattening — discriminants (rights_status) are preserved because they aren't named status. Confirmed.
  • ProtocolStatusKey<T> only strips status when NonNullable<T['status']> extends TaskStatus. MediaBuyStatus includes 'pending_creatives' | 'pending_start' | 'active' | 'paused' which aren't in TaskStatus, so the conditional resolves to never and status survives on CreateMediaBuySuccess / UpdateMediaBuySuccess. The dual lock at decisioning.type-checks.ts:386-402 (positive + @ts-expect-error negative) is the right shape.
  • AdcpToolMap coverage in create-adcp-server.ts. 51 entries, all ServerPayload<*>. Nothing missed.
  • cancelMediaBuyResponse behavior alignment. Previously returned structuredContent.status === 'canceled'. Now splits to media_buy_status: 'canceled' + envelope status: 'completed'. This brings cancelMediaBuyResponse in line with mediaBuyResponse and updateMediaBuyResponse, which already split. Test updates at test/server-responses.test.js:440-449 cover it.
  • governance_context preservation. test/server-create-adcp-server.test.js:476-492 asserts the new positive — check_governance handlers can return governance_context and the SDK passes it through unchanged. Intentional, not a strip-list miss.
  • The splitMediaBuyStatus value-set heuristic in responses.ts:122-134. MEDIA_BUY_STATUS_VALUES and TaskStatus collide on 'completed', 'canceled', 'rejected', but the three call sites pass ServerPayload<CreateMediaBuySuccess> / ServerPayload<UpdateMediaBuySuccess> whose status is typed as MediaBuyStatus, not TaskStatus. By type, the value is the domain status. Adopters who as any past 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. PreviewCreativePayload and ListCreativeFormatsPayload from specialisms/creative (line 160) and specialisms/creative-ad-server (lines 172-173) are collision-prefixed (CreativePreviewCreativePayload, CreativeAdServerListCreativeFormatsPayload). But the same names from specialisms/sales (lines 219-220) — ListCreativeFormatsPayload, ListCreativesPayload — re-export bare. All three currently resolve to the same ServerPayload<*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 as status: 'completed' at dispatch time"). Mostly true, except that cancelMediaBuyResponse used to stamp structuredContent.status === 'canceled' directly and now stamps 'completed' + splits to media_buy_status. Any adopter reading structuredContent.status on 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.ts module-resolution switch covers one shape. Going from commonjs/node to ESNext/bundler validates Vite/Next/esbuild adopters but loses CJS/node16/nodenext adopters. 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)

  1. completedStructuredContent early-return ordering. responses.ts:122-134 first deletes a splitMediaBuyStatus-matched status, then stamps 'completed' if undefined. Correct for the three call sites that pass splitMediaBuyStatus: 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.
  2. SponsoredIntelligencePlatform re-exports four SI*Payload aliases but the type-check at decisioning.type-checks.ts:425-432 only exercises getOffering's domain available: true slot. 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_fields lock only fires for CreateMediaBuySuccess. Worth a parallel lock for SISendMessageResponse.session_status if 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.

Copy link
Copy Markdown

@aao-ipr-bot aao-ipr-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-31 against core.generated.ts and the AdCP 3.0/3.1 task envelope. Matches. errors[] correctly NOT stripped — it's a payload field per spec. adcp_minor_version / webhook_url aren't envelope fields. push_notification_config (singular) is the wire name.
  • ProtocolStatusKey<T> discrimination: NonNullable<MediaBuyStatus> extends TaskStatus is false (pending_creatives, pending_start, paused aren't in TaskStatus), so domain status is preserved. Async-submitted envelopes like CreateMediaBuySubmitted where status: 'submitted' extends TaskStatus get stripped and re-stamped at dispatch. Right semantics.
  • AdcpToolMap[K]['result'] narrowing from TResponseServerPayload<TResponse> is structurally safe — the union at create-adcp-server.ts:832 also accepts AdcpToolMap[K]['response'], so adopters who still return the full wire shape continue to typecheck. Function return-type covariance covers the rest.
  • completedStructuredContent(data, { splitMediaBuyStatus }) in responses.ts:120-133 mirrors the dispatch-time normalizeMediaBuyStatusCollision at create-adcp-server.ts:2734-2746. v5-deprecated helpers now behave identically to the v6 dispatch path. New governance test at test/server-create-adcp-server.test.js:476-494 locks the governance_context pass-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, not patch. Adds ~30 new public type exports (ServerPayload<T> plus the per-specialism *Payload aliases). CLAUDE.md rule: "new export → minor." Repo is in 8.0.0-beta pre-mode so the rolled version doesn't shift today, but the migration narrative is wrong.
  • MEDIA_BUY_STATUS_VALUES duplicated between src/lib/server/responses.ts:112-120 and src/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 into media-buy-helpers.ts (already imported by responses.ts).
  • Barrel name collision in @adcp/sdk/server. src/lib/server/decisioning/index.ts:159-160, 171-173 aliases the creative-builder and creative-ad-server variants as Creative* / CreativeAdServer* prefixed names, but the sales-specialism ListCreativeFormatsPayload / ListCreativesPayload / PreviewCreativePayload at lines 219-220 export under the bare name. Two conventions in the same barrel. Either rename to Sales* 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 *Payload or unannotate. Adopters mid-migration who use the v5 mediaBuyResponse / updateMediaBuyResponse / cancelMediaBuyResponse helpers AND read structuredContent.status downstream now see 'completed' instead of 'canceled' / 'active' / etc. Worth one extra sentence.
  • @example block on ServerPayload<T> at src/lib/types/server-payload.ts:33-39. The discriminator-narrowing behavior (preserving domain status whose type isn't TaskStatus) 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.

@bokelley bokelley merged commit 314ea71 into main May 24, 2026
11 checks passed
@bokelley bokelley deleted the bokelley/sdk-v8-handler-types branch May 24, 2026 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant