fix(slack): ignore stale thread sessions - #55240
Conversation
alt-glitch
added
type/bug
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Clean fix — Slack stale thread sessions are now properly ignored by checking _should_reset before returning True. The test is well-designed with a mock Store class that simulates the stale session scenario.
Correctness: The check correctly prevents stale sessions from being treated as active.
Testing: Dedicated test case verifying the stale session returns False.
Reviewed by Hermes Agent
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Fixes Slack thread auto-engagement to treat existing session keys as active only if the session store would keep that entry fresh under the current reset policy.
- Clean fix with dedicated test
- Well-scoped: 2 files, 34 additions
Reviewed 2 files, 34 additions. Approved.
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused stale-session fix. The current-main premise is real: Slack still classifies any retained key as active at plugins/platforms/slack/adapter.py:4043, before routing later evaluates freshness.
Problems
plugins/platforms/slack/adapter.py:3814only consults_should_reset(). The real SessionStore transition additionally treats DB-ended sessions (gateway/session.py:1870,1903-1919),suspendedentries (gateway/session.py:1871-1872), and expiredresume_pendingentries (gateway/session.py:1873-1882) as non-reusable. Those states would still suppress the Slack thread-context reseed.- The added test only stubs an
"idle"result from_should_reset(); it does not compare the helper with actual SessionStore behavior for the other reset paths.
Suggested changes
- Make the Slack active-session check share a non-mutating SessionStore freshness predicate covering the full routing decision.
- Add real-SessionStore coverage for the stale states above.
Automated hermes-sweeper review.
| return False | ||
|
|
||
| should_reset = getattr(type(session_store), "_should_reset", None) | ||
| if callable(should_reset) and should_reset(session_store, entry, source): |
There was a problem hiding this comment.
_should_reset() is only one part of the routing freshness decision. SessionStore._get_or_create_session_impl() also invalidates DB-ended sessions, suspended entries, and expired resume_pending entries (gateway/session.py:1870-1884), so this still returns true for keys that the next routing step will replace.
teknium1
added
sweeper:risk-message-delivery
…eseed A session key that exists in the store but would be rolled to a fresh session by the reset policy (daily/idle/suspended) is not an active session. Treating it as active suppressed the first-turn Slack thread-history reseed after reset (#55239). _has_active_session_for_thread() now consults SessionStore._should_reset so a stale entry gates like a missing one, letting _fetch_thread_context reseed the fresh session with recent thread history. Fixes #55239. Salvaged from #55240 by @ooiuuii.
…eseed A session key that exists in the store but would be rolled to a fresh session by the reset policy (daily/idle/suspended) is not an active session. Treating it as active suppressed the first-turn Slack thread-history reseed after reset (#55239). _has_active_session_for_thread() now consults SessionStore._should_reset so a stale entry gates like a missing one, letting _fetch_thread_context reseed the fresh session with recent thread history. Fixes #55239. Salvaged from #55240 by @ooiuuii.
…eseed A session key that exists in the store but would be rolled to a fresh session by the reset policy (daily/idle/suspended) is not an active session. Treating it as active suppressed the first-turn Slack thread-history reseed after reset (NousResearch#55239). _has_active_session_for_thread() now consults SessionStore._should_reset so a stale entry gates like a missing one, letting _fetch_thread_context reseed the fresh session with recent thread history. Fixes NousResearch#55239. Salvaged from NousResearch#55240 by @ooiuuii.
4 participants
Summary
Fixes #55239.
Slack thread auto-engagement now treats an existing session key as active only if the session store would keep that entry fresh under the current reset policy. If
_should_reset()says the entry is stale,_has_active_session_for_thread()returns false so the first reply after reset can fetch Slack thread context before the fresh session turn starts.Validation
python -m pytest tests\gateway\test_slack_approval_buttons.py -q --basetemp .pytest-tmp-slack-reset— 27 passedpython -m pytest tests\gateway\test_slack.py -q --basetemp .pytest-tmp-slack-main— 209 passedpython -m ruff check plugins\platforms\slack\adapter.py tests\gateway\test_slack_approval_buttons.py— passedgit diff --check origin/main...HEAD— passedPytest emitted existing AsyncMock runtime warnings in Slack tests; no failures.
AI Assistance
Implemented with Codex assistance. I reviewed the diff and ran the focused validation above.