From b274b346d846fdd38ac650ebf00c50b84ad00ee2 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 23 Aug 2026 19:35:07 -0700 Subject: [PATCH] =?UTF-8?q?feat(bots):=20retry=20session=20policy=20?= =?UTF-8?q?=E2=80=94=20resume=20transient=20turns,=20compress-and-resume?= =?UTF-8?q?=20on=20context=20overflow=20(#93091=20item=205)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer ruling (2026-08-23): a retried bot turn never mints a fresh session. retry_action() maps the #93091 item-1 reason enum to one of resume / compress_then_resume / none: - transient classes (runtime_offline, delivery_timeout, rate limit, server error) re-run the same Bot Chat session once; - context_overflow also re-runs the same session — the retried turn goes through the pre-API compaction pass in conversation_loop.py, which compacts the over-threshold transcript first (the one sanctioned context mutation); no fresh-session escape hatch exists; - auth/quota/config/model classes never auto-retry. Wired at both delivery surfaces (fix the class, not one site): bot_relay.deliver (relay handler) and _run_delivery (local message_agent runner). Failed deliveries now carry the classified reason in the structured error payload (error.data.reason). Sabotage-verified: with the retry blocks removed, 3 consumer tests fail; with them present, 22/22 pass. --- tests/tools/test_bot_retry_policy.py | 210 +++++++++++++++++++++++++++ tools/bot_failure_reasons.py | 34 +++++ tools/bot_mode_dm.py | 38 ++++- tui_gateway/methods_bot_relay.py | 36 ++++- website/docs/user-guide/bot-mode.md | 4 + 5 files changed, 317 insertions(+), 5 deletions(-) create mode 100644 tests/tools/test_bot_retry_policy.py diff --git a/tests/tools/test_bot_retry_policy.py b/tests/tools/test_bot_retry_policy.py new file mode 100644 index 0000000000..06b302d67f --- /dev/null +++ b/tests/tools/test_bot_retry_policy.py @@ -0,0 +1,210 @@ +"""Tests: bot-turn retry session policy (#93091 item 5). + +Maintainer ruling (2026-08-23): a retried bot turn never mints a fresh +session. Transient classes resume; context_overflow re-runs the same session +so the retried turn's pre-API compaction pass compacts first; auth/quota/ +config classes never auto-retry. These tests pin the policy function and the +two delivery surfaces that consume it (relay handler + local delivery +runner) — same-session argv identity is the load-bearing assertion. +""" + +from __future__ import annotations + +import pytest + +from tools import bot_failure_reasons as bfr + + +# ── policy function ────────────────────────────────────────────────────────── + + +@pytest.mark.parametrize( + "reason", + sorted(bfr.AUTO_RETRYABLE), +) +def test_transient_reasons_resume(reason): + assert bfr.retry_action(reason) == bfr.RETRY_RESUME + + +def test_context_overflow_compresses_then_resumes(): + assert bfr.retry_action(bfr.CONTEXT_OVERFLOW) == bfr.RETRY_COMPRESS_THEN_RESUME + + +@pytest.mark.parametrize( + "reason", + [ + bfr.PROVIDER_AUTH_OR_ACCESS, + bfr.PROVIDER_QUOTA_LIMIT, + bfr.MISSING_CONFIG, + bfr.MODEL_UNAVAILABLE, + bfr.AGENT_BLOCKED, + bfr.CANCELLED, + bfr.QUEUED_EXPIRED, + bfr.UNKNOWN, + "", + "not-a-reason", + ], +) +def test_non_retryable_reasons_stop(reason): + assert bfr.retry_action(reason) == bfr.RETRY_NONE + + +def test_every_reason_has_a_defined_action(): + """Invariant: the policy is total over the closed reason vocabulary.""" + for reason in bfr.ALL_REASONS: + assert bfr.retry_action(reason) in { + bfr.RETRY_RESUME, + bfr.RETRY_COMPRESS_THEN_RESUME, + bfr.RETRY_NONE, + } + + +# ── relay deliver handler consumes the policy ──────────────────────────────── + + +@pytest.fixture +def home(tmp_path, monkeypatch): + h = tmp_path / ".hermes" + (h / "profiles" / "ops").mkdir(parents=True) + monkeypatch.setenv("HERMES_HOME", str(h)) + return h + + +def _deliver(params): + import tui_gateway.server as srv + + return srv._methods["bot_relay.deliver"](1, params) + + +def _transport_calls(calls): + """Only the Bot Chat transport spawns — a global subprocess.run patch also + catches unrelated maintenance calls (git version probes on first server + import), which must not count as delivery attempts.""" + return [argv for argv in calls if argv and argv[0] == "hermes"] + + +class _Proc: + def __init__(self, returncode, stdout="", stderr=""): + self.returncode = returncode + self.stdout = stdout + self.stderr = stderr + + +def test_deliver_retries_same_argv_on_transient_failure(home, monkeypatch): + """First run 429s → exactly one re-run with the IDENTICAL argv (same + profile, same query file — i.e. the same session), which then succeeds.""" + calls = [] + + def _fake_run(argv, **kwargs): + calls.append(list(argv)) + if list(argv)[:1] != ["hermes"]: + return _Proc(0) + if len(_transport_calls(calls)) == 1: + return _Proc(1, stderr="Error code: 429 - rate limit exceeded") + return _Proc(0, stdout="recovered reply") + + monkeypatch.setattr("subprocess.run", _fake_run) + out = _deliver({"profile": "ops", "message": "ping"}) + assert out["result"]["reply"] == "recovered reply" + turns = _transport_calls(calls) + assert len(turns) == 2 + assert turns[0] == turns[1], "retry must re-run the SAME session/argv" + + +def test_deliver_retries_once_on_context_overflow(home, monkeypatch): + """context_overflow gets the compress-then-resume re-run: same argv (the + retried turn's own pre-API compaction does the compress), never a + different/fresh target.""" + calls = [] + + def _fake_run(argv, **kwargs): + calls.append(list(argv)) + if list(argv)[:1] != ["hermes"]: + return _Proc(0) + if len(_transport_calls(calls)) == 1: + return _Proc(1, stderr="This model's maximum context length is 200000 tokens") + return _Proc(0, stdout="fits after compaction") + + monkeypatch.setattr("subprocess.run", _fake_run) + out = _deliver({"profile": "ops", "message": "ping"}) + assert out["result"]["reply"] == "fits after compaction" + turns = _transport_calls(calls) + assert len(turns) == 2 + assert turns[0] == turns[1] + + +def test_deliver_never_retries_auth_failure(home, monkeypatch): + """Auth/quota/config classes must not burn a second turn.""" + calls = [] + + def _fake_run(argv, **kwargs): + calls.append(list(argv)) + if list(argv)[:1] != ["hermes"]: + return _Proc(0) + return _Proc(1, stderr="Error code: 401 - Your API key is invalid") + + monkeypatch.setattr("subprocess.run", _fake_run) + out = _deliver({"profile": "ops", "message": "ping"}) + assert "error" in out + assert len(_transport_calls(calls)) == 1, "auth failures must not auto-retry" + # typed reason rides the structured error payload + assert out["error"]["data"]["reason"] == bfr.PROVIDER_AUTH_OR_ACCESS + + +def test_deliver_failure_carries_typed_reason(home, monkeypatch): + """A still-failing retryable error surfaces its classified reason.""" + monkeypatch.setattr( + "subprocess.run", + lambda argv, **k: _Proc(1, stderr="502 server error - overloaded") + if list(argv)[:1] == ["hermes"] + else _Proc(0), + ) + out = _deliver({"profile": "ops", "message": "ping"}) + assert "error" in out + assert out["error"]["data"]["reason"] == bfr.PROVIDER_SERVER_ERROR + + +# ── local delivery runner consumes the policy ──────────────────────────────── + + +def test_run_delivery_retries_transient_and_reemits_stdout(monkeypatch, tmp_path, capsys): + from tools import bot_mode_dm + + dm = tmp_path / "dm.txt" + dm.write_text("hello") + calls = [] + + def _fake_run(argv, **kwargs): + calls.append(list(argv)) + if len(calls) == 1: + return _Proc(1, stderr="server error - overloaded") + return _Proc(0, stdout="the reply text") + + monkeypatch.setattr(bot_mode_dm.subprocess, "run", _fake_run) + rc = bot_mode_dm._run_delivery( + ["hermes", "-p", "ops", "chat"], str(dm), stdin_file=False + ) + assert rc == 0 + assert len(calls) == 2 + assert calls[0] == calls[1] + assert "the reply text" in capsys.readouterr().out + assert not dm.exists(), "dm file must be cleaned up" + + +def test_run_delivery_no_retry_for_missing_config(monkeypatch, tmp_path): + from tools import bot_mode_dm + + dm = tmp_path / "dm.txt" + dm.write_text("hello") + calls = [] + + def _fake_run(argv, **kwargs): + calls.append(list(argv)) + return _Proc(1, stderr="No LLM provider configured") + + monkeypatch.setattr(bot_mode_dm.subprocess, "run", _fake_run) + rc = bot_mode_dm._run_delivery( + ["hermes", "-p", "ops", "chat"], str(dm), stdin_file=False + ) + assert rc == 1 + assert len(calls) == 1 diff --git a/tools/bot_failure_reasons.py b/tools/bot_failure_reasons.py index b8e57962f6..2fa5975f93 100644 --- a/tools/bot_failure_reasons.py +++ b/tools/bot_failure_reasons.py @@ -70,6 +70,40 @@ def is_auto_retryable(reason: str) -> bool: return reason in AUTO_RETRYABLE +# ── retry session policy (#93091 item 5) ───────────────────────────────────── +# +# Maintainer ruling (2026-08-23, #93091): a retried bot turn NEVER mints a +# fresh session. Transient classes resume the session as-is. context_overflow +# runs context compression — the one sanctioned context mutation, already in +# the agent core — on the same session and retries against the compacted +# context. Everything else (auth/quota/config/model/unknown) is not +# auto-retried at all: surface the typed reason and stop. + +#: Retry actions returned by :func:`retry_action`. +RETRY_RESUME = "resume" +RETRY_COMPRESS_THEN_RESUME = "compress_then_resume" +RETRY_NONE = "none" + + +def retry_action(reason: str) -> str: + """Map a failure reason to the bot-turn retry action. + + - transient (:data:`AUTO_RETRYABLE`) → ``'resume'``: retry the same + session unchanged, bounded by the caller's backoff ladder. + - :data:`CONTEXT_OVERFLOW` → ``'compress_then_resume'``: run context + compression on the session, then retry the same session. Resending + the identical overflowing context would fail identically, and a + fresh-session escape hatch is explicitly not wanted. + - anything else → ``'none'``: never auto-retry auth/quota/config + failures; a retry cannot fix them and only burns quota. + """ + if reason in AUTO_RETRYABLE: + return RETRY_RESUME + if reason == CONTEXT_OVERFLOW: + return RETRY_COMPRESS_THEN_RESUME + return RETRY_NONE + + # Ordered (pattern, code) rules — first match wins. See module docstring for # the precedence rationale (auth beats quota by design). _RULES: tuple[tuple[re.Pattern[str], str], ...] = ( diff --git a/tools/bot_mode_dm.py b/tools/bot_mode_dm.py index 43fa53c9ed..6d88082f58 100644 --- a/tools/bot_mode_dm.py +++ b/tools/bot_mode_dm.py @@ -541,6 +541,14 @@ def _run_delivery(argv: list[str], dm_file: str, *, stdin_file: bool) -> int: The turn execution window (not the enqueue) holds the target profile's cross-process lock, so two deliveries into one profile queue instead of racing; a bounded wait ends in a structured 'target_busy' refusal. + + Local (query-file) turns get one policy-gated retry (#93091 item 5): + transient failures re-run the same session; a context_overflow re-run + lets the retried turn's pre-API compaction pass compact the Bot Chat + transcript first (agent/conversation_loop.py) — the sanctioned + compression lever; no fresh session is ever minted. Auth/quota/config + failures never retry. Peer transports (stdin mode) retry on their own + gateway's deliver path, not here. """ try: with _delivery_lock(argv, stdin_file=stdin_file): @@ -549,10 +557,36 @@ def _run_delivery(argv: list[str], dm_file: str, *, stdin_file: bool) -> int: # after subprocess.run returns, not merely after stdin reaches EOF. with open(dm_file, "r", encoding="utf-8") as stream: return subprocess.run(argv, stdin=stream, check=False).returncode - return subprocess.run( + proc = subprocess.run( [*argv, "--query-file", dm_file], check=False, - ).returncode + capture_output=True, + text=True, + ) + if proc.returncode != 0: + from tools.bot_failure_reasons import ( + RETRY_NONE, + classify_agent_error, + retry_action, + ) + + detail = (proc.stderr or proc.stdout or "").strip()[-500:] + if retry_action(classify_agent_error(detail)) != RETRY_NONE: + proc = subprocess.run( + [*argv, "--query-file", dm_file], + check=False, + capture_output=True, + text=True, + ) + # Re-emit the transport's streams: stdout is the reply text the + # completion notification carries back to the sending agent. + if proc.stdout: + sys.stdout.write(proc.stdout) + sys.stdout.flush() + if proc.stderr: + sys.stderr.write(proc.stderr) + sys.stderr.flush() + return proc.returncode finally: _unlink_dm_file(dm_file) diff --git a/tui_gateway/methods_bot_relay.py b/tui_gateway/methods_bot_relay.py index a3cf7b8605..0e853f857f 100644 --- a/tui_gateway/methods_bot_relay.py +++ b/tui_gateway/methods_bot_relay.py @@ -117,8 +117,9 @@ def _(rid, params: dict) -> dict: # delivery turn into this profile (relay or local message_agent). # The lock covers only the turn execution window. Worst-case # handler hold is lock wait (bot_mode.turn_wait_seconds, default - # 120s) + the 600s turn timeout below — clients calling - # bot_relay.deliver must tolerate ~720s before assuming failure. + # 120s) + the 600s turn timeout below — doubled when the retry + # policy grants one bounded re-run — so clients calling + # bot_relay.deliver must tolerate ~1320s before assuming failure. with acquire_turn_lock(root, resolved): proc = subprocess.run( local_delivery_command(resolved, tmp), @@ -126,14 +127,43 @@ def _(rid, params: dict) -> dict: text=True, timeout=600, ) + if proc.returncode != 0: + # Retry session policy (#93091 item 5): transient classes + # re-run the SAME session once; context_overflow also + # re-runs the same session — the retried turn's pre-API + # compaction pass (agent/conversation_loop.py) compacts + # the over-threshold Bot Chat transcript first, which is + # the sanctioned compression lever (no fresh session is + # ever minted). Auth/quota/config classes never retry. + from tools.bot_failure_reasons import ( + RETRY_NONE, + classify_agent_error, + retry_action, + ) + + first_detail = (proc.stderr or proc.stdout or "").strip()[-500:] + if retry_action(classify_agent_error(first_detail)) != RETRY_NONE: + proc = subprocess.run( + local_delivery_command(resolved, tmp), + capture_output=True, + text=True, + timeout=600, + ) finally: try: os.unlink(tmp) except OSError: pass if proc.returncode != 0: + from tools.bot_failure_reasons import classify_agent_error + detail = (proc.stderr or proc.stdout or "").strip()[-500:] - return _err(rid, 5092, f"delivery turn failed: {detail or proc.returncode}") + return _err( + rid, + 5092, + f"delivery turn failed: {detail or proc.returncode}", + data={"reason": classify_agent_error(detail)}, + ) return _ok(rid, {"reply": (proc.stdout or "").strip()}) except subprocess.TimeoutExpired: return _err(rid, 5093, "delivery turn timed out") diff --git a/website/docs/user-guide/bot-mode.md b/website/docs/user-guide/bot-mode.md index 7a9e198cb5..1005b4c5b0 100644 --- a/website/docs/user-guide/bot-mode.md +++ b/website/docs/user-guide/bot-mode.md @@ -109,6 +109,10 @@ agent: Bot-to-bot delivery is per-invocation: the receiving Bot picks the message up when it next runs. Live interrupt of a Bot mid-conversation is future work. ::: +### Failed turns retry safely + +A failed delivery turn is retried at most once, and only when a retry can actually help. Transient failures (target runtime offline, delivery timeout, provider rate limit or server error) re-run the same Bot Chat session unchanged. A context-overflow failure also re-runs the same session — the retried turn compacts the over-threshold transcript via the standard context-compression pass before calling the model, so the retry fits where the original didn't. Auth, quota, and configuration failures never auto-retry: a second attempt cannot fix them and only burns quota, so the failure is surfaced immediately. A retried turn never starts a fresh session — your Bot Chat history and context stay intact. + ### Messaging across connected machines (the Desktop relay) Every gateway you register in **Settings → Connections** — local, remote URL, SSH, Hermes Cloud, docker — is a persistent line the Desktop holds open, and Bot Mode uses those lines for messaging automatically. No extra setup: