fix: include operation ids in task webhooks

[bokelley]

Summary

• include operation_id in framework-emitted task webhook payloads
• echo explicit push_notification_config.operation_id in the webhook payload without parsing the webhook URL
• keep webhook delivery/idempotency keyed by a seller-scoped internal operation id, not the buyer-controlled payload correlation id
• validate explicit push notification operation ids at the request boundary when framework request validation is disabled
• add regression coverage for schema-valid emitted payloads, explicit operation id echoing, URL opacity, and invalid operation id rejection

Fixes #2128.

Why

@adcp/sdk@8.1.0-beta.17 picked up the rc4 webhook schema where task webhook payloads require top-level operation_id. The framework auto-emitter still omitted that field, which blocks downstream 3.0 compatibility storyboards for the AdCP training agent.

The final patch keeps the spec boundary clear: sellers echo the explicit operation id field in the payload, but do not derive it from push_notification_config.url. Storyboard coverage for the black-box protocol expectation is tracked separately in adcontextprotocol/adcp#5208.

Expert review

• code-reviewer: no blockers after validation/idempotency fixes
• protocol_reviewer: no blockers for this SDK PR; storyboard coverage tracked in test: add storyboard coverage for task webhook operation_id echo adcp#5208
• security-reviewer: no blockers after separating payload correlation id from seller-scoped delivery idempotency key

Validation

• npm run build:lib
• NODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/server-decisioning-from-platform.test.js test/lib/webhook-conformance.test.js
• npx prettier --check src/lib/server/decisioning/runtime/from-platform.ts test/server-decisioning-from-platform.test.js .changeset/framework-webhook-operation-id.md package-lock.json
• git diff --check
• pre-push hook: typecheck, build

[aao-ipr-bot]

The required-field fix is right and lands cleanly. The URL-parsing fallback is the open question — it's a spec MUST-NOT that the SDK itself documents.

Things I checked

• Required-field correction is real. core.generated.ts:8237 has operation_id: string (no ?), so the JSDoc rewrite in from-platform.ts:2840-2845 moving operation_id from optional to required matches the rc.4 schema.
• PushNotificationConfig.operation_id is a spec field, not an SDK extension (core.generated.ts:8598-8609, @pattern ^[A-Za-z0-9_.:-]{1,255}$). Extending extractPushConfig to read it is the right shape.
• Fallback chain cfg.operation_id → legacy URL parse → ${tool}.${taskId} is layered correctly in resolveWebhookOperationId at from-platform.ts:2892. The synthetic default satisfies the operation_id regex.
• Two emit sites (emitSyncCompletionWebhook L2682, emitTaskWebhook L2931) now both route through resolveWebhookOperationId(opts, taskId) — same semantics.
• decodeURIComponent and new URL() are both wrapped in the extractLegacyOperationIdFromWebhookUrl try/catch — malformed escapes return undefined, no crash path.
• DispatchHitlOpts, extractPushConfig, resolveWebhookOperationId, and extractLegacyOperationIdFromWebhookUrl are not exported from src/lib/index.ts. Patch bump is correct.
• Tests exercise both the explicit-operation_id path and the legacy-URL fallback path.

The open question — blocking my approval

extractLegacyOperationIdFromWebhookUrl parses the webhook URL to recover operation_id. The rc.4 spec text the SDK ships in its own generated types (core.generated.ts:8235) reads verbatim:

Sellers MUST echo it verbatim in every webhook payload. Sellers MUST NOT derive operation_id by parsing push_notification_config.url — the URL is opaque to the seller. Receivers MUST correlate webhooks using this payload field, never URL-path inspection.

The framework runs in the seller-adapter role for the adopter. Adopters built on this code path are doing the thing the spec says they MUST NOT do, gated only by the buyer-side absence of explicit operation_id. The rc.4 schema bump made operation_id required precisely to eliminate URL-parsing seller behavior — this PR partially undoes that intent.

Two paths to flip me to approve:

1. Gate it behind storyboard-runner-only opt-in. The /step/<tool>/<operation_id>/... shape is the storyboard runner's URL convention (per ad-tech-protocol-expert's review, it traces to src/lib/testing/storyboard/context.ts and webhook-receiver.ts, not legacy AdCP). Move the URL-parsing call behind a flag set by the runner, so adopter sellers never inherit the MUST-NOT behavior. Real production buyers will send operation_id per rc.4; the synthetic ${tool}.${taskId} covers everything else.
2. Drop the URL parse entirely and let ${tool}.${taskId} carry the load when the buyer omitted the field. The webhook still validates; the storyboard fixtures correlate by task_id and operation_id independently anyway.

