fix: a clarify card that resolves AMBIGUOUS after the ack window stays armed for the late tap
The late-failure watch treated every non-"sent" outcome as definitive: a relay lost-ack (raw_response.ambiguous=True, the card may well have posted) arriving after SEND_ACK_WINDOW tore down the registration and returned the delivery notice, so the user's button tap on a card that WAS rendered found no pending entry and was lost. That breaks the invariant _abort_for_outcome (and main) keeps for an immediate ambiguous outcome. _on_card_done now returns early on "ambiguous" and lets the bounded wait's own timeout cover a card that truly never arrived. Also while here: - a late DECLINE releases with UNDELIVERED_DECLINED, matching the immediate path, instead of the generic notice (_release carries the outcome); - when the text fallback cannot even be scheduled (fallback() -> None) the card's "failed" verdict stands instead of re-classifying None, which logged a misleading "no scheduling future (loop unavailable)"; - test_card_failing_after_the_ack_window_... now asserts the text prompt was sent exactly once and the wait released within ack window + 2 s, and the helper answers only after observing the prompt — with fallback/late-watch disabled it stayed green before (the helper answered on a 10 s deadline); - telegram.md: clarify_timeout default is 3600 s (resolve_clarify_timeout, configuration.md), not 600. Part of #112684
This commit is contained in:
teknium1
@@ -94,7 +94,10 @@ def _clarify_send_then_wait(fut, *, clarify_id: str, session_key: str, clarify_m
|
||||
if outcome == "failed" and fallback is not None:
|
||||
# The text prompt is the last resort: a late failure of ITS send has nothing to retry.
|
||||
fut, fallback = fallback(), None
|
||||
outcome = _approval_send_outcome(fut, timeout=SEND_ACK_WINDOW)
|
||||
# ``None`` = the fallback could not even be scheduled; the card failure already stands,
|
||||
# so re-classifying would only log a misleading "no scheduling future".
|
||||
if fut is not None:
|
||||
outcome = _approval_send_outcome(fut, timeout=SEND_ACK_WINDOW)
|
||||
if outcome == "sent":
|
||||
logger.info("Clarify card undeliverable; plain-text prompt sent instead (id=%s)", clarify_id)
|
||||
abort = _abort_for_outcome(outcome, session_key=session_key, clarify_mod=clarify_mod)
|
||||
@@ -106,7 +109,7 @@ def _clarify_send_then_wait(fut, *, clarify_id: str, session_key: str, clarify_m
|
||||
response = clarify_mod.wait_for_response(clarify_id, timeout=float(timeout))
|
||||
late.disarm()
|
||||
if late.undeliverable:
|
||||
return UNDELIVERED, False
|
||||
return late.undeliverable, False
|
||||
if response is None or response == "":
|
||||
return f"[user did not respond within {int(timeout / 60)}m]", False
|
||||
return response, True
|
||||
@@ -117,11 +120,12 @@ class _LateFailureWatch:
|
||||
ack window, try the text fallback once, then release the waiter with the delivery notice.
|
||||
|
||||
Callbacks run on the gateway loop thread (the send future completes there); ``clear_session``
|
||||
wakes the agent thread blocked in ``wait_for_response`` and ``undeliverable`` tells it why.
|
||||
wakes the agent thread blocked in ``wait_for_response`` and ``undeliverable`` (the delivery
|
||||
sentinel, or ``None`` while nothing definitive happened) tells it why.
|
||||
Armed only while the future is still pending: a sent card needs no watch."""
|
||||
|
||||
def __init__(self, fut, *, clarify_id: str, session_key: str, clarify_mod, fallback) -> None:
|
||||
self.undeliverable = False
|
||||
self.undeliverable: Optional[str] = None
|
||||
self._armed = False
|
||||
self._clarify_id = clarify_id
|
||||
self._session_key = session_key
|
||||
@@ -149,7 +153,17 @@ class _LateFailureWatch:
|
||||
if outcome == "sent":
|
||||
return
|
||||
logger.warning("Clarify card send resolved %s after the ack window (id=%s)", outcome, self._clarify_id)
|
||||
fallback_fut = self._fallback() if outcome == "failed" and self._fallback is not None else None
|
||||
if outcome == "ambiguous":
|
||||
# Lost ack (``raw_response.ambiguous``): the card may well have posted. Same invariant as
|
||||
# ``_abort_for_outcome`` — stay armed for the late button tap, never re-send, never
|
||||
# release; the bounded wait's own timeout covers a card that truly never arrived.
|
||||
return
|
||||
if outcome == "declined":
|
||||
# Refused destination: no text retry (see ``_clarify_send_then_wait``), and the notice
|
||||
# says so rather than the generic delivery failure.
|
||||
self._release(UNDELIVERED_DECLINED)
|
||||
return
|
||||
fallback_fut = self._fallback() if self._fallback is not None else None
|
||||
if fallback_fut is None:
|
||||
self._release()
|
||||
return
|
||||
@@ -163,6 +177,6 @@ class _LateFailureWatch:
|
||||
return
|
||||
self._release()
|
||||
|
||||
def _release(self) -> None:
|
||||
self.undeliverable = True
|
||||
def _release(self, notice: str = UNDELIVERED) -> None:
|
||||
self.undeliverable = notice
|
||||
self._clarify_mod.clear_session(self._session_key)
|
||||
|
||||
@@ -78,10 +78,14 @@ def _runner(adapter, loop, monkeypatch, timeout=5):
|
||||
|
||||
|
||||
def _answer_once_text_prompt_is_seen(adapter, text):
|
||||
"""Answer ONLY after the plain-text prompt was observed — never on a deadline, so a missing
|
||||
fallback leaves the waiter blocked and the test fails on elapsed time / sent_text, not luck."""
|
||||
def _wait_then_answer():
|
||||
deadline = time.monotonic() + 10
|
||||
while time.monotonic() < deadline and not adapter.sent_text:
|
||||
time.sleep(0.02)
|
||||
if not adapter.sent_text:
|
||||
return
|
||||
time.sleep(0.1)
|
||||
cm.resolve_text_response_for_session("sk-fallback", text)
|
||||
threading.Thread(target=_wait_then_answer, daemon=True).start()
|
||||
@@ -130,8 +134,44 @@ def test_card_failing_after_the_ack_window_falls_back_to_text_instead_of_waiting
|
||||
_answer_once_text_prompt_is_seen(adapter, "beta")
|
||||
started = time.monotonic()
|
||||
response = _runner(adapter, loop, monkeypatch, timeout=30)._clarify_callback_sync("Pick?", ["alpha", "beta"])
|
||||
elapsed = time.monotonic() - started
|
||||
assert response == "beta"
|
||||
assert time.monotonic() - started < 10 # never the full clarify_timeout
|
||||
assert len(adapter.sent_text) == 1 # the plain-text prompt was actually sent, once
|
||||
assert elapsed < 0.2 + 2 # released right after the ack window + late failure, never clarify_timeout
|
||||
|
||||
|
||||
def test_card_resolving_ambiguous_after_the_ack_window_stays_armed_for_a_button_tap(loop, monkeypatch):
|
||||
"""A relay lost-ack (``raw_response.ambiguous``) after the window means the card MAY have posted:
|
||||
the registration must stay armed so the user's later button tap still answers — the same
|
||||
invariant ``_abort_for_outcome`` keeps for an immediate ambiguous outcome. Before: the late watch
|
||||
treated it like a definitive failure, released the wait with the delivery notice and the tap was
|
||||
lost (no pending entry)."""
|
||||
from gateway import run_turn_runner_clarify_delivery as delivery
|
||||
|
||||
monkeypatch.setattr(delivery, "SEND_ACK_WINDOW", 0.2)
|
||||
|
||||
async def late_ambiguous():
|
||||
await asyncio.sleep(0.6)
|
||||
return SendResult(success=False, raw_response={"ambiguous": True})
|
||||
|
||||
adapter = _CardAdapter(late_ambiguous)
|
||||
pending_at_tap = []
|
||||
|
||||
def _tap_button():
|
||||
time.sleep(1.5)
|
||||
entry = cm.get_pending_for_session("sk-fallback", include_choice_prompts=True)
|
||||
pending_at_tap.append(entry)
|
||||
if entry is not None:
|
||||
cm.resolve_gateway_clarify(entry.clarify_id, "beta")
|
||||
tap = threading.Thread(target=_tap_button, daemon=True)
|
||||
tap.start()
|
||||
|
||||
response, answered = _runner(adapter, loop, monkeypatch, timeout=30)._ask_clarify_question(
|
||||
"Pick?", ["alpha", "beta"], False)
|
||||
tap.join(timeout=5) # never let a late tap leak into the next test's registration
|
||||
assert pending_at_tap and pending_at_tap[0] is not None # still armed when the tap arrived
|
||||
assert (response, answered) == ("beta", True)
|
||||
assert adapter.sent_text == [] # possibly-delivered: never re-sent as text
|
||||
|
||||
|
||||
def test_card_and_text_both_failing_late_release_the_wait_with_the_delivery_notice(loop, monkeypatch):
|
||||
@@ -160,6 +200,24 @@ def test_card_and_text_both_failing_late_release_the_wait_with_the_delivery_noti
|
||||
assert len(adapter.sent_text) == 1 # the text fallback was tried exactly once
|
||||
|
||||
|
||||
def test_card_declined_after_the_ack_window_releases_with_the_declined_notice(loop, monkeypatch):
|
||||
"""A late connector DECLINE is as definitive as an immediate one: no text retry, and the
|
||||
notice names the refusal (``UNDELIVERED_DECLINED``) instead of the generic delivery failure."""
|
||||
from gateway import run_turn_runner_clarify_delivery as delivery
|
||||
|
||||
monkeypatch.setattr(delivery, "SEND_ACK_WINDOW", 0.2)
|
||||
|
||||
async def late_decline():
|
||||
await asyncio.sleep(0.6)
|
||||
return SendResult(success=False, error="egress declined: destination not allowed")
|
||||
|
||||
adapter = _CardAdapter(late_decline)
|
||||
response, answered = _runner(adapter, loop, monkeypatch, timeout=30)._ask_clarify_question(
|
||||
"Pick?", ["alpha", "beta"], False)
|
||||
assert (response, answered) == (delivery.UNDELIVERED_DECLINED, False)
|
||||
assert adapter.sent_text == []
|
||||
|
||||
|
||||
# --- Atom: no chat surface at all -----------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
@@ -1326,7 +1326,7 @@ When the agent calls the `clarify` tool — to ask which approach you prefer, ge
|
||||
|
||||
Tap a button to answer, or tap **Other** to type a free-form response (the next message you send becomes the answer). Open-ended `clarify` calls (no preset choices) skip the buttons and just capture your next message.
|
||||
|
||||
Configure the response timeout via `agent.clarify_timeout` in `~/.hermes/config.yaml` (default `600` seconds). If you don't respond within the timeout, the agent unblocks with a sentinel message and adapts rather than hanging.
|
||||
Configure the response timeout via `agent.clarify_timeout` in `~/.hermes/config.yaml` (default `3600` seconds). If you don't respond within the timeout, the agent unblocks with a sentinel message and adapts rather than hanging.
|
||||
|
||||
If Telegram cannot render the button card (the Bot API rejects it, or the send fails after its 15-second acknowledgement window), Hermes re-asks the same question as a plain numbered-list message and your typed reply (a number or the option text) is taken as the answer. When even that cannot be delivered, the agent is released at once with `[clarify prompt could not be delivered]` instead of waiting out the timeout and mistaking the silence for you not answering.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user