fix: retire clarify cards on an explicit no-answer signal, not the '[' prefix
`_clarify_callback_sync` decided "no answer arrived" by testing whether the response text starts with '[' (the shape of the timeout / undeliverable sentinels). A real answer can start with '[' too — a "[A] staging" choice label picked by number, or "[urgent] ..." free text after Other — so the clarify resolved and the agent got the answer, yet the Slack card was rewritten to "This prompt expired" and typing was never re-armed. `_clarify_send_then_wait` now returns `(response, answered)` and the runner branches on that flag only. The Slack click handler popped the retire entry as soon as Other was clicked, but Other is not terminal: the clarify stays pending for typed text, so a later timeout or /new reset found nothing to retire and the card stayed stuck on "Awaiting typed answer". The entry is now popped only on a terminal outcome (a choice click, or Other on an already-dead entry). A typed answer to a native card (numeric pick, or text after Other) never reaches the click handler, so the card kept its buttons forever; the TEXT_RESOLVED intercept now retires it with the answer. Review finding: '[' prefix mistaken for the timeout sentinel; Other click dropped the retire entry; typed answers never rewrote the card.
This commit is contained in:
+8
-5
@@ -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(
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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"]
|
||||
|
||||
@@ -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)
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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)
|
||||
# ===========================================================================
|
||||
|
||||
Reference in New Issue
Block a user