diff --git a/cli.py b/cli.py index 7acf94517f6..afbdd6cb76a 100644 --- a/cli.py +++ b/cli.py @@ -11533,7 +11533,11 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): """ import time as _time - timeout = CLI_CONFIG.get("clarify", {}).get("timeout", 120) + from tools.clarify_gateway import resolve_clarify_timeout + + # Canonical clarify timeout, shared with the gateway/TUI path. `<= 0` + # means unlimited (never auto-skip mid-think) → a null deadline. + timeout = resolve_clarify_timeout(CLI_CONFIG) response_queue = queue.Queue() is_open_ended = not choices @@ -11543,7 +11547,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): "selected": 0, "response_queue": response_queue, } - self._clarify_deadline = _time.monotonic() + timeout + self._clarify_deadline = None if timeout <= 0 else _time.monotonic() + timeout # Open-ended questions skip straight to freetext input self._clarify_freetext = is_open_ended @@ -11560,13 +11564,15 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): while True: try: result = response_queue.get(timeout=1) - self._clarify_deadline = 0 + self._clarify_deadline = None self._persist_prompt_summary("?", "Clarify", question, str(result)) return result except queue.Empty: - remaining = self._clarify_deadline - _time.monotonic() - if remaining <= 0: - break + # None deadline = unlimited: never auto-skip, just keep polling. + if self._clarify_deadline is not None: + remaining = self._clarify_deadline - _time.monotonic() + if remaining <= 0: + break now = _time.monotonic() if now - _last_countdown_refresh >= 1.0: _last_countdown_refresh = now @@ -11575,7 +11581,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): # Timed out — tear down the UI and let the agent decide self._clarify_state = None self._clarify_freetext = False - self._clarify_deadline = 0 + self._clarify_deadline = None self._paint_now() _cprint(f"\n{_DIM}(clarify timed out after {timeout}s — agent will decide){_RST}") return ( @@ -14583,8 +14589,12 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin): ] if cli_ref._clarify_state: - remaining = max(0, int(cli_ref._clarify_deadline - time.monotonic())) - countdown = f' ({remaining}s)' if cli_ref._clarify_deadline else '' + # None deadline = unlimited wait → hide the countdown entirely. + if cli_ref._clarify_deadline is None: + countdown = '' + else: + remaining = max(0, int(cli_ref._clarify_deadline - time.monotonic())) + countdown = f' ({remaining}s)' if cli_ref._clarify_freetext: return [ ('class:hint', ' type your answer and press Enter'), diff --git a/hermes_cli/callbacks.py b/hermes_cli/callbacks.py index b0279ff73de..f28577cf302 100644 --- a/hermes_cli/callbacks.py +++ b/hermes_cli/callbacks.py @@ -22,8 +22,11 @@ def clarify_callback(cli, question, choices): responds. Returns the user's choice or a timeout message. """ from cli import CLI_CONFIG + from tools.clarify_gateway import resolve_clarify_timeout - timeout = CLI_CONFIG.get("clarify", {}).get("timeout", 120) + # Canonical clarify timeout, shared with the gateway/TUI path. `<= 0` + # means unlimited (never auto-skip mid-think) → a null deadline. + timeout = resolve_clarify_timeout(CLI_CONFIG) response_queue = queue.Queue() is_open_ended = not choices @@ -33,7 +36,7 @@ def clarify_callback(cli, question, choices): "selected": 0, "response_queue": response_queue, } - cli._clarify_deadline = _time.monotonic() + timeout + cli._clarify_deadline = None if timeout <= 0 else _time.monotonic() + timeout cli._clarify_freetext = is_open_ended if hasattr(cli, "_app") and cli._app: @@ -42,18 +45,20 @@ def clarify_callback(cli, question, choices): while True: try: result = response_queue.get(timeout=1) - cli._clarify_deadline = 0 + cli._clarify_deadline = None return result except queue.Empty: - remaining = cli._clarify_deadline - _time.monotonic() - if remaining <= 0: - break + # None deadline = unlimited: never auto-skip, just keep polling. + if cli._clarify_deadline is not None: + remaining = cli._clarify_deadline - _time.monotonic() + if remaining <= 0: + break if hasattr(cli, "_app") and cli._app: cli._app.invalidate() cli._clarify_state = None cli._clarify_freetext = False - cli._clarify_deadline = 0 + cli._clarify_deadline = None if hasattr(cli, "_app") and cli._app: cli._app.invalidate() cprint(f"\n{_DIM}(clarify timed out after {timeout}s — agent will decide){_RST}") diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 4387eab7d5c..eb295799270 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -12110,3 +12110,36 @@ def test_tts_stream_vad_barge_in_cuts_pipeline_and_submits_capture(monkeypatch, assert not wav.exists() # capture temp file cleaned up assert ts.take_speech_interrupted() is True # VAD cut latches the model note server._tts_stream_stop() + + +def test_clarify_callback_uses_configured_timeout(monkeypatch): + """The TUI/desktop clarify bridge honors the canonical clarify timeout + (via _clarify_timeout_seconds) instead of the hardcoded _block default.""" + captured = {} + + monkeypatch.setattr(server, "_clarify_timeout_seconds", lambda: 42) + + def fake_block(event, sid, payload, timeout=300): + captured.update(event=event, sid=sid, payload=payload, timeout=timeout) + return "answer" + + monkeypatch.setattr(server, "_block", fake_block) + + result = server._agent_cbs("sid-1")["clarify_callback"]("Pick one", ["a", "b"]) + + assert result == "answer" + assert captured["event"] == "clarify.request" + assert captured["timeout"] == 42 + assert captured["payload"] == {"question": "Pick one", "choices": ["a", "b"]} + + +@pytest.mark.parametrize( + ("configured", "expected"), + [(0, None), (-1, None), (42, 42)], +) +def test_clarify_timeout_seconds_maps_non_positive_to_unlimited(monkeypatch, configured, expected): + """A ``<= 0`` clarify timeout means unlimited and reaches _block as None + (ev.wait(None) waits forever) rather than an immediate ev.wait(0) skip.""" + monkeypatch.setattr("tools.clarify_gateway.get_clarify_timeout", lambda: configured) + + assert server._clarify_timeout_seconds() == expected diff --git a/tests/tools/test_clarify_gateway.py b/tests/tools/test_clarify_gateway.py index a7082708a6e..fab1ebedcc9 100644 --- a/tests/tools/test_clarify_gateway.py +++ b/tests/tools/test_clarify_gateway.py @@ -294,3 +294,148 @@ class TestGatewayTextIntercept: # Clean up cm.clear_session("sk-tf") + + +class TestCoverageGaps: + """Cover remaining branches: signature(), get_entry miss, find_awaiting + with deleted entry, cancel with None entry, timeout exception, get_notify.""" + + def setup_method(self): + _clear_clarify_state() + + def test_entry_signature(self): + """_ClarifyEntry.signature() returns the expected dict.""" + from tools import clarify_gateway as cm + + entry = cm.register("sig1", "sk", "Q?", ["A", "B"]) + sig = entry.signature() + assert sig["clarify_id"] == "sig1" + assert sig["session_key"] == "sk" + assert sig["question"] == "Q?" + assert sig["choices"] == ["A", "B"] + + def test_entry_signature_no_choices(self): + """signature() returns None for choices when open-ended.""" + from tools import clarify_gateway as cm + + entry = cm.register("sig2", "sk", "Q?", None) + sig = entry.signature() + assert sig["choices"] is None + + def test_wait_for_response_unknown_id_returns_none(self): + """wait_for_response on a non-existent id returns None immediately.""" + from tools import clarify_gateway as cm + + assert cm.wait_for_response("nonexistent-id", timeout=0.1) is None + + def test_find_awaiting_skips_deleted_entry(self): + """get_pending_for_session skips entries that were removed from _entries + but still listed in _session_index.""" + from tools import clarify_gateway as cm + + cm.register("a1", "sk", "Q?", None) + # Manually remove from _entries but leave in _session_index + with cm._lock: + cm._entries.pop("a1", None) + # No entry to find → returns None + assert cm.get_pending_for_session("sk") is None + + def test_clear_session_skips_deleted_entry(self): + """clear_session skips entries that are None (already removed).""" + from tools import clarify_gateway as cm + + cm.register("c1", "sk", "Q?", ["A"]) + # Manually remove from _entries but leave in _session_index + with cm._lock: + cm._entries.pop("c1", None) + # Should return 0 cancelled (entry was already gone) + cancelled = cm.clear_session("sk") + assert cancelled == 0 + + def test_get_clarify_timeout_exception_returns_default(self, monkeypatch): + """get_clarify_timeout returns 3600 when load_config raises.""" + from tools import clarify_gateway as cm + + monkeypatch.setattr("hermes_cli.config.load_config", + lambda: (_ for _ in ()).throw(RuntimeError("boom"))) + assert cm.get_clarify_timeout() == 3600 + + def test_get_notify_returns_callback(self): + """get_notify returns the registered callback.""" + from tools import clarify_gateway as cm + + cb = lambda entry: None + cm.register_notify("sk-notify", cb) + assert cm.get_notify("sk-notify") is cb + + def test_get_notify_returns_none_when_not_registered(self): + """get_notify returns None for an unregistered session.""" + from tools import clarify_gateway as cm + + assert cm.get_notify("unregistered") is None + + +class TestClarifyTimeoutResolution: + """resolve_clarify_timeout is the single source of truth for the clarify + timeout, shared by the CLI, TUI/desktop, and messaging-gateway paths.""" + + def test_canonical_agent_key(self): + from tools import clarify_gateway as cm + + assert cm.resolve_clarify_timeout({"agent": {"clarify_timeout": 900}}) == 900 + + def test_legacy_clarify_key_overrides(self): + """An explicitly-set legacy top-level clarify.timeout wins, for + back-compat with users who set it before agent.clarify_timeout existed.""" + from tools import clarify_gateway as cm + + cfg = {"clarify": {"timeout": 42}, "agent": {"clarify_timeout": 900}} + assert cm.resolve_clarify_timeout(cfg) == 42 + + def test_default_when_unset(self): + from tools import clarify_gateway as cm + + assert cm.resolve_clarify_timeout({}) == 3600 + + def test_non_numeric_falls_back_to_default(self): + from tools import clarify_gateway as cm + + assert cm.resolve_clarify_timeout({"agent": {"clarify_timeout": "nope"}}) == 3600 + + def test_non_positive_preserved_as_unlimited_sentinel(self): + """<= 0 is passed through verbatim — the waiting loops read it as + 'unlimited', so the resolver must not clamp it to a positive default.""" + from tools import clarify_gateway as cm + + assert cm.resolve_clarify_timeout({"agent": {"clarify_timeout": 0}}) == 0 + assert cm.resolve_clarify_timeout({"clarify": {"timeout": -1}}) == -1 + + +class TestUnlimitedWait: + """timeout <= 0 makes wait_for_response block until the answer arrives + instead of auto-skipping.""" + + def setup_method(self): + _clear_clarify_state() + + def test_zero_timeout_waits_until_resolved(self): + from tools import clarify_gateway as cm + + cm.register("u1", "sk", "Q?", ["A", "B"]) + result_box = {} + + def waiter(): + result_box["r"] = cm.wait_for_response("u1", timeout=0) + + t = threading.Thread(target=waiter) + t.start() + # An unlimited wait cannot finish while nothing resolves it: still + # running after a comfortable margin (old code auto-skipped at once). + t.join(timeout=1.5) + assert t.is_alive() + + # Once resolved, the unlimited wait returns the real answer. + cm.resolve_gateway_clarify("u1", "B") + t.join(timeout=5.0) + assert not t.is_alive() + assert result_box["r"] == "B" diff --git a/tools/clarify_gateway.py b/tools/clarify_gateway.py index be527008a22..bb7ed4a9707 100644 --- a/tools/clarify_gateway.py +++ b/tools/clarify_gateway.py @@ -108,6 +108,10 @@ def wait_for_response(clarify_id: str, timeout: float) -> Optional[str]: for 10 minutes with zero activity touches and the gateway's inactivity watchdog kills the agent while the user is still typing. + ``timeout <= 0`` means an unlimited wait (never auto-skip mid-think); the + heartbeat still fires each slice so inactivity watchdogs don't kill a live + prompt. + Returns the resolved response string, or ``None`` on timeout. """ with _lock: @@ -120,13 +124,19 @@ def wait_for_response(clarify_id: str, timeout: float) -> Optional[str]: except Exception: # pragma: no cover - optional touch_activity_if_due = None - deadline = time.monotonic() + max(timeout, 0.0) + # 0 / negative → unlimited: no deadline, poll forever in 1s slices. + unlimited = timeout is None or float(timeout) <= 0.0 + deadline = None if unlimited else time.monotonic() + float(timeout) activity_state = {"last_touch": time.monotonic(), "start": time.monotonic()} while True: - remaining = deadline - time.monotonic() - if remaining <= 0: - break - if entry.event.wait(timeout=min(1.0, remaining)): + if deadline is None: + slice_s = 1.0 + else: + remaining = deadline - time.monotonic() + if remaining <= 0: + break + slice_s = min(1.0, remaining) + if entry.event.wait(timeout=slice_s): break if touch_activity_if_due is not None: touch_activity_if_due(activity_state, "waiting for user clarify response") @@ -300,6 +310,29 @@ def clear_session(session_key: str) -> int: # Config # ========================================================================= +def resolve_clarify_timeout(config: dict) -> int: + """Resolve the clarify timeout (seconds) from an already-loaded config dict. + + Single source of truth shared by every surface (messaging gateway, CLI, + TUI/desktop) so the timeout can't drift between them. Resolution order: + + 1. legacy top-level ``clarify.timeout`` if a user explicitly set it, + 2. else the canonical ``agent.clarify_timeout``, + 3. else 3600 (1 hour). + + ``<= 0`` is preserved verbatim and means *unlimited* to callers (never + auto-skip while the user is still deciding); the waiting loops translate + that into a null deadline. A non-numeric value falls back to 3600. + """ + raw = (config.get("clarify") or {}).get("timeout") + if raw is None: + raw = (config.get("agent") or {}).get("clarify_timeout", 3600) + try: + return int(raw) + except (TypeError, ValueError): + return 3600 + + def get_clarify_timeout() -> int: """Read the clarify response timeout (seconds) from config. @@ -311,13 +344,14 @@ def get_clarify_timeout() -> int: tap landed on a dead entry and the agent hung on ``running: clarify`` (#32762). - Reads ``agent.clarify_timeout`` from config.yaml. + Reads ``agent.clarify_timeout`` from config.yaml (see + :func:`resolve_clarify_timeout` for the full resolution order). Set to + ``0`` (or negative) for an unlimited wait — never auto-skip while the user + is still deciding. """ try: from hermes_cli.config import load_config - cfg = load_config() or {} - agent_cfg = cfg.get("agent", {}) or {} - return int(agent_cfg.get("clarify_timeout", 3600)) + return resolve_clarify_timeout(load_config() or {}) except Exception: return 3600 diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 70a47d673e2..e1a41570802 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -2411,7 +2411,7 @@ def _enable_gateway_prompts() -> None: # ── Blocking prompt factory ────────────────────────────────────────── -def _block(event: str, sid: str, payload: dict, timeout: int = 300) -> str: +def _block(event: str, sid: str, payload: dict, timeout: float | None = 300) -> str: rid = uuid.uuid4().hex[:8] ev = threading.Event() with _prompt_lock: @@ -2423,7 +2423,10 @@ def _block(event: str, sid: str, payload: dict, timeout: int = 300) -> str: answer_present = False try: _emit(event, sid, payload) - answered = ev.wait(timeout=timeout) + # Natural Event semantics: None → wait forever (clarify configured with + # clarify_timeout <= 0, released only by a real answer or + # session.interrupt), 0 → return immediately, > 0 → bounded wait. + answered = ev.wait(timeout) finally: with _prompt_lock: _pending.pop(rid, None) @@ -2452,6 +2455,19 @@ def _block(event: str, sid: str, payload: dict, timeout: int = 300) -> str: return answer +def _clarify_timeout_seconds() -> float | None: + """Clarify wait (seconds) for the TUI/desktop bridge, from the same + canonical config the messaging gateway and CLI use. Falls back to the + historical 300s _block default if config can't be read. ``<= 0`` in config + means unlimited and is returned as ``None`` (never auto-skip).""" + try: + from tools.clarify_gateway import get_clarify_timeout + timeout = get_clarify_timeout() + return timeout if timeout > 0 else None + except Exception: + return 300 + + def _clear_pending(sid: str | None = None) -> None: """Release pending prompts with an empty answer. @@ -4531,7 +4547,10 @@ def _agent_cbs(sid: str) -> dict: "notification.clear", sid, {"key": key} ), "clarify_callback": lambda q, c: _block( - "clarify.request", sid, {"question": q, "choices": c} + "clarify.request", + sid, + {"question": q, "choices": c}, + timeout=_clarify_timeout_seconds(), ), # read_terminal tool (desktop GUI): same blocking bridge as clarify — the # renderer answers terminal.read.respond with the serialized buffer.