Skip to content

fix(slack): resume unmentioned bot thread replies after restart - #44800

Closed
m2jvrk8yhv-droid wants to merge 1 commit into
NousResearch:mainfrom
m2jvrk8yhv-droid:fix/slack-thread-reply-restart-safe
Closed

fix(slack): resume unmentioned bot thread replies after restart#44800
m2jvrk8yhv-droid wants to merge 1 commit into
NousResearch:mainfrom
m2jvrk8yhv-droid:fix/slack-thread-reply-restart-safe

Conversation

@m2jvrk8yhv-droid

Copy link
Copy Markdown

Summary

  • Allows unmentioned replies inside Slack channel threads to continue being processed after a gateway restart when require_mention=true and strict_mention=false, if Slack thread history proves this bot already participated earlier in the thread.
  • Keeps top-level channel messages mention-gated.
  • Records confirmed bot thread participation back into the bounded in-memory thread participation cache.

Safety

  • Mention gate preserved: the new Slack history fallback only runs for channel thread replies; top-level channel messages remain gated by mention/session rules.
  • Strict mention preserved: strict_mention=true returns before any history lookup and does not call conversations.replies for unmentioned thread replies.
  • Fail-closed Slack history lookup: Slack API exceptions and ok: false responses return False and do not route the message.
  • Bounded lookup: uses conversations.replies with latest=<current message ts>, inclusive=false, and limit=30; no unbounded scans.

Verification

  • uv run pytest -q tests/gateway/test_slack.py::TestMessageRouting -o 'addopts=' -> 11 passed
  • uv run pytest -q tests/gateway/test_slack.py -o 'addopts=' -> 200 passed, 2 warnings
  • uv run python -m compileall -q gateway/platforms/slack.py tests/gateway/test_slack.py -> passed
  • uv run ruff check gateway/platforms/slack.py tests/gateway/test_slack.py -> All checks passed

Caveat

No intentional live Slack channel write in this PR; runtime already restarted from local checkout by Bob.

@m2jvrk8yhv-droid
m2jvrk8yhv-droid marked this pull request as ready for review June 12, 2026 09:33
@liuhao1024

Copy link
Copy Markdown
Contributor

Code Review — Verified Clean ✅

Reviewed the full diff (gateway/platforms/slack.py + tests/gateway/test_slack.py) — this looks solid.

What I checked:

  • Fail-closed on API errors (returns False when conversations_replies fails) — correct
  • Bounded window (latest=current_ts, limit=30) avoids fetching full thread history
  • _record_bot_thread_participation is called on match so subsequent checks use in-memory cache instead of re-fetching
  • Existing _bot_message_ts / has_session / in_mentioned_thread guards run first — API call only triggers when all three are empty (restart scenario)
  • Backward-compatible wrapper for _record_uploaded_file_thread preserves existing call sites
  • Test coverage: bot participation found, no participation, bounded window, and API failure paths

No issues found. The restart-safe fallback pattern (check in-memory state → fall back to platform API → fail closed) is well-designed for the Slack threading model.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery platform/slack Slack app adapter P2 Medium — degraded but workaround exists labels Jun 12, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verified: the restart-safe fallback for unmentioned thread replies correctly falls back to Slack history when in-memory state is lost.

What was checked:

  • The _thread_has_prior_bot_participation method is gated by is_thread_reply — non-thread messages never trigger the extra API call.
  • Fail-closed design: API exceptions, ok: false responses, and empty message lists all return False, which means the reply is dropped (correct conservative behavior for channel messages without a mention).
  • The conversations.replies call uses latest=current_ts, inclusive=False, limit=30 — bounded window before the triggering message, not the full thread history.
  • The current message's ts is explicitly skipped via timestamp comparison to avoid self-matching.
  • strict_mention mode correctly bypasses the entire history check (the fetch never runs).
  • Test coverage is solid: happy path (prior bot reply found), no-participation (dropped), API failure (fail-closed), ok: false response (fail-closed), strict mention (bypass), and long-thread bounded window.

LGTM. Clean architectural fix — the conversations.replies call is correctly bounded and gated so it doesn't add overhead for non-thread or already-tracked messages.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Overview

Fixes Slack bot to resume unmentioned bot thread replies after restart by properly tracking and backfilling missed mentions.

Assessment

  • Correctness: 419 lines with 15 test references. The fix appears to address the restart scenario where the bot needs to track which threads it was participating in.
  • Code Quality: Clean, well-tested.
  • Testing: Good test coverage (15 test references).
  • Security: No security concerns. 0 print statements — no debug artifacts.

Reviewed by Hermes Agent

@m2jvrk8yhv-droid

Copy link
Copy Markdown
Author

Closing this for now since we no longer need the Slack thread restart behavior on our side.

Focused local verification passed before closing:

  • tests/gateway/test_slack.py: 200 passed
  • ruff check gateway/platforms/slack.py tests/gateway/test_slack.py: passed

Happy for maintainers to reopen or reuse the patch if this is still useful upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists platform/slack Slack app adapter type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants