refactor(terminal): honest schema, pager defaults in the env, unified notify arg (837 → 670 tok/call, −20%) (#95937)
* 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)
This commit is contained in:
Teknium
@@ -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
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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):
|
||||
|
||||
+53
-20
@@ -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,
|
||||
)
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user