Skip to content

feat(decisioning): async (handoff) discovery for get_products / get_signals#930

Merged
bokelley merged 2 commits into
mainfrom
claude/issue-924-async-discovery
Jun 7, 2026
Merged

feat(decisioning): async (handoff) discovery for get_products / get_signals#930
bokelley merged 2 commits into
mainfrom
claude/issue-924-async-discovery

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented Jun 7, 2026

Closes #924

Wires ctx.handoff_to_task(fn) for async get_products / get_signals discovery (JS adcp-client#2170 parity), plus the four INVALID_REQUEST rejection guards that keep async discovery spec-conformant.

What changed

  • decisioning/types.pyDiscoveryResult[T] alias (same arms as SalesResult): T | TaskHandoff[T] | Awaitable[...]. Exported from adcp.decisioning.
  • specialisms/sales.py / signals.py — return types MaybeAsync[...]DiscoveryResult[...]; fixed the now-false "sync only" / "no async envelope" docstrings. Documented that wholesale MUST stay synchronous (incomplete[], never a handoff) and that get_signals has no input_required arm (submitted/working only).
  • types/aliases.py + types/__init__.py — exposed the orphaned async arm classes as semantic aliases (GetProducts{Submitted,Working,InputRequired}Response, GetSignals{Submitted,Working}Response) plus GetProductsResponseUnion / GetSignalsResponseUnion. GetProductsResponse / GetSignalsResponse stay the constructable success class — the rc.9 schema is flat (not a oneOf), so this is a patch via aliases, not codegen; generated_poc/ is untouched.
  • decisioning/discovery_guards.py (new) — four guards, all AdcpError(INVALID_REQUEST, recovery='correctable'):
    • (a) wholesale + push_notification_configpre-dispatch reject, field='push_notification_config' (platform method not invoked)
    • (b) wholesale + adopter handoff → post-dispatch reject, field='buying_mode' (products) / 'discovery_mode' (signals)
    • (c) async (handoff OR push present) + unresolved account ('<unset>'/empty id) → field='account'
    • (d) hand-rolled {'status':'submitted',...} on the sync arm → guiding error pointing at ctx.handoff_to_task
  • decisioning/handler.py — wired the guards into both shims and threaded the persist-draft terminal side-effect as an on_complete hook so it fires on completion in both the sync and handoff paths (mirrors create_media_buy).

No codegen / version changes

rc.9 already ships the bundled submitted/working/input-required schemas and the get_products/get_signals TaskType enum values, so wire validation and tasks/get round-trip with no codegen and no ADCP_VERSION bump. Public-API snapshot regenerated for the new exports.

Tests

tests/test_decisioning_async_discovery.py (23 tests, public API + real .model_validate): handoff submitted envelope + registry completion for both verbs; response-union membership (products has submitted/working/input_required, signals has submitted/working and not input_required); the 4 guards per verb (asserting code + field, and call-count 0 for the wholesale+push pre-dispatch); sync path unchanged; persist-draft on both sync and handoff completion; task-type enum + wire-validator coverage. Full suite green (5544 passed).

🤖 Generated with Claude Code

bokelley and others added 2 commits June 7, 2026 04:44
…ignals

Wire ctx.handoff_to_task(fn) for get_products and get_signals so brief /
refine discovery MAY background to a long-running task, mirroring JS
adcp-client#2170 (rc8 async discovery parity). Closes #924.

- types.py: add DiscoveryResult[T] alias (same arms as SalesResult) — the
  typing gate for discovery handoff.
- specialisms/sales.py + signals.py: return types MaybeAsync -> DiscoveryResult;
  fix the now-false "sync only / no async envelope" docstrings. Document
  that wholesale MUST stay synchronous (incomplete[]) and that get_signals
  has NO input_required arm (submitted/working only).
- types/aliases.py + types/__init__.py: expose the orphaned async arm classes
  as semantic aliases (GetProducts{Submitted,Working,InputRequired}Response,
  GetSignals{Submitted,Working}Response) plus GetProductsResponseUnion /
  GetSignalsResponseUnion. GetProductsResponse / GetSignalsResponse stay the
  constructable success class (the rc.9 schema is flat, not a union — patch
  via aliases, no codegen, no generated_poc edits).
- discovery_guards.py: four rejection guards (all INVALID_REQUEST/correctable):
  (a) wholesale + push_notification_config -> pre-dispatch reject, no platform
  call; (b) wholesale + adopter handoff -> post-dispatch reject; (c) async +
  unresolved account -> field='account'; (d) hand-rolled submitted dict ->
  guiding error pointing at ctx.handoff_to_task.
- handler.py: wire the guards into the get_products / get_signals shims and
  thread the persist-draft terminal side-effect as an on_complete hook so it
  fires on COMPLETION in both the sync and handoff paths (mirrors
  create_media_buy).

rc.9 already ships the bundled submitted/working/input-required schemas and
the get_products/get_signals TaskType enum values, so wire validation and
tasks/get round-trip without codegen changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…view fixes

Deliver the spec-required terminal completion / failure webhook on the
async (handoff) path of every spec-eligible verb when the buyer
registered push_notification_config (adcp#5389). Previously the SDK
emitted only on the sync path; create_media_buy / get_products /
get_signals and all other async ops left a registered push URL silent
and the buyer polling tasks/get.

Seam (approach a): emit centrally from the background completion path in
dispatch._project_handoff — the single seam every async task flows
through (_invoke_platform_method for all verbs, plus proposal finalize).
The terminal webhook fires EXACTLY ONCE after registry.complete /
registry.fail, with the buyer's operation_id echoed verbatim and the
registry task_id included. The sync auto-emit gate already skips the
{task_id, status} submitted projection, so the two paths never
double-deliver. Webhook emission lives in the decisioning runtime
(webhook_emit.emit_terminal_completion_webhook), reusing the sync path's
WebhookSender / supervisor and send_mcp payload builder; the pluggable
TaskRegistry stays webhook-agnostic. Gated by auto_emit_completion_webhooks
so manual-emit adopters are unaffected; no change to the sync defaults.

- webhook_emit.py: emit_terminal_completion_webhook (self-isolating,
  logged-and-swallowed) + operation_id extraction from push config.
- dispatch.py: thread webhook_target / webhook_auto_emit through
  _invoke_platform_method -> _project_handoff; fire on both the
  registry.complete (completed) and _fail (failed) terminal arms.
- handler.py: _handoff_webhook_kwargs() threaded into every spec-eligible
  shim (create/update_media_buy, sync_creatives, get/activate_signal,
  get_products, sync_audiences/catalogs, brand/rights, property_list).
- webhook_supervisor{,_pg}.py: add operation_id to send_mcp; Pg persists
  it on the queue row (CREATE column + ADD COLUMN IF NOT EXISTS backfill)
  and replays it from the worker.

Review fixes on #930:
- MUST-FIX docstrings: corrected the now-true claims — async terminal
  completion delivers via push webhook when configured, always via
  tasks/get polling (handler / serve / specialism docstrings).
- MUST-FIX dead guard arm (c): removed the unreachable post-dispatch
  assert_account_resolved_for_async call; documented that
  compose_caller_identity owns the no-push-handoff + unresolved-account
  case (fails closed terminally at _build_ctx before any task is minted).
  Added a test asserting no registry row is issued.
- SHOULD-FIX side-effect leak (b): reject_wholesale_handoff_before_launch
  wired as a pre_handoff_reject callback so a wholesale handoff is
  rejected BEFORE the registry row / background task / draft / webhook --
  no side effects for a buyer told 'rejected'. Test asserts an empty
  registry after rejection.
- SHOULD-FIX persist-draft: regression test pinning that fields= /
  pagination= projections shape only the wire response, never the
  persisted draft (full product pricing retained).
- SHOULD-FIX wire-validator: tightened the discovery submitted test to
  assert variant == 'submitted' (no skip escape hatch).
- Added async-completion webhook tests on get_products, get_signals,
  create_media_buy (one completed webhook; operation_id echoed, task_id,
  result in payload), the failure path (one failed webhook), and the
  no-push no-webhook case. Updated the create_media_buy handoff test to
  assert the conformant exactly-once behavior.

No ADCP_VERSION bump, no schema re-download, no public-API export change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the claude/issue-924-async-discovery branch from 0ba8357 to f8d421e Compare June 7, 2026 09:15
@bokelley bokelley marked this pull request as ready for review June 7, 2026 09:36
@bokelley bokelley enabled auto-merge (squash) June 7, 2026 09:36
Copy link
Copy Markdown
Contributor

@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.

Solid async-discovery wiring — the four guards, exactly-once webhook semantics, and the pg operation_id threading are correct. The async arms are exposed via aliases.py rather than codegen because the rc.9 get_products_response / get_signals_response schemas are flat success objects, not oneOf unions — right call, and GetProductsResponse / GetSignalsResponse stay the constructable success class on the wire.

One follow-up worth fixing before it bites an adopter: the refine handoff path delivers the submitted envelope but not the terminal webhook the docstrings promise. Details below. Not a block — wire lifecycle stays conformant (tasks/get works), no crash, no data loss, no public-API break.

ad-tech-protocol-expert: sound — verified against schemas/cache/3.1.0-rc.9/. get_products ships submitted/working/input_required; get_signals ships submitted/working only (no get_signals_async_response_input_required exists); status:'submitted' discriminator, the get_products/get_signals task-type.json enum members, and the operation_id-must-echo contract all check out. security-reviewer: no High, no Medium — credential strip applied to both completion and failure payloads, operation_id is a bound %s param, guard (c) + compose_caller_identity fail-closed keep async tasks from minting against <unset> accounts.

Things I checked

  • Guards (a)-(d) (discovery_guards.py:114-304). _account_resolved (L255-259) matches dispatch.compose_caller_identity's not id / strip() / '<unset>' contract. Guard (d) reject_hand_rolled_submitted (L262) has no false-positive on a sync GetProductsResponse — the flat success model has no top-level status field, so hasattr(result,"status") is False on the happy path, and the framework's own 2-key {task_id,status} projection is whitelisted via _is_submitted_projection (L103-111).
  • Exactly-once webhook (dispatch.py:1751-1788 / 1870-1882). _fail emits failed after registry.fail; _run emits completed after registry.complete; the two arms are mutually exclusive within one task, and no prior emit site existed in _project_handoff, so no double-delivery. pre_handoff_reject fires before any registry row / background task / webhook side effect.
  • pg column threading (webhook_supervisor_pg.py:298-310, 371-384, 526, 605). INSERT (11 cols / 11 %s) and the enqueue tuple stay positionally in sync; SELECT and the poll unpack both append operation_id last; ADD COLUMN IF NOT EXISTS operation_id TEXT targets only {qt} and converges fresh + upgrading deployments. DDL-count test bumped 6 to 7.
  • emit_terminal_completion_webhook (webhook_emit.py:353-410). Self-isolating (outer try/except Exception, logged-and-swallowed), strips credentials before send_mcp, gates on enabled, warns on missing-target-with-push and non-spec method_name. WebhookSender.send_mcp already accepts operation_id (webhook_sender.py:567) — supervisor/pg threading lands cleanly.
  • persist-draft seam (handler.py:1903-1927, 2026-2029). _persist_draft_hook on on_complete fires on both sync (inline) and handoff (bg, before registry.complete) paths; the old inline maybe_persist_draft_after_get_products in the brief/wholesale arm was correctly removed. Two regression tests pin it.
  • Public API additive (tests/fixtures/public_api_snapshot.json, types/__init__.py, aliases.py). Ten new adcp.types exports + DiscoveryResult on adcp.decisioning — all additive, snapshot regenerated, feat: is the correct semver signal. No removed/renamed exports, no required-to-optional flips.

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

  • refine handoff drops the terminal webhook. handler.py:1829 dispatches refine_get_products through _invoke_platform_method with no **self._handoff_webhook_kwargs(), no pre_handoff_reject, and no post-dispatch guards, then returns early at L1851 before the brief/wholesale arm that threads them. An adopter who returns ctx.handoff_to_task(fn) from refine_get_products with a buyer-registered push_notification_config (resolved account) gets a correct submitted envelope but no terminal push webhook ever fires — they're silently forced to poll tasks/get, contradicting the sales.py docstring ("Brief / refine discovery MAY hand off ... the framework also delivers the terminal completion / failure webhook ... exactly once") and the adcp#5389 MUST. No test covers refine+handoff+push. Minimum fix: narrow the docstring + DiscoveryResult commentary to scope refine handoff as polling-only; better fix: thread webhook_target (and the post-dispatch guards) into the refine _invoke_platform_method call.
  • New public exports undocumented. Ten new adcp.types response-variant exports — check README.md / AGENTS.md for drift (additive, so non-blocking).

Minor nits (non-blocking)

  1. GetProductsInputRequired has no explicit status literal. Per ad-tech-protocol-expert, the generated input_required class relies on extra='allow' rather than a status field (unlike submitted/working). Pre-existing codegen property, not introduced here — flag only if downstream code tries to discriminate the input_required arm by a status attribute.
  2. Buyer callback URL logged at WARNING. webhook_emit.py target=None branch logs url=%s. Mirrors the existing sync-gate pattern (the auth token is not logged), so no new disclosure class — but a buyer who smuggles a secret into their own URL query string deposits it in seller logs. Consider redacting the query string via urlsplit on both gates. (security-reviewer: Low, defense-in-depth.)

The PR is titled async discovery for two verbs; the second commit quietly threaded completion webhooks through fifteen — the diff outgrew its title. Semver signal is still correct, so a follow-up note, not a block.

LGTM. Follow-ups noted above — the refine-handoff webhook gap is the one to close (or document away) before an adopter relies on it.

@bokelley bokelley merged commit 76fa4fa into main Jun 7, 2026
27 checks passed
@bokelley bokelley deleted the claude/issue-924-async-discovery branch June 7, 2026 09:44
bokelley added a commit that referenced this pull request Jun 7, 2026
…TYPES (#932)

After #930, _project_handoff calls emit_terminal_completion_webhook for
every async task. SDK-internal, non-spec task types (notably
finalize_proposal, an interception of get_products in proposal_dispatch.py
that is not a spec wire op and not in SPEC_WEBHOOK_TASK_TYPES) flow through
with no webhook target wired, so the emitter logged a spurious "neither
webhook_sender nor webhook_supervisor is wired — terminal webhook silently
dropped" WARNING on every async finalize, even on a correctly-configured
server.

Hoist the SPEC_WEBHOOK_TASK_TYPES check to the top of the emitter (after
the enabled gate, before the target-None warning) and return silently for
non-spec task types, mirroring how the sync emitter gates. Non-spec types
rely on tasks/get polling / publishStatusChange per the documented rule
above SPEC_WEBHOOK_TASK_TYPES. Spec types with a real target still emit;
spec types with target=None (genuine misconfig) still warn.

Closes #931

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

feat(decisioning): async (handoff) discovery for get_products / get_signals (JS #2170 parity)

1 participant