If the storyboard fixtures genuinely depend on the URL-derived id today, option 1 is the right shape — the framework should not advertise a spec MUST-NOT as a generic SDK code path.

Minor nits (non-blocking)

1. "Legacy" is the wrong label. The URL shape is the current storyboard runner convention, not historical AdCP. Rename to something like extractStoryboardRunnerOperationIdFromUrl and add a one-line // SPEC: comment citing the rc.4 MUST-NOT plus a removal milestone.
2. from-platform.ts:3588 — segments[stepIndex + 2] silently skips segments[stepIndex + 1] (the tool). The test URL implicitly documents the /step/<tool>/<operation_id>/... shape; a one-line comment above the indexOf would make it explicit.

Spec compliance is the same load-bearing invariant that motivated this PR in the first place — the fix shouldn't reintroduce a different conformance gap on the side. Once the URL-parse path is gated (or dropped), happy to flip to approve.

[bokelley]

Issue #2128 was filed after this PR and tracks the same scope. Adding Closes #2128 to the PR body before merge would auto-close the issue and keep the audit trail complete.

---

Generated by Claude Code

[aao-ipr-bot]

LGTM. The two-id split is the right shape — payload operation_id echoes the buyer for correlation, delivery operation_id stays seller-scoped so buyer input can't influence idempotency.

Things I checked

• Wire payload satisfies AdCP 3.0 GA core/mcp-webhook-payload.json (top-level operation_id now required). Regex /^[A-Za-z0-9_.:-]{1,255}$/ is byte-identical to the spec's PushNotificationConfig.operation_id pattern, and "URL is opaque" is normative ("Sellers MUST NOT parse the URL to recover operation_id"), not an SDK-side invention.
• Buyer/seller idempotency boundary holds end-to-end. pushNotificationOperationId flows only into resolveWebhookPayloadOperationId (payload echo). Delivery key everywhere is task-webhook:${accountId}:${tool}:${taskId} — both emitTaskWebhook and emitSyncCompletionWebhook route through resolveWebhookDeliveryOperationId (from-platform.ts:2682, :2935). No buyer path into delivery dedup.
• All five HITL dispatch call sites in buildMediaBuyHandlers + buildCreativeHandlers thread pushNotificationOperationId. Type addition on DispatchHitlOpts is ?: — sync-arm site at ~:4025 uses conditional spread, no breakage.
• assertMcpWebhookPayloadValid uses getSchemaValidatorByRef('core/mcp-webhook-payload.json') — real spec assertion, not hand-rolled shape check.
• "URL is opaque" test asserts the framework does not parse op_url_must_not_be_parsed from the URL path. Invariant enforced.
• Changeset present, patch — correct for a spec-compliance fix on a previously-ignored optional field. No public API surface change.
• No fabrication, no fallback injection. Witness, not translator.

Follow-ups (non-blocking — file as issues)

• Changeset body undersells the delivery contract change. The emitWebhook({ operation_id }) argument shape changed from ${tool}.${taskId} to task-webhook:${accountId}:${tool}:${taskId}. Adopters with a custom taskWebhookEmitter that parse this value for observability or dedup will see a new shape. Patch bump is still right (framework-internal contract), but the changeset should call it out. One-line edit to .changeset/framework-webhook-operation-id.md.
• extractPushConfig validation asymmetry. operation_id rejects { operation_id: undefined } via hasOwnProperty-gate plus typecheck (from-platform.ts:3574-3590). Sibling url/token paths silently accept { url: '...', token: undefined }. Pick one policy — suggest tightening url/token to match the strict form.
• Synthesized fallback ${tool}.${taskId} could collide with a buyer's own operation_id namespace. Spec is silent on seller-side fallback shape. A UUID would be more defensible. Worth raising on adcontextprotocol/adcp#5208.

Minor nits (non-blocking)

1. Opaqueness test assertion is loose. payload.operation_id.startsWith('create_media_buy.') would still pass create_media_buy.op_url_must_not_be_parsed if a future regression copied the URL slug into the synthesized value. Tighter: assert.match(payload.operation_id, /^create_media_buy\.task_/) to match the registry-issued task_ prefix. test/server-decisioning-from-platform.test.js ~:2782.

Approving on the strength of the buyer/seller idempotency split plus the spec-exact regex.