diff --git a/gateway/run.py b/gateway/run.py index 2ff17be402..67e88e823f 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -786,16 +786,19 @@ def _clarify_send_disposition(fut, *, session_key: str, clarify_mod) -> "str | N return None -def _clarify_send_then_wait(fut, *, clarify_id: str, session_key: str, clarify_mod) -> str: - """Resolve a clarify prompt: send disposition, then the bounded wait.""" +def _clarify_send_then_wait(fut, *, clarify_id: str, session_key: str, clarify_mod) -> tuple[str, bool]: + """Resolve a clarify prompt: send disposition, then the bounded wait. + + Returns ``(response, answered)``. ``answered`` is the only signal that a user reply arrived; + callers must not infer it from the text (a real answer may start with '[' like a sentinel).""" abort = _clarify_send_disposition(fut, session_key=session_key, clarify_mod=clarify_mod) if abort is not None: - return abort + return abort, False timeout = clarify_mod.get_clarify_timeout() response = clarify_mod.wait_for_response(clarify_id, timeout=float(timeout)) if response is None or response == "": - return f"[user did not respond within {int(timeout / 60)}m]" - return response + return f"[user did not respond within {int(timeout / 60)}m]", False + return response, True def _resolve_progress_thread_id( diff --git a/gateway/run_inbound.py b/gateway/run_inbound.py index cc29da3629..adca445ff0 100644 --- a/gateway/run_inbound.py +++ b/gateway/run_inbound.py @@ -391,6 +391,15 @@ class GatewayInboundMixin: _clarify_adapter.resume_typing_for_chat(source.chat_id) except Exception: logger.debug("Failed to resume typing after clarify response", exc_info=True) + # A typed answer to a native card (numeric pick, or text after "Other") never + # reaches the click handler, so the card would keep its buttons forever. + if callable(getattr(type(_clarify_adapter), "retire_clarify_card", None)): + try: + await _clarify_adapter.retire_clarify_card( + _pending_clarify.clarify_id, + f"✅ answered: {_pending_clarify.response or _raw_clarify_reply}") + except Exception: + logger.debug("Failed to retire clarify card after typed answer", exc_info=True) return "" if _text_outcome == _clarify_mod.TEXT_REJECTED_SELECTION: # Selection-shaped but invalid (out-of-range number, bad comma-list): keep the clarify diff --git a/gateway/run_turn_runner.py b/gateway/run_turn_runner.py index 4e271dfc25..a2ee1c4667 100644 --- a/gateway/run_turn_runner.py +++ b/gateway/run_turn_runner.py @@ -1337,10 +1337,11 @@ class TurnRunner: # Boundary rule (see _approval_send_outcome): a send timeout is AMBIGUOUS — the card may # have posted with a late ack. Only a definitive failure tears down the registration; # ambiguous falls through to the bounded wait so a late reply resolves. - 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 isinstance(response, str) and response.startswith("["): + response, answered = _clarify_send_then_wait( + fut, clarify_id=clarify_id, session_key=session_key, clarify_mod=clarify_mod) + # Branch on the explicit flag, never on the text: a real answer can start with '[' (a + # "[A] staging" label, "[urgent] ..." free text) and must not be mistaken for a sentinel. + if not answered: # 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) diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 8ed32fddab..0a06ec5a1a 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -5445,7 +5445,6 @@ 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 @@ -5454,8 +5453,11 @@ class SlackAdapter(BasePlatformAdapter): if action_id == "hermes_clarify_other" or token == "other": if not _clarify_mod.mark_awaiting_text(clarify_id): # Entry evicted/gateway restarted — a typed answer would go nowhere. + self._clarify_messages.pop(clarify_id, None) await self._update_clarify_message(channel_id, msg_ts, original_text, expired_text) return + # Not terminal: the clarify stays pending for typed text, so keep the card entry — + # the gateway still has to retire it on timeout / reset / typed answer. await self._update_clarify_message( channel_id, msg_ts, original_text, f"✏️ Awaiting typed answer from {user_name}…") return @@ -5464,6 +5466,8 @@ class SlackAdapter(BasePlatformAdapter): except (ValueError, TypeError): logger.warning("[Slack] Invalid clarify choice token: %s", token) return + # A choice click is terminal either way (✅ or expired): the card no longer needs retiring. + self._clarify_messages.pop(clarify_id, None) # Canonical choice text from the entry; positional fallback on timeout/reset race. resolved_text: Optional[str] = None try: diff --git a/tests/gateway/test_clarify_card_retire_on_timeout.py b/tests/gateway/test_clarify_card_retire_on_timeout.py index bd774c7d30..e3480c355a 100644 --- a/tests/gateway/test_clarify_card_retire_on_timeout.py +++ b/tests/gateway/test_clarify_card_retire_on_timeout.py @@ -33,8 +33,9 @@ class _TextAdapter(_CardAdapter): retire_clarify_card = None # type: ignore[assignment] -def _run_clarify(adapter): - """Returns (clarify response, labels of every coroutine the runner scheduled).""" +def _run_clarify(adapter, answer=None): + """Returns (clarify response, labels of every coroutine the runner scheduled). + ``answer`` resolves the pending clarify with that text instead of letting it time out.""" from gateway.run_turn_runner import TurnRunner runner = object.__new__(TurnRunner) @@ -53,7 +54,18 @@ def _run_clarify(adapter): runner._schedule = _schedule runner._close_native_stream_boundary = lambda *a, **k: None - with patch("tools.clarify_gateway.get_clarify_timeout", return_value=1): + if answer is not None: + from tools import clarify_gateway as cm + real_register = cm.register + + def _register_and_answer(**kwargs): + entry = real_register(**kwargs) + cm.resolve_gateway_clarify(kwargs["clarify_id"], answer) + return entry + register_patch = patch.object(cm, "register", _register_and_answer) + else: + register_patch = patch("tools.clarify_gateway.get_clarify_timeout", return_value=1) + with register_patch: return runner._clarify_callback_sync("Pick one", ["a", "b"]), labels @@ -68,3 +80,12 @@ def test_timeout_retires_the_native_card_with_the_expired_notice(): def test_timeout_schedules_nothing_for_adapters_without_a_card(): _response, labels = _run_clarify(_TextAdapter()) assert labels == ["Clarify send failed to schedule"] + + +def test_real_answer_starting_with_a_bracket_is_not_mistaken_for_a_sentinel(): + """'[A] staging' is a user answer, not a timeout: no card retirement, typing re-armed.""" + adapter = _CardAdapter() + response, labels = _run_clarify(adapter, answer="[A] staging") + assert response == "[A] staging" + assert adapter.retired == [] + assert labels == ["Clarify send failed to schedule"] diff --git a/tests/gateway/test_clarify_send_timeout_ambiguity.py b/tests/gateway/test_clarify_send_timeout_ambiguity.py index a226a89774..e9df2f21f9 100644 --- a/tests/gateway/test_clarify_send_timeout_ambiguity.py +++ b/tests/gateway/test_clarify_send_timeout_ambiguity.py @@ -103,7 +103,7 @@ def test_ambiguous_send_reaches_wait_for_response(): fut, clarify_id="cid123", session_key="sk", clarify_mod=clarify_mod ) - assert out == "user picked B" + assert out == ("user picked B", True) clarify_mod.clear_session.assert_not_called() clarify_mod.wait_for_response.assert_called_once_with("cid123", timeout=600.0) @@ -119,7 +119,7 @@ def test_sent_reaches_wait_for_response(): _clarify_send_then_wait( fut, clarify_id="cid123", session_key="sk", clarify_mod=clarify_mod ) - == "answer" + == ("answer", True) ) clarify_mod.wait_for_response.assert_called_once_with("cid123", timeout=600.0) @@ -133,7 +133,7 @@ def test_definitive_failure_never_waits(): _clarify_send_then_wait( fut, clarify_id="cid123", session_key="sk", clarify_mod=clarify_mod ) - == SENTINEL + == (SENTINEL, False) ) clarify_mod.wait_for_response.assert_not_called() clarify_mod.clear_session.assert_called_once_with("sk") @@ -150,7 +150,7 @@ def test_no_response_returns_timeout_sentinel(): _clarify_send_then_wait( fut, clarify_id="cid123", session_key="sk", clarify_mod=clarify_mod ) - == "[user did not respond within 10m]" + == ("[user did not respond within 10m]", False) ) diff --git a/tests/gateway/test_clarify_thread_followup_not_swallowed.py b/tests/gateway/test_clarify_thread_followup_not_swallowed.py index 3974275bb9..e730878dc1 100644 --- a/tests/gateway/test_clarify_thread_followup_not_swallowed.py +++ b/tests/gateway/test_clarify_thread_followup_not_swallowed.py @@ -161,6 +161,23 @@ async def test_thread_prose_retires_the_native_card_before_falling_through(): _clear_clarify_state() +@pytest.mark.asyncio +async def test_typed_selection_retires_the_native_card_with_the_answer(): + """A numeric pick typed into the thread resolves the clarify AND rewrites the card.""" + _clear_clarify_state() + from tools import clarify_gateway as cm + + adapter = _CardAdapter() + runner = _make_runner(adapter) + entry = cm.register("cl-typed-card", SESSION_KEY, "Pick a UI variant", ["buttons", "dropdown"]) + + assert await _dispatch(runner, _event("2")) == "" + + assert entry.response == "dropdown" + assert adapter.retired == [("cl-typed-card", "✅ answered: dropdown")] + _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.""" diff --git a/tests/gateway/test_slack_clarify_buttons.py b/tests/gateway/test_slack_clarify_buttons.py index bae1a4385b..82c65c1c91 100644 --- a/tests/gateway/test_slack_clarify_buttons.py +++ b/tests/gateway/test_slack_clarify_buttons.py @@ -181,6 +181,33 @@ class TestSlackSendClarify: assert mock_client.chat_update.await_count == 1 assert not cm._entries["cid-retire"].event.is_set() + @pytest.mark.asyncio + async def test_other_click_keeps_the_card_retirable_until_the_clarify_ends(self): + """'Other' is not terminal: the clarify stays pending for typed text, so a later + timeout/reset must still be able to rewrite the '✏️ Awaiting typed answer' card.""" + 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-other", "sk-other", "Which environment?", ["staging", "production"]) + await adapter.send_clarify( + chat_id="C1", question="Which environment?", choices=["staging", "production"], + clarify_id="cid-other", session_key="sk-other") + await adapter._handle_clarify_action(AsyncMock(), { + "message": {"ts": "1.2", "blocks": []}, + "channel": {"id": "C1"}, "user": {"name": "norbert", "id": "U_N"}, + }, {"action_id": "hermes_clarify_other", "value": "cid-other|other"}) + assert "Awaiting typed answer" in mock_client.chat_update.call_args.kwargs["text"] + + cm.clear_session("sk-other") + await adapter.retire_clarify_card("cid-other", "⏳ expired") + + assert mock_client.chat_update.await_count == 2 + assert mock_client.chat_update.call_args.kwargs["text"] == "⏳ expired" + # =========================================================================== # _handle_clarify_action — choice click resolves (b) # ===========================================================================