From a41187e1874cc5a84381c4fe2b181c9a2d09f10d Mon Sep 17 00:00:00 2001 From: KoNit-K <124019182+KoNit-K@users.noreply.github.com> Date: Tue, 15 Sep 2026 00:36:52 +0800 Subject: [PATCH] fix(gateway): retire Slack clarify cards on prose cancellation --- gateway/run_inbound.py | 13 +++++++- plugins/platforms/slack/adapter.py | 30 ++++++++++++++++++- ...t_clarify_thread_followup_not_swallowed.py | 29 +++++++++++++++++- tests/gateway/test_slack_clarify_buttons.py | 24 +++++++++++++++ 4 files changed, 93 insertions(+), 3 deletions(-) diff --git a/gateway/run_inbound.py b/gateway/run_inbound.py index c4f8292284..73fb8151af 100644 --- a/gateway/run_inbound.py +++ b/gateway/run_inbound.py @@ -400,7 +400,18 @@ class GatewayInboundMixin: # Native-choice prompts reject unmatched prose so it continues through normal busy # routing. Release this clarify first: redirect() degrades to steer() while tools # execute, and that steer cannot drain until the clarify tool returns. - _clarify_mod.resolve_gateway_clarify(_pending_clarify.clarify_id, "") + if _clarify_mod.resolve_gateway_clarify(_pending_clarify.clarify_id, ""): + # Slack's native clarify prompt is a persistent Block Kit card. Once this + # unmatched prose releases the wait, retire that card before routing the prose + # normally so its buttons cannot advertise a stale answer path. Other adapters + # intentionally have no callback and retain the existing generic behaviour. + _clarify_adapter = self._adapter_for_source(source) + _cancel_card = getattr(_clarify_adapter, "cancel_clarify_message", None) + if source.platform == Platform.SLACK and callable(_cancel_card): + try: + await _cancel_card(_pending_clarify.clarify_id) + except Exception: + logger.debug("Failed to retire clarify card after prose cancellation", exc_info=True) return None # Reply → choice for a pending slash-confirm prompt; the command spelling wins over the diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 143fb03bce..5e487c02e0 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -999,6 +999,7 @@ class SlackAdapter(BasePlatformAdapter): _REACTING_MESSAGE_IDS_MAX = _TITLED_ASSISTANT_THREADS_MAX = 5000 _CHANNEL_TEAM_MAX = 10000 _APPROVAL_RESOLVED_MAX = _CLARIFY_RESOLVED_MAX = _ACTIVE_STATUS_THREADS_MAX = 1000 + _CLARIFY_MESSAGE_MAX = 1000 # Tighter cap than the approval/clarify dicts: each entry holds the # full provider list, and a picker is only live for minutes. _MODEL_PICKER_STATE_MAX = 100 @@ -1047,6 +1048,9 @@ class SlackAdapter(BasePlatformAdapter): # Bounded: never-clicked prompts would otherwise leak forever. self._approval_resolved: Dict[Any, bool] = {} self._clarify_resolved: Dict[Any, bool] = {} + # clarify_id → (channel_id, message_ts, rendered_question). This lets the inbound + # free-prose path retire a native card that will no longer accept a response. + self._clarify_messages: Dict[str, Tuple[str, str, str]] = {} # Model picker state keyed by workspace message marker (team_id, ts) → # picker context (providers, session_key, on_model_selected, stage). # Mirrors _approval_resolved / _clarify_resolved: bounded, and the @@ -5201,10 +5205,16 @@ class SlackAdapter(BasePlatformAdapter): # Bare-ts key (not workspace-scoped) so the action handler's atomic-pop guard # can reject double-clicks (mirrors _approval_resolved). - return await self._send_interactive_prompt( + result = await self._send_interactive_prompt( chat_id, metadata, _build, "send_clarify", resolved=self._clarify_resolved, resolved_max=self._CLARIFY_RESOLVED_MAX, team_scoped_key=False, sanitize=False) + if result.success and result.message_id: + question_text, _blocks = _build() + response_channel = str((result.raw_response or {}).get("channel") or chat_id) + self._clarify_messages[clarify_id] = (response_channel, result.message_id, question_text) + self._trim_oldest_dict_entries(self._clarify_messages, self._CLARIFY_MESSAGE_MAX) + return result def _is_interactive_user_authorized( self, user_id: str, *, channel_id: str = "", user_name: Optional[str] = None, @@ -5406,6 +5416,23 @@ class SlackAdapter(BasePlatformAdapter): channel_id, msg_ts, question_text, decision_text, "Clarification", "clarify", sanitize=False ) + async def cancel_clarify_message(self, clarify_id: str) -> None: + """Retire a Block Kit clarify card released by unmatched free prose. + + The generic inbound path intentionally lets that prose continue as a normal follow-up. + Slack alone needs to edit its already-posted interactive card so its buttons do not + advertise an answer path that the released clarify can no longer accept. + """ + target = self._clarify_messages.pop(clarify_id, None) + if target is None: + return + channel_id, msg_ts, question_text = target + # A late action handler must be a no-op while the best-effort chat.update is in flight. + self._clarify_resolved[msg_ts] = True + await self._update_clarify_message( + channel_id, msg_ts, question_text, + "↩️ Clarification cancelled — your message will be handled as a follow-up.") + async def _handle_clarify_action(self, ack, body, action) -> None: """Handle a clarify button click (a choice or "Other") from Block Kit.""" started = await self._begin_interaction(ack, body, action, "clarify", team_scoped=False) @@ -5419,6 +5446,7 @@ class SlackAdapter(BasePlatformAdapter): # Double-click guard — atomic pop (mirrors approval). if self._clarify_resolved.pop(msg_ts, True): return + self._clarify_messages.pop(clarify_id, None) original_text = self._section_text(message, limit=None) from tools import clarify_gateway as _clarify_mod # "Other" → text-capture mode: mark_awaiting_text flips the entry and the diff --git a/tests/gateway/test_clarify_thread_followup_not_swallowed.py b/tests/gateway/test_clarify_thread_followup_not_swallowed.py index 9fd8359314..33e08d3b7e 100644 --- a/tests/gateway/test_clarify_thread_followup_not_swallowed.py +++ b/tests/gateway/test_clarify_thread_followup_not_swallowed.py @@ -52,6 +52,17 @@ class _StubAdapter(BasePlatformAdapter): return {"id": chat_id, "type": "im"} +class _SlackClarifyCancelAdapter(_StubAdapter): + """Records the Slack-only stale-card cancellation callback.""" + + def __init__(self): + super().__init__() + self.cancelled_clarify_ids: list[str] = [] + + async def cancel_clarify_message(self, clarify_id: str) -> None: + self.cancelled_clarify_ids.append(clarify_id) + + class _FellThroughIntercept(Exception): """Sentinel: _handle_message got PAST the clarify text-intercept.""" @@ -132,6 +143,23 @@ async def test_thread_prose_not_swallowed_by_native_multi_choice_clarify(): _clear_clarify_state() +@pytest.mark.asyncio +async def test_thread_prose_cancels_the_slack_clarify_card_before_falling_through(): + """Slack receives a stale-card update while the prose keeps normal follow-up routing.""" + _clear_clarify_state() + from tools import clarify_gateway as cm + + adapter = _SlackClarifyCancelAdapter() + runner = _make_runner(adapter) + cm.register("cl-slack-card", SESSION_KEY, "Pick a UI variant", ["buttons", "dropdown"]) + + with pytest.raises(_FellThroughIntercept): + await _dispatch(runner, _event("just checking the visual UI, no need to pass any data")) + + assert adapter.cancelled_clarify_ids == ["cl-slack-card"] + _clear_clarify_state() + + @pytest.mark.asyncio async def test_thread_prose_does_not_overwrite_concurrent_button_choice(): """A button result that wins the race remains the clarify response.""" @@ -262,4 +290,3 @@ async def test_prose_still_accepted_after_other_flips_text_capture(): assert entry.event.is_set() assert entry.response == "a carousel actually" _clear_clarify_state() - diff --git a/tests/gateway/test_slack_clarify_buttons.py b/tests/gateway/test_slack_clarify_buttons.py index b056da89f6..7fda60fd96 100644 --- a/tests/gateway/test_slack_clarify_buttons.py +++ b/tests/gateway/test_slack_clarify_buttons.py @@ -152,6 +152,30 @@ class TestSlackSendClarify: assert "<A>" in section_text assert "&" in section_text + @pytest.mark.asyncio + async def test_free_prose_cancellation_rewrites_card_without_actions(self): + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.chat_postMessage = AsyncMock(return_value={"ts": "1.2"}) + mock_client.chat_update = AsyncMock() + + await adapter.send_clarify( + chat_id="C1", + question="Which environment?", + choices=["staging", "production"], + clarify_id="cid-cancel", + session_key="sk-cancel", + ) + + await adapter.cancel_clarify_message("cid-cancel") + + kwargs = mock_client.chat_update.call_args.kwargs + assert kwargs["channel"] == "C1" + assert kwargs["ts"] == "1.2" + assert "cancelled" in kwargs["text"].lower() + assert all(block["type"] != "actions" for block in kwargs["blocks"]) + assert adapter._clarify_resolved["1.2"] is True + # =========================================================================== # _handle_clarify_action — choice click resolves (b)