diff --git a/tests/tools/test_terminal_foreground_timeout_cap.py b/tests/tools/test_terminal_foreground_timeout_cap.py index 0081de88a5..b43c4941b9 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,27 @@ 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 + + +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 9ee4eca08b..736e8e2ba0 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -879,6 +879,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( @@ -942,23 +953,50 @@ 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: - 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." - )) + # 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. + # 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 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, ) +_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). 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 + 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) + + def _acquire_env(plan: _ExecPlan, task_id: Optional[str]) -> Any: """Cached env for the task, else create it under the per-task creation lock. @@ -1175,14 +1213,21 @@ 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: + # 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: - 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, @@ -1226,7 +1271,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": {