fix(slack): route replies from mentioned thread parents - #24848
Closed
kaiyisg wants to merge 1 commit into
Closed
Conversation
alt-glitch
added
type/bug
Contributor
|
Thanks for isolating a real Slack routing gap. Current main still rejects an unmentioned thread reply with no session before inspecting the parent: Problems
Suggested changes
Automated hermes-sweeper review. |
teknium1
added
sweeper:risk-session-state
teknium1
pushed a commit
that referenced
this pull request
Jul 22, 2026
Two mention-tracking gaps around thread parents (#24848): 1. When a thread PARENT @-mentioned the bot (e.g. '<@bot> check this and ask me before running'), a later bare reply like 'run' fell through every wake check if the mention event predated this process (restart) — _mentioned_threads is in-memory only. Add a 5th wake check that fetches the parent text (with the bot mention preserved via strip_bot_mention=False) and wakes when the parent addressed the bot, registering the thread so later replies skip the fetch. 2. A TOP-LEVEL @mention starts a thread (session keying falls back to the message ts), but only the raw event thread_ts was registered in _mentioned_threads — so replies to a top-level mention did not auto-trigger. Register the session-scoped thread_ts instead. _fetch_thread_parent_text reuses the shared thread-context cache (raw payloads) so the parent check costs at most one conversations.replies call per thread; _register_mentioned_thread centralizes the bounded-set eviction. Salvaged from #24848 by @kaiyisg, rebased onto the extracted _should_wake_on_unmentioned_message helper.
teknium1
added a commit
that referenced
this pull request
Jul 22, 2026
teknium1
pushed a commit
that referenced
this pull request
Jul 22, 2026
Two mention-tracking gaps around thread parents (#24848): 1. When a thread PARENT @-mentioned the bot (e.g. '<@bot> check this and ask me before running'), a later bare reply like 'run' fell through every wake check if the mention event predated this process (restart) — _mentioned_threads is in-memory only. Add a 5th wake check that fetches the parent text (with the bot mention preserved via strip_bot_mention=False) and wakes when the parent addressed the bot, registering the thread so later replies skip the fetch. 2. A TOP-LEVEL @mention starts a thread (session keying falls back to the message ts), but only the raw event thread_ts was registered in _mentioned_threads — so replies to a top-level mention did not auto-trigger. Register the session-scoped thread_ts instead. _fetch_thread_parent_text reuses the shared thread-context cache (raw payloads) so the parent check costs at most one conversations.replies call per thread; _register_mentioned_thread centralizes the bounded-set eviction. Salvaged from #24848 by @kaiyisg, rebased onto the extracted _should_wake_on_unmentioned_message helper.
teknium1
added a commit
that referenced
this pull request
Jul 22, 2026
Contributor
|
Merged via #69320 — your commit was cherry-picked onto current main with your authorship preserved in git history: your mentioned-thread-parent reply routing was cherry-picked directly. Thanks for the contribution! |
teknium1
added a commit
that referenced
this pull request
Jul 22, 2026
…ure parity Follow-up rework on the #51627 cherry-pick: - Guard the 5th wake check (parent-mentioned-bot, #24848) against a None parent_text — _fetch_thread_parent_text is typed to return str but tests (and defensive callers) can surface None; 'in None' raised TypeError. - Drop the PR's fixture-level _fetch_thread_parent_text/_fetch_thread_context AsyncMocks from TestThreadReplyHandling/TestAssistantThreadLifecycle: main's #24848 tests exercise the real parent-text path via conversations_replies side effects, and the blanket mocks broke them. - _resolve_user_is_bot reworked to the workspace-scoped (team_id, user_id) cache key introduced by the multi-workspace name cache on main.
teknium1
added a commit
that referenced
this pull request
Jul 23, 2026
…ure parity Follow-up rework on the #51627 cherry-pick: - Guard the 5th wake check (parent-mentioned-bot, #24848) against a None parent_text — _fetch_thread_parent_text is typed to return str but tests (and defensive callers) can surface None; 'in None' raised TypeError. - Drop the PR's fixture-level _fetch_thread_parent_text/_fetch_thread_context AsyncMocks from TestThreadReplyHandling/TestAssistantThreadLifecycle: main's #24848 tests exercise the real parent-text path via conversations_replies side effects, and the blanket mocks broke them. - _resolve_user_is_bot reworked to the workspace-scoped (team_id, user_id) cache key introduced by the multi-workspace name cache on main.
teknium1
added a commit
that referenced
this pull request
Jul 23, 2026
…ure parity Follow-up rework on the #51627 cherry-pick: - Guard the 5th wake check (parent-mentioned-bot, #24848) against a None parent_text — _fetch_thread_parent_text is typed to return str but tests (and defensive callers) can surface None; 'in None' raised TypeError. - Drop the PR's fixture-level _fetch_thread_parent_text/_fetch_thread_context AsyncMocks from TestThreadReplyHandling/TestAssistantThreadLifecycle: main's #24848 tests exercise the real parent-text path via conversations_replies side effects, and the blanket mocks broke them. - _resolve_user_is_bot reworked to the workspace-scoped (team_id, user_id) cache key introduced by the multi-workspace name cache on main.
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
Two mention-tracking gaps around thread parents (NousResearch#24848): 1. When a thread PARENT @-mentioned the bot (e.g. '<@bot> check this and ask me before running'), a later bare reply like 'run' fell through every wake check if the mention event predated this process (restart) — _mentioned_threads is in-memory only. Add a 5th wake check that fetches the parent text (with the bot mention preserved via strip_bot_mention=False) and wakes when the parent addressed the bot, registering the thread so later replies skip the fetch. 2. A TOP-LEVEL @mention starts a thread (session keying falls back to the message ts), but only the raw event thread_ts was registered in _mentioned_threads — so replies to a top-level mention did not auto-trigger. Register the session-scoped thread_ts instead. _fetch_thread_parent_text reuses the shared thread-context cache (raw payloads) so the parent check costs at most one conversations.replies call per thread; _register_mentioned_thread centralizes the bounded-set eviction. Salvaged from NousResearch#24848 by @kaiyisg, rebased onto the extracted _should_wake_on_unmentioned_message helper.
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
…lvage LevSky22 (NousResearch#66069), vexclawx31 (NousResearch#33215), knoal (NousResearch#64067), kaiyisg (NousResearch#24848).
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
…ure parity Follow-up rework on the NousResearch#51627 cherry-pick: - Guard the 5th wake check (parent-mentioned-bot, NousResearch#24848) against a None parent_text — _fetch_thread_parent_text is typed to return str but tests (and defensive callers) can surface None; 'in None' raised TypeError. - Drop the PR's fixture-level _fetch_thread_parent_text/_fetch_thread_context AsyncMocks from TestThreadReplyHandling/TestAssistantThreadLifecycle: main's NousResearch#24848 tests exercise the real parent-text path via conversations_replies side effects, and the blanket mocks broke them. - _resolve_user_is_bot reworked to the workspace-scoped (team_id, user_id) cache key introduced by the multi-workspace name cache on main.
nikitaBarkov
added a commit
to JetBrains/hermes-agent
that referenced
this pull request
Aug 15, 2026
The adapter deletes the bot's own `<@U…>` token from the text before the agent reads it, and reports nothing in its place. A thread keeps waking the bot after the first mention, so every delivered turn then looks identical: the agent cannot tell "someone tagged me" from "I was woken by thread routing" — the distinction an agent needs to decide for itself whether a turn deserves an answer. Add `slack.strip_bot_mentions` (default `true` — today's behavior byte for byte, so nothing changes for anyone who does not opt in). With `false` the mention stays where the author put it, rendered as `@BotName`, the same shape `_humanize_user_mentions()` gives mentions of other participants; its absence then means the bot was woken by channel or thread routing. The asymmetry is the signal — no marker text is injected. The name comes from `_team_bot_names` / `_bot_display_name`, both resolved at connect time, so there is no extra Slack call; an unresolved name leaves the raw token rather than deleting it. Routing is untouched (`is_mentioned`, `_mentioned_threads`, `require_mention`, `strict_mention`, `thread_require_mention` are all evaluated before this), and so is command parsing, which runs off the separate `mention_stripped` variable. - `_slack_strip_bot_mentions()` resolves `config.extra` → `SLACK_STRIP_BOT_MENTIONS` → default `true`, like its four siblings. - Thread history follows the same policy (`_render_message_text`, `_format_thread_context`), so past turns don't read as "nobody ever tagged me"; block content is compared against the text as written, since the blocks carry the raw token. - `_fetch_thread_parent_text()` forwards the caller's `strip_bot_mention` into the render on a cache miss. That path is the root-mention wake check (NousResearch#24848), which greps the parent for the raw `<@id>`, and the render deleted it unconditionally — so on a cold cache the check could never match, in either flag state. - Documented in the Slack guide and the environment-variable reference; `config.yaml` is the canonical place, the env var is a mirror. - 51 tests over both flag states. Co-authored-by: Junie <junie@jetbrains.com>
19 tasks
chenwei791129
added a commit
to chenwei791129/hermes-agent
that referenced
this pull request
Aug 16, 2026
…ments NousResearch#52387 was fixed for the carrier Slack's WYSIWYG composer produces — a rich_text tree with a structured `user` element. Two other carriers are still dropped, so a bot that explicitly @-mentions the gateway stays invisible to the allow_bots="mentions" gate (adapter.py:5329) and to is_mentioned routing (adapter.py:5603). Root cause, two independent gaps: 1. `_collect_slack_block_mentions` recurses only through ("elements", "element") and appends only for `type == "user"` nodes. A section/header/context block carries its content under "text" (a dict) or "fields" (a list), neither of which is walked; and a hand-built app writes the mention as a raw <@uid> substring, so there is no `user` node to match even once the subtree is entered. 2. Detection returns early when `event["blocks"]` is falsy and never consults `attachments`, so a mention living in an attachment field or in attachment-nested blocks is invisible. NousResearch#69316 added `_extract_text_from_slack_attachments` for exactly these apps (Alertmanager, Grafana, PagerDuty, CI) but applied it to display only. The walker also descends "text"/"fields" and harvests raw tokens from string values; a new `_collect_slack_attachment_mentions` covers the legacy carrier including attachment-nested blocks. A shared `_SLACK_USER_MENTION_RE` normalizes the labelled `<@u123|alice>` form to the bare token the gates compare against, with a deliberately permissive ID class: the gates substring-compare against whatever auth.test returned, so a narrower class would silently drop the mention this recovers. Extraction is factored into `_extract_mention_tokens`, so the carve-outs below live in one place and apply to every carrier. Recovered mentions are returned as a list by `_slack_recovered_mentions` rather than spliced into the routing text. Splicing would corrupt the two other consumers of that text: `_slack_message_addressed_to_other_user` reads its *first* token — and with an empty top-level text (the alert-bot shape this fixes) the appended tail becomes that token, so the message is dropped as "addressed to someone else" — while user-configured wake-word regexes are matched with `.search`, so an anchored pattern like `^hey hermes$` stops matching the moment a tail is appended. The gates now consume `_slack_event_mentions_bot` / `_slack_mention_gate_inputs`, which keep the routing text byte-identical to `event["text"]` and report recovered mentions through `is_mentioned` instead. The quote carve-out from NousResearch#52390 is preserved and extended to the carriers the structured `rich_text_quote` check cannot see: - a leading mrkdwn blockquote marker (`>` / the escaped `>`) in any scanned string, so a peer app quoting an earlier request as context does not re-summon the bot; - attachments flagged `is_msg_unfurl` or `is_share`, mirroring the skip the agent-text path already performs at adapter.py:5483, so pasting a permalink to an old bot request does not wake it; - `fallback` is not scanned at all — Slack never renders it, so a mention living only there is invisible in the channel and notifies nobody. Defensive handling is per-attachment rather than around the whole loop: one malformed sibling now skips itself instead of discarding the genuine mentions already collected, which would otherwise reintroduce the exact silent drop this commit fixes. The thread-parent wake check (NousResearch#24848) is migrated too. Its cached branch returned the raw `msg["text"]` — empty for app-authored parents — so a plain follow-up reply in an alert thread was dropped after a restart. Tests: 28 new cases in tests/gateway/test_slack_mention.py (27 -> 55) covering all seven carriers, the labelled form, dedupe, six malformed payloads, each carve-out, and the two routing-text regressions. The gating simulation and the new gate cases call the production predicates directly, so the tests cannot pass while the real gate diverges. They fail on unmodified main and pass with the fix. All 27 tests/gateway/test_slack*.py files pass (337 tests).
chenwei791129
added a commit
to chenwei791129/hermes-agent
that referenced
this pull request
Aug 16, 2026
Review catch on NousResearch#75312: the NousResearch#24848 thread-parent wake check read `_fetch_thread_parent_text()`, but that is *display* text. `_render_message_text` deliberately preserves `rich_text_quote` content so the agent can read what was quoted, and extracts attachment text with no `is_msg_unfurl`/`is_share` exclusion and a `fallback` fallthrough. Since the caller woke on any `<@bot>` substring, a thread whose parent merely quoted or shared a mention of the bot would wake on every subsequent plain reply — re-opening the agent-agent re-trigger loop the carve-outs exist to close. The two parent paths also disagreed: the cached path already ran the filtered predicate while the cold path did not, so behavior depended on whether the thread cache happened to be warm. Split the two concerns instead of filtering display text: - `_fetch_thread_parent_event()` returns the raw parent payload, cache-first. - `_thread_parent_mentions_bot()` decides the wake through `_slack_event_mentions_bot()` — the same predicate the live channel gates use, so every carve-out applies to both paths by construction rather than by being re-added here. - `_fetch_thread_parent_text()` goes back to being display-only for reply_to_text injection, with a docstring saying it must not be substring-tested. Its `strip_bot_mention` flag existed solely for the wake caller and is gone. `_slack_mention_detection_text()` had that same caller as its last consumer, so it is removed rather than left as a helper whose docstring warns against every remaining use of it; its tests now assert on the recovered-token list, which is the actual contract. Cold and cached parents are covered for quote, unfurl, share, fallback and preformatted carriers, plus positive cases so the wake check itself is pinned, plus one test asserting the display renderer still surfaces quoted content — that split is the point of the change.
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
3 participants
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
Verification