Merge pull request #103529 from NousResearch/fix/terminal-auto-background

fix(terminal): an over-cap foreground timeout runs as a tracked background process instead of being refused (454 refusals in one run)

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"]

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": {