fix(platforms): clear home channel when setup prompt left blank (#12423) - #58421
fix(platforms): clear home channel when setup prompt left blank (#12423)#58421DavidMetcalfe wants to merge 3 commits into
Conversation
…Research#12423) All 5 platform adapters (Feishu, Discord, Slack, Mattermost, WhatsApp) now call remove_env_value() when the home-channel prompt is left blank, instead of silently preserving the old value. Also: .strip() prompt input, else-block instead of elif side-effect, mock remove_env_value in tests, add 4 clearing tests.
|
Thanks for tracing the Feishu report through the plugin migration and applying the existing Problems
Suggested changes
Automated hermes-sweeper review. |
Extends the home-channel clear-on-blank behavior from NousResearch#58421 to the two remaining adapters whose home-channel prompts still preserved the old value silently: - Matrix (`plugins/platforms/matrix/adapter.py`): the `MATRIX_HOME_ROOM` prompt at line 4651 was the last truthy-only-save site. Both Matrix (`MATRIX_HOME_ROOM`) and WeCom (`WECOM_HOME_CHANNEL`) are listed in `cron/scheduler.py::_HOME_TARGET_ENV_VARS` as active cron home delivery targets — leaving them on the old behavior silently re-sent cron job output to a channel/room the user had tried to clear. - WeCom (`plugins/platforms/wecom/adapter.py`): the `WECOM_HOME_CHANNEL` prompt at line 1820 had the same truthy-only-save shape. Both adapters now: 1. `.strip()` the prompt input so whitespace-only answers don't get persisted as a literal space. 2. In the else branch, call `remove_env_value()` and print "Home channel cleared." when the prior value was actually removed. The 5 already-patched adapters in NousResearch#58421 follow the same shape. This addresses the second finding in maintainer teknium1's `keep_open / salvageability=high` review of NousResearch#58421 (comment 4983105207): the same bug class was still present in Matrix and WeCom on current `main`. Refs: PR NousResearch#58421, comment 4983105207.
Adds 4 home-channel tests for each of the 6 platform setup wizards that already (Discord, Slack, Mattermost, WhatsApp, Matrix, WeCom) or will (Matrix, WeCom — see prior commit) call `remove_env_value()` on a blank home-channel prompt answer: 1. `test_blank_removes_existing_home_channel` — pre-seed env with the home-channel var, run setup with blank input, assert `remove_env_value` was called AND the key is no longer in saved env. 2. `test_blank_without_prior_home_still_attempts_remove` — no pre-seeded env, run setup with blank input, assert `remove_env_value` was called once (safe no-op, but the call confirms the else-branch runs). 3. `test_nonempty_saves_home_channel` — no pre-seeded env, run setup with a real channel ID, assert `save_env_value` was called AND `remove_env_value` was NOT called. 4. `test_whitespace_only_clears_home_channel` — pre-seed env with home-channel, run setup with `" "` input, assert it cleared. This addresses the first finding in maintainer teknium1's `keep_open / salvageability=high` review of NousResearch#58421 (comment 4983105207): the prior regression coverage was Feishu-only, and the existing Slack coverage only confirmed blank input is not saved — it didn't exercise removal of an existing target. The new `TestSlackHomeChannelClear` class augments the existing Slack test file (`tests/gateway/test_slack_plugin_setup.py`) without breaking the two pre-existing tests in that file. The new test files use the Slack-style `monkeypatch` helper rather than the heavier `unittest.mock.patch` style of `tests/gateway/test_setup_feishu.py` because the setup-time I/O surface for each adapter is simpler than Feishu's (QR scan, manual connection mode, etc.). The Matrix test additionally patches `tools.lazy_deps.feature_missing` and `ensure` so the wizard's mautrix auto-install path is a no-op — no pip invocations from unit tests. The WeCom test additionally stubs `qr_scan_for_bot_info` and patches `hermes_cli.setup.prompt_choice` so the QR path is skipped. Total new coverage: 24 tests across 6 new test files + a Slack augmentation (4 tests in `TestSlackHomeChannelClear`). All 44 home-channel tests across 7 adapters pass locally (`pytest tests/gateway/test_discord_plugin_setup.py tests/gateway/test_mattermost_plugin_setup.py tests/gateway/test_whatsapp_plugin_setup.py tests/gateway/test_matrix_plugin_setup.py tests/gateway/test_wecom_plugin_setup.py tests/gateway/test_slack_plugin_setup.py tests/gateway/test_setup_feishu.py`). Refs: PR NousResearch#58421, comment 4983105207.
|
Thanks for the trace and the salvageability tag — both findings addressed in two follow-up commits on top of
Matrix (
5 new test files (Discord/Mattermost/WhatsApp/Matrix/WeCom) + a
Local verification: 44 tests across 7 adapters, all green. The Matrix test additionally patches Cross-vendor dual review (Flash + GPT-OSS) on the rework passed with no BLOCKERs; both reviewers independently accepted the adapter patches, mock semantics, prompt-choice sequence, scope, and commit boundary. Two SHOULD-FIX findings (None-safety on Happy to split the test commit per-adapter or follow up on the prompt-text-consistency NIT separately if you'd prefer. |
Blank (or whitespace-only) answers to the home-channel prompt in the interactive setup wizards previously left any previously saved *_HOME_CHANNEL / *_HOME_ROOM env value in place, so operators could not clear a stale home channel by re-running setup. Strip the prompt input and call remove_env_value() on blank answers across the Discord, Slack, Feishu, Matrix, Mattermost, WeCom and WhatsApp plugin setup wizards, with per-adapter wizard tests covering set/clear/whitespace flows. Squash of the three commits from PR #58421 (setup-wizard fix, matrix/ wecom extension, and 6-adapter test coverage) — one commit per contributor on this salvage branch. Fixes #12423 Salvaged from #58421
Blank (or whitespace-only) answers to the home-channel prompt in the interactive setup wizards previously left any previously saved *_HOME_CHANNEL / *_HOME_ROOM env value in place, so operators could not clear a stale home channel by re-running setup. Strip the prompt input and call remove_env_value() on blank answers across the Discord, Slack, Feishu, Matrix, Mattermost, WeCom and WhatsApp plugin setup wizards, with per-adapter wizard tests covering set/clear/whitespace flows. Squash of the three commits from PR #58421 (setup-wizard fix, matrix/ wecom extension, and 6-adapter test coverage) — one commit per contributor on this salvage branch. Fixes #12423 Salvaged from #58421
Blank (or whitespace-only) answers to the home-channel prompt in the interactive setup wizards previously left any previously saved *_HOME_CHANNEL / *_HOME_ROOM env value in place, so operators could not clear a stale home channel by re-running setup. Strip the prompt input and call remove_env_value() on blank answers across the Discord, Slack, Feishu, Matrix, Mattermost, WeCom and WhatsApp plugin setup wizards, with per-adapter wizard tests covering set/clear/whitespace flows. Squash of the three commits from PR #58421 (setup-wizard fix, matrix/ wecom extension, and 6-adapter test coverage) — one commit per contributor on this salvage branch. Fixes #12423 Salvaged from #58421
Blank (or whitespace-only) answers to the home-channel prompt in the interactive setup wizards previously left any previously saved *_HOME_CHANNEL / *_HOME_ROOM env value in place, so operators could not clear a stale home channel by re-running setup. Strip the prompt input and call remove_env_value() on blank answers across the Discord, Slack, Feishu, Matrix, Mattermost, WeCom and WhatsApp plugin setup wizards, with per-adapter wizard tests covering set/clear/whitespace flows. Squash of the three commits from PR #58421 (setup-wizard fix, matrix/ wecom extension, and 6-adapter test coverage) — one commit per contributor on this salvage branch. Fixes #12423 Salvaged from #58421
Blank (or whitespace-only) answers to the home-channel prompt in the interactive setup wizards previously left any previously saved *_HOME_CHANNEL / *_HOME_ROOM env value in place, so operators could not clear a stale home channel by re-running setup. Strip the prompt input and call remove_env_value() on blank answers across the Discord, Slack, Feishu, Matrix, Mattermost, WeCom and WhatsApp plugin setup wizards, with per-adapter wizard tests covering set/clear/whitespace flows. Squash of the three commits from PR NousResearch#58421 (setup-wizard fix, matrix/ wecom extension, and 6-adapter test coverage) — one commit per contributor on this salvage branch. Fixes NousResearch#12423 Salvaged from NousResearch#58421
Problem
Interactive platform setup wizards (Feishu, Discord, Slack, Mattermost, WhatsApp) could not clear an existing
*_HOME_CHANNELenv var. If a user re-ran setup and left the home-channel prompt blank, the old value was silently preserved. Users had to manually edit~/.hermes/.envto clear it.Reported in #12423 for Feishu; the same pattern existed in 4 other adapters.
Changes
remove_env_valueto the lazy-import block and anelsebranch that calls it when the prompt is left blank, clearing the persisted env var..strip()the prompt result before the emptiness check, so whitespace-only input clears instead of being saved.print_infoline before the prompt explaining that leaving it blank clears a previously saved home channel.else: if remove_env_value(...)instead ofelif remove_env_value(...)to avoid side effects in a conditional expression (caught by review).Tests
_run_setup_feishuhelper to mockremove_env_valueand track removed keys.TestSetupFeishuHomeChannelwith 4 tests:Verification
Notes
hermes_cli/gateway.py; function moved toplugins/platforms/feishu/adapter.py).elsebranch now makes that accurate (empty = clear). Prompt wording could be updated in a follow-up for full consistency.