From 0b5d2f369830fb406efe95a6d0639793f414c413 Mon Sep 17 00:00:00 2001 From: BBaFSE <914322+BBaFSE@users.noreply.github.com> Date: Thu, 20 Aug 2026 05:28:23 -0400 Subject: [PATCH] fix(clarify): honor agent.clarify_timeout and support unlimited wait (#7163) The WebUI clarify bridge resolved the prompt timeout from a local, divergent copy (legacy clarify.timeout, 120s fallback) and treated 0 as invalid, so agent.clarify_timeout: 0 (unlimited) could not be honored and there was no way to make a clarification card wait for the user. Resolve the timeout with the agent core's canonical resolve_clarify_timeout() (clarify.timeout -> agent.clarify_timeout -> 3600, <= 0 = unlimited) against the session's own profile config (the streaming worker thread does not inherit the request's per-profile context), wait with a null deadline when unlimited, and stop the frontend rendering an expired countdown for non-positive timeouts. - api/streaming.py: per-session profile config resolution, canonical resolver with a compatibility fallback, and an extracted _await_clarify_response() helper that polls in 1s slices and honours cancellation. - api/clarify.py: preserve timeout_seconds <= 0 and emit no expires_at. - static/messages.js: _clarifyExpiryMs() returns 0 for non-positive timeouts so the card stays up without a countdown. - tests/test_sprint42.py: resolution matrix, per-session profile divergence, unlimited resolve-later / cancel-exit, and no-expiry payload. Closes #7159 Co-authored-by: nesquena-hermes --- api/clarify.py | 11 ++-- api/streaming.py | 115 +++++++++++++++++++++++++++++------------ static/messages.js | 2 +- tests/test_sprint42.py | 115 +++++++++++++++++++++++++++++++++++++++-- 4 files changed, 203 insertions(+), 40 deletions(-) diff --git a/api/clarify.py b/api/clarify.py index b42b0f13..e66bffcf 100644 --- a/api/clarify.py +++ b/api/clarify.py @@ -86,11 +86,16 @@ def clear_pending(session_key: str) -> int: def _with_timeout_metadata(data: dict) -> dict: item = dict(data or {}) requested_at = float(item.get("requested_at") or time.time()) - timeout_seconds = int(item.get("timeout_seconds") or DEFAULT_TIMEOUT_SECONDS) - expires_at = float(item.get("expires_at") or requested_at + timeout_seconds) + raw_timeout = item.get("timeout_seconds") + timeout_seconds = int(raw_timeout) if raw_timeout is not None else DEFAULT_TIMEOUT_SECONDS item["requested_at"] = requested_at item["timeout_seconds"] = timeout_seconds - item["expires_at"] = expires_at + if timeout_seconds <= 0: + # Unlimited: no expiry. The frontend renders no countdown and the card + # waits until the user answers (or the run is cancelled). + item["expires_at"] = 0 + return item + item["expires_at"] = float(item.get("expires_at") or requested_at + timeout_seconds) return item diff --git a/api/streaming.py b/api/streaming.py index 57105d89..35a9b091 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -790,17 +790,82 @@ def _is_quota_error_text(err_text: str) -> bool: ) -def _clarify_timeout_seconds(default: int = 120) -> int: - """Resolve clarify timeout from config, with bounded fallback.""" +def _clarify_session_config(sid: str) -> dict | None: + """Config dict for the clarify session's profile, or None to use ambient. + + The agent runs on a detached worker thread that does not inherit the + per-request TLS profile context, so the ambient ``get_config()`` would + resolve to the process-global profile (usually ``default``) even for a + session running under a named profile (issue #3294 pattern). Resolve the + session's own profile home instead, mirroring how the rest of the run is + configured. + """ try: - cfg = get_config() - raw = cfg.get("clarify", {}).get("timeout", default) - timeout_seconds = int(raw) - if timeout_seconds <= 0: - return default - return timeout_seconds + from api.models import _get_profile_home + from api.config import get_config_for_profile_home + session = get_session(sid) + if session is None: + return None + return get_config_for_profile_home(_get_profile_home(getattr(session, "profile", None))) except Exception: - return default + return None + + +def _clarify_timeout_seconds(config: dict | None = None, default: int = 3600) -> int: + """Resolve the clarify timeout (seconds) for WebUI clarify prompts. + + Uses the agent core's canonical resolver when available + (``tools.clarify_gateway.resolve_clarify_timeout`` — the same function the + CLI and messaging gateways use), falling back to an inline mirror of its + resolution order when the agent package is not importable. Resolution: + legacy top-level ``clarify.timeout`` if explicitly set, else the canonical + ``agent.clarify_timeout``, else 3600. ``<= 0`` is preserved verbatim and + means *unlimited*: the prompt waits until the user answers or the run is + cancelled — it is never auto-skipped. + + ``config`` may be an explicit profile config dict (see + :func:`_clarify_session_config`); when None, the ambient ``get_config()`` + is used. + """ + cfg = config if config is not None else (get_config() or {}) + try: + from tools.clarify_gateway import resolve_clarify_timeout + return int(resolve_clarify_timeout(cfg)) + except Exception: + pass + try: + raw = (cfg.get("clarify") or {}).get("timeout") + if raw is None: + raw = (cfg.get("agent") or {}).get("clarify_timeout", 3600) + return int(raw) + except Exception: + return int(default) + + +def _await_clarify_response(entry, timeout, cancel_evt) -> tuple[str, bool]: + """Block until a clarify entry resolves, or (finite timeout only) expires. + + ``timeout <= 0`` means *unlimited*: no deadline — the prompt waits until + the user answers or the run is cancelled. Polls the entry's event in ~1s + slices so a cancelled run unblocks promptly, and still honours the + ``is_interrupted``-style cancel check each slice. + + Returns ``(response, expired)``: ``expired`` is True when it returned + because of cancellation or a finite timeout without a user response (the + caller should clear pending and fall back), False when the user answered. + """ + deadline = None if timeout <= 0 else time.monotonic() + timeout + while True: + if cancel_evt.is_set(): + return "", True + wait_for = 1.0 + if deadline is not None: + remaining = deadline - time.monotonic() + if remaining <= 0: + return "", True + wait_for = min(1.0, remaining) + if entry.event.wait(timeout=wait_for): + return str(entry.result or "").strip(), False _CANCEL_MARKER_PATTERNS = ('task cancelled', 'task canceled', 'response interrupted') @@ -8859,7 +8924,7 @@ def _run_agent_streaming( def _clarify_callback_impl(question, choices, sid, cancel_evt, put_event): """Bridge Hermes clarify prompts to the WebUI.""" - timeout = _clarify_timeout_seconds() + timeout = _clarify_timeout_seconds(_clarify_session_config(sid)) choices_list = [str(choice) for choice in (choices or [])] data = { 'question': str(question or ''), @@ -8878,28 +8943,14 @@ def _run_agent_streaming( ) entry = _submit_clarify_pending(sid, data) - deadline = time.monotonic() + timeout - while True: - if cancel_evt.is_set(): - _clear_clarify_pending(sid) - return ( - "The user did not provide a response within the time limit. " - "Use your best judgement to make the choice and proceed." - ) - remaining = deadline - time.monotonic() - if remaining <= 0: - _clear_clarify_pending(sid) - return ( - "The user did not provide a response within the time limit. " - "Use your best judgement to make the choice and proceed." - ) - if entry.event.wait(timeout=min(1.0, remaining)): - response = str(entry.result or "").strip() - return ( - response - or "The user did not provide a response within the time limit. " - "Use your best judgement to make the choice and proceed." - ) + response, expired = _await_clarify_response(entry, timeout, cancel_evt) + if expired: + _clear_clarify_pending(sid) + return ( + response + or "The user did not provide a response within the time limit. " + "Use your best judgement to make the choice and proceed." + ) try: _token_sent = False # tracks whether any streamed tokens were sent diff --git a/static/messages.js b/static/messages.js index 5954902e..77d8cf08 100644 --- a/static/messages.js +++ b/static/messages.js @@ -8605,7 +8605,7 @@ function _clarifyExpiryMs(pending) { if (Number.isFinite(expiresAt) && expiresAt > 0) return expiresAt * 1000; const requestedAt = Number(pending && pending.requested_at); const timeoutSeconds = Number(pending && pending.timeout_seconds); - if (Number.isFinite(requestedAt) && Number.isFinite(timeoutSeconds)) { + if (Number.isFinite(requestedAt) && Number.isFinite(timeoutSeconds) && timeoutSeconds > 0) { return (requestedAt + timeoutSeconds) * 1000; } return 0; diff --git a/tests/test_sprint42.py b/tests/test_sprint42.py index c629e0f6..b3bc27ee 100644 --- a/tests/test_sprint42.py +++ b/tests/test_sprint42.py @@ -10,6 +10,7 @@ Covers: """ import ast import threading +import time import pathlib import re import queue @@ -465,6 +466,113 @@ class TestRuntimeRouteInjection(unittest.TestCase): def test_clarify_callback_passes_configured_timeout_seconds(self): """clarify prompt data should use clarify.timeout from config when present.""" + result = self._capture_clarify_timeout({"clarify": {"timeout": 300}}) + self.assertEqual(result["clarify_result"], "selected") + self.assertEqual(len(result["payloads"]), 1) + self.assertEqual(result["payloads"][0]["timeout_seconds"], 300) + + def test_clarify_callback_zero_timeout_is_unlimited(self): + """0 (or negative) must pass through as unlimited — never fall back to a default.""" + for config in ({"clarify": {"timeout": 0}}, {"agent": {"clarify_timeout": 0}}): + with self.subTest(config=config): + result = self._capture_clarify_timeout(config) + self.assertEqual(len(result["payloads"]), 1) + self.assertEqual(result["payloads"][0]["timeout_seconds"], 0) + + def test_clarify_timeout_falls_back_to_agent_clarify_timeout(self): + """Without the legacy clarify.timeout key, agent.clarify_timeout governs (canonical order).""" + import api.streaming as streaming + with mock.patch.object(streaming, "get_config", return_value={"agent": {"clarify_timeout": 45}}): + self.assertEqual(streaming._clarify_timeout_seconds(), 45) + with mock.patch.object(streaming, "get_config", return_value={}): + self.assertEqual(streaming._clarify_timeout_seconds(), 3600) + + def test_clarify_session_profile_beats_ambient(self): + """A non-default session's profile config must win over the ambient + (process-default) config — the exact divergence this fix resolves.""" + import api.streaming as streaming + fake_session = type("FakeSession", (), {"profile": "ops"})() + with mock.patch.object(streaming, "get_session", return_value=fake_session), \ + mock.patch("api.models._get_profile_home", return_value=pathlib.Path("/fake/profiles/ops")), \ + mock.patch("api.config.get_config_for_profile_home", return_value={"agent": {"clarify_timeout": 0}}), \ + mock.patch.object(streaming, "get_config", return_value={"agent": {"clarify_timeout": 600}}): + # session profile says 0 (unlimited); ambient says 600 → session wins + self.assertEqual( + streaming._clarify_timeout_seconds(streaming._clarify_session_config("sess-x")), + 0, + ) + + def test_clarify_timeout_resolution_matrix(self): + """The resolver must match the Agent core's canonical resolution order: + legacy clarify.timeout wins, else agent.clarify_timeout, else 3600; + non-positive values stay unlimited, invalid values fall back to 3600.""" + import api.streaming as streaming + cases = [ + ({"clarify": {"timeout": 120}}, 120), # legacy wins + ({"clarify": {"timeout": 120}, "agent": {"clarify_timeout": 0}}, 120), # legacy beats canonical + ({"agent": {"clarify_timeout": 45}}, 45), # canonical fallback + ({}, 3600), # absent → 3600 + ({"agent": {"clarify_timeout": "abc"}}, 3600), # invalid → 3600 + ({"clarify": {"timeout": "abc"}}, 3600), # invalid legacy → 3600 + ({"agent": {"clarify_timeout": 0}}, 0), # unlimited + ({"agent": {"clarify_timeout": -5}}, -5), # negative → unlimited + ({"clarify": {"timeout": -1}}, -1), # legacy negative → unlimited + ] + for cfg, expected in cases: + with self.subTest(config=cfg): + self.assertEqual(streaming._clarify_timeout_seconds(cfg), expected) + + def test_await_clarify_unlimited_resolves_later(self): + """An unlimited (timeout<=0) prompt keeps waiting until the user answers.""" + import api.streaming as streaming + entry = types.SimpleNamespace(result="yes", event=threading.Event()) + cancel = threading.Event() + threading.Timer(0.05, entry.event.set).start() + response, expired = streaming._await_clarify_response(entry, 0, cancel) + self.assertEqual(response, "yes") + self.assertFalse(expired) + + def test_await_clarify_unlimited_cancel_exits(self): + """Cancellation still unblocks an unlimited wait promptly.""" + import api.streaming as streaming + entry = types.SimpleNamespace(result=None, event=threading.Event()) + cancel = threading.Event() + cancel.set() + started = time.monotonic() + response, expired = streaming._await_clarify_response(entry, 0, cancel) + self.assertTrue(expired) + self.assertLess(time.monotonic() - started, 1.0) + + def test_await_clarify_finite_times_out(self): + """A finite timeout still expires without a response (fail-safe kept).""" + import api.streaming as streaming + entry = types.SimpleNamespace(result=None, event=threading.Event()) + response, expired = streaming._await_clarify_response(entry, 0.05, threading.Event()) + self.assertTrue(expired) + + def test_await_clarify_finite_resolves(self): + """A finite timeout with a user response resolves normally.""" + import api.streaming as streaming + entry = types.SimpleNamespace(result="ok", event=threading.Event()) + entry.event.set() + response, expired = streaming._await_clarify_response(entry, 300, threading.Event()) + self.assertEqual(response, "ok") + self.assertFalse(expired) + + def test_clarify_metadata_unlimited_has_no_expiry(self): + """timeout_seconds <= 0 must not fall back to the 120s default or set an expires_at.""" + from api.clarify import _with_timeout_metadata + item = _with_timeout_metadata({"timeout_seconds": 0, "requested_at": 1234.0}) + self.assertEqual(item["timeout_seconds"], 0) + self.assertEqual(item["expires_at"], 0) + # Finite timeouts still get an expires_at. + item2 = _with_timeout_metadata({"timeout_seconds": 300, "requested_at": 1234.0}) + self.assertEqual(item2["expires_at"], 1534.0) + + def _capture_clarify_timeout(self, config): + """Drive _run_agent_streaming with a fake agent that invokes the clarify + callback, capturing the payload the bridge submits. Returns a dict with + ``clarify_result`` and ``payloads``.""" import api.streaming as streaming captured = {} @@ -572,7 +680,8 @@ class TestRuntimeRouteInjection(unittest.TestCase): with mock.patch.object(streaming, "get_session", return_value=fake_session), \ mock.patch.object(streaming, "_get_ai_agent", return_value=CapturingAgent), \ mock.patch.object(streaming, "resolve_model_provider", return_value=("gpt-5.4", "openai-codex", None)), \ - mock.patch.object(streaming, "get_config", return_value={"clarify": {"timeout": 300}}), \ + mock.patch.object(streaming, "get_config", return_value=config), \ + mock.patch("api.config.get_config_for_profile_home", return_value=config), \ mock.patch("api.config._resolve_cli_toolsets", return_value=[]), \ mock.patch("api.clarify.submit_pending", side_effect=fake_submit_pending), \ mock.patch.dict(sys.modules, { @@ -589,9 +698,7 @@ class TestRuntimeRouteInjection(unittest.TestCase): stream_id=fake_stream_id, ) - self.assertEqual(captured["clarify_result"], "selected") - self.assertEqual(len(submit_payloads), 1) - self.assertEqual(submit_payloads[0]["timeout_seconds"], 300) + return {"clarify_result": captured.get("clarify_result"), "payloads": submit_payloads} class TestSessionDBAST(unittest.TestCase):