fix(gateway): suppress NO_REPLY/[SILENT] markers on the streaming path - #179
Conversation
|
Review Complete Files Reviewed: 3 By Severity:
The silence-marker suppression feature has two leak paths where raw control markers (e.g. 'NO_REPLY', '[SILENT]') can reach chat platforms: async task cancellation in the stream consumer leaves stale state, and the pending-follow-up delivery path bypasses the intentional-silence filter. Files Reviewed (3 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟠 High (68/100) — 1 high finding · 347 LOC across 3 files
PR #179 — Silenco-marker suppression review
This PR adds intentional silence marker suppression via _suppress_silence_marker() in gateway/stream_consumer.py and related filtering in gateway/response_filters.py. The feature correctly suppresses streamed silence markers during normal operation but exposes two leak paths that can deliver raw control markers to end-users on chat platforms.
finding-001 (high) — Cancellation leak in stream_consumer.py
The state-reset block in _suppress_silence_marker() runs after the delete_fn() deletion loop. If the consumer task is cancelled by asyncio during an await delete_fn() call, CancelledError propagates to the outer handler in run(), where _accumulated still holds the raw marker text and _message_id is set. The handler's _send_or_edit then delivers the raw marker permanently to the platform.
finding-002 (medium) — Pending-follow-up bypass in run.py
After silence suppression clears _final_response_sent, the pending-follow-up delivery path (when a user sends a queued message during agent processing) reads result['final_response'] and sends it without checking for intentional silence. The _stream_confirmed_final_delivery guard that previously prevented this relied on _final_response_sent being True — but suppression now sets it to False.
Files reviewed
gateway/response_filters.py— newis_intentional_silence_agent_result()and silence marker definitionsgateway/stream_consumer.py—_suppress_silence_marker()and async cancellation handlingtests/gateway/test_stream_consumer_silence.py— test coverage for suppression
| stale_ids = set(self._preview_message_ids) | ||
| if self._message_id and self._message_id != "__no_edit__": | ||
| stale_ids.add(self._message_id) | ||
| delete_fn = getattr(self.adapter, "delete_message", None) | ||
| if delete_fn is not None: | ||
| for stale_id in stale_ids: | ||
| if not stale_id or stale_id == "__no_edit__": | ||
| continue | ||
| try: | ||
| await delete_fn(self.chat_id, stale_id) | ||
| except Exception as e: | ||
| logger.debug( | ||
| "Silence-marker preview cleanup failed (%s): %s", | ||
| stale_id, e, | ||
| ) | ||
| self._preview_message_ids = set() | ||
| self._message_id = None | ||
| self._accumulated = "" | ||
| self._last_sent_text = "" | ||
| self._already_sent = False | ||
| self._final_response_sent = False | ||
| self._final_content_delivered = False | ||
| logger.info( | ||
| "Suppressed streamed intentional-silence marker (chat=%s)", | ||
| self.chat_id, | ||
| ) |
There was a problem hiding this comment.
🟠 Task cancellation during silence-marker suppression leaks raw marker to platform (bug)
_suppress_silence_marker() in gateway/stream_consumer.py resets delivery state flags (_accumulated, _message_id, _already_sent, _final_response_sent, _final_content_delivered) AFTER the await delete_fn() deletion loop. If the consumer task is cancelled by asyncio during an await delete_fn() call, the CancelledError propagates to the outer except asyncio.CancelledError: handler in run(). At that point _accumulated still holds the marker text (e.g. 'NO_REPLY', '[SILENT]') and _message_id may still be set, so the handler's _send_or_edit call delivers the raw marker to the platform, then sets _final_response_sent = True — which prevents the gateway's downstream whole-response silence filter from running. The silence marker that should have been completely suppressed ends up permanently visible on the chat.
💡 Suggestion: Reset the delivery state flags BEFORE the deletion loop so that a mid-deletion CancelledError leaves the consumer in a clean state. The stale_ids set is built from a snapshot before the loop, so resetting flags early is safe — the deletion loop iterates over the snapshot, not the consumer's live fields.
| stale_ids = set(self._preview_message_ids) | |
| if self._message_id and self._message_id != "__no_edit__": | |
| stale_ids.add(self._message_id) | |
| delete_fn = getattr(self.adapter, "delete_message", None) | |
| if delete_fn is not None: | |
| for stale_id in stale_ids: | |
| if not stale_id or stale_id == "__no_edit__": | |
| continue | |
| try: | |
| await delete_fn(self.chat_id, stale_id) | |
| except Exception as e: | |
| logger.debug( | |
| "Silence-marker preview cleanup failed (%s): %s", | |
| stale_id, e, | |
| ) | |
| self._preview_message_ids = set() | |
| self._message_id = None | |
| self._accumulated = "" | |
| self._last_sent_text = "" | |
| self._already_sent = False | |
| self._final_response_sent = False | |
| self._final_content_delivered = False | |
| logger.info( | |
| "Suppressed streamed intentional-silence marker (chat=%s)", | |
| self.chat_id, | |
| ) | |
| stale_ids = set(self._preview_message_ids) | |
| if self._message_id and self._message_id != "__no_edit__": | |
| stale_ids.add(self._message_id) | |
| self._preview_message_ids = set() | |
| self._message_id = None | |
| self._accumulated = "" | |
| self._last_sent_text = "" | |
| self._already_sent = False | |
| self._final_response_sent = False | |
| self._final_content_delivered = False | |
| delete_fn = getattr(self.adapter, "delete_message", None) | |
| if delete_fn is not None: | |
| for stale_id in stale_ids: | |
| if not stale_id or stale_id == "__no_edit__": | |
| continue | |
| try: | |
| await delete_fn(self.chat_id, stale_id) | |
| except Exception as e: | |
| logger.debug( | |
| "Silence-marker preview cleanup failed (%s): %s", | |
| stale_id, e, | |
| ) | |
| logger.info( | |
| "Suppressed streamed intentional-silence marker (chat=%s)", | |
| self.chat_id, | |
| ) |
📋 Prompt for AI Agents
In gateway/stream_consumer.py method _suppress_silence_marker(), move the state-reset block (lines 1431-1437: setting _preview_message_ids, _message_id, _accumulated, _last_sent_text, _already_sent, _final_response_sent, _final_content_delivered) to BEFORE the for stale_id in stale_ids: deletion loop. Insert the flag reset block immediately after the stale_ids.add(self._message_id) line (line 1418), before the delete_fn = getattr(...) line (line 1419). The deletion loop remains best-effort with try/except, but the state flags are already clean if the task is cancelled mid-deletion. This ensures the outer CancelledError handler does not see stale accumulated text or message_id and therefore does not send the raw marker to the platform.
1 participant
Infographic
Summary
Intentional-silence markers (
NO_REPLY/[SILENT]/SILENT/NO REPLY) are now suppressed on the streaming delivery path, not just the non-streaming one. A user reported a literalNO_REPLYbubble leaking into Slack.The gateway already suppressed whole-response silence markers via
response_filters.is_intentional_silence_response+run.py's whole-response filter — but only on the non-streaming path.GatewayStreamConsumeredits the reply onto the screen delta-by-delta, before that filter runs, so by the time the filter fires the marker is already visible. This affects any streaming-capable adapter (Slack, Telegram, Discord, Matrix, …); Slack has editing on by default, which is where it was caught.Both streaming call sites (
run.pyproxy SSE and local) funnel throughon_delta, so the fix has a single correct home: the consumer.Changes
gateway/response_filters.py: addis_partial_silence_marker(), the streaming counterpart tois_intentional_silence_response(). Shares the sameLIVE_GATEWAY_SILENT_MARKERSset + canonicalization so the two never drift.gateway/stream_consumer.py:got_done, if the final buffer is exactly a marker, retract any preview (best-effortdelete_message, reusing the_try_fresh_finalcleanup path) and leave delivery flags False so the gateway's own filter drops it and no fallback send fires.tests/gateway/test_stream_consumer_silence.py: predicate truth table + end-to-endrun()suppression (single-shot and token-by-tokenNO_REPLY,[SILENT]parity, preview retraction, no-delete-support best-effort, prose-passthrough).No new config, no platform-specific code.
Validation
NO_REPLYNO_REPLY30 tests pass (
scripts/run_tests.sh tests/gateway/test_stream_consumer_silence.py). Matches the narrow intentional-silence contract: suppress only when the whole response is an exact marker; never substring-match prose.Salvaged from NousResearch#56042 by @benbarclay; authorship preserved via rebase-merge.
Mirror-of: NousResearch#56099
NousResearch#56099