fix(tour): an unanswered tour bridge no longer costs 45s per action
The renderer's `tour.request` handler ships in the desktop bundle, but the tool is offered by the backend, and the two update on different clocks. A desktop build older than the tour tool receives the event in a renderer with no branch for it, so `tour.respond` never comes and the agent blocks for the full 45s deadline — once per action the model tries. A single "give me a tour" turn (targets, then narrate, then stop) stacked those waits into minutes of dead air, which is what got reported against #89620. Hold a session's first action to a deadline a working renderer cannot miss, and let an unanswered probe mark the bridge unavailable for that session: later calls return immediately with an error naming the actual fix instead of stalling again. Once a client has answered, real actions get the full deadline back, so a preview tour injecting into a live page still works and one slow action no longer condemns a live client. The verdict lives on the session record, so it dies with the session and a new one re-probes. The same five-action sequence goes from ~225s of dead air to a single 10s probe. Toolset gating is unchanged: removing the tool outright needs a client capability declared at session.create, which prompt caching means can only take effect for a new session.
This commit is contained in:
@@ -0,0 +1,80 @@
|
||||
"""Tests for the GUI-surface ``tour`` tool."""
|
||||
|
||||
import json
|
||||
|
||||
from tools import tour_tool as tt
|
||||
from tools.registry import registry
|
||||
|
||||
|
||||
def _run(**kwargs):
|
||||
kwargs.setdefault("callback", lambda _payload: json.dumps({"success": True}))
|
||||
return json.loads(tt.tour_tool(**kwargs))
|
||||
|
||||
|
||||
def test_lives_in_the_gui_surface_toolset(monkeypatch):
|
||||
"""Scoped by toolset, not by the backend's env — see AGENTS.md."""
|
||||
monkeypatch.delenv("HERMES_DESKTOP", raising=False)
|
||||
entry = registry.get_entry("tour")
|
||||
|
||||
assert entry is not None
|
||||
assert entry.toolset == "desktop_ui"
|
||||
assert entry.check_fn is None
|
||||
|
||||
|
||||
def test_requires_callback():
|
||||
"""Outside the desktop GUI there is no bridge — a clear error, no crash."""
|
||||
assert "desktop" in json.loads(tt.tour_tool(action="targets", callback=None))["error"]
|
||||
|
||||
|
||||
def test_rejects_unknown_action_and_surface():
|
||||
assert "action must be one of" in _run(action="dance")["error"]
|
||||
assert "surface must be one of" in _run(action="targets", surface="hologram")["error"]
|
||||
assert "side must be one of" in _run(action="show", selector="#a", side="diagonal")["error"]
|
||||
|
||||
|
||||
def test_show_needs_something_to_point_at_or_say():
|
||||
assert "show needs" in _run(action="show")["error"]
|
||||
assert "error" not in _run(action="show", text="just narration")
|
||||
|
||||
|
||||
def test_start_validates_its_steps():
|
||||
assert "non-empty steps" in _run(action="start")["error"]
|
||||
assert "non-empty steps" in _run(action="start", steps=[])["error"]
|
||||
assert "steps[1] must be an object" in _run(action="start", steps=[{"selector": "#a"}, "nope"])["error"]
|
||||
assert "steps[1] needs" in _run(action="start", steps=[{"selector": "#a"}, {}])["error"]
|
||||
|
||||
|
||||
def test_payload_omits_unset_fields_and_defaults_the_surface():
|
||||
seen = {}
|
||||
|
||||
def cb(payload):
|
||||
seen.update(payload)
|
||||
return json.dumps({"success": True})
|
||||
|
||||
tt.tour_tool(action="show", selector="#composer", title="Composer", callback=cb)
|
||||
assert seen == {
|
||||
"action": "show",
|
||||
"surface": "app",
|
||||
"selector": "#composer",
|
||||
"title": "Composer",
|
||||
}
|
||||
|
||||
|
||||
def test_unanswered_bridge_is_reported_rather_than_faked_as_success():
|
||||
assert "error" in _run(action="targets", callback=lambda _p: "")
|
||||
|
||||
|
||||
def test_passes_renderer_json_through():
|
||||
payload = {"success": True, "matched": True, "step": 2}
|
||||
assert _run(action="next", callback=lambda _p: json.dumps(payload)) == payload
|
||||
|
||||
|
||||
def test_wraps_non_json_text():
|
||||
assert _run(action="stop", callback=lambda _p: "stopped") == {"text": "stopped"}
|
||||
|
||||
|
||||
def test_callback_failure_is_reported():
|
||||
def _boom(_payload):
|
||||
raise RuntimeError("renderer went away")
|
||||
|
||||
assert "renderer went away" in _run(action="stop", callback=_boom)["error"]
|
||||
@@ -0,0 +1,101 @@
|
||||
"""A desktop client that cannot answer ``tour.request`` must not cost a full
|
||||
bridge timeout per call.
|
||||
|
||||
The renderer's handler ships in the desktop bundle; the tool is offered by the
|
||||
backend. An app build older than the tour tool has no branch for the event, so
|
||||
nothing ever calls ``tour.respond`` and the agent blocks for the whole deadline
|
||||
— once per action the model tries. See tui_gateway.server._tour_request.
|
||||
"""
|
||||
|
||||
import json
|
||||
|
||||
import pytest
|
||||
|
||||
import tui_gateway.server as server
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def session(monkeypatch):
|
||||
record = {}
|
||||
monkeypatch.setitem(server._sessions, "s1", record)
|
||||
return record
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def bridge(monkeypatch):
|
||||
"""Record every _block call and serve canned answers."""
|
||||
calls = []
|
||||
|
||||
def fake_block(event, sid, payload, timeout=None, **_kw):
|
||||
answer = fake_block.answers.pop(0) if fake_block.answers else ""
|
||||
calls.append({"event": event, "sid": sid, "payload": payload, "timeout": timeout})
|
||||
return answer
|
||||
|
||||
fake_block.answers = []
|
||||
fake_block.calls = calls
|
||||
monkeypatch.setattr(server, "_block", fake_block)
|
||||
return fake_block
|
||||
|
||||
|
||||
def test_first_action_is_probed_on_a_short_deadline(session, bridge):
|
||||
bridge.answers = [json.dumps({"success": True})]
|
||||
server._tour_request("s1", {"action": "targets"})
|
||||
|
||||
assert bridge.calls[0]["event"] == "tour.request"
|
||||
assert bridge.calls[0]["timeout"] == server._TOUR_PROBE_TIMEOUT_S
|
||||
assert server._TOUR_PROBE_TIMEOUT_S < server._TOUR_TIMEOUT_S
|
||||
|
||||
|
||||
def test_a_client_that_answers_gets_the_full_deadline_back(session, bridge):
|
||||
"""The generous deadline exists for a preview tour injecting into a live
|
||||
page; only an unproven client is held to the probe."""
|
||||
bridge.answers = [json.dumps({"success": True}), json.dumps({"success": True})]
|
||||
server._tour_request("s1", {"action": "targets"})
|
||||
server._tour_request("s1", {"action": "show", "selector": "#composer"})
|
||||
|
||||
assert bridge.calls[1]["timeout"] == server._TOUR_TIMEOUT_S
|
||||
|
||||
|
||||
def test_unanswered_probe_explains_the_real_problem(session, bridge):
|
||||
result = json.loads(server._tour_request("s1", {"action": "targets"}))
|
||||
|
||||
assert result["success"] is False
|
||||
assert "desktop" in result["error"].lower()
|
||||
assert "update" in result["error"].lower()
|
||||
|
||||
|
||||
def test_later_actions_short_circuit_instead_of_stalling_again(session, bridge):
|
||||
"""The regression: a "give me a tour" turn stacks one timeout per action."""
|
||||
server._tour_request("s1", {"action": "targets"})
|
||||
for action in ("show", "start", "next", "stop"):
|
||||
assert json.loads(server._tour_request("s1", {"action": action}))["success"] is False
|
||||
|
||||
assert len(bridge.calls) == 1
|
||||
|
||||
|
||||
def test_a_proven_client_is_not_condemned_by_one_slow_action(session, bridge):
|
||||
"""A transient miss on a live renderer must not disable the tour."""
|
||||
bridge.answers = [json.dumps({"success": True})]
|
||||
server._tour_request("s1", {"action": "targets"})
|
||||
server._tour_request("s1", {"action": "show", "selector": "#composer"})
|
||||
|
||||
bridge.answers = [json.dumps({"success": True})]
|
||||
assert json.loads(server._tour_request("s1", {"action": "next"}))["success"] is True
|
||||
assert len(bridge.calls) == 3
|
||||
|
||||
|
||||
def test_a_new_session_reprobes(bridge, monkeypatch):
|
||||
"""The verdict lives on the session record, so it dies with the session."""
|
||||
monkeypatch.setitem(server._sessions, "dead", {})
|
||||
monkeypatch.setitem(server._sessions, "fresh", {})
|
||||
|
||||
server._tour_request("dead", {"action": "targets"})
|
||||
bridge.answers = [json.dumps({"success": True})]
|
||||
|
||||
assert json.loads(server._tour_request("fresh", {"action": "targets"}))["success"] is True
|
||||
|
||||
|
||||
def test_a_session_with_no_record_still_bridges(bridge):
|
||||
"""Detached callers have no session dict; they keep the plain bridge."""
|
||||
bridge.answers = [json.dumps({"success": True})]
|
||||
assert json.loads(server._tour_request("gone", {"action": "targets"}))["success"] is True
|
||||
+70
-8
@@ -3618,6 +3618,74 @@ def _clarify_block(sid: str, q, c, multi_select=False, questions=None) -> str:
|
||||
)
|
||||
|
||||
|
||||
# A tour action is a DOM operation the renderer performs and answers straight
|
||||
# back, so a client that implements the bridge replies in milliseconds. The
|
||||
# generous deadline exists for one case only: a preview tour's first action
|
||||
# injects the engine into a live page.
|
||||
_TOUR_TIMEOUT_S = 45
|
||||
# Until a session's client has proven it answers at all, hold it to a deadline
|
||||
# a working renderer cannot miss. See _tour_request.
|
||||
_TOUR_PROBE_TIMEOUT_S = 10
|
||||
|
||||
_TOUR_BRIDGE_UNAVAILABLE = json.dumps(
|
||||
{
|
||||
"success": False,
|
||||
"error": (
|
||||
"No Hermes Desktop window answered the tour request. The tour is "
|
||||
"driven by the desktop app's renderer, which updates separately "
|
||||
"from this backend, so an app build older than the tour tool has "
|
||||
"nothing listening. Update the Hermes Desktop app and start a new "
|
||||
"session. Do not retry tour in this session."
|
||||
),
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
def _tour_request(sid: str, payload: dict) -> str:
|
||||
"""Bridge the tour tool callback onto _block, without paying for a client
|
||||
that cannot answer it.
|
||||
|
||||
The renderer's ``tour.request`` handler ships in the desktop bundle, but
|
||||
the tool is offered by this backend — and the two update on different
|
||||
clocks. Against an app older than the tool the event lands in a renderer
|
||||
with no branch for it, nobody ever calls ``tour.respond``, and the agent
|
||||
blocks for the full deadline. The model then does what the schema tells it
|
||||
to and tries the next action, so a single "give me a tour" turn stacks
|
||||
those waits (the timeouts reported against #89620).
|
||||
|
||||
A session's first action therefore gets the probe deadline, and an
|
||||
unanswered probe marks the bridge unavailable for that session: every later
|
||||
call returns immediately, telling the user what to actually fix instead of
|
||||
stalling again. Once a client has answered, real actions get the full
|
||||
deadline back and a single slow one no longer condemns it. The verdict
|
||||
lives on the session record, so it dies with the session and a new one
|
||||
re-probes.
|
||||
"""
|
||||
# A detached caller has no session record; the throwaway keeps it on the
|
||||
# plain bridge, unprobed.
|
||||
session = _sessions.get(sid)
|
||||
if session is None:
|
||||
session = {}
|
||||
state = session.get("tour_bridge")
|
||||
|
||||
if state == "unanswered":
|
||||
return _TOUR_BRIDGE_UNAVAILABLE
|
||||
|
||||
answer = _block(
|
||||
"tour.request",
|
||||
sid,
|
||||
dict(payload),
|
||||
timeout=_TOUR_TIMEOUT_S if state == "answered" else _TOUR_PROBE_TIMEOUT_S,
|
||||
)
|
||||
|
||||
if answer:
|
||||
session["tour_bridge"] = "answered"
|
||||
elif state != "answered":
|
||||
session["tour_bridge"] = "unanswered"
|
||||
|
||||
return answer or _TOUR_BRIDGE_UNAVAILABLE
|
||||
|
||||
|
||||
def _clear_pending(sid: str | None = None) -> None:
|
||||
"""Release pending prompts with an empty answer.
|
||||
|
||||
@@ -6325,14 +6393,8 @@ def _agent_cbs(sid: str) -> dict:
|
||||
# tour tool (desktop GUI): the renderer drives driver.js — highlighting
|
||||
# elements in the app's own DOM or injecting the engine into the
|
||||
# preview pane's webview — and answers tour.respond with the outcome
|
||||
# (did the selector match, which step is active). Generous timeout: a
|
||||
# preview tour's first action loads the engine into a live page.
|
||||
"tour_callback": lambda payload: _block(
|
||||
"tour.request",
|
||||
sid,
|
||||
dict(payload),
|
||||
timeout=45,
|
||||
),
|
||||
# (did the selector match, which step is active).
|
||||
"tour_callback": lambda payload: _tour_request(sid, payload),
|
||||
}
|
||||
|
||||
# Interim assistant commentary (text alongside tool calls, or the attempted
|
||||
|
||||
Reference in New Issue
Block a user