feat(doctor): nudge when Slack has no channel allowlist - #86
Conversation
hermes doctor now warns when SLACK_BOT_TOKEN is set but slack.allowed_channels is empty, since the bot will respond in every channel it's a member of (including any privileged ones it's since been invited to). Informational only — it never sets or infers a channel list; that decision stays with the user. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P96ZSh1Dc9mX3vCd29dieZ
🔎 Lint report:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aba64abce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raw = (cfg.get("slack") or {}).get("allowed_channels") | ||
| if raw is None: | ||
| raw = os.environ.get("SLACK_ALLOWED_CHANNELS", "") |
There was a problem hiding this comment.
load_config_readonly() deep-merges DEFAULT_CONFIG, where slack.allowed_channels is always present as "", so an env-only deployment with SLACK_ALLOWED_CHANNELS=C123 never reaches this raw is None fallback and is incorrectly reported as unrestricted. This contradicts the documented env configuration and the gateway's env-precedence behavior; check the environment value first or read the raw YAML rather than the default-merged config.
Useful? React with 👍 / 👎.
| if not get_env_value("SLACK_BOT_TOKEN"): | ||
| return # Slack isn't configured; nothing to warn about. |
There was a problem hiding this comment.
A retained SLACK_BOT_TOKEN does not mean the Slack gateway is active: slack.enabled: false and platforms.slack.enabled: false are explicitly supported so the token can remain available to outbound skills without activating the adapter. In that configuration this check still claims the bot responds in every joined channel and adds an issue, so it should inspect the effective enabled setting before warning.
Useful? React with 👍 / 👎.
|
|
||
| _section("Slack Configuration") | ||
| cfg = load_config_readonly() | ||
| raw = (cfg.get("slack") or {}).get("allowed_channels") |
There was a problem hiding this comment.
If a syntactically valid but malformed config contains a truthy non-mapping Slack section, such as slack: disabled, this expression calls .get() on a string and aborts the entire doctor run whenever a Slack token exists. Since the existing structure validator does not validate the Slack section and doctor is the recovery path for bad configuration, guard the section type or use the repository's safe cfg_get() helper.
Useful? React with 👍 / 👎.
| cfg = load_config_readonly() | ||
| raw = (cfg.get("slack") or {}).get("allowed_channels") | ||
| if raw is None: | ||
| raw = os.environ.get("SLACK_ALLOWED_CHANNELS", "") |
There was a problem hiding this comment.
Slack's adapter also accepts allowed_channels through platforms.slack.extra and the legacy gateway.json platform configuration, but this check reads only the top-level slack section. Deployments using either supported gateway configuration path therefore receive an unrestricted-channel warning even though SlackAdapter._slack_allowed_channels() enforces their allowlist; derive this diagnostic from the effective gateway configuration or check those sources too.
Useful? React with 👍 / 👎.
2 participants
What does this PR do?
Adds a "Slack Configuration" section to
hermes doctorthat fires only whenSLACK_BOT_TOKENis set, warning ifslack.allowed_channels(config.yaml orSLACK_ALLOWED_CHANNELS) is empty — since an empty allowlist is the documented, backward-compatible default where the bot responds in every channel it's a member of, including any privileged ones it's since been invited to.This is informational only. It does not set, infer, or recommend a specific channel list — that decision (which channels are privileged, which should stay open) is the deployment owner's call, not doctor's.
Related Issue
Linear DAN-2043 ("Scope Hermes allowed_channels to an explicit allowlist") raises the concern that an empty allowlist means the bot responds in every channel it's a member of, including privileged ones (#accounting, #legal, #security in the reporter's workspace), and explicitly says the actual channel-list decision "NEEDS USER SIGN-OFF." This PR only adds the visibility half of that ticket — a
hermes doctornudge — not the channel-list decision itself.Type of Change
Changes Made
hermes_cli/doctor.py: new_check_slack_channel_exposure()helper, wired intorun_doctor()right after the gateway-service-linger check. Silent when Slack isn't configured (noSLACK_BOT_TOKEN); otherwise reportscheck_okwith the channel count orcheck_warn+ an actionable issue when the allowlist is empty.tests/hermes_cli/test_doctor.py: newTestSlackChannelExposureCheckcovering silent-when-unconfigured, warn-when-empty, and ok-when-set (both config.yaml and env var paths).How to Test
uv run pytest tests/hermes_cli/test_doctor.py -q— 67 passed.run_doctor()withSLACK_BOT_TOKENset and no allowlist — shows the new◆ Slack Configurationsection with the warning and fix hint.Checklist
Code
pytest tests/hermes_cli/test_doctor.py -qand all tests passDocumentation & Housekeeping
slack.allowed_channels/SLACK_ALLOWED_CHANNELS), no docs/tool-schema changesGenerated by Claude Code