Skip to content

fix(gateway/slack): batch salvage — reserved commands, session isolation, delivery fixes - #18553

Merged
kshitijk4poor merged 6 commits into
mainfrom
fix/slack-batch-salvage
May 1, 2026
Merged

fix(gateway/slack): batch salvage — reserved commands, session isolation, delivery fixes#18553
kshitijk4poor merged 6 commits into
mainfrom
fix/slack-batch-salvage

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Summary

Batch salvage of 5 focused Slack bug fixes from open community PRs. Each fix is a clean cherry-pick with original authorship preserved; no modifications needed.

Salvaged PRs

PR Author Priority What it fixes
#18456 @priveperfumes P2 bug Reserved Slack built-in commands (/status, /me, /join, etc.) included in the native slash manifest cause Slack API registration failures
#14932 @valda P2 bug YAML free_response_channels: 1234567890 loaded as int silently returns empty set — isinstance(raw, str) gate rejects it
#11893 @nightq P2 bug DeliveryTarget.parse() calls .lower() on the entire target string including Slack channel IDs (C0ABC123) which are case-sensitive
#9361 @Hinotoi-agent P1 security Slash commands hardcode chat_type="dm" for all channels — users in shared channels share one agent session (session collision)
#17798 @amroessam P2 bug Slack "is thinking…" assistant status indicator lingers after the bot replies — no stop_typing() call in send()/edit_message()

Changes by file

File PRs What
hermes_cli/commands.py #18456 _SLACK_RESERVED_COMMANDS frozenset + filter in _add() to skip 19 reserved Slack built-ins
gateway/platforms/slack.py #14932, #9361, #17798 Coerce free_response_channels to str; detect DM vs group channel for slash commands; call stop_typing() after final send/edit
gateway/platforms/discord.py #14932 Same free_response_channels str coercion (Discord adapter has the identical bug)
gateway/delivery.py #11893 Only lowercase the platform prefix, preserve case for chat_id and thread_id
scripts/release.py AUTHOR_MAP entries for 4 new contributors
tests/ all Tests from each PR cherry-picked with the fix

PRs closed as redundant (in follow-up comments)

PR Reason
#18453 Redundant with #18456 — renames /status/status1 (worse UX vs clean exclusion)
#12193 Already fixed in PR #18198 (app_mention forwarding)
#17081 Incorrect — Slack API docs allow hyphens in command names
#10875 Overlaps with #9361; riskier scope (thread_ts session key changes)

Test plan

  • 429 tests passed across affected test files (0 failures)
  • All changed .py files pass py_compile
  • Each cherry-pick applied cleanly with zero conflicts

prive-fe-bot and others added 6 commits May 2, 2026 02:18
Slack has built-in slash commands (e.g. /status, /me, /join) that apps
cannot register. When running `hermes slack manifest --write`, the
generated manifest included /status, causing Slack to reject the entire
manifest with a reserved-command error.

Add _SLACK_RESERVED_COMMANDS frozenset of all known Slack built-ins and
skip them in slack_native_slashes(). Affected commands remain reachable
via /hermes <command>.

Tests updated:
- New test_excludes_slack_reserved_commands validates no leaks
- test_includes_canonical_commands no longer asserts /status
- test_telegram_parity accounts for expected Slack-only exclusions
YAML loads a bare numeric value such as
    discord:
      free_response_channels: 1491973769726791812
as an int.  _discord_free_response_channels() / _slack_free_response_channels()
checked `isinstance(raw, list)` and `isinstance(raw, str)` in that order and
then fell through to `return set()`, so a single-channel config that happened
to be unquoted was silently dropped with no log line — the bot kept demanding
@mentions even though the channel was configured to free-response.

A multi-channel value like `1234567890,9876543210` does not trip this because
the comma forces YAML to parse it as a string.  Single-channel configs are
the only case that breaks, which is exactly the footgun that's hardest to
diagnose (the config "looks right" and the feature just doesn't activate).

Note that the old-schema env-var bridge at gateway/config.py:614+ already
runs `str(frc)` when forwarding to SLACK_/DISCORD_FREE_RESPONSE_CHANNELS,
so the env-var fallback worked.  The bug only surfaces on the
`config.extra["free_response_channels"]` path populated by the `platforms:`
bridge at gateway/config.py:576, which passes the raw YAML value through
unchanged.

Fix at the reader: treat any non-list value as a scalar, coerce with str(),
then apply the same CSV split semantics.  This keeps the public contract
stable (list or str-like continues to work identically) while accepting
the ints that the YAML loader is free to hand us.

Added tests for both Discord and Slack covering:
  - bare int value in config.extra
  - list of ints in config.extra
Fixes #11768

Root cause: target.strip().lower() was lowercasing the entire target string,
corrupting case-sensitive chat IDs like Slack C123ABC and Matrix !RoomABC.

Fix: Only lowercase the platform prefix for case-insensitive matching;
preserve the original case for chat_id and thread_id values.
Adds email→username mappings for:
- priveperfumes (PR #18456)
- amroessam (PR #17798)
- Hinotoi-agent (PR #9361)
- valda (PR #14932)
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery comp/cli CLI entry point, hermes_cli/, setup wizard platform/slack Slack app adapter platform/discord Discord bot adapter labels May 1, 2026
@kshitijk4poor
kshitijk4poor merged commit f903cee into main May 1, 2026
10 of 11 checks passed
@kshitijk4poor
kshitijk4poor deleted the fix/slack-batch-salvage branch May 1, 2026 21:01
briandevans added a commit to briandevans/hermes-agent that referenced this pull request May 30, 2026
…hats

A single Slack channel / DM can host multiple Assistant threads at the
same time (overlapping user requests, parallel sessions, etc.). The
typing-indicator tracker was keyed by chat_id alone:

    self._active_status_threads: Dict[str, str] = {}

so the second concurrent `send_typing` for the same chat overwrote the
first thread's `thread_ts`. When the earlier thread's send finished,
`stop_typing` popped the wrong (newer) `thread_ts` — and the older
thread stayed stuck in "is thinking…" forever, as reported in NousResearch#24117.

The existing fix landed via NousResearch#18553 (April 22) covered the single-thread
lifecycle (set status on send_typing, clear on send / edit-finalize).
This PR extends that lifecycle to concurrent threads:

- Track per-thread instead of per-chat: `Dict[str, set[str]]`.
- `send_typing` adds the thread_ts to the chat's set (bounded at 128
  active threads per chat as a defensive guard against missed clears).
- `stop_typing(chat_id, metadata={"thread_ts": ts})` clears that
  specific thread. Without metadata, every tracked thread in the chat
  is cleared (preserves the legacy base.py wiring that has no thread
  context).
- `send()` finalize now passes its own `thread_ts` to `stop_typing` so
  it clears only its own thread among concurrent ones.
- `send()` exception path now also clears the thread's status, so a
  failed Slack post doesn't leave the user staring at "is thinking…".
- `edit_message(finalize=True)` keeps its existing behavior (clears
  whatever's tracked for the chat — it has no thread context to
  target a specific thread).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery P1 High — major feature broken, no workaround platform/discord Discord bot adapter platform/slack Slack app adapter type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants