fix(decisioning): gate async-completion webhook on SPEC_WEBHOOK_TASK_TYPES#932
Merged
Conversation
…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>
Contributor
There was a problem hiding this comment.
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: returnatwebhook_emit.py:429runs beforestrip_credentials_from_wire_result(L457) and the extractors — all pure/read-only, and only ever consumed by thesend_mcpcall non-spec types never reach. No registry write lives in this function;_project_handoffalready 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 areturnthat 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=Noneshort-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-92—create_media_buyis in the enum,finalize_proposalis not. The firing test'sresult == {\"media_buy_id\": \"mb_1\"}assertion survives the strip (media_buy_idisn't a scrubbed key). The third test confirms the genuine-misconfig warning still fires for a spec type withtarget=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)
- 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After #930,
_project_handoff(src/adcp/decisioning/dispatch.py) callsemit_terminal_completion_webhook(src/adcp/decisioning/webhook_emit.py) for every async task. SDK-internal, non-spec task types — notablyfinalize_proposal, an interception ofget_productsinproposal_dispatch.pythat is not a spec wire op and not inSPEC_WEBHOOK_TASK_TYPES— flow through with no webhook target wired. The emitter then hit itstarget is Nonebranch and logged a spurious WARNING: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 ontasks/getpolling /publishStatusChange.Fix
Hoist the
SPEC_WEBHOOK_TASK_TYPEScheck to the top ofemit_terminal_completion_webhook— after theenabledgate, before thetarget is Nonewarning — 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:
finalize_proposal) → skip silently, no warning, no emission (the bug);target=None(genuine misconfig) → the existing target-None warning still fires.Tests
Three new tests in
tests/test_decisioning_webhook_emit.pyexercising the emitter directly:finalize_proposal) with push config andtarget=None→ no emission AND asserts via caplog that no "silently dropped" warning is logged;create_media_buy) with a target → still emits with correct payload;target=None→ the misconfig warning still fires.Verification:
pytest tests/test_decisioning_webhook_emit.py(29 passed),make lint,make typecheckall green.Closes #931
🤖 Generated with Claude Code