Skip to content

feat(terminal): accept argv-list form to bypass bash - #25864

Open
sparkeros wants to merge 1 commit into
NousResearch:mainfrom
sparkeros:pr/feat-terminal-argv-form
Open

feat(terminal): accept argv-list form to bypass bash#25864
sparkeros wants to merge 1 commit into
NousResearch:mainfrom
sparkeros:pr/feat-terminal-argv-form

Conversation

@sparkeros

Copy link
Copy Markdown

What does this PR do?

Adds a sibling `argv` parameter on the `terminal` tool. When the model passes a non-empty argv list, the command bypasses bash entirely: `subprocess.Popen` is called with `shell=False`, so arguments reach the target program byte-for-byte exactly as the model assembled them.

Why: today every terminal call is `bash -c `, which means the model has to escape its arguments into a shell-safe string. A single apostrophe in a JSON body is enough to break the outer quoting and the call fails with `unexpected EOF while looking for matching '`. argv form removes the shell from the equation:

```python
argv=["curl", "-X", "POST", "-H", "Content-Type: application/json",
"-d", '{"body":"It\'s done"}',
"http://localhost:3100/api/issues/AOS-8"]
```

Bash never sees the body — curl receives the bytes verbatim. Also saves the bash fork+parse overhead (~30–80 ms per call).

Complements PR #25861 (the `http` tool) and #25862 (quoting-error hint). Independent — works without either.

Type of Change

  • ✨ New feature (non-breaking change that adds functionality)

Changes Made

  • `tools/environments/base.py`:
    • New `execute_argv(argv, cwd, timeout, stdin_data)` alongside existing `execute(command, ...)`. Bypasses shell-only machinery (sudo prompt rewriting, compound-background rewrite, CWD-tracking wrapper, snapshot env). Documented trade-off: argv processes can't change the session's cwd; that's correct semantics for a single-program invocation.
    • New `_run_argv(argv, ...)` hook with a default `_run_bash(shlex.join(argv))` fallback. Backends that can spawn argv directly override it to skip the bash wrap.
  • `tools/environments/local.py`: overrides `_run_argv` with a direct `subprocess.Popen(argv, shell=False, ...)` call. Mirrors the cwd-recovery, env-merge, and process-group setup of `_run_bash` so process management (kill, timeout, output capture) is identical.
  • `tools/terminal_tool.py`:
    • New `argv` parameter in `terminal_tool()` (default None) and corresponding schema property (`type: array, items: type: string`).
    • Validation: argv and command are mutually exclusive; argv is foreground-only (background and pty paths go through `process_registry` which expects shell strings — separate plumbing); argv elements must all be strings.
    • Dispatch branch: when `argv` is set and validation passes, route to `env.execute_argv(argv, ...)` instead of `env.execute(command, ...)`. The string view (`shlex.join(argv)`) is still used for safety checks, logging, and exit-code interpretation, but is never executed.
    • Schema descriptions on `command` and `argv` make the mutual exclusion and shell-feature trade-offs explicit.
  • `tests/tools/test_terminal_argv_form.py` (13 tests): validation paths (both/neither, non-string entries, background/pty rejection), end-to-end dispatch (argv goes to execute_argv, string command goes to execute, apostrophe-in-payload survives byte-for-byte), schema shape, and the `local._run_argv` shell=False guarantee.

How to Test

`pytest tests/tools/test_terminal_argv_form.py -q` — 13 passed.

Checklist

  • Read the Contributing Guide
  • Conventional Commits (`feat(terminal):`)
  • Searched for existing PRs
  • Only the one feature
  • Tests added + passing on Ubuntu 24.04 / WSL2 (aarch64)
  • Schema description updated to document argv vs command mutual exclusion
  • No new config keys
  • Cross-platform: `subprocess.Popen(argv, shell=False)` works identically on Linux / macOS / Windows; uses the same cwd-recovery / process-group setup as the existing `_run_bash` path

Behavior notes

  • Foreground only for now. `background=true` and `pty=true` go through `process_registry` which expects shell strings; supporting them in argv form is a separate plumbing change. The handler returns a clear error if combined.
  • Schema kept JSON-Schema-friendly. `command` stays as `type: string` (provider-safe; some sanitisers reject `oneOf` mixed-form). `argv` is a sibling property. The model passes empty `command` plus populated `argv` for the new path.
  • Backends that haven't implemented `_run_argv` (docker, ssh, modal, etc. for now) inherit the default that degrades to `_run_bash(shlex.join(argv))`. This still gets the safety win (no shell parsing — `shlex.join` produces a provably-valid bash string), just not the perf win. Native argv overrides for those backends are easy follow-ups.

