feat(shell): --bypass-permissions lifts the shell guardrails too - #3394

Issues

🟡 A newline is not a segment separator, so a multi-line command is validated as one command and executed as several (src/gaia/agents/tools/shell_tools.py:509, :512)

_tokenize sets whitespace_split = True and leaves \n in shlex's default whitespace, so it is dropped rather than emitted as a token, and _SEGMENT_SEPARATORS never sees it. Reproduced standalone:

'ls\nrm -rf /tmp/x' -> ['ls', 'rm', '-rf', '/tmp/x'] -> segments: ['ls']
'ls; rm -rf x' -> ['ls', ';', 'rm', '-rf', 'x'] -> segments: ['ls', 'rm'] # correct

_validate_bypass_command then only ever sees ls, returns None, and run_shell_command executes with shell=True on the original string (exec_cmd = command), so the shell runs both. Consequences:

1. rm is reachable under bypass without the refusal the set exists to produce. Explicitly not a security boundary per your own note — but it is the accident tripwire, and this is exactly the accident it was for.
2. _audit_shell_execution writes segments=[["ls","rm","-rf","/tmp/x"]] — one invocation that never happened, instead of the two that did. docs/plans/security-model.mdx ("Command substitution is the one thing bypass cannot audit precisely") and SPEC.md both become inaccurate.

Rewriting \n to ; before lexing would fix both but mis-splits a literal newline inside a quoted operand. The safe-fail version, if you'd rather not touch tokenisation:

 bypass = self.bypass_gates_active()
 if bypass and "\n" in command.strip():
 return (
 {
 "status": "error",
 "error": (
 "Multi-line commands are not accepted, even with bypass "
 "permissions active."
 ),
 "has_errors": True,
 "hint": (
 "A line break is a command separator the per-segment "
 "audit record cannot see. Join the commands with '&&' "
 "or ';', or put the script in a file and run it."
 ),
 },
 [],
 )

(goes in _validate_shell_command right after bypass = self.bypass_gates_active(), before the operator check.) Whichever route you pick, add a case to TestPerSegmentWalkSurvivesBypass — check("ls\nrm -rf /tmp/foo", bypass=True) is not None — since that class is precisely where a reader will look for this guarantee.

🟢 bypass_gates_active's docstring names a gate that doesn't read it (shell_tools.py:582)

Every gate reads this — _validate_shell_command, skill_grant_covers_call, the tool description, the executor

The run_shell_command description is a static string (shell_tools.py:910) that still says "Pipes (|) are supported" and gives only read-only examples. So the model is never told the surface widened, which is the PR's stated goal ("verify your work before claiming it is done"). Either drop the clause from the docstring, or make the description bypass-aware — the latter is a tool-description change and would need an eval run per CLAUDE.md, so the docstring edit is the cheap correct fix if you want to keep the scope tight.

🟢 Redirection is silently swallowed for a lone skill-granted segment under bypass (shell_tools.py:1128)

> is not in _SEGMENT_SEPARATORS, so gh issue list > out.txt is one segment; with a shell:execute:gh grant active lone_granted_segment is true, use_shell stays false, and gh receives literal > and out.txt as argv. Narrow (needs both a grant and bypass), and keeping argv for granted binaries is deliberate and right — but the result is a wrong answer rather than a refusal. Worth either excluding redirect tokens from lone_granted_segment, or a comment saying redirection is intentionally inert on that path.

🟢 An unmentioned second thread rides along (tests/conftest.py:38, tests/unit/test_no_real_browser_launch.py)

The session-scoped browser guard is a good fix and well-explained in its own docstring, but it is unrelated to bypass permissions and gets no line in the PR description — a reviewer has to discover it from the diff. Worth a sentence in the description, or its own PR. Minor gap while you're there: it covers webbrowser.open/open_new/open_new_tab but not webbrowser.get(...).open(...), which is the other way start_authorization-style code reaches a real browser.

Strengths

• Every bypass test asserts both states. TestDeveloperBinariesRefusedByDefault and TestReadOnlySubGuardsLiftUnderBypassOnly pin the default tier alongside the bypass tier, so a binary leaking into ALLOWED_COMMANDS fails here rather than shipping. test_allowed_commands_carries_no_developer_binary is the right invariant to have written down.
• The tests go past validation into real execution. TestExecutorUnderBypass spawns actual processes, which is what catches the two things validation-only tests cannot: that the operators reach a shell at all, and the lazily-created rate-limit deque that bypass skips creating. test_the_rate_limit_is_lifted's docstring names the AttributeError it exists to prevent.
• The two-attribute split is the right call and is enforced. bypass_permissions separate from auto_approve_gated_tools, with test_auto_approve_alone_does_not_lift_the_shell_gates pinning it, means GAIA_AUTO_APPROVE_TOOLS can never quietly inherit an unguarded shell.
• The HTTP transport is pinned shut from three directions — the handler assignment, extra="forbid" on QueryRequest, and a route-name assertion that fails if a bypass endpoint is ever added. test_the_http_transport_exposes_no_bypass_control is the one that will still be doing work in a year.
• Doc sync is complete — SPEC.md, SKILL.md, CHANGELOG.md, security-model.mdx and shell-tools-mixin.mdx all carry the same claims, and the developer set listed in the spec matches DEVELOPER_COMMANDS exactly. test_a_pipe_is_refused_for_its_binary_not_for_the_pipe pinning the correct reason against the issue's wrong one is a nice touch.
• ensure_audit_log_handler extracted from PathValidator._setup_audit_logging rather than a second handler — the audit log stays one file with one rotation policy.

Reproducer (matches _tokenize exactly):

import shlex
lexer = shlex.shlex("ls\nrm -rf /tmp/x", posix=True, punctuation_chars=";&|<>")
lexer.whitespace_split = True
lexer.commenters = ""
print(list(lexer)) # ['ls', 'rm', '-rf', '/tmp/x'] — no separator token

_SEGMENT_SEPARATORS never fires → one segment → _validate_bypass_command("ls") → None (allowed) → executor runs the original string with shell=True → both commands execute.

The first review offered two options:

1. Treat \n as ; before lexing (with a quoted-string caveat).
2. Refuse any multi-line command under bypass with a clear error.

Option 2 is the safe-fail path and avoids touching the lexer — add a check before _tokenize in _validate_shell_command:

if bypass and "\n" in command.strip():
 return (
 {
 "status": "error",
 "error": "Multi-line commands are not accepted under bypass permissions.",
 "has_errors": True,
 },
 [],
 )

A matching test (TestOperatorsUnderBypass or TestPerSegmentWalkSurvivesBypass) should assert that "ls\nrm -rf /tmp/x" is refused under bypass and that the audit for "ls\necho hi" records two segments, not one.