From 55d61f16bab79826d5cced66ee604c8df8037ecd Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 5 Sep 2026 01:33:27 -0700 Subject: [PATCH 1/2] fix(terminal): a foreground timeout above the cap runs as a tracked background process instead of being refused `Foreground timeout Ns exceeds the maximum of 600s` was the second most frequent tool-layer error in the 1,393-agent refactor run: 454 refusals (285 asked 900 s, 41 asked 620, 20 asked 1800), 251 of them test-suite invocations. Every retry was mechanical: lower the timeout (167), split into sleeps (82) or re-send with background=true (86). The schema invited it ("set high for long tasks", max 600) while the suites take 10-30 min. An over-cap foreground timeout is a bounded job the caller wants to wait for. terminal_tool now runs it as a tracked background process with notify_on_complete=true and says so in the result (`promoted_from_foreground`, naming the requested and cap seconds, with "do NOT re-run it"); the schema text describes the new behaviour. The `&`/nohup/setsid and long-lived-server guidance stays a refusal: those need the command itself rewritten, which the tool cannot do safely. Live (real local backend): main refuses `sleep 1; echo LIVE_OK` at timeout=900; branch returns a proc_* session with notify_on_complete and the note, and the command runs once. Tests: the promotion test runs a real command through the real registry and asserts the result shape plus that it executed exactly once in the background; `&` still refuses; schema text updated. Terminal/process suites (27 files) 378 passed. --- .../test_terminal_foreground_timeout_cap.py | 46 ++++++++++------ tools/terminal_tool.py | 52 +++++++++++++++---- 2 files changed, 73 insertions(+), 25 deletions(-) diff --git a/tests/tools/test_terminal_foreground_timeout_cap.py b/tests/tools/test_terminal_foreground_timeout_cap.py index 0081de88a5..7d437c304b 100644 --- a/tests/tools/test_terminal_foreground_timeout_cap.py +++ b/tests/tools/test_terminal_foreground_timeout_cap.py @@ -1,7 +1,8 @@ """Tests for foreground timeout cap in terminal_tool. -Ensures that foreground commands with timeout > FOREGROUND_MAX_TIMEOUT -are rejected with an error suggesting background=true. +A foreground command with timeout > FOREGROUND_MAX_TIMEOUT is promoted to a tracked background +process with notify_on_complete (never refused: in one 1,393-agent run 454 refusals were every one +re-sent lower/split/background, 251 of them test suites). """ import json from unittest.mock import patch, MagicMock @@ -30,22 +31,37 @@ def _make_env_config(**overrides): class TestForegroundTimeoutCap: """FOREGROUND_MAX_TIMEOUT rejects foreground commands that exceed it.""" - def test_foreground_timeout_rejected_above_max(self): - """When model requests timeout > FOREGROUND_MAX_TIMEOUT, return error.""" + def test_foreground_timeout_above_max_is_promoted_to_tracked_background(self, tmp_path, monkeypatch): + """Real local backend, real registry: the command runs (once), the result is a background + session with notify_on_complete and a note naming the requested and cap seconds.""" + import time from tools.terminal_tool import terminal_tool, FOREGROUND_MAX_TIMEOUT + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hh")) + marker = tmp_path / "ran" + with patch("tools.terminal_tool._get_env_config", return_value=_make_env_config(cwd=str(tmp_path))), \ + patch("tools.terminal_tool._start_cleanup_thread"), \ + patch("tools.terminal_tool._check_all_guards", return_value={"approved": True}): + result = json.loads(terminal_tool(command=f"echo x >> {marker}", timeout=9999)) + + assert result.get("error") is None + assert result["output"] == "Background process started" and result["session_id"].startswith("proc_") + assert result["notify_on_complete"] is True + assert "9999" in result["promoted_from_foreground"] + assert str(FOREGROUND_MAX_TIMEOUT) in result["promoted_from_foreground"] + deadline = time.time() + 10 + while not marker.exists() and time.time() < deadline: + time.sleep(0.05) + assert marker.read_text().count("x") == 1 # ran exactly once, in the background + + def test_shell_backgrounding_is_still_refused(self): + """`&`/nohup need the command rewritten; the tool cannot do that safely, so it still refuses.""" + from tools.terminal_tool import terminal_tool + with patch("tools.terminal_tool._get_env_config", return_value=_make_env_config()), \ patch("tools.terminal_tool._start_cleanup_thread"): - - result = json.loads(terminal_tool( - command="echo hello", - timeout=9999, # Way above max - )) - - assert "error" in result - assert "9999" in result["error"] - assert str(FOREGROUND_MAX_TIMEOUT) in result["error"] - assert "background=true" in result["error"] + result = json.loads(terminal_tool(command="sleep 5 &")) + assert "'&' backgrounding" in result["error"] def test_zero_timeout_rejected(self): """timeout=0 must be rejected, not silently coerced to the default.""" @@ -153,4 +169,4 @@ class TestForegroundMaxTimeoutConstant: from tools.terminal_tool import TERMINAL_SCHEMA, FOREGROUND_MAX_TIMEOUT timeout_desc = TERMINAL_SCHEMA["parameters"]["properties"]["timeout"]["description"] assert str(FOREGROUND_MAX_TIMEOUT) in timeout_desc - assert "background=true" in timeout_desc + assert "background process" in timeout_desc diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index c5e8419822..6815ad542a 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -873,6 +873,17 @@ class _ExecPlan: cwd: str host_cwd: Optional[str] effective_timeout: int + # Set when a foreground call asked for more than FOREGROUND_MAX_TIMEOUT and was promoted to a + # tracked background process instead of being refused (the requested seconds, for the note). + promoted_from_foreground_timeout: Optional[int] = None + + +_PROMOTED_NOTE = ( + "Requested foreground timeout {requested}s exceeds the {cap}s cap, so this command was started as a " + "tracked background process with notify_on_complete=true instead of being refused. Do NOT re-run it. " + "Its completion (exit code + output tail) arrives as a notification; poll with " + "process(action=\"poll\", session_id=...) if you need it sooner." +) def _plan_execution( @@ -936,23 +947,39 @@ def _plan_execution( # value is truthy and would fire an immediate "-Ns" timeout. if timeout is not None and timeout <= 0: raise _Rejected(tool_error(f"timeout must be a positive number of seconds (got {timeout}).")) + promoted = None if not background: + # An over-cap foreground timeout is a bounded job the caller wants to wait for (test suites, + # builds). Refusing it only bought a mechanical retry: 454 refusals in one run, every one + # re-sent lower/split/background. Promote to a tracked background process instead; the + # caller is told in the result. The `&`/nohup/server guidance below stays a refusal: those + # need the command itself rewritten, which the tool cannot do safely. if timeout and timeout > FOREGROUND_MAX_TIMEOUT: - raise _Rejected(tool_error( - f"Foreground timeout {timeout}s exceeds the maximum of " - f"{FOREGROUND_MAX_TIMEOUT}s. Use background=true with " - f"notify_on_complete=true for long-running commands." - )) - guidance = _foreground_background_guidance(command) - if guidance: - raise _Rejected(_error_json(guidance, status="error")) + promoted = timeout + else: + guidance = _foreground_background_guidance(command) + if guidance: + raise _Rejected(_error_json(guidance, status="error")) return _ExecPlan( config=config, env_type=env_type, effective_task_id=effective_task_id, image=image, cwd=cwd, host_cwd=host_cwd, effective_timeout=timeout or config["timeout"], + promoted_from_foreground_timeout=promoted, ) +def _with_promoted_note(result_json: str, requested_timeout: int) -> str: + """Attach the foreground->background promotion note to a spawn result (unchanged on error).""" + try: + data = json.loads(result_json) + except (TypeError, ValueError): + return result_json + if not isinstance(data, dict) or data.get("error"): + return result_json + data["promoted_from_foreground"] = _PROMOTED_NOTE.format(requested=requested_timeout, cap=FOREGROUND_MAX_TIMEOUT) + return json.dumps(data, ensure_ascii=False) + + def _acquire_env(plan: _ExecPlan, task_id: Optional[str]) -> Any: """Cached env for the task, else create it under the per-task creation lock. @@ -1153,14 +1180,19 @@ def terminal_tool( verdict = _run_approval_guards(command, env_type, plan.config, force=force) pty_disabled = pty and _command_requires_pipe_stdin(command) + if plan.promoted_from_foreground_timeout is not None: + background, notify_on_complete, watch_patterns = True, True, None if background: - return spawn_background_process( + result = spawn_background_process( command=command, env=env, env_type=env_type, effective_task_id=effective_task_id, task_id=task_id, session_key=session_key, workdir=workdir, cwd=cwd, effective_pty=pty and not pty_disabled, notify_on_complete=notify_on_complete, watch_patterns=watch_patterns, approval_note=verdict.note, pty_disabled_reason=_PTY_DISABLED_REASON if pty_disabled else None, ) + if plan.promoted_from_foreground_timeout is not None: + result = _with_promoted_note(result, plan.promoted_from_foreground_timeout) + return result return _run_foreground( command, env, plan, task_id=task_id, session_id=session_id, session_key=session_key, @@ -1204,7 +1236,7 @@ TERMINAL_SCHEMA = { }, "timeout": { "type": "integer", - "description": f"Max seconds to wait (default: 180, foreground max: {FOREGROUND_MAX_TIMEOUT}). Returns INSTANTLY when command finishes — set high for long tasks, you won't wait unnecessarily. Foreground timeout above {FOREGROUND_MAX_TIMEOUT}s is rejected; use background=true for longer commands.", + "description": f"Max seconds to wait (default: 180, foreground max: {FOREGROUND_MAX_TIMEOUT}). Returns INSTANTLY when command finishes — set high for long tasks, you won't wait unnecessarily. A foreground timeout above {FOREGROUND_MAX_TIMEOUT}s runs the command as a tracked background process with notify_on_complete=true instead (the result says so; do not re-run it).", "minimum": 1 }, "workdir": { From 8581b57f4671a67c640d377f02101203e8baf626 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 5 Sep 2026 06:30:33 -0700 Subject: [PATCH 2/2] fix(terminal): promotion keeps the shell-detachment refusal; the note only promises a notification the session can receive Independent review: an over-cap timeout on `cmd &` was promoted, so the tracked shell exited at once while the payload ran untracked (the exact thing the '&' guidance exists to prevent); and the note promised a notification even on finite sessions where async delivery is disabled and the spawn had already cleared notify_on_complete. The detachment guidance now runs before the promotion decision regardless of timeout, and the note reads the spawn's actual notify_on_complete: notification wording when kept, poll-only wording when the session cannot receive one. Tests (2 new): '&' and nohup with an over-cap timeout still refuse; the note matches the delivery capability. --- .../test_terminal_foreground_timeout_cap.py | 23 +++++++++++++++++ tools/terminal_tool.py | 25 ++++++++++++++----- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/tests/tools/test_terminal_foreground_timeout_cap.py b/tests/tools/test_terminal_foreground_timeout_cap.py index 7d437c304b..b43c4941b9 100644 --- a/tests/tools/test_terminal_foreground_timeout_cap.py +++ b/tests/tools/test_terminal_foreground_timeout_cap.py @@ -170,3 +170,26 @@ class TestForegroundMaxTimeoutConstant: timeout_desc = TERMINAL_SCHEMA["parameters"]["properties"]["timeout"]["description"] assert str(FOREGROUND_MAX_TIMEOUT) in timeout_desc assert "background process" in timeout_desc + + +class TestPromotionKeepsTheDetachmentGuard: + def test_over_cap_timeout_with_shell_backgrounding_is_still_refused(self): + """Independent-review witness: a promoted `cmd &` started a tracked shell that exited at once + while the payload ran untracked, defeating the guidance the refusal exists for.""" + from tools.terminal_tool import terminal_tool + + with patch("tools.terminal_tool._get_env_config", return_value=_make_env_config()), \ + patch("tools.terminal_tool._start_cleanup_thread"): + result = json.loads(terminal_tool(command="sleep 5 &", timeout=9999)) + result2 = json.loads(terminal_tool(command="nohup make test", timeout=9999)) + assert "'&' backgrounding" in result["error"] + assert "nohup" in result2["error"] + + def test_note_does_not_promise_a_notification_the_session_cannot_receive(self): + from tools.terminal_tool import _with_promoted_note + + kept = json.loads(_with_promoted_note(json.dumps({"session_id": "proc_x", "error": None, "notify_on_complete": True}), 900)) + assert "arrives as a notification" in kept["promoted_from_foreground"] + dropped = json.loads(_with_promoted_note(json.dumps({"session_id": "proc_x", "error": None, "notify_on_complete": False}), 900)) + assert "cannot receive completion notifications" in dropped["promoted_from_foreground"] + assert "poll" in dropped["promoted_from_foreground"] diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 6815ad542a..d199c255fd 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -954,12 +954,13 @@ def _plan_execution( # re-sent lower/split/background. Promote to a tracked background process instead; the # caller is told in the result. The `&`/nohup/server guidance below stays a refusal: those # need the command itself rewritten, which the tool cannot do safely. + # The detachment guidance applies whether or not the call is promoted: a promoted `cmd &` + # would start a tracked shell that exits at once while its payload runs untracked. + guidance = _foreground_background_guidance(command) + if guidance: + raise _Rejected(_error_json(guidance, status="error")) if timeout and timeout > FOREGROUND_MAX_TIMEOUT: promoted = timeout - else: - guidance = _foreground_background_guidance(command) - if guidance: - raise _Rejected(_error_json(guidance, status="error")) return _ExecPlan( config=config, env_type=env_type, effective_task_id=effective_task_id, @@ -968,15 +969,25 @@ def _plan_execution( ) +_PROMOTED_NOTE_POLL_ONLY = ( + "Requested foreground timeout {requested}s exceeds the {cap}s cap, so this command was started as a " + "tracked background process instead of being refused. Do NOT re-run it. This session cannot receive " + "completion notifications, so poll it with process(action=\"poll\", session_id=...) until it exits." +) + + def _with_promoted_note(result_json: str, requested_timeout: int) -> str: - """Attach the foreground->background promotion note to a spawn result (unchanged on error).""" + """Attach the foreground->background promotion note to a spawn result (unchanged on error). The + note only promises a notification when the spawn actually kept notify_on_complete (finite sessions + such as one-shot runners cannot route one back; the spawn already said so and cleared the flag).""" try: data = json.loads(result_json) except (TypeError, ValueError): return result_json if not isinstance(data, dict) or data.get("error"): return result_json - data["promoted_from_foreground"] = _PROMOTED_NOTE.format(requested=requested_timeout, cap=FOREGROUND_MAX_TIMEOUT) + template = _PROMOTED_NOTE if data.get("notify_on_complete") else _PROMOTED_NOTE_POLL_ONLY + data["promoted_from_foreground"] = template.format(requested=requested_timeout, cap=FOREGROUND_MAX_TIMEOUT) return json.dumps(data, ensure_ascii=False) @@ -1181,6 +1192,8 @@ def terminal_tool( pty_disabled = pty and _command_requires_pipe_stdin(command) if plan.promoted_from_foreground_timeout is not None: + # Promotion implies notify_on_complete; watch_patterns is a background-only flag the + # caller could not have meant for a foreground call, and the two are exclusive anyway. background, notify_on_complete, watch_patterns = True, True, None if background: result = spawn_background_process(