Skip to content

Navigation Menu

Sign in
Sign up

feat(shell): one persistent shell per task so cwd and env survive #3398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kovtcharov-amd wants to merge 8 commits into main
base: main
Choose a base branch
Loading
from feat/persistent-shell-session
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions .security-suppressions.json
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,45 @@
{
"_comment": "Approved security suppressions. Every '# noqa: S<n>' (flake8-bandit) and '# nosec' (bandit) comment in src/ or hub/ MUST be listed here with a justification, or CI fails (util/check_security_gates.py, run by util/lint.py). Adding an entry requires PR review — this is the gate that would have caught the GAIA hub tar-slip (CWE-22), where a '# noqa: S202 - hub artifacts are trusted' silenced an unvalidated tarfile.extractall. Keyed by (path, rule), not line number. See SECURITY.md.",
"suppressions": [
{"path": "src/gaia/eval/scorecard_gate.py", "rule": "S603", "justification": "git is a fixed, trusted executable; args are a constructed list, never shell-interpreted"},
{"path": "src/gaia/hub/installer.py", "rule": "S603", "justification": "'uv pip install' args are a constructed list (no user string), never shell-interpreted"},
{"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/eval/scorecard_gate.py",
"rule": "S603",
"justification": "git is a fixed, trusted executable; args are a constructed list, never shell-interpreted"
},
{
"path": "src/gaia/hub/installer.py",
"rule": "S603",
"justification": "'uv pip install' args are a constructed list (no user string), never shell-interpreted"
},
{
"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": "Importing subprocess is this module's purpose: it exists to hold one long-lived shell per task. The import itself carries no risk; the execution sites below are where the review belongs."
},
{
"path": "src/gaia/agents/tools/shell_session.py",
"rule": "B607",
"justification": "taskkill is resolved from PATH rather than an absolute path. It is a Windows system binary in System32, and hard-coding a path would break on non-default system roots. An attacker able to shadow taskkill on PATH already has code execution on the box, so this widens nothing."
},
{
"path": "src/gaia/agents/tools/shell_session.py",
"rule": "B603",
"justification": "Two sites. The taskkill call is a fixed argv list with no shell, and its only variable is a PID taken from a child this process started. The Popen at the session's exec site DOES honour shell=True when the caller asks for it - that is the feature, not an oversight: the command has already passed _validate_shell_command (operator blocklist plus per-segment binary policy), and under --bypass-permissions it is deliberately unrestricted, which is the documented purpose of that mode. The gate to review is the validator and the bypass flag, not this call."
}
]
}
7 changes: 7 additions & 0 deletions docs/sdk/mixins/tool-mixins.mdx
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
55 changes: 51 additions & 4 deletions docs/spec/shell-tools-mixin.mdx
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion hub/agents/chat/python/gaia-agent.yaml
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ icon: message-circle
# Real registered-tool count for entry_class's default config (prompt_profile
# defaults to "full") — introspected, drift-guarded by
# tests/unit/test_chat_fix_contracts.py.
tools_count: 54
tools_count: 57

language: python
min_gaia_version: "0.22.0"
Expand Down
10 changes: 7 additions & 3 deletions hub/agents/chat/python/gaia_agent_chat/agent.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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}")
15 changes: 13 additions & 2 deletions hub/agents/chat/python/gaia_agent_chat/tool_bundles.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -351,6 +359,9 @@
members=frozenset(
{
"run_shell_command",
"get_shell_state",
"set_shell_variable",
"reset_shell_session",
"execute_python_file",
"get_system_info",
}
Expand Down
2 changes: 1 addition & 1 deletion hub/agents/gaia/python/gaia-agent.yaml
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ icon: sparkles
# + the load_tools escape hatch, which registers because dynamic_tools is on.
# This is the REGISTERED size — what the agent can do. Dynamic tool loading
# means a single turn only shows the model a subset of it.
tools_count: 67
tools_count: 70

language: python
min_gaia_version: "0.23.0"
Expand Down
2 changes: 1 addition & 1 deletion hub/agents/gaia/python/gaia_agent/__init__.py
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def build_gaia():
icon="sparkles",
# Must equal the real registry size for the default construction, and
# the manifest's own tools_count. Drift-guarded by tests/test_gaia_agent.py.
tools_count=67,
tools_count=70,
# ChatAgent loads MCP servers dynamically, so the Settings "Active for"
# panel must list this agent for MCP-server connectors.
consumes_mcp_servers=True,
Expand Down
Loading
Loading

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