From 1dc552d5d190d94a7e72f836b366b287eacac433 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:23:39 -0700 Subject: [PATCH] =?UTF-8?q?refactor(terminal):=20honest=20schema,=20pager?= =?UTF-8?q?=20defaults=20in=20the=20env,=20unified=20notify=20arg=20(837?= =?UTF-8?q?=20=E2=86=92=20670=20tok/call,=20=E2=88=9220%)=20(#95937)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): stop claiming a Linux environment — point at the env section; near-neutral tokens * feat(terminal): default GIT_PAGER/PAGER=cat in session env; drop schema lines the runtime already enforces; fix pty backend claim * refactor(terminal): unify notify_on_complete+watch_patterns into notify (bool|list); trim pipe-masking prose (runtime hint owns it) * fix(terminal): background param referenced the unadvertised legacy arg name * refactor(terminal): background-only modifiers (pty, notify) fail loud on foreground calls with corrected shape * fix(execute_code): block the new notify arg in the sandbox terminal stub (foreground-only) --- tests/tools/test_code_execution.py | 2 +- tests/tools/test_notify_on_complete.py | 14 +++-- tests/tools/test_watch_patterns.py | 25 ++++++--- tools/code_execution_tool.py | 2 +- tools/environments/base.py | 10 ++++ tools/process_registry.py | 5 ++ tools/terminal_tool.py | 73 +++++++++++++++++++------- 7 files changed, 99 insertions(+), 32 deletions(-) diff --git a/tests/tools/test_code_execution.py b/tests/tools/test_code_execution.py index 3c03c320a6..629157a213 100644 --- a/tests/tools/test_code_execution.py +++ b/tests/tools/test_code_execution.py @@ -411,7 +411,7 @@ class TestStubSchemaDrift(unittest.TestCase): # Parameters that are internal (injected by the handler, not user-facing) _INTERNAL_PARAMS = {"task_id", "user_task"} # Parameters intentionally blocked in the sandbox - _BLOCKED_TERMINAL_PARAMS = {"background", "pty", "notify_on_complete", "watch_patterns"} + _BLOCKED_TERMINAL_PARAMS = {"background", "pty", "notify", "notify_on_complete", "watch_patterns"} def test_stubs_cover_all_schema_params(self): """Every user-facing parameter in the real schema must appear in the diff --git a/tests/tools/test_notify_on_complete.py b/tests/tools/test_notify_on_complete.py index 00da38cb11..203399b898 100644 --- a/tests/tools/test_notify_on_complete.py +++ b/tests/tools/test_notify_on_complete.py @@ -168,12 +168,18 @@ class TestCheckpointNotify: # ========================================================================= class TestTerminalSchema: - def test_schema_has_notify_on_complete(self): + def test_schema_advertises_unified_notify(self): + """`notify` is the single advertised notification arg: bool (notify on + exit) or list of strings (notify on pattern). The legacy + notify_on_complete/watch_patterns args stay handler-accepted but + unadvertised.""" from tools.terminal_tool import TERMINAL_SCHEMA props = TERMINAL_SCHEMA["parameters"]["properties"] - assert "notify_on_complete" in props - assert props["notify_on_complete"]["type"] == "boolean" - assert props["notify_on_complete"]["default"] is False + assert "notify" in props + types = {alt["type"] for alt in props["notify"]["anyOf"]} + assert types == {"boolean", "array"} + assert "notify_on_complete" not in props + assert "watch_patterns" not in props def test_handler_passes_notify(self): """_handle_terminal passes notify_on_complete to terminal_tool.""" diff --git a/tests/tools/test_watch_patterns.py b/tests/tools/test_watch_patterns.py index fbe46e352a..2802aed302 100644 --- a/tests/tools/test_watch_patterns.py +++ b/tests/tools/test_watch_patterns.py @@ -273,25 +273,38 @@ class TestCheckpointPersistence: # ========================================================================= class TestTerminalToolSchema: - def test_schema_includes_watch_patterns(self): + def test_schema_unified_notify_covers_patterns(self): + """Pattern-watching is advertised through `notify` (list form); the + legacy watch_patterns arg stays handler-accepted but unadvertised.""" from tools.terminal_tool import TERMINAL_SCHEMA props = TERMINAL_SCHEMA["parameters"]["properties"] - assert "watch_patterns" in props - assert props["watch_patterns"]["type"] == "array" - assert props["watch_patterns"]["items"] == {"type": "string"} + assert "watch_patterns" not in props + array_alts = [alt for alt in props["notify"]["anyOf"] if alt["type"] == "array"] + assert array_alts and array_alts[0]["items"] == {"type": "string"} def test_handler_passes_watch_patterns(self): - """_handle_terminal passes watch_patterns to terminal_tool.""" + """_handle_terminal passes legacy watch_patterns through to + terminal_tool (background call — foreground+watch now errors).""" from tools.terminal_tool import _handle_terminal with patch("tools.terminal_tool.terminal_tool") as mock_tt: mock_tt.return_value = json.dumps({"output": "ok", "exit_code": 0}) _handle_terminal( - {"command": "echo hi", "watch_patterns": ["ERR"]}, + {"command": "echo hi", "background": True, "watch_patterns": ["ERR"]}, task_id="t1", ) _, kwargs = mock_tt.call_args assert kwargs.get("watch_patterns") == ["ERR"] + def test_foreground_watch_patterns_rejected_with_teaching_error(self): + """Background-only modifiers on a foreground call fail loud with the + corrected call shape instead of being silently ignored.""" + from tools.terminal_tool import _handle_terminal + result = json.loads( + _handle_terminal({"command": "echo hi", "watch_patterns": ["ERR"]}, task_id="t1") + ) + assert "error" in result + assert "background=true" in result["error"] + # ========================================================================= # Code execution tool blocked params diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index 97ee9f8608..442b0fe9e1 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -646,7 +646,7 @@ def _call(tool_name, args): # --------------------------------------------------------------------------- # Terminal parameters that must not be used from ephemeral sandbox scripts -_TERMINAL_BLOCKED_PARAMS = {"background", "pty", "notify_on_complete", "watch_patterns"} +_TERMINAL_BLOCKED_PARAMS = {"background", "pty", "notify", "notify_on_complete", "watch_patterns"} def _rpc_server_loop( diff --git a/tools/environments/base.py b/tools/environments/base.py index aeee54e47c..1ab32f8a80 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -979,6 +979,16 @@ class BaseEnvironment(ABC): 'HERMES_AGENT="${HERMES_AGENT:-true}"' ) + # Non-interactive pager defaults: git log/diff/branch and similar + # pager-happy tools hang a captured (non-TTY writing to a pipe is + # fine, but PTY mode IS a TTY) or PTY-backed command waiting for `q`. + # GIT_PAGER=cat neutralizes git specifically; PAGER=cat catches the + # long tail (man, systemctl, psql, ...). ${VAR:-default} semantics: + # a user who exported their own pager in the session keeps it. + parts.append( + 'export GIT_PAGER="${GIT_PAGER:-cat}" PAGER="${PAGER:-cat}"' + ) + # Preserve bare ``~`` expansion, but rewrite ``~/...`` through # ``$HOME`` so suffixes with spaces remain a single shell word. quoted_cwd = self._quote_cwd_for_cd(cwd) diff --git a/tools/process_registry.py b/tools/process_registry.py index bea6118a98..6ca23c83aa 100644 --- a/tools/process_registry.py +++ b/tools/process_registry.py @@ -1078,6 +1078,11 @@ class ProcessRegistry: user_shell = _find_shell() pty_env = _sanitize_subprocess_env(os.environ, env_vars) pty_env["PYTHONUNBUFFERED"] = "1" + # PTY mode is a real TTY, so pager-happy tools (git log/diff, + # man) WILL page and hang waiting for `q` — default them to + # cat, honoring any pager the user already exported. + pty_env.setdefault("GIT_PAGER", "cat") + pty_env.setdefault("PAGER", "cat") pty_argv = [user_shell, "-lic", f"set +m; {safe_command}"] # Cgroup isolation for PTY mode (#70716, reviewer gap #1): diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index fe1b17039d..2b267e3503 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -1130,16 +1130,15 @@ import sys # Tool description for LLM -TERMINAL_TOOL_DESCRIPTION = """Execute shell commands on a Linux environment. Filesystem, current working directory, and exported environment variables persist between calls. +TERMINAL_TOOL_DESCRIPTION = """Execute shell commands. The host OS, shell, and terminal backend are stated in your environment section — write commands for THAT platform. Filesystem, current working directory, and exported environment variables persist between calls. -Do NOT use cat/head/tail (use read_file), grep/rg/find/ls (use search_files), sed/awk (use patch), or echo/heredoc file creation (use write_file). Reserve terminal for: builds, installs, git, processes, scripts, network, package managers, and anything that needs a shell. -NEVER pipe a build/test command through tail/head/cat to shorten output (e.g. `cargo build | tail -20`): output is auto-truncated with the full text saved to a file, and the pipe makes exit_code report the LAST pipeline command's status (tail's 0), masking real failures. Run the command bare; the same applies to `cmd || echo failed`, which also masks the exit code. +Do NOT use cat/head/tail (use read_file), grep/rg/find/ls (use search_files), sed/awk (use patch), or echo/heredoc file creation (use write_file). Reserve terminal for: builds, installs, git, processes, scripts, network, package managers — anything that needs a shell. Output is auto-truncated with the full text saved to a file — never pipe through tail/head to shorten it. Environment state persists: activate a virtualenv or export variables once per session, not before every command. Foreground (default): returns INSTANTLY when the command finishes, even with a high timeout — set timeout generously for long builds. -Background: set background=true (returns a session_id). Pair with notify_on_complete=true for bounded tasks; leave silent only for servers/daemons that never exit. Never use nohup/setsid/trailing '&' — use background=true so Hermes tracks the process. After starting a server, verify readiness with a health check, then act in a separate call; no blind sleep loops. Manage with process(action="poll"/"wait"). -Working directory: use 'workdir' for per-command cwd. When a command changes the session cwd (cd, pushd), the result includes a "cwd" field — trust it instead of prefixing every command with 'cd'. -PTY: set pty=true for interactive CLIs (they hang without it). Pipe git output to cat if it might page. +Background: set background=true (returns a session_id); add notify=true for bounded tasks, leave silent only for servers/daemons that never exit. After starting a server, verify readiness with a health check in a separate call (no blind sleep loops); manage with process(action="poll"/"wait"). +Working directory: use 'workdir' for per-command cwd; when a command changes the session cwd (cd, pushd), trust the result's "cwd" field instead of prefixing every command with 'cd'. +PTY: pty=true + background=true for interactive CLIs (they hang without a terminal); drive them with process(action="write"/"submit"). Local backend only. """ # Global state for environment lifecycle management @@ -4101,11 +4100,11 @@ TERMINAL_SCHEMA = { "properties": { "command": { "type": "string", - "description": "The command to execute on the VM" + "description": "The shell command to execute" }, "background": { "type": "boolean", - "description": "Run in the background, returning a session_id. Pair with notify_on_complete=true for anything with a defined end (tests, builds, deploys) — without it the process runs silently. Only servers/watchers/daemons that never exit should stay silent. Short commands: prefer foreground with a generous timeout.", + "description": "Run in the background, returning a session_id. Pair with notify=true for anything with a defined end (tests, builds, deploys) — without it the process runs silently. Only servers/watchers/daemons that never exit should stay silent. Short commands: prefer foreground with a generous timeout.", "default": False }, "timeout": { @@ -4119,19 +4118,19 @@ TERMINAL_SCHEMA = { }, "pty": { "type": "boolean", - "description": "Run in pseudo-terminal (PTY) mode for interactive CLI tools like Codex, Claude Code, or Python REPL. Only works with local and SSH backends. Default: false.", + "description": "With background=true: run in a pseudo-terminal for interactive CLI tools (Codex, Claude Code, Python REPL). Local backend only. Default: false.", "default": False }, - "notify_on_complete": { - "type": "boolean", - "description": "With background=true: get exactly one notification when the process exits. The right choice for nearly every bounded long task — set it and keep working. MUTUALLY EXCLUSIVE with watch_patterns (watch_patterns is dropped when both are set).", - "default": False - }, - "watch_patterns": { - "type": "array", - "items": {"type": "string"}, - "description": "Strings to watch for in background output. ONLY for rare one-shot mid-process signals on processes that never exit (e.g. ['Application startup complete'] on a server). NOT for end-of-run markers (use notify_on_complete) and NOT for per-iteration patterns like 'ERROR' in loops — rate-limited to 1 notification/15s, capped at a small number of matches over the process's lifetime; over-firing auto-disables it and falls back to notify-on-exit. When in doubt, use notify_on_complete. MUTUALLY EXCLUSIVE with notify_on_complete." + "notify": { + "description": "With background=true: notify=true fires exactly one notification when the process exits (the right choice for nearly every bounded task — builds, tests, deploys). notify=['pattern', ...] instead notifies when a line matches a pattern — ONLY for one-shot readiness signals on processes that never exit (e.g. ['Application startup complete']); rate-limited and auto-disabled if it over-fires. Omit for silent daemons.", + "anyOf": [ + {"type": "boolean"}, + {"type": "array", "items": {"type": "string"}} + ] } + # Legacy aliases (unadvertised, still accepted): notify_on_complete + # (bool) and watch_patterns (list). notify=true|[...] maps onto + # them in the dispatch wrapper; explicit notify wins on conflict. }, "required": ["command"] } @@ -4150,6 +4149,40 @@ def _handle_terminal(args, **kw): "command in 'command'. Use execute_code(code=...) for Python; " "for shell, retry as terminal(command=...)." ) + # `notify` is the advertised interface: true → notify_on_complete, + # ['pat', ...] → watch_patterns. The legacy args remain accepted + # (old transcripts, internal callers); explicit `notify` wins. + notify = args.get("notify") + notify_on_complete = args.get("notify_on_complete", False) + watch_patterns = args.get("watch_patterns") + # Background-only modifiers on a foreground call were silently ignored; + # fail with the corrected call instead (poka-yoke, no schema cost). + if not args.get("background", False): + if notify or watch_patterns or notify_on_complete: + return tool_error( + "notify only applies to background commands (foreground " + "results return directly). Either drop notify, or run as " + "terminal(command=..., background=true, notify=...)." + ) + if args.get("pty", False): + return tool_error( + "pty requires background=true (a PTY session is interacted " + "with via process(action='write'/'submit'), which needs a " + "tracked background process). Retry as terminal(command=..., " + "background=true, pty=true)." + ) + if notify is not None: + if isinstance(notify, bool): + notify_on_complete = notify + watch_patterns = None + elif isinstance(notify, list): + watch_patterns = notify + notify_on_complete = False + else: + return tool_error( + "notify must be true/false (notify on exit) or a list of " + "strings (notify on output pattern match)." + ) return terminal_tool( command=args.get("command"), background=args.get("background", False), @@ -4158,8 +4191,8 @@ def _handle_terminal(args, **kw): session_id=kw.get("session_id"), workdir=args.get("workdir"), pty=args.get("pty", False), - notify_on_complete=args.get("notify_on_complete", False), - watch_patterns=args.get("watch_patterns"), + notify_on_complete=notify_on_complete, + watch_patterns=watch_patterns, )