fix(whatsapp): read WHATSAPP_GROUP_ALLOW_FROM env var as group allowlist fallback - #56769
liuhao1024 wants to merge 3 commits into
Conversation
…ist fallback After the built-in → plugin migration, the group allowlist lost its env var fallback. _group_policy reads WHATSAPP_GROUP_POLICY from env (line 410) but _group_allow_from only reads from config.extra (line 411), causing group messages to be silently dropped when the allowlist is configured via .env. Add os.getenv fallback for both WHATSAPP_GROUP_ALLOW_FROM and WHATSAPP_GROUP_ALLOWED_USERS (the name used by the setup wizard). Fixes NousResearch#56767
|
Recommendation: keep the implementation direction, but tighten the regression so it exercises the production adapter path. The production change is the right narrow fix, and CI is green. The current new tests are weaker than they look, though: A stronger test can be small and should still stay focused: monkeypatch.setenv("WHATSAPP_GROUP_POLICY", "allowlist")
monkeypatch.setenv("WHATSAPP_GROUP_ALLOW_FROM", "120363001234567890@g.us")
adapter = WhatsAppAdapter(PlatformConfig(enabled=True, extra={}))
assert adapter._group_allow_from == {"120363001234567890@g.us"}
assert adapter._is_group_allowed("120363001234567890@g.us") is True
assert adapter._is_group_allowed("999999999999@g.us") is FalseThat would cover the actual regression described in #56767: env-only |
…allowlist Address review feedback from harjothkhara: replace the _init_group_allow_from() helper (which reimplemented the __init__ expression) with direct WhatsAppAdapter construction + monkeypatched env vars. This proves the adapter actually wires the env fallback into _group_allow_from and _is_group_allowed, not just that the expression logic is correct in isolation. Also adds an end-to-end test that sets WHATSAPP_GROUP_POLICY=allowlist and WHATSAPP_GROUP_ALLOW_FROM and asserts the adapter accepts listed groups while blocking unlisted ones.
|
Thanks for the thorough review, @harjothkhara! Great catch — the Pushed a fix that replaces the helper with direct
All 7 env fallback tests + 30 existing gating tests pass. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real WhatsApp allowlist regression. Current main still initializes the group allowlist only from config.extra at plugins/platforms/whatsapp/adapter.py:412, while the allowlist gate rejects the resulting empty set at gateway/platforms/whatsapp_common.py:242.
Problems
- The proposed
orchain treats an explicit emptygroup_allow_fromconfig value as missing, then falls through to the environment. That can widen an explicitly emptyallowlist; the new precedence tests cover only non-empty config values. - The matching DM path remains env-blind at
plugins/platforms/whatsapp/adapter.py:410, even though the plugin bridge writesWHATSAPP_ALLOWED_USERSatplugins/platforms/whatsapp/adapter.py:1731-1735. Related PR #37452 identified this sibling path.
Suggested changes
- Distinguish absent config keys from present-but-empty values for group fallback, with empty-list and empty-string regressions.
- Apply the same safe fallback pattern to the DM allowlist and add an env-only DM intake test.
Automated hermes-sweeper review.
| config.extra.get("group_allow_from") | ||
| or config.extra.get("groupAllowFrom") | ||
| or os.getenv("WHATSAPP_GROUP_ALLOW_FROM") | ||
| or os.getenv("WHATSAPP_GROUP_ALLOWED_USERS") |
There was a problem hiding this comment.
This truthiness chain makes an explicit group_allow_from: [] or "" fall through to the environment and can widen an allowlist from a stale env value. Select an env fallback only when neither config key is present, and add regressions for explicit empty snake_case and camelCase config values.
What does this PR do?
Restores the
WHATSAPP_GROUP_ALLOW_FROM/WHATSAPP_GROUP_ALLOWED_USERSenv-var fallback for the group allowlist that was lost when WhatsApp migrated from a built-in adapter (gateway/platforms/whatsapp.py) to a plugin (plugins/platforms/whatsapp/adapter.py).Without this fix, users who configure the group allowlist via
.env(the standard way) have their group messages silently dropped —group_policyreports "allowlist" but the allowlist is empty.Related Issue
Fixes #56767
Type of Change
Changes Made
plugins/platforms/whatsapp/adapter.py: Addedos.getenv("WHATSAPP_GROUP_ALLOW_FROM")andos.getenv("WHATSAPP_GROUP_ALLOWED_USERS")as fallback sources for_group_allow_from(line 411), matching the pattern already used by_group_policy(line 410) and the setup wizard (line 1547).tests/gateway/test_whatsapp_group_env_fallback.py: 6 regression tests covering env-only fallback, config precedence over env, and dual-env-var priority.How to Test
pytest tests/gateway/test_whatsapp_group_env_fallback.py -v— all 6 should pass.pytest tests/gateway/test_whatsapp_group_gating.py -v— all 30 should pass.WHATSAPP_GROUP_POLICY=allowlistandWHATSAPP_GROUP_ALLOW_FROM=120363001234567890@g.usin.env, remove anywhatsapp:section fromconfig.yaml, and observe that group messages are now processed (previously silently dropped).Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A