Add a sibling `argv` parameter on the terminal tool. When the model
passes a non-empty argv list, the command bypasses bash entirely:
subprocess.Popen is called with shell=False, so arguments reach the
target program byte-for-byte exactly as the model assembled them.

Why: today every terminal call is `bash -c <string>`, which means the
model has to escape its arguments into a shell-safe string. A single
apostrophe in a JSON body is enough to break the outer quoting and the
call fails with `unexpected EOF while looking for matching '`. argv
form removes the shell from the equation:

  argv=["curl", "-X", "POST", "-H", "Content-Type: application/json",
        "-d", '{"body":"It\'s done"}',
        "http://localhost:3100/api/issues/AOS-8"]

Bash never sees the body — curl receives the bytes verbatim.

Plumbing:
- BaseEnvironment gains `execute_argv(argv, cwd, timeout, stdin_data)`
  alongside `execute(command, ...)`. It bypasses the shell-only
  machinery: no sudo prompt rewriting, no compound-background rewrite,
  no CWD-tracking wrapper, no snapshot env. Trade-off: argv processes
  cannot change the session's cwd; that's correct semantics for a
  single-program invocation.
- BaseEnvironment gains `_run_argv(argv, ...)` abstract-ish hook with
  a default implementation that degrades to `_run_bash(shlex.join(argv))`
  for backends that haven't implemented native argv. Backends that can
  spawn argv directly (LocalEnvironment via subprocess.Popen, future
  docker/ssh implementations) override this to skip the bash wrap and
  recover the ~30-80 ms per-call overhead.
- LocalEnvironment overrides `_run_argv` with a direct
  `subprocess.Popen(argv, shell=False, ...)` call. Mirrors the
  cwd-recovery, env-merge, and process-group setup of `_run_bash` so
  process management (kill, timeout, output capture) is identical.

Tool-level guard:
- argv form is foreground-only for now. background=true and pty=true
  go through process_registry which expects shell strings; supporting
  them in argv form is a separate plumbing change. The handler returns
  a clear error if the model combines argv with either flag.
- argv and command are mutually exclusive — the handler returns an
  error if both are non-empty.
- Schema-wise, `command` stays required (provider-safe; some sanitisers
  reject mixed-form `oneOf`). The model passes empty `command` plus
  populated `argv` for the new path.
- The `command` description now mentions argv as the alternative, and
  the `argv` description warns about no shell features (pipes, &&,
  redirection) so the model knows when to fall back to `command`.

Tests in tests/tools/test_terminal_argv_form.py cover validation
(both/neither, non-string entries, background/pty rejection), end-to-end
dispatch (argv goes to execute_argv, string command goes to execute,
apostrophe-in-payload survives byte-for-byte), schema shape, and the
local._run_argv shell=False guarantee.
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management labels May 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating a real shell-quoting pain point. Current main still runs local foreground terminal commands through bash -c (tools/environments/local.py:1003), so the premise remains valid.

Problems

  • In 25e9452edd9bec82a6e3ee3e6d55f2e302ae6f0c, the new BaseEnvironment._run_argv fallback calls _run_bash(shlex.join(argv)). That conflicts with the schema claim that argv skips Bash: current Docker alone builds bash -c in tools/environments/docker.py:1074-1076, and the other remote backends expose the same _run_bash model.
  • The new local path restores preexec_fn=os.setsid. Main intentionally replaced that with start_new_session=True at tools/environments/local.py:1044 in 515192c4b90c934c26389da37c47877e2cd274db, because Python code between fork and exec is unsafe in Hermes's multithreaded process. The proposed path also misses main's Windows spawn flags at tools/environments/local.py:1033.

Suggested changes

  • Make native argv execution an explicit backend capability, and only advertise it where Bash is not used.
  • Salvage the local spawn path onto current main's start_new_session, Windows flags, environment, and CWD-recovery behavior; add backend-contract tests.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-massive Sweeper blast radius: massive — everyone, every turn (invariant surface) labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-massive Sweeper blast radius: massive — everyone, every turn (invariant surface) sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/terminal Terminal execution and process management type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants