Skip to content

fix(decisioning): gate async-completion webhook on SPEC_WEBHOOK_TASK_TYPES#932

Merged
bokelley merged 1 commit into
mainfrom
claude/issue-931-webhook-spec-gate
Jun 7, 2026
Merged

fix(decisioning): gate async-completion webhook on SPEC_WEBHOOK_TASK_TYPES#932
bokelley merged 1 commit into
mainfrom
claude/issue-931-webhook-spec-gate

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented Jun 7, 2026

Problem

After #930, _project_handoff (src/adcp/decisioning/dispatch.py) calls emit_terminal_completion_webhook (src/adcp/decisioning/webhook_emit.py) 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. The emitter then hit its target is None branch and logged a spurious WARNING:

neither webhook_sender nor webhook_supervisor is wired — terminal webhook silently dropped

on every async finalize, even on a correctly-configured server.

The SDK's own documented rule (the docstring above SPEC_WEBHOOK_TASK_TYPES) is that task types not in the spec enum skip webhook delivery and rely on tasks/get polling / publishStatusChange.

Fix

Hoist the SPEC_WEBHOOK_TASK_TYPES check to the top of emit_terminal_completion_webhook — after the enabled gate, before the target is None warning — and return silently for non-spec task types. This mirrors how the sync emitter (maybe_emit_sync_completion) gates. The previous spec-enum check lower in the function (which logged a WARNING on skip) is removed since the new gate short-circuits earlier and silently.

Behavior:

  • non-spec task type (e.g. finalize_proposal) → skip silently, no warning, no emission (the bug);
  • spec task type with a real target → emits (unchanged);
  • spec task type with target=None (genuine misconfig) → the existing target-None warning still fires.

Tests

Three new tests in tests/test_decisioning_webhook_emit.py exercising the emitter directly:

  • non-spec task type (finalize_proposal) with push config and target=None → no emission AND asserts via caplog that no "silently dropped" warning is logged;
  • spec task type (create_media_buy) with a target → still emits with correct payload;
  • spec task type with target=None → the misconfig warning still fires.

Verification: pytest tests/test_decisioning_webhook_emit.py (29 passed), make lint, make typecheck all green.

Closes #931

🤖 Generated with Claude Code

…TYPES

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>
@bokelley bokelley enabled auto-merge (squash) June 7, 2026 09:50
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.

Clean fix. Hoisting the SPEC_WEBHOOK_TASK_TYPES gate above the target is None branch is the right shape — non-spec SDK-internal task types like finalize_proposal were never webhook-eligible, so they should never reach the misconfig warning meant for genuinely-dropped spec notifications.

Things I checked

  • Gate ordering loses no required side effect. The new if method_name not in SPEC_WEBHOOK_TASK_TYPES: return at webhook_emit.py:429 runs before strip_credentials_from_wire_result (L457) and the extractors — all pure/read-only, and only ever consumed by the send_mcp call non-spec types never reach. No registry write lives in this function; _project_handoff already persisted terminal state upstream.
  • No credential-strip bypass. Every path that still reaches target.send_mcp (L470) passes through the stripper at L457 unchanged — the reorder only removed a return that sat after the strip, it moved nothing between strip and send. CREDENTIAL_BEARING_METHODS ∩ SPEC_WEBHOOK_TASK_TYPES (create_media_buy, update_media_buy, sync_accounts, sync_creatives) all pass the moved-up gate and still hit the stripper. Credential-bearing non-spec methods now return at the top gate and never send — a strict reduction in delivery surface. security-reviewer: clean.
  • Old lower gate fully removed, no orphan. The L459-468 WARNING is deleted, not stranded. Behavior change is intentional and documented: non-spec types are now silent in both the target-None and target-present cases (previously a non-spec type with target=None short-circuited at the earlier warning, so the lower gate only fired when target was non-None).
  • Tests exercise the real gate. Membership matches webhook_emit.py:72-92create_media_buy is in the enum, finalize_proposal is not. The firing test's result == {\"media_buy_id\": \"mb_1\"} assertion survives the strip (media_buy_id isn't a scrubbed key). The third test confirms the genuine-misconfig warning still fires for a spec type with target=None. code-reviewer: clean.
  • Docstring updated to match. The diff removes the "non-spec type logs a WARNING" bullet from the "Logs a WARNING when" section and narrows the target-None bullet to SPEC-eligible types — consistent with the new control flow.
  • fix(decisioning): is the correct semver signal; no public-API surface changed, no breaking diff.

Minor nits (non-blocking)

  1. Two doc sites now carry the same rule. The hoisted-gate rationale is spelled out both in the docstring (L384-397) and in the inline comment at L418-427. Fine as-is — load-bearing reorders earn the redundancy.

Closes #931 with a regression test that asserts the WARNING is absent via caplog. Right way to lock a log-noise bug.

LGTM.

@bokelley bokelley merged commit c23b406 into main Jun 7, 2026
26 checks passed
@bokelley bokelley deleted the claude/issue-931-webhook-spec-gate branch June 7, 2026 09:56
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.

fix(decisioning): gate async-completion webhook emission on SPEC_WEBHOOK_TASK_TYPES (suppress spurious warning for non-spec task types)

1 participant