fix(slack): suppress explicit NO_REPLY sentinel before chat_postMessage#30936
Open
briandevans wants to merge 2 commits into
Open
fix(slack): suppress explicit NO_REPLY sentinel before chat_postMessage#30936briandevans wants to merge 2 commits into
briandevans wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds explicit handling for a NO_REPLY sentinel in the Slack adapter so intentional silence does not post the literal sentinel to Slack, and introduces tests validating suppression/posting behavior.
Changes:
- Introduce
_NO_REPLY_SENTINEL = "NO_REPLY"in the Slack platform adapter. - Suppress
chat_postMessagewhencontent.strip() == "NO_REPLY"and return a successfulSendResultwith a suppressed marker. - Add pytest coverage for exact-match, whitespace, case-variant, and “mentioned in prose” scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| gateway/platforms/slack.py | Adds a NO_REPLY sentinel constant and early-return suppression logic in SlackAdapter.send(). |
| tests/gateway/test_slack_no_reply_sentinel.py | Adds unit tests and module mocks to validate Slack suppression behavior without Slack deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
775
to
790
| if not self._app: | ||
| return SendResult(success=False, error="Not connected") | ||
|
|
||
| if isinstance(content, str) and content.strip() == _NO_REPLY_SENTINEL: | ||
| logger.debug( | ||
| "[Slack] Suppressed explicit NO_REPLY sentinel for channel %s", | ||
| chat_id, | ||
| ) | ||
| return SendResult( | ||
| success=True, | ||
| message_id=None, | ||
| raw_response={"suppressed": _NO_REPLY_SENTINEL}, | ||
| ) | ||
|
|
||
| try: | ||
| # Check for a pending slash-command context. When the user ran a |
Comment on lines
+778
to
+787
| if isinstance(content, str) and content.strip() == _NO_REPLY_SENTINEL: | ||
| logger.debug( | ||
| "[Slack] Suppressed explicit NO_REPLY sentinel for channel %s", | ||
| chat_id, | ||
| ) | ||
| return SendResult( | ||
| success=True, | ||
| message_id=None, | ||
| raw_response={"suppressed": _NO_REPLY_SENTINEL}, | ||
| ) |
Comment on lines
+18
to
+40
| if "slack_bolt" in sys.modules and hasattr(sys.modules["slack_bolt"], "__file__"): | ||
| return | ||
|
|
||
| slack_bolt = MagicMock() | ||
| slack_bolt.async_app.AsyncApp = MagicMock | ||
| slack_bolt.adapter.socket_mode.async_handler.AsyncSocketModeHandler = MagicMock | ||
|
|
||
| slack_sdk = MagicMock() | ||
| slack_sdk.web.async_client.AsyncWebClient = MagicMock | ||
|
|
||
| for name, mod in [ | ||
| ("slack_bolt", slack_bolt), | ||
| ("slack_bolt.async_app", slack_bolt.async_app), | ||
| ("slack_bolt.adapter", slack_bolt.adapter), | ||
| ("slack_bolt.adapter.socket_mode", slack_bolt.adapter.socket_mode), | ||
| ("slack_bolt.adapter.socket_mode.async_handler", slack_bolt.adapter.socket_mode.async_handler), | ||
| ("slack_sdk", slack_sdk), | ||
| ("slack_sdk.web", slack_sdk.web), | ||
| ("slack_sdk.web.async_client", slack_sdk.web.async_client), | ||
| ]: | ||
| sys.modules.setdefault(name, mod) | ||
|
|
||
| sys.modules.setdefault("aiohttp", MagicMock()) |
Comment on lines
+778
to
+787
| if isinstance(content, str) and content.strip() == _NO_REPLY_SENTINEL: | ||
| logger.debug( | ||
| "[Slack] Suppressed explicit NO_REPLY sentinel for channel %s", | ||
| chat_id, | ||
| ) | ||
| return SendResult( | ||
| success=True, | ||
| message_id=None, | ||
| raw_response={"suppressed": _NO_REPLY_SENTINEL}, | ||
| ) |
Collaborator
Contributor
Author
|
Thanks for the cross-link, @alt-glitch — for context, #30646 was self-closed by @shawnpetros without review, so there's no active competing PR. The two approaches are functionally equivalent (module-level The two CI failures ( |
|
To be fair, I think that only closed itself because of a visibility thing on my fork. Either way, would love to see this resolved upstream so I don't have to maintain the patch on my end. |
Contributor
Author
|
@copilot All four findings addressed in ee354d628:
Also added regression test Verified locally with the full slack test set (282 passed, no leaks). |
18543c2 to
d9e217a
Compare
9d52775 to
0992c64
Compare
c5323ab to
c29ff67
Compare
c110231 to
64e35bc
Compare
The Slack adapter posts whatever assistant text it is handed via ``send``,
including the explicit ``NO_REPLY`` sentinel some agents emit on group/thread
turns that should remain silent. That posts the literal "NO_REPLY" into the
channel, turning an intentional non-reply into visible clutter.
Match the precedent in ``gateway/platforms/feishu_comment.py``: when the
agent returns exactly ``NO_REPLY`` (after stripping surrounding whitespace),
short-circuit ``send`` with a ``SendResult(success=True, raw_response={
"suppressed": "NO_REPLY"})``. The check is exact post-strip so longer messages
that merely mention NO_REPLY ("I would not use NO_REPLY here") still post
normally; the guard is placed before the slash-command branch so ephemeral
slash replies are also covered.
Tests cover exact match, whitespace tolerance, the longer-prose case, the
empty-string fall-through, and the case-sensitive boundary.
Addresses Copilot review feedback on the NO_REPLY sentinel handling: - gateway/platforms/slack.py: the suppression branch now runs AFTER ``_pop_slash_context`` so it consumes any stashed slash-command ``response_url`` even when skipping the post. Previously a NO_REPLY turn that followed a native slash command (e.g. ``/q``) left the ``response_url`` un-popped, so a later unrelated ``send()`` to the same channel/user pair would inherit the stale context and be routed ephemerally to the wrong command. - gateway/platforms/slack.py: when suppressing after a slash command, call ``response_url`` with ``delete_original: true`` via a new ``_delete_slash_ephemeral`` helper so the dangling "Running /cmd…" placeholder is removed rather than left up forever. - gateway/platforms/slack.py: also call ``stop_typing(chat_id)`` so the assistant "is thinking…" indicator (set by ``send_typing``) is cleared when the turn returns silence — symmetric with the normal post-send clear at the end of ``send()``. - tests/gateway/test_slack_no_reply_sentinel.py: replaced the import- time ``sys.modules.setdefault`` shim (which could shadow real installed packages such as ``aiohttp``) with an autouse ``monkeypatch.setitem`` fixture that only installs stubs for modules that are genuinely absent and removes them after each test, so the stubs cannot leak into other gateway tests in the same run. - tests/gateway/test_slack_no_reply_sentinel.py: added regression test ``test_send_no_reply_consumes_slash_context_and_clears_typing`` that seeds a slash context, sends ``NO_REPLY``, and asserts the context was popped, ``_delete_slash_ephemeral`` was called with the seeded ``response_url``, ``stop_typing`` was called, and no public ``chat_postMessage`` was issued. Verified locally with the full slack test set (``test_slack.py``, ``test_slack_approval_buttons.py``, ``test_slack_channel_skills.py``, ``test_slack_mention.py``, ``test_slack_no_reply_sentinel.py``): 282 passed, no leaks.
64e35bc to
6e5afd6
Compare
Contributor
Author
|
Quick status check three weeks on: this PR is still conflict-free against current |
|
@briandevans thanks for keeping this warm. I re-checked the upstream state and I think this PR is still the right narrow fix for #30644. What I checked just now:
Given that, my ask to maintainers is: please merge this one as the surgical Slack fix for #30644, and let the broader gateway-level silence-token cleanup proceed separately when its stack is ready. This closes a real Slack footgun without committing the repo to the larger design yet. |
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
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.
What does this PR do?
When the agent returns exactly
NO_REPLYon a Slack turn (the explicit"leave the thread untouched" sentinel some agents emit on group/thread
conversations), the Slack adapter posts the literal sentinel into the
channel/thread instead of treating the turn as intentional silence.
This adds a small final-send guard at the top of
SlackAdapter.sendthatmatches the existing precedent in
gateway/platforms/feishu_comment.py(
_NO_REPLY_SENTINEL+ skip-delivery branch). Whencontent.strip() == "NO_REPLY",sendreturnsSendResult(success=True, raw_response={"suppressed": "NO_REPLY"})without callingchat_postMessage.The check is exact and case-sensitive after whitespace trimming, so:
"NO_REPLY"and" NO_REPLY\n"are suppressed."I would not use NO_REPLY here"(longer prose mentioning the token) isstill posted normally.
""and"no_reply"are not the sentinel and fall through to thenormal send path.
The guard is placed before the slash-command branch, so ephemeral slash
replies via
response_urlare also suppressed when the agent returns thesentinel.
Related Issue
Fixes #30644
Type of Change
Changes Made
gateway/platforms/slack.py— module-level_NO_REPLY_SENTINELand aguard at the top of
SlackAdapter.sendthat short-circuits delivery whencontent.strip() == _NO_REPLY_SENTINEL.tests/gateway/test_slack_no_reply_sentinel.py— new test file coveringexact-match suppression, whitespace tolerance, longer-prose preservation,
empty-string fall-through, and case-sensitive boundary.
How to Test
```
uv run --with pytest --with pytest-xdist --with pytest-asyncio --with pytest-timeout python3 -m pytest tests/gateway/test_slack_no_reply_sentinel.py tests/gateway/test_slack.py -v
```
Expected: 6 new tests pass + 186 existing Slack tests continue to pass.
Checklist
Code
Documentation & Housekeeping
Screenshots / Logs
N/A — covered by the new regression tests.