From 71cac9426df00a0da45e5fe26e5846a16bc913a7 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 18:27:59 -0700 Subject: [PATCH] fix(gateway): retire native clarify cards on timeout, reset and prose cancel One adapter-facing seam replaces the Slack-only callback: an adapter whose clarify prompt is a persistent card (Slack Block Kit) defines `retire_clarify_card(clarify_id, notice)`, and the gateway calls it from every path that ends a clarify without a button click: - TurnRunner._clarify_callback_sync: when the bounded wait returns a sentinel (timeout, /new or run-end clear_session), schedule the retire with the expired notice on the gateway loop (#110821). - run_inbound TEXT_REJECTED_PROSE: retire with the cancelled notice before the prose is routed as a follow-up (#111019). Lookup is on the adapter class so MagicMock doubles cannot fabricate the method; no platform == SLACK special-case. The Slack map is keyed by clarify_id and popped before the first await, so a late timer cannot touch a newer prompt and the button handler's ts-keyed guard makes a racing click a no-op. Gateway-restart-orphaned cards stay out of scope: nothing is waiting on the new process, and the click path already renders them expired. Tests trimmed to invariants: the runner-level timeout probe (card adapter vs no-card adapter), the inbound prose retire, and one Slack test covering buttons-dropped + late-click-noop. Docs updated for the new in-place edit. --- gateway/platforms/base.py | 4 +- gateway/run_inbound.py | 16 +++-- gateway/run_turn_runner.py | 15 +++- plugins/platforms/slack/adapter.py | 19 +++-- .../test_clarify_card_retire_on_timeout.py | 70 +++++++++++++++++++ ...t_clarify_thread_followup_not_swallowed.py | 19 ++--- tests/gateway/test_slack_clarify_buttons.py | 30 ++++---- website/docs/user-guide/messaging/slack.md | 9 ++- 8 files changed, 138 insertions(+), 44 deletions(-) create mode 100644 tests/gateway/test_clarify_card_retire_on_timeout.py diff --git a/gateway/platforms/base.py b/gateway/platforms/base.py index 42599a5808..a29918f9e0 100644 --- a/gateway/platforms/base.py +++ b/gateway/platforms/base.py @@ -2684,7 +2684,9 @@ class BasePlatformAdapter(ABC): ``tools.clarify_gateway.resolve_gateway_clarify(clarify_id, response)``, "Other" calls ``mark_awaiting_text(clarify_id)``. Open-ended: send the question as text (the gateway text-intercept resolves the next message). Default: numbered list + - ``mark_awaiting_text``.""" + ``mark_awaiting_text``. Adapters whose prompt is a persistent card MAY define + ``async retire_clarify_card(clarify_id, notice)``; the gateway calls it when the clarify + ends without a click (timeout, session reset, superseding free prose).""" if choices: # Multi-select flag lives on the pending entry (signature stays adapter-compatible). try: diff --git a/gateway/run_inbound.py b/gateway/run_inbound.py index 73fb8151af..cc29da3629 100644 --- a/gateway/run_inbound.py +++ b/gateway/run_inbound.py @@ -401,15 +401,17 @@ class GatewayInboundMixin: # routing. Release this clarify first: redirect() degrades to steer() while tools # execute, and that steer cannot drain until the clarify tool returns. 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. + # Adapters with a persistent native card (Slack Block Kit) retire it now, before the + # prose is routed, so its buttons stop advertising a dead answer path. The pop inside + # retire_clarify_card runs before its first await, so the agent thread's own expiry + # notice (scheduled once the wait unblocks) finds nothing and stays a no-op. _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): + # Class lookup: a MagicMock adapter must not fabricate the method. + if callable(getattr(type(_clarify_adapter), "retire_clarify_card", None)): try: - await _cancel_card(_pending_clarify.clarify_id) + await _clarify_adapter.retire_clarify_card( + _pending_clarify.clarify_id, + "↩️ Clarification cancelled — your message will be handled as a follow-up.") except Exception: logger.debug("Failed to retire clarify card after prose cancellation", exc_info=True) return None diff --git a/gateway/run_turn_runner.py b/gateway/run_turn_runner.py index 00729d72fb..4e271dfc25 100644 --- a/gateway/run_turn_runner.py +++ b/gateway/run_turn_runner.py @@ -53,6 +53,11 @@ def _renders_exec_approval_buttons(adapter_cls: type) -> bool: return getattr(adapter_cls, "send_exec_approval", None) is not None +# Rendered on a native clarify card whose wait ended without a click (mirrors the notice the +# Slack click handler shows on a dead entry). +_CLARIFY_EXPIRED_NOTICE = "⏳ This prompt expired — please send a new request." + + class _ExecApprovalDeclined(RuntimeError): """The connector refused the approval card's destination. @@ -1335,7 +1340,15 @@ class TurnRunner: response = _clarify_send_then_wait(fut, clarify_id=clarify_id, session_key=session_key, clarify_mod=clarify_mod) # Only re-arm typing when the user actually answered — the undeliverable sentinel and the # timeout/cancellation strings start with '[' and must pass through untouched. - if not (isinstance(response, str) and response.startswith("[")): + if isinstance(response, str) and response.startswith("["): + # No answer arrived (timeout, /new, run end): retire the native card so it stops + # looking answerable. Adapters without a persistent card have no such method. + retire = getattr(type(ctx._status_adapter), "retire_clarify_card", None) + if callable(retire): + self._schedule( + retire(ctx._status_adapter, clarify_id, _CLARIFY_EXPIRED_NOTICE), + "Clarify card retire failed to schedule") + else: # Reopen typing IMMEDIATELY, not on the LLM's first post-answer token (native streaming # otherwise re-seeds lazily on the first delta: ~48s of dead air). request_reopen_seed is # a no-op outside the reopen-pending native state. diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 5e487c02e0..8ed32fddab 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -1048,8 +1048,8 @@ 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. + # clarify_id → (channel_id, message_ts, rendered_question) so the gateway can retire a + # card whose clarify ended without a click (timeout, reset, superseding prose). 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). @@ -5416,12 +5416,13 @@ 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. + async def retire_clarify_card(self, clarify_id: str, notice: str) -> None: + """Rewrite a still-live clarify card into a terminal, button-less state. - 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. + The gateway calls this whenever it ends a clarify without a button click — the wait + timed out, the session was reset, or unmatched free prose superseded the prompt — so + the card stops advertising an answer path the released clarify can no longer accept. + Keyed by clarify_id, so a late call cannot touch a newer prompt; no-op once resolved. """ target = self._clarify_messages.pop(clarify_id, None) if target is None: @@ -5429,9 +5430,7 @@ class SlackAdapter(BasePlatformAdapter): 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.") + await self._update_clarify_message(channel_id, msg_ts, question_text, notice) async def _handle_clarify_action(self, ack, body, action) -> None: """Handle a clarify button click (a choice or "Other") from Block Kit.""" diff --git a/tests/gateway/test_clarify_card_retire_on_timeout.py b/tests/gateway/test_clarify_card_retire_on_timeout.py new file mode 100644 index 0000000000..bd774c7d30 --- /dev/null +++ b/tests/gateway/test_clarify_card_retire_on_timeout.py @@ -0,0 +1,70 @@ +"""A clarify that ends without a click retires the adapter's native card (#110821, #111019). + +Drives the real ``TurnRunner._clarify_callback_sync`` against a duck-typed adapter with a +persistent card: on timeout the gateway schedules ``retire_clarify_card`` with the expired +notice; an adapter without that method (text-prompt platforms) gets nothing scheduled. +""" + +import asyncio +from types import SimpleNamespace +from unittest.mock import patch + +from gateway.platforms.base import SendResult + + +class _CardAdapter: + def __init__(self): + self.retired: list[tuple[str, str]] = [] + + def pause_typing_for_chat(self, chat_id): + return None + + def resume_typing_for_chat(self, chat_id): + return None + + async def send_clarify(self, **kwargs): + return SendResult(success=True, message_id="1.2") + + async def retire_clarify_card(self, clarify_id, notice): + self.retired.append((clarify_id, notice)) + + +class _TextAdapter(_CardAdapter): + retire_clarify_card = None # type: ignore[assignment] + + +def _run_clarify(adapter): + """Returns (clarify response, labels of every coroutine the runner scheduled).""" + from gateway.run_turn_runner import TurnRunner + + runner = object.__new__(TurnRunner) + runner._ctx = SimpleNamespace( + _status_adapter=adapter, _status_chat_id="C1", _status_thread_metadata={}, + session_key="sk1", stream_consumer_holder=[None]) + labels: list[str] = [] + + class _Fut: + def __init__(self, r): self._r = r + def result(self, timeout=None): return self._r + + def _schedule(coro, label): + labels.append(label) + return _Fut(asyncio.run(coro)) + + runner._schedule = _schedule + runner._close_native_stream_boundary = lambda *a, **k: None + with patch("tools.clarify_gateway.get_clarify_timeout", return_value=1): + return runner._clarify_callback_sync("Pick one", ["a", "b"]), labels + + +def test_timeout_retires_the_native_card_with_the_expired_notice(): + adapter = _CardAdapter() + response, _labels = _run_clarify(adapter) + assert response.startswith("[user did not respond") + assert len(adapter.retired) == 1 + assert "expired" in adapter.retired[0][1].lower() + + +def test_timeout_schedules_nothing_for_adapters_without_a_card(): + _response, labels = _run_clarify(_TextAdapter()) + assert labels == ["Clarify send failed to schedule"] diff --git a/tests/gateway/test_clarify_thread_followup_not_swallowed.py b/tests/gateway/test_clarify_thread_followup_not_swallowed.py index 33e08d3b7e..3974275bb9 100644 --- a/tests/gateway/test_clarify_thread_followup_not_swallowed.py +++ b/tests/gateway/test_clarify_thread_followup_not_swallowed.py @@ -52,15 +52,15 @@ class _StubAdapter(BasePlatformAdapter): return {"id": chat_id, "type": "im"} -class _SlackClarifyCancelAdapter(_StubAdapter): - """Records the Slack-only stale-card cancellation callback.""" +class _CardAdapter(_StubAdapter): + """Adapter with a persistent native card: records the gateway's retire callback.""" def __init__(self): super().__init__() - self.cancelled_clarify_ids: list[str] = [] + self.retired: list[tuple[str, str]] = [] - async def cancel_clarify_message(self, clarify_id: str) -> None: - self.cancelled_clarify_ids.append(clarify_id) + async def retire_clarify_card(self, clarify_id: str, notice: str) -> None: + self.retired.append((clarify_id, notice)) class _FellThroughIntercept(Exception): @@ -144,19 +144,20 @@ async def test_thread_prose_not_swallowed_by_native_multi_choice_clarify(): @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.""" +async def test_thread_prose_retires_the_native_card_before_falling_through(): + """The card adapter gets one retire call (cancel notice) while the prose still falls through.""" _clear_clarify_state() from tools import clarify_gateway as cm - adapter = _SlackClarifyCancelAdapter() + adapter = _CardAdapter() 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"] + assert [cid for cid, _ in adapter.retired] == ["cl-slack-card"] + assert "cancelled" in adapter.retired[0][1].lower() _clear_clarify_state() diff --git a/tests/gateway/test_slack_clarify_buttons.py b/tests/gateway/test_slack_clarify_buttons.py index 7fda60fd96..bae1a4385b 100644 --- a/tests/gateway/test_slack_clarify_buttons.py +++ b/tests/gateway/test_slack_clarify_buttons.py @@ -153,29 +153,33 @@ class TestSlackSendClarify: assert "&" in section_text @pytest.mark.asyncio - async def test_free_prose_cancellation_rewrites_card_without_actions(self): + async def test_retire_clarify_card_drops_buttons_and_makes_a_late_click_a_noop(self): + """Gateway-driven retirement (timeout / prose / reset) rewrites the card and wins the race + against a later button click on the same message.""" + from tools import clarify_gateway as cm + adapter = _make_adapter() + _attach_auth_runner(adapter) mock_client = adapter._team_clients["T1"] mock_client.chat_postMessage = AsyncMock(return_value={"ts": "1.2"}) mock_client.chat_update = AsyncMock() - + cm.register("cid-retire", "sk-retire", "Which environment?", ["staging", "production"]) await adapter.send_clarify( - chat_id="C1", - question="Which environment?", - choices=["staging", "production"], - clarify_id="cid-cancel", - session_key="sk-cancel", - ) + chat_id="C1", question="Which environment?", choices=["staging", "production"], + clarify_id="cid-retire", session_key="sk-retire") - await adapter.cancel_clarify_message("cid-cancel") + await adapter.retire_clarify_card("cid-retire", "⏳ expired") kwargs = mock_client.chat_update.call_args.kwargs - assert kwargs["channel"] == "C1" - assert kwargs["ts"] == "1.2" - assert "cancelled" in kwargs["text"].lower() + assert (kwargs["channel"], kwargs["ts"], kwargs["text"]) == ("C1", "1.2", "⏳ expired") assert all(block["type"] != "actions" for block in kwargs["blocks"]) - assert adapter._clarify_resolved["1.2"] is True + await adapter._handle_clarify_action(AsyncMock(), { + "message": {"ts": "1.2", "blocks": kwargs["blocks"]}, + "channel": {"id": "C1"}, "user": {"name": "norbert", "id": "U_N"}, + }, {"action_id": "hermes_clarify_choice_0", "value": "cid-retire|0"}) + assert mock_client.chat_update.await_count == 1 + assert not cm._entries["cid-retire"].event.is_set() # =========================================================================== # _handle_clarify_action — choice click resolves (b) diff --git a/website/docs/user-guide/messaging/slack.md b/website/docs/user-guide/messaging/slack.md index 104d4a318f..d07e4ad58e 100644 --- a/website/docs/user-guide/messaging/slack.md +++ b/website/docs/user-guide/messaging/slack.md @@ -348,9 +348,12 @@ tool), Slack renders it as **Block Kit buttons** — one tap per option, plus an "✏️ Other…" button that switches to free-text mode (your next typed message becomes the answer). After a tap, the message updates in place to show who answered and what was chosen; further clicks on the same prompt are ignored. -Button clicks honor the same user authorization as messages, and expired -prompts (gateway restart, timeout) tell you to re-ask instead of silently -eating the click. Open-ended clarify questions render as a plain question and +Button clicks honor the same user authorization as messages. When the prompt +times out (`agent.clarify_timeout`), the session is reset, or you reply with +free text instead of tapping a button, the card is rewritten in place without +its buttons ("⏳ This prompt expired…" or "↩️ Clarification cancelled…"); a +click on a card orphaned by a gateway restart still tells you to re-ask +instead of silently eating the click. Open-ended clarify questions render as a plain question and accept your next typed reply. No configuration needed — this works regardless of the `rich_blocks` setting.