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.
This commit is contained in:
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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."""
|
||||
|
||||
70
tests/gateway/test_clarify_card_retire_on_timeout.py
Normal file
70
tests/gateway/test_clarify_card_retire_on_timeout.py
Normal file
@@ -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"]
|
||||
@@ -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()
|
||||
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user