diff --git a/.security-suppressions.json b/.security-suppressions.json index 404253b72..650dbc6a7 100644 --- a/.security-suppressions.json +++ b/.security-suppressions.json @@ -6,9 +6,19 @@ {"path": "src/gaia/hub/native_launcher.py", "rule": "S603", "justification": "native binary path is an installed hub artifact (operator-controlled); args are constructed, not shell"}, {"path": "src/gaia/mcp/mcp_bridge.py", "rule": "B104", "justification": "binds 0.0.0.0 only on explicit caller opt-in (documented flag), never by default"}, { - "path": "src/gaia/agents/tools/shell_tools.py", - "rule": "B602", - "justification": "Sandboxed shell tool. Every command (and each pipeline segment) is validated against a whitelist via _validate_command before execution; shell=True is enabled ONLY on Windows so cmd.exe can resolve built-ins (dir/cd/type) and pipes that Git-for-Windows tools rely on. Converting to args-list would break piped/whitelisted commands the tool exists to run." + "path": "src/gaia/agents/tools/shell_session.py", + "rule": "B404", + "justification": "Running shell commands is this module's purpose. The whitelist and path policy that decide what may run stay in shell_tools.py, and apply per pipeline segment before anything reaches here." + }, + { + "path": "src/gaia/agents/tools/shell_session.py", + "rule": "B603", + "justification": "argv is always a constructed list, never shell-interpreted by Python. It names either the generated script this module wrote (which sources a command validated per segment by shell_tools.py) or a skill-granted binary, which is run as argv precisely so its arguments cannot act." + }, + { + "path": "src/gaia/agents/tools/shell_session.py", + "rule": "B607", + "justification": "taskkill is a Windows system binary invoked by name with a pid from this module's own child. The alternative is orphaning the process tree a timed-out command started." } ] } diff --git a/docs/sdk/mixins/tool-mixins.mdx b/docs/sdk/mixins/tool-mixins.mdx index 8db710821..564805075 100644 --- a/docs/sdk/mixins/tool-mixins.mdx +++ b/docs/sdk/mixins/tool-mixins.mdx @@ -122,8 +122,15 @@ class MyAgent(Agent, ShellToolsMixin): self.register_shell_tools() # Registers: # - run_shell_command(command, working_directory=None, timeout=30) + # - get_shell_state() + # - set_shell_variable(name, value) + # - reset_shell_session() ``` +Commands share one shell session per agent, so a `cd` or a variable set in one +call is still in effect for the next. `get_shell_state` reads that state back, +and `reset_shell_session` returns it to where it started. + --- ## 9.4 File Search Mixin diff --git a/docs/spec/shell-tools-mixin.mdx b/docs/spec/shell-tools-mixin.mdx index 8726fa7ea..11a596fc9 100644 --- a/docs/spec/shell-tools-mixin.mdx +++ b/docs/spec/shell-tools-mixin.mdx @@ -19,6 +19,7 @@ ShellToolsMixin provides secure shell command execution with comprehensive rate **Key Features:** - Whitelist-based command security (only safe, read-only commands) +- One persistent shell session per agent — cwd and environment survive between calls - Dual-tier rate limiting (burst + sustained) - Path traversal prevention with argument validation - Git read-only operations support @@ -41,21 +42,28 @@ ShellToolsMixin provides secure shell command execution with comprehensive rate - Blacklist dangerous commands (rm, chmod, sudo, etc.) - Git read-only subcommands only - Working directory support - - Timeout protection (default: 30s) + - Timeout protection (default: 30s), killing the command's whole process tree -2. **Rate Limiting** +2. **Persistent Session State** + - One session per agent; `cd` and exported variables carry to the next call + - `get_shell_state` reports the directory and the changed environment + - `set_shell_variable` sets a variable for the rest of the session + - `reset_shell_session` returns the session to its starting state + - Serialised access; deterministic teardown with no surviving child + +3. **Rate Limiting** - Max 10 commands per minute (sustained) - Max 3 commands per 10 seconds (burst) - Clear wait time messaging - Timestamp tracking with deque -3. **Path Security** +4. **Path Security** - Validate working directory access - Check arguments for path traversal (.., separators) - Resolve relative paths to absolute - PathValidator integration -4. **Output Management** +5. **Output Management** - Capture stdout and stderr separately - Truncate large outputs (max 10,000 chars) - Return execution duration @@ -106,6 +114,9 @@ class ShellToolsMixin: Tools provided: - run_shell_command: Execute safe shell commands with security checks + - get_shell_state: Report the session's directory and changed environment + - set_shell_variable: Set a variable for the rest of the session + - reset_shell_session: Return the session to its starting state """ def __init__(self, *args, **kwargs): @@ -255,6 +266,42 @@ if cmd_base == "git": } ``` +### Persistent Session + +Every command runs in the agent's one `ShellSession` +([`shell_session.py`](https://github.com/amd/gaia/blob/main/src/gaia/agents/tools/shell_session.py)), +a port of the C++ toolbelt's `gaia::ShellSession`. What persists is the session +*state*, not a resident child process: each command runs a generated script that +restores the session's directory and variables, sources the command from its own +file, then reports the resulting `pwd` and environment to a side file the session +absorbs. Nothing outlives the call, so there is no child to orphan. + +- **Serialised.** One lock per session, taken for the whole command. A call that + cannot take it within its own timeout returns a "session busy" error rather + than blocking — a command that outran its tool timeout is still inside the + subprocess, and waiting on it forever would wedge the agent. +- **Timeouts kill the tree.** POSIX kills the process group; Windows uses + `taskkill /T`. A timed-out build does not leave its compilers running. +- **`cd` obeys the path policy.** A directory the agent may not access is not + absorbed, and the result carries a warning saying so. Otherwise persistence + would be a way to reach a forbidden directory and then read files by bare + name, which the per-argument check never sees. +- **Granted binaries still run as argv.** A skill-granted CLI is never handed to + a shell as a string; it gets the session's directory and environment, but its + arguments cannot act. +- **`working_directory` is unchanged.** An explicit per-call directory scopes + that call only; the session stays where it was. + +On Windows commands run as a `cmd.exe` batch script, so `%VAR%` expands as it +would at a prompt and a `for` variable is written `%%i`. Point `GAIA_SHELL` at a +POSIX shell (Git Bash, MSYS, WSL) to run shell scripts instead. + +Shell state that is not a directory or a variable — aliases, functions, shell +options — is not carried. The read-only whitelist has no `export`, so +`set_shell_variable` is how a variable gets set; it refuses `PATH`, +`PYTHONPATH`, `LD_*` and the rest of the names that decide which binary or which +code a later command runs. + ### Path Traversal Prevention ```python diff --git a/hub/agents/chat/python/gaia_agent_chat/agent.py b/hub/agents/chat/python/gaia_agent_chat/agent.py index 214851666..f8285699a 100644 --- a/hub/agents/chat/python/gaia_agent_chat/agent.py +++ b/hub/agents/chat/python/gaia_agent_chat/agent.py @@ -2346,9 +2346,9 @@ def save_current_session(self) -> bool: def __del__(self): """Cleanup when agent is destroyed. - Releases watchdog observers, HTTP session, and the two SQLite - connections owned by this agent. ``__del__`` is best-effort — Python - doesn't guarantee it fires on interpreter shutdown — but explicit + Releases watchdog observers, HTTP session, the shell session, and the + two SQLite connections owned by this agent. ``__del__`` is best-effort — + Python doesn't guarantee it fires on interpreter shutdown — but explicit close() makes tests deterministic (WAL journals released, file handles closed) and avoids leaking Session/connection objects in long-running services like the Agent UI backend. @@ -2372,3 +2372,7 @@ def __del__(self): self._scratchpad.close_db() except Exception as e: logger.error(f"Error closing scratchpad during cleanup: {e}") + try: + self.close_shell_session() + except Exception as e: + logger.error(f"Error closing the shell session during cleanup: {e}") diff --git a/hub/agents/chat/python/gaia_agent_chat/tool_bundles.py b/hub/agents/chat/python/gaia_agent_chat/tool_bundles.py index 00b396dda..80d7b698c 100644 --- a/hub/agents/chat/python/gaia_agent_chat/tool_bundles.py +++ b/hub/agents/chat/python/gaia_agent_chat/tool_bundles.py @@ -124,8 +124,16 @@ ), ToolBundle( name="shell", - members=frozenset({"run_shell_command", "get_system_info"}), - description="Run shell commands and query the system.", + members=frozenset( + { + "run_shell_command", + "get_shell_state", + "set_shell_variable", + "reset_shell_session", + "get_system_info", + } + ), + description="Run shell commands in a persistent session and query the system.", ), ToolBundle( name="clipboard", @@ -351,6 +359,9 @@ members=frozenset( { "run_shell_command", + "get_shell_state", + "set_shell_variable", + "reset_shell_session", "execute_python_file", "get_system_info", } diff --git a/src/gaia/agents/tools/shell_session.py b/src/gaia/agents/tools/shell_session.py new file mode 100644 index 000000000..0630b68f3 --- /dev/null +++ b/src/gaia/agents/tools/shell_session.py @@ -0,0 +1,677 @@ +# Copyright(C) 2025-2026 Advanced Micro Devices, Inc. All rights reserved. +# SPDX-License-Identifier: MIT +"""A shell whose working directory and environment survive between commands. + +One-shot execution is materially worse than a human terminal for the +build/test loop an agent spends most of its time in: ``cd build`` in one call +is invisible to the next, and so is ``export CC=clang`` or a virtualenv +activation. No system prompt can fix that — the state simply is not there to +observe. A :class:`ShellSession` keeps it. + +This is a port of the C++ toolbelt's ``gaia::ShellSession`` +(``cpp/include/gaia/process.h``, #2810). What persists is the *session state*, +not a resident child process: each call runs a generated script that restores +the session's cwd and variables, sources the command from its own file, then +writes the resulting ``pwd`` and environment to a side file that the session +absorbs. A resident shell driven over pipes would have to survive a timeout to +be worth anything, and a timed-out command that is still inside a shared shell +is exactly the corruption this is meant to avoid — so there is no long-lived +child to orphan, on any platform. + +The security model is unchanged. This class only preserves state; it does not +decide what may run. Command validation stays with the caller and applies per +command exactly as before. + +Two further properties of the generated script are worth knowing: + +- stdin is ``/dev/null``. An interactive command (``git commit`` opening an + editor, ``sudo``, ``npm login``) returns immediately instead of sitting until + the timeout, because a tool call has no way to answer it. +- On timeout the whole process *group* is killed, not just the shell — a build + or test command spawns children, and leaving them running is what makes a + timed-out build worse than useless. + +Windows without a POSIX shell: commands run as a ``cmd.exe`` batch script, +because keeping ``cd`` and ``set`` requires running in the same interpreter and +cmd only offers that to a script. ``%VAR%`` expansion is identical to a prompt, +but a ``for`` loop variable is written ``%%i`` rather than ``%i``. Point +``GAIA_SHELL`` at a POSIX shell (Git Bash, MSYS, WSL) and none of this applies. +""" + +import locale +import logging +import os +import re +import shutil +import signal +import subprocess # nosec B404 - running shell commands is this module's purpose +import tempfile +import threading +import time +from dataclasses import dataclass, field +from pathlib import Path +from typing import Callable, Dict, List, Optional + +logger = logging.getLogger(__name__) + +#: Separates the reported cwd from the environment dump in the state file. +_ENV_MARKER = "---GAIA-ENV---" + +#: Variables every shell rewrites on its own. Replaying them would make the +#: session drift a little further from the parent on every command. PATH is +#: deliberately absent: it is what virtualenv activation changes, so a session +#: that dropped it would activate a venv and then not use it. +_VOLATILE_ENV_NAMES = frozenset( + { + "_", + "PWD", + "OLDPWD", + "SHLVL", + "PS1", + "PS2", + "RANDOM", + "SECONDS", + "LINENO", + "PROMPT", + "CD", + "ERRORLEVEL", + "CMDCMDLINE", + "CMDEXTVERSION", + "__GAIA_RC", + } +) + +_VALID_ENV_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +def _env_key(name: str) -> str: + """The name to compare environment variables under. + + Windows variable names are case-insensitive and ``os.environ`` upper-cases + them, while ``set`` reports the original casing. Comparing raw would make + every inherited variable look changed *and* look removed, so the session + would replay the whole environment and unset it at the same time. + """ + return name.upper() if os.name == "nt" else name + + +#: How long to wait for a killed process tree to release its pipes. +_DRAIN_TIMEOUT_SECONDS = 5.0 + +#: How long to give the platform's tree-killer before giving up on it. +_TREE_KILL_TIMEOUT_SECONDS = 15.0 + + +class ShellSessionError(RuntimeError): + """Base for the errors a session raises instead of degrading quietly.""" + + +class ShellSessionBusy(ShellSessionError): + """Another command still holds the session. + + A tool call that exceeds its timeout leaves its worker thread running + (#2600), so a previous command can still be inside the subprocess when the + next one arrives. Blocking forever would wedge the agent; this says so. + """ + + +class ShellSessionClosed(ShellSessionError): + """The session was torn down and will not run anything else.""" + + +@dataclass +class ShellResult: + """What one command in a session produced.""" + + stdout: str = "" + stderr: str = "" + return_code: int = 0 + timed_out: bool = False + duration_seconds: float = 0.0 + cwd: str = "" + #: Set when the command's ``cd`` was refused by the session's cwd guard. + cwd_change_rejected: Optional[str] = None + + +@dataclass +class _State: + """The cwd and environment divergence the session carries forward.""" + + cwd: str + overrides: Dict[str, str] = field(default_factory=dict) + unset: set = field(default_factory=set) + + +def _is_valid_env_name(name: str) -> bool: + return bool(_VALID_ENV_NAME.match(name)) + + +def _posix_quote(value: str) -> str: + """Wrap *value* in single quotes for POSIX sh.""" + return "'" + value.replace("'", "'\\''") + "'" + + +def _batch_quote(value: str) -> str: + """Escape *value* for use inside a batch file. + + Only ``%`` needs escaping. ``"`` must be left alone: ``set "K=V"`` takes + everything up to the *last* quote on the line, so doubling a quote is not + undone — and because the session re-captures and re-emits the value, every + command would double it again until the line broke. + """ + return value.replace("%", "%%") + + +def _generic_path(path: str) -> str: + """Forward-slash form, which every POSIX shell accepts — Git Bash included.""" + return Path(path).as_posix() + + +def _split_env_record(record: str, out: Dict[str, str]) -> None: + """Split one ``NAME=VALUE`` record. + + Names are not validated here: ``ProgramFiles(x86)`` is a real Windows + variable, and mis-parsing it would corrupt the neighbour it sorts next to. + """ + name, sep, value = record.partition("=") + if not sep or not name: + return + out[name] = value + + +def _parse_env_records_nul(text: str) -> Dict[str, str]: + """Parse the NUL-delimited environment dump the POSIX script emits. + + NUL is the one byte an environment value cannot contain, so the record + boundary is unambiguous. Line-delimited ``env`` output is not: a value + containing a newline followed by ``SOMETHING=x`` is indistinguishable from + a second variable, which would let a command's *data* become the session's + *configuration*. + """ + out: Dict[str, str] = {} + for record in text.split("0円"): + if record: + _split_env_record(record, out) + return out + + +def _parse_env_lines(text: str) -> Dict[str, str]: + """Parse ``cmd.exe`` ``set`` output. + + Windows environment values cannot contain a newline, so one line is exactly + one variable and an unparseable line is dropped rather than glued onto its + predecessor. + """ + out: Dict[str, str] = {} + for raw in text.splitlines(): + line = raw.rstrip("\r") + if not line or line.startswith("="): + # cmd.exe lists internal "=C:" / "=ExitCode" entries; not variables. + continue + _split_env_record(line, out) + return out + + +def _terminate_tree(proc: "subprocess.Popen") -> None: + """Kill *proc* and everything it spawned. + + Killing only the shell leaves the build it started running, which is what + makes a timed-out command worse than useless. + """ + if proc.poll() is not None: + return + if os.name == "nt": + # No process groups: taskkill /T walks the parent-child tree instead. + # It takes seconds on a busy box, which is the price of not orphaning + # the build a timed-out command started. + try: + subprocess.run( # nosec B603 B607 - fixed argv, pid from our own child + ["taskkill", "/F", "/T", "/PID", str(proc.pid)], + capture_output=True, + check=False, + timeout=_TREE_KILL_TIMEOUT_SECONDS, + ) + except subprocess.TimeoutExpired: + logger.warning("taskkill did not finish for pid %s", proc.pid) + proc.kill() + else: + try: + # pylint: disable=no-member # POSIX-only; the branch never runs on nt + os.killpg(os.getpgid(proc.pid), signal.SIGKILL) # type: ignore[attr-defined] + except (ProcessLookupError, PermissionError) as exc: + logger.warning("Could not kill process group for pid %s: %s", proc.pid, exc) + proc.kill() + + +class ShellSession: + """A shell session whose cwd and exported environment persist across calls. + + Calls on the same session are serialised by an internal lock, because they + share one logical shell state. Distinct sessions never collide: the cwd and + the variables are applied *inside the child*, so the calling process is + never mutated. + """ + + def __init__( + self, + start_cwd: Optional[str] = None, + shell: Optional[str] = None, + cwd_guard: Optional[Callable[[str], bool]] = None, + ): + """ + Args: + start_cwd: Initial working directory. Defaults to the process cwd. + shell: Shell to run commands with. ``None`` means ``/bin/sh`` on + POSIX and ``cmd.exe`` on Windows; ``GAIA_SHELL`` overrides both. + Naming a POSIX shell on Windows makes the session generate a + shell script for it instead of a batch file. + cwd_guard: Consulted before absorbing a directory a command changed + into. Returning False keeps the session where it was — without + it, ``cd`` would be a way to reach paths the caller's path + policy refuses. + """ + resolved = Path(start_cwd).resolve() if start_cwd else Path.cwd() + self._state = _State(cwd=str(resolved)) + self._start_cwd = str(resolved) + self._baseline_env = {_env_key(k): v for k, v in os.environ.items()} + self._cwd_guard = cwd_guard + self._lock = threading.Lock() + self._closed = False + self._temp_dir: Optional[str] = None + + configured = shell if shell is not None else os.environ.get("GAIA_SHELL", "") + configured = configured.strip() + if os.name == "nt": + self._shell = configured + self._posix_script = bool(configured) + else: + self._shell = configured or "/bin/sh" + self._posix_script = True + + # -- state ------------------------------------------------------------ + + @property + def cwd(self) -> str: + """Current working directory of the session.""" + return self._state.cwd + + @property + def posix_script(self) -> bool: + """True when commands run as a POSIX shell script rather than a batch file.""" + return self._posix_script + + def environment(self) -> Dict[str, str]: + """Variables the session has diverged from the parent environment.""" + return dict(self._state.overrides) + + def removed_environment(self) -> List[str]: + """Inherited variables the session's commands have unset.""" + return sorted(self._state.unset) + + def effective_env(self) -> Dict[str, str]: + """The environment a command would see, for callers that bypass the script.""" + env = os.environ.copy() + for name in self._state.unset: + env.pop(name, None) + env.update(self._state.overrides) + return env + + def set_cwd(self, directory: str) -> bool: + """Set the working directory. False (and unchanged) if it is not a directory.""" + resolved = Path(directory).resolve() + if not resolved.is_dir(): + return False + self._state.cwd = str(resolved) + return True + + def set_env(self, name: str, value: str) -> None: + """Set a variable for subsequent commands in this session.""" + self._state.overrides[name] = value + self._state.unset.discard(name) + + def reset(self) -> None: + """Forget every environment change and return to the starting directory.""" + self._state = _State(cwd=self._start_cwd) + + def close(self) -> None: + """Tear the session down. Later calls raise :class:`ShellSessionClosed`. + + There is no resident child to kill — every command is waited on or + killed (with its process group) inside :meth:`run`. What is left is the + session's temp directory, which this removes. + + A command still running past its tool timeout (#2600) is holding files + in that directory, so the removal is handed to it rather than pulled out + from under it. + """ + self._closed = True + if self._lock.acquire(blocking=False): + try: + self._discard_temp_dir() + finally: + self._lock.release() + else: + logger.info( + "Shell session closed while a command was still running; its temp " + "directory is removed when that command finishes." + ) + + def _discard_temp_dir(self) -> None: + temp_dir, self._temp_dir = self._temp_dir, None + if temp_dir: + shutil.rmtree(temp_dir, ignore_errors=True) + + @property + def closed(self) -> bool: + return self._closed + + # -- execution -------------------------------------------------------- + + def run( + self, + command: str, + timeout: float = 30.0, + working_directory: Optional[str] = None, + ) -> ShellResult: + """Run *command* in the session, then absorb the state it left behind. + + Args: + command: The shell command to execute. + timeout: Seconds before the command's process group is killed. + working_directory: Run this one command elsewhere. A one-shot + override: the session's own directory is untouched, and a + ``cd`` inside such a command is not absorbed. + + Raises: + ShellSessionClosed: the session was torn down. + ShellSessionBusy: another command still holds the session. + """ + if self._closed: + raise ShellSessionClosed( + "This shell session was closed. Start a new task, or reset the " + "session, to run commands again." + ) + if not command.strip(): + raise ValueError("Empty command") + + # Bounded, so a command still running past its tool timeout (#2600) + # surfaces as an actionable error instead of wedging the agent. + if not self._lock.acquire(timeout=max(1.0, float(timeout))): + raise ShellSessionBusy( + "Another command is still running in this shell session. Wait for " + "it to finish, or reset the session to abandon it and start clean." + ) + try: + return self._run_locked(command, timeout, working_directory) + finally: + self._lock.release() + + def run_argv( + self, + argv: List[str], + timeout: float = 30.0, + working_directory: Optional[str] = None, + ) -> ShellResult: + """Run *argv* directly, with the session's cwd and environment applied. + + For callers that must not hand a command string to a shell — a binary + invoked with arguments built from untrusted text. The session's state is + applied, but nothing is absorbed: an argv call cannot ``cd`` or + ``export`` in the first place. + """ + if self._closed: + raise ShellSessionClosed( + "This shell session was closed. Start a new task, or reset the " + "session, to run commands again." + ) + if not self._lock.acquire(timeout=max(1.0, float(timeout))): + raise ShellSessionBusy( + "Another command is still running in this shell session. Wait for " + "it to finish, or reset the session to abandon it and start clean." + ) + try: + cwd = working_directory or self._state.cwd + result = self._spawn(argv, cwd, self.effective_env(), timeout) + result.cwd = self._state.cwd + return result + finally: + self._lock.release() + + # -- internals -------------------------------------------------------- + + def _run_locked( + self, + command: str, + timeout: float, + working_directory: Optional[str], + ) -> ShellResult: + temp_dir = self._ensure_temp_dir() + script_ext = ".sh" if self._posix_script else ".cmd" + start_cwd = working_directory or self._state.cwd + + state_file = self._write_temp(temp_dir, ".state", "") + command_file = self._write_temp(temp_dir, script_ext, command + "\n") + script_file = self._write_temp( + temp_dir, + script_ext, + self._build_script(start_cwd, command_file, state_file), + ) + + try: + if self._posix_script: + argv = [self._shell, _generic_path(script_file)] + else: + # /d skips AutoRun, which would otherwise prepend a user's + # profile commands to every agent command. + argv = [os.environ.get("COMSPEC", "cmd.exe"), "/d", "/c", script_file] + + # No cwd/env arguments: the script applies both inside the child, so + # the calling process is never mutated and sessions cannot collide. + result = self._spawn(argv, None, None, timeout) + rejected = self._absorb_state( + self._read_state(state_file), absorb_cwd=working_directory is None + ) + result.cwd = start_cwd if working_directory else self._state.cwd + result.cwd_change_rejected = rejected + return result + finally: + for path in (state_file, command_file, script_file): + try: + os.remove(path) + except OSError as exc: + logger.warning("Could not remove temp file %s: %s", path, exc) + if self._closed: + # Closed while this command was running; it is ours to clean up. + self._discard_temp_dir() + + def _ensure_temp_dir(self) -> str: + if self._temp_dir is None or not os.path.isdir(self._temp_dir): + self._temp_dir = tempfile.mkdtemp(prefix="gaia_shell_") + return self._temp_dir + + @staticmethod + def _write_temp(temp_dir: str, extension: str, contents: str) -> str: + """Create a uniquely-named file in *temp_dir* and write *contents* to it. + + Exclusive creation, so a pre-planted symlink cannot be written through. + """ + fd, path = tempfile.mkstemp(suffix=extension, dir=temp_dir) + with os.fdopen(fd, "w", encoding="utf-8", newline="") as handle: + handle.write(contents) + return path + + def _build_script(self, cwd: str, command_file: str, state_file: str) -> str: + """The per-command script: restore state, run the command, report state. + + The command lives in its own file and is sourced (``.`` / ``call``) + rather than pasted in here. Pasting lets a command ending in a line + continuation, or an unterminated heredoc, swallow the framework's own + bookkeeping lines — which corrupts the reported exit code and leaks + internals into what the model reads back. + """ + state = self._state + lines: List[str] = [] + if self._posix_script: + lines.append(f"cd {_posix_quote(_generic_path(cwd))} || exit 127") + for name in sorted(state.unset): + if _is_valid_env_name(name): + lines.append(f"unset {name}") + for name, value in sorted(state.overrides.items()): + if _is_valid_env_name(name): + lines.append(f"{name}={_posix_quote(value)}; export {name}") + lines.append(f". {_posix_quote(_generic_path(command_file))} < /dev/null") + lines.append("__gaia_rc=$?") + # awk's ENVIRON gives NUL-delimited records; `env` output cannot be + # parsed unambiguously (see _parse_env_records_nul). + lines.append( + "{ pwd; printf '%s\\n' " + + _posix_quote(_ENV_MARKER) + + '; awk \'BEGIN { for (k in ENVIRON) printf "%s=%s%c", k, ' + "ENVIRON[k], 0 }'; }> " + + _posix_quote(_generic_path(state_file)) + + " 2>/dev/null" + ) + lines.append("exit $__gaia_rc") + return "\n".join(lines) + "\n" + + lines.append("@echo off") + lines.append(f'cd /d "{_batch_quote(cwd)}"') + lines.append("if errorlevel 1 exit /b 127") + for name in sorted(state.unset): + if _is_valid_env_name(name): + lines.append(f'set "{name}="') + for name, value in sorted(state.overrides.items()): + if _is_valid_env_name(name): + lines.append(f'set "{name}={_batch_quote(value)}"') + lines.append(f'call "{command_file}" "{state_file}" (') + lines.append(" cd") + lines.append(f" echo {_ENV_MARKER}") + lines.append(" set") + lines.append(")") + lines.append("exit /b %__GAIA_RC%") + return "\r\n".join(lines) + "\r\n" + + def _read_state(self, state_file: str) -> str: + try: + raw = Path(state_file).read_bytes() + except OSError: + return "" + encoding = "utf-8" if self._posix_script else locale.getpreferredencoding(False) + return raw.decode(encoding, errors="replace") + + def _absorb_state(self, state_text: str, absorb_cwd: bool) -> Optional[str]: + """Take on the cwd and environment the command left behind. + + A command that calls ``exit`` terminates the script before the + bookkeeping runs, so its changes are not captured; the session keeps its + previous state rather than guessing. + + Returns a message when a directory change was refused by the cwd guard. + """ + marker = state_text.find(_ENV_MARKER) + if marker == -1: + return None + + rejected: Optional[str] = None + reported_cwd = state_text[:marker].strip() + if reported_cwd and absorb_cwd: + # The shell is the authority on where it ended up. With a Git Bash + # shell on Windows that is an MSYS path (`/c/...`) the Win32 API + # does not recognise but the next script — run by the same shell — + # does, so it is taken as reported rather than validated away. + if self._cwd_guard is not None and not self._cwd_guard(reported_cwd): + rejected = ( + f"Directory change to '{reported_cwd}' was not applied: that " + "path is outside the directories this agent may access. The " + f"session stayed in '{self._state.cwd}'." + ) + logger.info("Refused session cwd change to %r", reported_cwd) + else: + self._state.cwd = reported_cwd + + env_text = state_text[marker + len(_ENV_MARKER) :].lstrip("\r\n") + captured = ( + _parse_env_records_nul(env_text) + if self._posix_script + else _parse_env_lines(env_text) + ) + if not captured: + return rejected + + overrides: Dict[str, str] = {} + unset = set() + # Only replayable names participate: `ProgramFiles(x86)` is a real + # Windows variable but no `set NAME=` / `export NAME` can name it. + captured_keys = { + _env_key(name) for name in captured if _is_valid_env_name(name) + } + for name, value in captured.items(): + key = _env_key(name) + if key in _VOLATILE_ENV_NAMES or not _is_valid_env_name(name): + continue + if self._baseline_env.get(key) != value: + overrides[name] = value + for key in self._baseline_env: + if key in _VOLATILE_ENV_NAMES or not _is_valid_env_name(key): + continue + if key not in captured_keys: + unset.add(key) + self._state.overrides = overrides + self._state.unset = unset + return rejected + + @staticmethod + def _spawn( + argv: List[str], + cwd: Optional[str], + env: Optional[Dict[str, str]], + timeout: float, + ) -> ShellResult: + """Run *argv*, capping the wait and killing the whole tree on expiry. + + encoding/errors are explicit, and load-bearing. Bare ``text=True`` + decodes with the locale codec — cp1252 on a default Windows box — inside + subprocess's pipe reader THREAD. A byte that codec cannot map raises + there, the thread dies, and the caller gets returncode 0 with EMPTY + stdout: the command succeeded and its output was silently discarded. + """ + start = time.monotonic() + with subprocess.Popen( # nosec B603 - argv is a list, never shell-parsed + argv, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + # stdin is DEVNULL, never inherited. This process's stdin is the + # agent transport's pipe — held open and never written to — so a + # child that reads it blocks forever on input that cannot arrive. + stdin=subprocess.DEVNULL, + cwd=cwd, + env=env, + encoding="utf-8", + errors="replace", + # Its own process group on POSIX, so a timeout can kill the + # command's children too rather than just the shell that started + # them. Windows has no equivalent; _terminate_tree walks the + # parent-child tree with taskkill instead. + start_new_session=os.name != "nt", + ) as proc: + try: + stdout, stderr = proc.communicate(timeout=timeout) + timed_out = False + except subprocess.TimeoutExpired: + _terminate_tree(proc) + timed_out = True + try: + stdout, stderr = proc.communicate(timeout=_DRAIN_TIMEOUT_SECONDS) + except subprocess.TimeoutExpired: + stdout, stderr = "", "" + return_code = proc.returncode if proc.returncode is not None else -1 + + return ShellResult( + stdout=stdout or "", + stderr=stderr or "", + return_code=return_code, + timed_out=timed_out, + duration_seconds=time.monotonic() - start, + ) diff --git a/src/gaia/agents/tools/shell_tools.py b/src/gaia/agents/tools/shell_tools.py index d4fabe917..4fd4370a0 100644 --- a/src/gaia/agents/tools/shell_tools.py +++ b/src/gaia/agents/tools/shell_tools.py @@ -10,11 +10,17 @@ import os import re import shlex -import subprocess +import shutil import time from collections import deque from pathlib import Path -from typing import Any, Dict, Optional +from typing import Any, Callable, Dict, Optional + +from gaia.agents.tools.shell_session import ( + ShellSession, + ShellSessionBusy, + ShellSessionClosed, +) logger = logging.getLogger(__name__) @@ -198,6 +204,33 @@ #: only one a ``shell:execute`` grant may exempt from confirmation. _POLICY_GATED_SHELL_TOOL = "run_shell_command" +#: Variables that decide which binary or which code a later command actually +#: runs. Setting one turns the read-only whitelist into a name the agent +#: controls: `PATH` picked the `ls` that ran, `PYTHONPATH` picked the module a +#: script imported. The whitelist checks names, not what they resolve to, so +#: these stay with the parent process. +_UNSETTABLE_ENV_PREFIXES = ("LD_", "DYLD_") +_UNSETTABLE_ENV_NAMES = frozenset( + { + "PATH", + "PATHEXT", + "COMSPEC", + "SHELL", + "IFS", + "ENV", + "BASH_ENV", + "PYTHONPATH", + "PYTHONHOME", + "PYTHONSTARTUP", + "NODE_OPTIONS", + "PERL5LIB", + "RUBYOPT", + "GAIA_SHELL", + } +) + +_VALID_SHELL_VAR_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + def skill_granted_binaries(host: Any) -> frozenset: """CLIs *host*'s loaded skills granted via ``shell:execute:``. @@ -246,6 +279,21 @@ def _operator_check_text(command: str) -> str: return " ".join(outer) +def _requote_for_posix(segments: list) -> str: + """Rebuild the command from its validated tokens, quoted for POSIX sh. + + A session needs a shell — ``cd`` and ``export`` exist nowhere else — but + handing sh the raw string would newly let it expand what the checks above + read literally: ``~``, ``$VAR``, ``*``. ``cat ~/../secret`` is one token to + the path validator and a different file to the shell. Re-quoting the tokens + keeps the pipeline and gives the shell back exactly the argv it had before + there was a session. + """ + return " | ".join( + " ".join(shlex.quote(token) for token in segment) for segment in segments + ) + + def _split_pipeline(cmd_parts: list) -> list: """Split a shlex-split command on ``|`` into its non-empty segments.""" segments: list = [] @@ -268,6 +316,12 @@ class ShellToolsMixin: Tools provided: - run_shell_command: Execute terminal commands with timeout and safety checks + - get_shell_state: Read the session's directory and changed environment + - reset_shell_session: Return the session to the state it started in + + Commands share one :class:`ShellSession` per agent, so a directory change or + an exported variable is still there on the next call. Access is serialised; + validation is unchanged and still applies per command. Rate Limiting: - Max 10 commands per minute to prevent DOS @@ -283,6 +337,56 @@ def __init__(self, *args, **kwargs): self.max_commands_per_minute = 10 self.max_commands_per_10_seconds = 3 + # Created on first use: an agent that never runs a command should not + # pay for a temp directory. + self._shell_session: Optional[ShellSession] = None + + def _session_cwd_guard(self) -> Optional[Callable[[str], bool]]: + """The predicate the session asks before following a ``cd``. + + Without it, persistence would be a way around the path policy: ``cd`` to + a forbidden directory, then read a file by bare name, and the per-argument + check never sees a path to reject. + """ + validator = getattr(self, "path_validator", None) + if validator is not None: + guard: Callable[[str], bool] = validator.is_path_allowed + return guard + checker: Optional[Callable[[str], bool]] = getattr( + self, "_is_path_allowed", None + ) + return checker + + @property + def shell_session(self) -> ShellSession: + """This agent's shell session, created on first use.""" + session = getattr(self, "_shell_session", None) + if session is None or session.closed: + session = ShellSession(cwd_guard=self._session_cwd_guard()) + self._shell_session = session + return session + + def reset_shell_session(self) -> ShellSession: + """Replace the session with a clean one and return it. + + Replaced rather than reset in place, so this also recovers a session + wedged by a command that outlived its tool timeout (#2600): the new + session has its own lock, and the old one is closed as soon as the + straggler lets go of it. + """ + previous = getattr(self, "_shell_session", None) + self._shell_session = ShellSession(cwd_guard=self._session_cwd_guard()) + if previous is not None: + previous.close() + return self._shell_session + + def close_shell_session(self) -> None: + """Tear the session down at task end. Safe to call more than once.""" + session = getattr(self, "_shell_session", None) + if session is not None: + session.close() + self._shell_session = None + def _validate_shell_command(self, command: str) -> tuple: """Every refusal ``command`` earns on its text alone, plus its segments. @@ -829,9 +933,14 @@ def run_shell_command( "has_errors": True, } - cwd = str(Path(working_directory).resolve()) + session_cwd_override = str(Path(working_directory).resolve()) + cwd = session_cwd_override else: - cwd = str(Path.cwd()) + # The session's directory, not the process's: a `cd` from an + # earlier call is where this command actually runs, so it is + # also what relative arguments must be checked against. + session_cwd_override = None + cwd = self.shell_session.cwd # Operators, syntax, and the per-command whitelist. Shared with # the pre-flight that runs before the confirmation prompt, so a @@ -922,22 +1031,16 @@ def run_shell_command( if hasattr(self, "debug") and self.debug: logger.info(f"Executing command: {command} in {cwd}") - # On Windows, many commands are shell built-ins (dir, cd, type, - # echo) and Unix commands (ls, pwd, cat) don't exist as .exe - # files. Since we have already validated the command against the - # whitelist, we use shell=True on Windows so cmd.exe can resolve - # both built-ins and commands on PATH (including those from Git - # for Windows which provides ls, cat, grep, etc.). - # - # A skill-granted CLI is the exception, and must stay one. It is - # a real executable — it needs no built-in resolution — and it - # is the one path that can run without a confirmation prompt, on - # arguments built from untrusted remote text (an issue body the - # model just read). Handing cmd.exe the raw STRING there would - # let that text act: `--search "x|whoami"` is one argv token to - # every check above and two commands to cmd.exe, and `%VAR%` - # expands into a value the approval prompt never showed. argv - # goes to the process verbatim, so neither is possible. + # A skill-granted CLI runs as argv, never as a command string, + # and must keep doing so. It is a real executable — it needs no + # built-in resolution — and it is the one path that can run + # without a confirmation prompt, on arguments built from + # untrusted remote text (an issue body the model just read). + # Handing a shell the raw STRING there would let that text act: + # `--search "x|whoami"` is one argv token to every check above + # and two commands to a shell, and `%VAR%` expands into a value + # the approval prompt never showed. argv goes to the process + # verbatim, so neither is possible. # One segment only: a pipeline needs a shell to be a pipeline, # and `cmd_parts` has already dropped the `|` tokens, so an argv # run of one would silently concatenate the two commands. @@ -946,19 +1049,18 @@ def run_shell_command( and bool(granted) and _is_granted_binary(segments[0][0], granted) ) - use_shell = os.name == "nt" and not lone_granted_segment - - # Build the command string for execution - # On Windows with shell=True, use the ORIGINAL command string - # to preserve quoting (critical for PowerShell pipe commands) - exec_cmd = cmd_parts # Default: list for subprocess - if use_shell: - # Start with original command to preserve quoting - exec_cmd = command + session = self.shell_session + if session.posix_script: + exec_command = _requote_for_posix(segments) + else: + # cmd.exe gets the ORIGINAL string: its quoting rules are not + # shlex's, and re-quoting a PowerShell -Command body breaks + # it. Unchanged from before the session existed. + exec_command = command - # Map common Unix commands to Windows equivalents - # when Git-for-Windows tools aren't on PATH + # Map Unix names to cmd built-ins when the Git-for-Windows + # tools that provide them aren't on PATH. _UNIX_TO_WIN = { "ls": "dir", "pwd": "cd", @@ -967,106 +1069,62 @@ def run_shell_command( "cp": "copy", "mv": "move", } - if cmd_base in _UNIX_TO_WIN: - import shutil - - if not shutil.which(cmd_base): - win_cmd = _UNIX_TO_WIN[cmd_base] - logger.info( - f"Mapping Unix command '{cmd_base}' -> Windows '{win_cmd}'" - ) - # Replace just the command name in the original string - exec_cmd = win_cmd + exec_cmd[len(cmd_base) :] - - # Execute command - # - # encoding/errors are explicit, and load-bearing. Bare - # ``text=True`` decodes with the locale codec — cp1252 on a - # default Windows box — and subprocess does that decode inside - # its pipe reader THREAD. A byte that codec cannot map raises - # UnicodeDecodeError in that thread, which dies, and - # subprocess.run then returns returncode 0 with EMPTY stdout. - # The command succeeded and its output was silently discarded. - # - # That is not an edge case: `gh issue list` on amd/gaia returns - # an issue title containing "⚠️", so GitHub triage got back - # nothing and the model reported an empty backlog it had never - # actually read. Any tool emitting UTF-8 (git, gh, npm, docker) - # hits it. errors="replace" keeps a stray undecodable byte from - # costing the whole output. - start_time = time.monotonic() + if cmd_base in _UNIX_TO_WIN and not shutil.which(cmd_base): + win_cmd = _UNIX_TO_WIN[cmd_base] + logger.info( + f"Mapping Unix command '{cmd_base}' -> Windows '{win_cmd}'" + ) + exec_command = win_cmd + command[len(cmd_base) :] + try: - result = subprocess.run( - exec_cmd, - cwd=cwd, - capture_output=True, - # stdin is DEVNULL, never inherited. capture_output - # redirects stdout/stderr but leaves stdin alone, and - # this process's stdin is the agent transport's pipe — - # held open by the TUI and never written to. A child - # that reads it (directly, or by probing whether it is - # interactive) blocks forever on input that cannot - # arrive, because there is no human on that pipe. - # - # The hang was not theoretical: `gh` spawned from the - # agent never exited, while the identical command took - # 0.07s from a shell. Worse, subprocess.run's own - # timeout does not save it — on expiry it kills the - # cmd.exe it launched, then calls communicate() again - # with NO timeout, which waits on pipes the surviving - # grandchild still holds. That is the 180s tool timeout - # and the orphaned gh.exe left behind by every attempt. - # - # DEVNULL gives an immediate EOF, which is the honest - # answer here: an agent's shell command is - # non-interactive by construction. - stdin=subprocess.DEVNULL, - encoding="utf-8", - errors="replace", - timeout=timeout, - check=False, - env=os.environ.copy(), - shell=use_shell, # nosec B602 - Windows-only; command whitelist-validated above, shell needed for cmd.exe built-ins/pipes - ) - duration = time.monotonic() - start_time - - # Record successful command execution for rate limiting - self._record_command_execution() - except subprocess.TimeoutExpired as exc: - duration = time.monotonic() - start_time - - # Handle timeout gracefully - stdout_str = "" - stderr_str = "" - if exc.stdout: - stdout_str = ( - exc.stdout - if isinstance(exc.stdout, str) - else exc.stdout.decode("utf-8", errors="replace") + if lone_granted_segment: + result = session.run_argv( + cmd_parts, + timeout=timeout, + working_directory=session_cwd_override, ) - if exc.stderr: - stderr_str = ( - exc.stderr - if isinstance(exc.stderr, str) - else exc.stderr.decode("utf-8", errors="replace") + else: + result = session.run( + exec_command, + timeout=timeout, + working_directory=session_cwd_override, ) + except ShellSessionBusy as exc: + return { + "status": "error", + "error": str(exc), + "command": command, + "has_errors": True, + "session_busy": True, + "hint": "Call reset_shell_session to abandon the stuck command and start clean.", + } + except ShellSessionClosed as exc: + return { + "status": "error", + "error": str(exc), + "command": command, + "has_errors": True, + } + if result.timed_out: return { "status": "error", "error": f"Command timed out after {timeout} seconds", "command": command, - "stdout": stdout_str, - "stderr": stderr_str, + "stdout": result.stdout, + "stderr": result.stderr, "has_errors": True, "timed_out": True, "timeout": timeout, - "duration_seconds": duration, - "cwd": cwd, + "duration_seconds": result.duration_seconds, + "cwd": result.cwd, } + self._record_command_execution() + # Capture and truncate output if too long - stdout = result.stdout or "" - stderr = result.stderr or "" + stdout = result.stdout + stderr = result.stderr truncated = False max_output = 10_000 @@ -1081,22 +1139,106 @@ def run_shell_command( # Debug logging if hasattr(self, "debug") and self.debug: logger.info( - f"Command completed in {duration:.2f}s with return code {result.returncode}" + f"Command completed in {result.duration_seconds:.2f}s " + f"with return code {result.return_code}" ) - return { + response = { "status": "success", "command": command, "stdout": stdout, "stderr": stderr, - "return_code": result.returncode, - "has_errors": result.returncode != 0, - "duration_seconds": duration, + "return_code": result.return_code, + "has_errors": result.return_code != 0, + "duration_seconds": result.duration_seconds, "timeout": timeout, - "cwd": cwd, + "cwd": result.cwd, + "session_cwd": session.cwd, "output_truncated": truncated, } + if result.cwd_change_rejected: + response["warning"] = result.cwd_change_rejected + return response except Exception as exc: logger.error(f"Error executing shell command: {exc}") return {"status": "error", "error": str(exc), "has_errors": True} + + @tool( + atomic=True, + display_label="Shell state", + ) + def get_shell_state() -> Dict[str, Any]: + """Report the shell session's current directory and changed environment. + + Shell commands share one session, so a `cd` or an exported variable + from an earlier command is still in effect. Call this to read that + state instead of guessing it. + """ + session = self.shell_session + return { + "status": "success", + "cwd": session.cwd, + "environment": session.environment(), + "removed_environment": session.removed_environment(), + "interpreter": "posix-shell" if session.posix_script else "cmd.exe", + "has_errors": False, + } + + @tool( + atomic=True, + display_label="Set shell variable", + ) + def set_shell_variable(name: str, value: str) -> Dict[str, Any]: + """Set an environment variable for every later command this turn. + + The shell's own `export` is not on the read-only command whitelist, + so this is how a variable gets set. Variables that decide which + binary or which code runs next — PATH, PYTHONPATH, LD_* and the + like — are refused. + """ + upper = name.strip().upper() + if not _VALID_SHELL_VAR_NAME.match(name.strip()): + return { + "status": "error", + "error": f"'{name}' is not a valid environment variable name.", + "has_errors": True, + "hint": "Use letters, digits and underscores, starting with a letter or underscore.", + } + if upper in _UNSETTABLE_ENV_NAMES or upper.startswith( + _UNSETTABLE_ENV_PREFIXES + ): + return { + "status": "error", + "error": ( + f"'{name}' decides which binary or which code a later command " + "runs, so it cannot be set from here." + ), + "has_errors": True, + "hint": "Pass the value on the command line instead, or use an absolute path.", + } + self.shell_session.set_env(name.strip(), value) + return { + "status": "success", + "message": f"{name.strip()} is set for the rest of this session.", + "has_errors": False, + } + + @tool( + atomic=True, + display_label="Reset shell", + ) + def reset_shell_session() -> Dict[str, Any]: + """Return the shell session to the directory and environment it started in. + + Use this when the session is in a bad state — a wrong directory, a + variable that should not be set, or a command that hung and left the + session busy. + """ + session = self.reset_shell_session() + return { + "status": "success", + "message": "Shell session reset.", + "cwd": session.cwd, + "has_errors": False, + } diff --git a/tests/unit/test_shell_output_encoding.py b/tests/unit/test_shell_output_encoding.py index 4990aee1d..b87a738bb 100644 --- a/tests/unit/test_shell_output_encoding.py +++ b/tests/unit/test_shell_output_encoding.py @@ -76,6 +76,20 @@ def test_output_with_non_locale_bytes_survives(tmp_path): assert "café" in result["stdout"] +def _spawn_kwargs(monkeypatch) -> dict: + """The kwargs the executor hands the child process for `echo hello`.""" + seen = {} + real_popen = subprocess.Popen + + def spy(*args, **kwargs): + seen.update(kwargs) + return real_popen(*args, **kwargs) + + monkeypatch.setattr(subprocess, "Popen", spy) + _shell_tool()(command="echo hello") + return seen + + def test_executor_decodes_utf8_not_the_locale(monkeypatch): """Pin the call itself, so a future edit cannot quietly restore text=True. @@ -83,15 +97,7 @@ def test_executor_decodes_utf8_not_the_locale(monkeypatch): invisible in the result: the regression looks like a command that returned nothing, on every platform whose locale codec happens to be strict. """ - seen = {} - real_run = subprocess.run - - def spy(*args, **kwargs): - seen.update(kwargs) - return real_run(*args, **kwargs) - - monkeypatch.setattr(subprocess, "run", spy) - _shell_tool()(command="echo hello") + seen = _spawn_kwargs(monkeypatch) assert seen.get("encoding") == "utf-8" assert seen.get("errors") == "replace" @@ -114,15 +120,7 @@ def test_child_never_inherits_the_agent_transport_stdin(monkeypatch): in 0.07s from a shell. The tool reported a 180s timeout and GitHub triage was unusable. """ - seen = {} - real_run = subprocess.run - - def spy(*args, **kwargs): - seen.update(kwargs) - return real_run(*args, **kwargs) - - monkeypatch.setattr(subprocess, "run", spy) - _shell_tool()(command="echo hello") + seen = _spawn_kwargs(monkeypatch) assert seen["stdin"] is subprocess.DEVNULL diff --git a/tests/unit/test_shell_session.py b/tests/unit/test_shell_session.py new file mode 100644 index 000000000..81e675778 --- /dev/null +++ b/tests/unit/test_shell_session.py @@ -0,0 +1,257 @@ +# Copyright(C) 2025-2026 Advanced Micro Devices, Inc. All rights reserved. +# SPDX-License-Identifier: MIT +"""Tests for the persistent shell session (issue #3380). + +These run real subprocesses on purpose. A mocked session would prove only that +we called ``subprocess`` — it could not prove that ``cd`` in one call is +visible to the next, which is the entire feature. +""" + +import os +import sys +import threading +import time + +import pytest + +from gaia.agents.tools.shell_session import ( + ShellSession, + ShellSessionBusy, + ShellSessionClosed, +) + +#: True when the session runs commands as a cmd.exe batch file. Setting +#: GAIA_SHELL to a POSIX shell exercises the other script flavour on the same +#: box — worth doing before touching the script generator. +IS_WINDOWS = os.name == "nt" and not os.environ.get("GAIA_SHELL", "").strip() + + +def export_command(name: str, value: str) -> str: + """The platform's way of exporting a variable for later commands.""" + return f"set {name}={value}" if IS_WINDOWS else f"export {name}={value}" + + +def echo_var_command(name: str) -> str: + return f"echo %{name}%" if IS_WINDOWS else f'echo "${name}"' + + +def sleep_command(seconds: float, marker: str = "") -> str: + """A command whose *child process* blocks, then optionally leaves a marker. + + A grandchild rather than the shell itself: killing only the shell is the + failure mode the process-group kill exists to prevent, and a marker written + after the sleep is how a survivor announces itself. + """ + body = f"import time; time.sleep({seconds})" + if marker: + body += f"; open(r'{marker}', 'w').write('survived')" + return f'"{sys.executable}" -c "{body}"' + + +@pytest.fixture +def session(tmp_path): + shell = ShellSession(start_cwd=str(tmp_path)) + yield shell + shell.close() + + +class TestWorkingDirectoryPersists: + def test_cd_survives_to_the_next_command(self, session, tmp_path): + (tmp_path / "sub").mkdir() + + session.run("cd sub") + + assert session.cwd.replace("\\", "/").endswith("/sub") + result = session.run("cd" if IS_WINDOWS else "pwd") + assert "sub" in result.stdout + + def test_working_directory_argument_is_one_shot(self, session, tmp_path): + """An explicit per-call directory scopes that call, as it always has.""" + (tmp_path / "elsewhere").mkdir() + before = session.cwd + + session.run( + "cd" if IS_WINDOWS else "pwd", working_directory=str(tmp_path / "elsewhere") + ) + + assert session.cwd == before + + def test_set_cwd_rejects_a_non_directory(self, session, tmp_path): + before = session.cwd + + assert session.set_cwd(str(tmp_path / "does-not-exist")) is False + assert session.cwd == before + + def test_cwd_guard_refuses_a_directory_change(self, tmp_path): + (tmp_path / "off-limits").mkdir() + allowed = str(tmp_path) + shell = ShellSession( + start_cwd=allowed, + cwd_guard=lambda path: "off-limits" not in path.replace("\\", "/"), + ) + try: + result = shell.run("cd off-limits") + + assert shell.cwd == allowed + assert result.cwd_change_rejected is not None + finally: + shell.close() + + +class TestEnvironmentPersists: + def test_exported_variable_survives_to_the_next_command(self, session): + session.run(export_command("GAIA_TEST_VAR", "persisted")) + + assert session.environment().get("GAIA_TEST_VAR") == "persisted" + result = session.run(echo_var_command("GAIA_TEST_VAR")) + assert "persisted" in result.stdout + + def test_inherited_variables_are_not_reported_as_changes(self, session): + """Only what the session actually diverged, or Windows folds every name.""" + session.run("echo hello") + + # The bug this pins: Windows folds variable names and os.environ + # upper-cases them, so comparing raw made every inherited variable both + # an override and a removal — the session would replay the whole + # environment and unset it at the same time. + diverged = session.environment() + assert len(diverged) < len(os.environ) / 4, diverged + assert session.removed_environment() == [] + + def test_set_env_applies_to_the_next_command(self, session): + session.set_env("GAIA_TEST_PRESET", "from-api") + + result = session.run(echo_var_command("GAIA_TEST_PRESET")) + assert "from-api" in result.stdout + + def test_a_virtualenv_style_activation_persists(self, session, tmp_path): + """What venv activation actually is: PATH plus a marker variable.""" + session.run(export_command("VIRTUAL_ENV", str(tmp_path / "venv"))) + + assert "VIRTUAL_ENV" in session.environment() + result = session.run(echo_var_command("VIRTUAL_ENV")) + assert "venv" in result.stdout + + def test_the_parent_process_is_never_mutated(self, session, tmp_path): + before_cwd = os.getcwd() + session.run(export_command("GAIA_TEST_LEAK", "leaked")) + session.run("cd .") + + assert "GAIA_TEST_LEAK" not in os.environ + assert os.getcwd() == before_cwd + + +class TestReset: + def test_reset_restores_the_starting_state(self, session, tmp_path): + (tmp_path / "sub").mkdir() + session.run("cd sub") + session.run(export_command("GAIA_TEST_RESET", "x")) + + session.reset() + + assert session.cwd == str(tmp_path.resolve()) + assert session.environment() == {} + result = session.run(echo_var_command("GAIA_TEST_RESET")) + assert result.stdout.strip() != "x" + + +class TestTeardown: + def test_close_makes_further_commands_an_error(self, tmp_path): + shell = ShellSession(start_cwd=str(tmp_path)) + shell.run("echo hello") + + shell.close() + + assert shell.closed is True + with pytest.raises(ShellSessionClosed): + shell.run("echo hello") + + def test_close_removes_the_session_temp_directory(self, tmp_path): + shell = ShellSession(start_cwd=str(tmp_path)) + shell.run("echo hello") + temp_dir = shell._temp_dir # pylint: disable=protected-access + assert temp_dir and os.path.isdir(temp_dir) + + shell.close() + + assert not os.path.isdir(temp_dir) + + def test_a_timed_out_command_leaves_no_running_child(self, session, tmp_path): + """The command's children are killed too, not just the shell.""" + marker = tmp_path / "survivor.txt" + + result = session.run(sleep_command(4, str(marker)), timeout=1) + + assert result.timed_out is True + time.sleep(6) + assert not marker.exists(), "a child outlived the timeout and kept running" + + +class TestSerialisation: + def test_a_second_command_waits_for_the_first(self, session): + """Two threads against one session must not interleave.""" + order = [] + barrier = threading.Barrier(2) + + def slow(): + barrier.wait() + session.run(sleep_command(2), timeout=20) + order.append("slow") + + def quick(): + barrier.wait() + time.sleep(0.3) + session.run("echo quick", timeout=20) + order.append("quick") + + threads = [threading.Thread(target=slow), threading.Thread(target=quick)] + for thread in threads: + thread.start() + for thread in threads: + thread.join(timeout=60) + + assert order == ["slow", "quick"] + + def test_a_busy_session_says_so_instead_of_hanging(self, session): + """#2600: a command past its tool timeout is still inside the session.""" + started = threading.Event() + + def hog(): + started.set() + session.run(sleep_command(6), timeout=30) + + thread = threading.Thread(target=hog) + thread.start() + started.wait(timeout=5) + time.sleep(0.5) + try: + with pytest.raises(ShellSessionBusy): + session.run("echo blocked", timeout=1) + finally: + thread.join(timeout=60) + + +class TestExecution: + def test_exit_code_is_reported(self, session): + result = session.run("exit 3") + + assert result.return_code == 3 + + def test_stdout_is_captured(self, session): + result = session.run("echo captured") + + assert "captured" in result.stdout + assert result.return_code == 0 + + def test_run_argv_applies_the_session_environment(self, session): + session.set_env("GAIA_TEST_ARGV", "argv-value") + + result = session.run_argv( + [sys.executable, "-c", "import os; print(os.environ['GAIA_TEST_ARGV'])"] + ) + + assert "argv-value" in result.stdout + + def test_empty_command_is_refused(self, session): + with pytest.raises(ValueError): + session.run(" ") diff --git a/tests/unit/test_shell_tools_session.py b/tests/unit/test_shell_tools_session.py new file mode 100644 index 000000000..5021ec12d --- /dev/null +++ b/tests/unit/test_shell_tools_session.py @@ -0,0 +1,187 @@ +# Copyright(C) 2025-2026 Advanced Micro Devices, Inc. All rights reserved. +# SPDX-License-Identifier: MIT +"""run_shell_command shares one session, so state survives between calls (#3380). + +Driven through the registered tools rather than the session object, because the +regression these guard is at the seam: a tool that builds a fresh subprocess per +call is indistinguishable from one that reuses a session until you ask it where +it is. +""" + +import os +import shlex + +import pytest + +from gaia.agents.tools.shell_tools import ShellToolsMixin + + +class _Host(ShellToolsMixin): + """Minimal host: the mixin only needs its own __init__ for rate limiting.""" + + +@pytest.fixture +def tools(): + """The registered shell tools, keyed by name, on one fresh host.""" + captured = {} + import gaia.agents.base.tools as tools_module + + original = tools_module.tool + + def spy(**kwargs): + def decorate(fn): + captured[fn.__name__] = fn + return original(**kwargs)(fn) + + return decorate + + tools_module.tool = spy + try: + host = _Host() + # No rate limit: these tests run more than three commands in ten seconds. + host.max_commands_per_10_seconds = 1000 + host.max_commands_per_minute = 1000 + host.register_shell_tools() + finally: + tools_module.tool = original + captured["_host"] = host + yield captured + host.close_shell_session() + + +def test_cd_in_one_call_is_where_the_next_call_runs(tools, tmp_path): + host = tools["_host"] + (tmp_path / "workspace").mkdir() + host.shell_session.set_cwd(str(tmp_path)) + + tools["run_shell_command"](command="cd workspace") + result = tools["run_shell_command"](command="pwd") + + assert result["status"] == "success", result + assert result["session_cwd"].replace("\\", "/").endswith("/workspace") + + +def test_the_probe_reports_the_session_directory(tools, tmp_path): + host = tools["_host"] + host.shell_session.set_cwd(str(tmp_path)) + + state = tools["get_shell_state"]() + + assert state["status"] == "success" + assert state["cwd"] == str(tmp_path.resolve()) + assert state["environment"] == {} + + +def test_a_variable_set_through_the_tool_reaches_later_commands(tools): + assert tools["set_shell_variable"](name="GAIA_TOOL_VAR", value="kept")[ + "status" + ] == ("success") + + state = tools["get_shell_state"]() + assert state["environment"]["GAIA_TOOL_VAR"] == "kept" + + echo = "echo %GAIA_TOOL_VAR%" if os.name == "nt" else "echo $GAIA_TOOL_VAR" + result = tools["run_shell_command"](command=echo) + assert "kept" in result["stdout"] + + +@pytest.mark.parametrize( + "name", ["PATH", "PYTHONPATH", "LD_PRELOAD", "dyld_insert_libraries"] +) +def test_variables_that_choose_the_binary_are_refused(tools, name): + """Setting PATH would let the agent pick which `ls` the whitelist approved.""" + result = tools["set_shell_variable"](name=name, value="/tmp/evil") + + assert result["status"] == "error" + assert tools["get_shell_state"]()["environment"] == {} + + +def test_an_invalid_variable_name_is_refused(tools): + result = tools["set_shell_variable"](name="not a name", value="x") + + assert result["status"] == "error" + + +def test_reset_returns_the_session_to_where_it_started(tools, tmp_path): + host = tools["_host"] + (tmp_path / "sub").mkdir() + start = host.shell_session.cwd + tools["set_shell_variable"](name="GAIA_TOOL_RESET", value="x") + host.shell_session.set_cwd(str(tmp_path / "sub")) + + result = tools["reset_shell_session"]() + + assert result["status"] == "success" + assert result["cwd"] == start + assert tools["get_shell_state"]()["environment"] == {} + + +def test_working_directory_still_scopes_a_single_call(tools, tmp_path): + """The existing argument keeps its meaning: this call only.""" + host = tools["_host"] + before = host.shell_session.cwd + + result = tools["run_shell_command"](command="pwd", working_directory=str(tmp_path)) + + assert result["status"] == "success", result + assert host.shell_session.cwd == before + + +def test_a_forbidden_directory_change_is_not_absorbed(tools, tmp_path): + """`cd` must not become a way around the path policy. + + Without this, the agent could step into a directory it may not read and then + open a file by bare name — the per-argument check only sees paths. + """ + host = tools["_host"] + (tmp_path / "secrets").mkdir() + host.shell_session.set_cwd(str(tmp_path)) + host._is_path_allowed = lambda path: "secrets" not in path.replace("\\", "/") + host._shell_session = None # rebuild the session with the guard attached + + host.shell_session.set_cwd(str(tmp_path)) + result = tools["run_shell_command"](command="cd secrets") + + assert host.shell_session.cwd == str(tmp_path.resolve()) + assert "warning" in result + + +def test_the_guardrails_still_refuse_a_blocked_command(tools): + """Persistence is about state, not policy (#3380 acceptance criterion 6).""" + result = tools["run_shell_command"](command="rm -rf /") + + assert result["status"] == "error" + assert result["has_errors"] is True + + +@pytest.mark.parametrize( + "command, expected", + [ + ("cat ~/../secret", "cat '~/../secret'"), + ("echo $HOME", "echo '$HOME'"), + ("ls *.py", "ls '*.py'"), + ("grep -r 'foo bar' .", "grep -r 'foo bar' ."), + ("ls | head -3", "ls | head -3"), + ], +) +def test_the_posix_shell_gets_argv_back_not_a_string_to_expand(command, expected): + """A session needs a shell, but the shell must not re-read the arguments. + + `cat ~/../secret` is one literal token to the path validator and a different + file to sh. Re-quoting the validated tokens keeps the pipeline and leaves + expansion off, as it was before commands went through a shell at all. + """ + from gaia.agents.tools.shell_tools import _requote_for_posix, _split_pipeline + + assert _requote_for_posix(_split_pipeline(shlex.split(command))) == expected + + +def test_teardown_is_repeatable(tools): + host = tools["_host"] + tools["run_shell_command"](command="echo hello") + + host.close_shell_session() + host.close_shell_session() + + # A closed session is replaced on next use rather than left broken. + assert tools["run_shell_command"](command="echo again")["status"] == "success" diff --git a/tests/unit/test_skill_binary_grants.py b/tests/unit/test_skill_binary_grants.py index 42e4320ec..e30b303d2 100644 --- a/tests/unit/test_skill_binary_grants.py +++ b/tests/unit/test_skill_binary_grants.py @@ -1289,25 +1289,52 @@ def decorate(fn): return captured["run_shell_command"] +class _FakeChild: + """Stands in for the spawned process, recording the argv it was given.""" + + def __init__(self, args): + self.args = args + self.returncode = 0 + self.pid = -1 + + def __enter__(self): + return self + + def __exit__(self, *exc_info): + return False + + def communicate(self, timeout=None): # noqa: ARG002 - signature match + return "", "" + + def poll(self): + return 0 + + def _run_capturing_subprocess(host, command): - """Run *command* through the tool, intercepting the subprocess call.""" - import subprocess as subprocess_module + """Run *command* through the tool, intercepting the spawned child. - import gaia.agents.tools.shell_tools as shell_module + ``ran_directly`` is True when the binary itself was executed, False when the + tool handed the command to a shell as a generated script — the distinction + the grant path depends on. + """ + import gaia.agents.tools.shell_session as session_module seen = {} - real_run = shell_module.subprocess.run + real_popen = session_module.subprocess.Popen - def fake_run(args, **kwargs): + def fake_popen(args, **kwargs): seen["args"] = args - seen["shell"] = kwargs.get("shell", False) - return subprocess_module.CompletedProcess(args, 0, "", "") + first = os.path.basename(str(args[0])).lower() + seen["ran_directly"] = not ( + first.startswith(("cmd", "sh", "bash", "dash")) and first != "sh.py" + ) + return _FakeChild(args) - shell_module.subprocess.run = fake_run + session_module.subprocess.Popen = fake_popen try: _captured_shell_tool(host)(command=command) finally: - shell_module.subprocess.run = real_run + session_module.subprocess.Popen = real_popen return seen @@ -1315,7 +1342,7 @@ def test_a_granted_cli_is_handed_argv_not_a_shell_string(): call = _run_capturing_subprocess( _Gated("gh"), "gh issue list --search x|echo pwned" ) - assert call["shell"] is False, "a granted CLI must not go through cmd.exe" + assert call["ran_directly"] is True, "a granted CLI must not go through a shell" assert isinstance(call["args"], list) # The metacharacter stays inside one argument instead of becoming a pipe. assert "x|echo" in call["args"], call["args"] @@ -1326,22 +1353,22 @@ def test_an_env_var_in_a_granted_write_reaches_the_process_unexpanded(): call = _run_capturing_subprocess( _Gated("gh"), "gh issue comment 1 --body %GITHUB_TOKEN%" ) - assert call["shell"] is False + assert call["ran_directly"] is True assert "%GITHUB_TOKEN%" in call["args"] def test_an_ungranted_command_keeps_the_shell_path(): - """The exemption is for granted CLIs only. `pwd`/`ls` still need cmd.exe on - Windows to resolve built-ins, and this change must not touch them.""" + """The exemption is for granted CLIs only. `pwd`/`ls` still need a shell to + resolve built-ins, and this change must not touch them.""" call = _run_capturing_subprocess(_Gated(), "pwd") - assert call["shell"] is (os.name == "nt") + assert call["ran_directly"] is False def test_a_pipeline_is_not_run_as_argv(): """`cmd_parts` has dropped the `|`, so an argv run of a pipeline would silently concatenate two commands into one. Only a lone segment qualifies.""" call = _run_capturing_subprocess(_Gated("gh"), "gh issue list | head -5") - assert call["shell"] is (os.name == "nt") + assert call["ran_directly"] is False def test_pytest_has_no_write_tier():

AltStyle によって変換されたページ (->オリジナル) /