chore(channels/whatsapp-sidecar): 3 nits from post-merge audit#5454
Merged
Conversation
Surfaces from the post-#5445 audit (6th in the dingtalk/qq/omnibus review chain — first verdict that wasn't BROKEN/NEEDS_HOTFIX). All three nits are doc/UX cleanups, none are runtime regressions: 1. **Docstring claim #3 was wrong** — said "429 Retry-After honoured on every outbound POST", but only Cloud API uses `_cloud_post_with_retry`; the gateway path calls `_http_request` directly and raises on any non-2xx. Soften the claim to "Cloud-API outbound POSTs only" + explain when gateway-side retry would matter (operators proxying the local Baileys gateway behind a rate-limiting reverse-proxy). 2. **WHATSAPP_VERIFY_TOKEN unset failed silently** — Meta's subscription handshake returned 403 with no log line pointing back to the missing env var; operators saw "subscription failed" in the Meta dashboard with nothing in their daemon logs. Now warns at __init__, matching the WHATSAPP_APP_SECRET pattern already there. 3. **WHATSAPP_GROUP_POLICY is dead config** — Cloud API webhook payloads don't surface a group/conversation distinction (we hardcode `is_group=False` at `_handle_post_webhook`), and gateway mode delegates inbound entirely to the Node Baileys gateway which never calls back into the sidecar's `should_handle_message` filter. So `WHATSAPP_GROUP_POLICY` has zero effect today. Mark the schema field with an explicit "(currently inert)" note in the label so operators don't waste time setting it. Kept the field itself for forward-compat — Meta has been rolling out group-chat support to the Cloud API gradually and we want the schema to be ready when it lands. Drive-by: dropped the misleading send-voice docstring claim (lines 22-23 + 36-39 promised a `{gateway}/message/send-voice` multipart upload route that doesn't exist in code; rationale comment about the daemon's ChannelContent::Voice always carrying a URL at the dispatch boundary explains why we don't need it). Test: - New test_get_verify_empty_self_token_rejects asserts BOTH the __init__ warn fires AND the handshake fails closed. Regression guard for #2 — without this an attacker who guesses the empty hub.verify_token could subscribe their own callback URL. - cd sdk/python && pytest tests/test_whatsapp_adapter.py — 79 passed (was 78; +1 regression guard). This closes the audit chain. WhatsApp itself is structurally clean — natural routing key (phone) is preserved as channel_id both directions, the bug family from #5417 → #5449 doesn't apply.
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.
Summary
Surfaces from the post-#5445 audit (6th in the dingtalk/qq/omnibus review chain — first verdict that wasn't BROKEN/NEEDS_HOTFIX). All three are doc/UX cleanups, none are runtime regressions:
Docstring claim Bump docker/login-action from 3 to 4 #3 was wrong — said "429 Retry-After honoured on every outbound POST", but only Cloud API uses `_cloud_post_with_retry`; the gateway path calls `_http_request` directly. Softened to "Cloud-API outbound POSTs only" + explained when gateway-side retry would matter.
WHATSAPP_VERIFY_TOKEN unset failed silently — Meta's subscription handshake returned 403 with no log line, operators saw "subscription failed" in the Meta dashboard with nothing in their daemon logs. Now warns at `init`.
WHATSAPP_GROUP_POLICY is dead config — Cloud API webhook doesn't surface groups (we hardcode `is_group=False`); gateway mode delegates inbound entirely to the Node gateway. So all values are ignored. Schema field kept for forward-compat but labeled `(currently inert)`.
Drive-by
Dropped the misleading `send-voice` docstring claim — promised a `{gateway}/message/send-voice` multipart upload route that doesn't exist in code; rationale comment explains why we don't need it (daemon's `ChannelContent::Voice` only carries a URL at dispatch boundary).
Tests
Audit chain
This closes the 6-review chain. WhatsApp itself is structurally clean — natural routing key (phone) is preserved as `channel_id` both directions, so the bug family from #5417 → #5449 doesn't apply by construction.