fix(slack): clear assistant typing indicator after response delivery - #8414
fix(slack): clear assistant typing indicator after response delivery#8414lawrence3699 wants to merge 1 commit into
Conversation
When MCP tools execute after the main text response is sent, Slack's
assistant.threads.setStatus indicator ("is thinking...") can persist
for up to 2 minutes instead of auto-clearing.
Override stop_typing() in SlackAdapter to explicitly clear the status
via setStatus(status="") after all processing completes. The base
class cleanup code already calls stop_typing() in the finally block
of _process_message_background(), so this override is picked up
automatically.
Fixes NousResearch#8387
There was a problem hiding this comment.
Pull request overview
Fixes a Slack Assistant UX bug where the assistant.threads.setStatus (“is thinking…”) indicator can persist after the user-visible reply is delivered by explicitly clearing the status at the end of processing.
Changes:
- Track the active Slack
thread_tsused forassistant.threads.setStatusso it can be cleared later. - Add a Slack
stop_typing()override that callsassistant.threads.setStatus(status=""). - Add unit tests validating the new
stop_typing()behavior (clear, no-op, idempotency, error handling).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
gateway/platforms/slack.py |
Tracks thread timestamp used for typing and explicitly clears Slack Assistant status via stop_typing(). |
tests/gateway/test_slack.py |
Adds regression tests for clearing the Slack Assistant typing/status indicator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._typing_thread_ts[chat_id] = thread_ts | ||
|
|
There was a problem hiding this comment.
_typing_thread_ts is keyed only by chat_id (channel_id). BasePlatformAdapter can run multiple concurrent sessions in the same channel differentiated by source.thread_id (session_key includes thread_id), and each session starts its own _keep_typing loop that calls send_typing(chat_id, metadata={"thread_id": ...}). In that case, concurrent send_typing calls will overwrite this dict entry and stop_typing() may clear the wrong thread (or no-op for the correct one), leaving a stuck indicator or clearing an in-flight one. Consider keying the tracking by (chat_id, thread_ts) and/or updating the stop_typing call path to include thread metadata so the clear targets the correct thread.
| # 1 call for send_typing + 1 call for first stop_typing = 2 | ||
| assert adapter._app.client.assistant_threads_setStatus.call_count == 2 | ||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
The regression tests cover single-thread usage, but they don’t exercise the multi-thread-in-one-channel case where two different thread_ids can be active concurrently (a case Hermes supports via session_key including source.thread_id). Adding a test that simulates two send_typing() calls with different thread_id values for the same channel and verifies each corresponding stop_typing clears the right thread would help prevent indicator-clearing regressions once the implementation is updated to track per-thread state.
| @pytest.mark.asyncio | |
| @pytest.mark.asyncio | |
| async def test_clears_matching_thread_when_multiple_threads_active(self, adapter): | |
| adapter._app.client.assistant_threads_setStatus = AsyncMock() | |
| await adapter.send_typing("C123", metadata={"thread_id": "ts1"}) | |
| await adapter.send_typing("C123", metadata={"thread_id": "ts2"}) | |
| await adapter.stop_typing("C123", metadata={"thread_id": "ts1"}) | |
| first_clear_calls = [ | |
| call | |
| for call in adapter._app.client.assistant_threads_setStatus.await_args_list | |
| if call.kwargs == { | |
| "channel_id": "C123", | |
| "thread_ts": "ts1", | |
| "status": "", | |
| } | |
| ] | |
| assert len(first_clear_calls) == 1 | |
| second_thread_clear_calls = [ | |
| call | |
| for call in adapter._app.client.assistant_threads_setStatus.await_args_list | |
| if call.kwargs == { | |
| "channel_id": "C123", | |
| "thread_ts": "ts2", | |
| "status": "", | |
| } | |
| ] | |
| assert second_thread_clear_calls == [] | |
| await adapter.stop_typing("C123", metadata={"thread_id": "ts2"}) | |
| second_thread_clear_calls = [ | |
| call | |
| for call in adapter._app.client.assistant_threads_setStatus.await_args_list | |
| if call.kwargs == { | |
| "channel_id": "C123", | |
| "thread_ts": "ts2", | |
| "status": "", | |
| } | |
| ] | |
| assert len(second_thread_clear_calls) == 1 | |
| @pytest.mark.asyncio |
alt-glitch
added
type/bug
What does this PR do?
When MCP tools execute after the main text response is sent, Slack's
assistant.threads.setStatusindicator ("is thinking...") can persist for up to 2 minutes instead of auto-clearing. This adds an explicitstop_typing()override to clear the status after all processing completes.Related Issue
Fixes #8387
Type of Change
Changes Made
gateway/platforms/slack.py: Added_typing_thread_tsdict to track active thread timestamps, storedthread_tsduringsend_typing(), and addedstop_typing()override that callssetStatus(status="")to explicitly clear the indicator.tests/gateway/test_slack.py: Added 4 regression tests covering: normal clear, no-op without priorsend_typing, idempotent double-call, and graceful API error handling.How to Test
Unit tests:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A