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

Verdict: Request changes

This gives shell commands a real session — cd, exported variables and virtualenv activation now survive to the next call — and the design, docs and tests are unusually thorough. But it also changes how commands run on Linux and macOS, and that change opens a hole the old code didn't have.

🔒 SECURITY CONCERN: the read-only command whitelist can now be walked past. Before this PR, a command on POSIX was split into arguments and handed straight to the OS, so shell punctuation inside it was inert. Now the command text is written to a script and run by a shell. The guardrail that blocks shell operators only looks for ;, &&, |, backticks and redirection — it has no rule for a newline, and the argument splitter silently folds newlines into ordinary words. So a command that reads as one allowed, read-only command to every check runs as several commands once the shell sees it, and the extra ones never went past the whitelist at all. I confirmed the shell executes both halves. Fix: reject control characters (newline and carriage return) in the command text, in both the validation path and the pre-authorisation check that lets a skill-granted binary skip the confirmation modal.

Second, related: the new set_shell_variable refuses PATH, PYTHONPATH, LD_* and friends, but that deny-list is name-based and incomplete. Several other standard variables make an already-whitelisted command run a program of the caller's choosing — git's external-diff and ssh-command settings are the clearest, and the pager/LESS* and PERL5OPT/RUBYLIB families behave the same way. An allow-list of settable names would close the category instead of one name at a time.

@kovtcharov-amd — please take a look at both before this merges.

Nothing else is blocking. Smaller items (a missing tool in a docstring, a timeout that can be waited twice, two doc surfaces that still list the old tool set, and cleanup that leans on __del__) are in the details below.

Real-world evidence

The automated evidence stage failed to run — evidence-bundle.md says it errored before exercising anything (auth / install / rate-limit; see the run logs). The changed surface was not exercised by CI, and the PR description's test plan is all unchecked.

I could not run the test suite in this environment either (python-dotenv is not installed here, so import gaia fails). What I did do is load shell_session.py directly and run a command through it; that is what confirmed the security finding above. Everything else in this review is static.

Before merge, please tick the three test-plan items and paste the real run_shell_command tool output for the cd-then-next-call case — that's the CLI/tool surface this PR exists to change.

 if any(ch in command for ch in ("\n", "\r", "0円")):
 return (
 {
 "status": "error",
 "error": "A command may not span multiple lines.",
 "has_errors": True,
 "hint": "Run one command per call.",
 },
 [],
 )
 if DANGEROUS_SHELL_OPERATORS.search(_operator_check_text(command)):

 - get_shell_state: Read 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 the state it started in

Verdict: Approve with suggestions

The design is careful and the implementation matches the stated intent: state is captured via a side file rather than a resident shell, which is the right call for correctness under timeouts. Security model is preserved, docs are updated, and the tests run real subprocesses — exactly right for a feature where mocking would prove nothing.

Three small items worth fixing before merge:

🟢 run_argv reports the wrong cwd when working_directory is given. A skill-granted binary run with an explicit working_directory executes in that directory, but ShellSession.run_argv always sets result.cwd = self._state.cwd (the session's directory, not the override). The response the model reads back says the command ran in the session directory, not where it actually ran. The fix is one line: set result.cwd to working_directory when that argument is provided.

🟢 test_a_timed_out_command_leaves_no_running_child adds 6+ seconds of mandatory sleep to the unit suite. The wait is real and necessary — a child that survived a kill won't announce itself immediately — but the unconditional time.sleep(6) applies to every run, including fast CI boxes where the process group kill is nearly instantaneous. Consider polling with a short interval (e.g., every 0.25 s, up to 8 s) so the common path finishes in under a second while the slow path still catches survivors.

🟢 RUBYLIB is not in _UNSETTABLE_ENV_NAMES but RUBYOPT is. Both control Ruby's load path and can redirect which Ruby code runs. One-line addition to the set.

# run_argv, line ~667
result.cwd = self._state.cwd # always session cwd

result.cwd = working_directory if working_directory else self._state.cwd

# current
time.sleep(6)
assert not marker.exists()
# alternative: poll up to 8 s, pass instantly when the child is already gone
deadline = time.monotonic() + 8
while time.monotonic() < deadline and not marker.exists():
 time.sleep(0.25)
assert not marker.exists(), "a child outlived the timeout and kept running"

_UNSETTABLE_ENV_NAMES = frozenset({
 ...
 "RUBYOPT",
+ "RUBYLIB", # also controls which Ruby files load
 ...
})

🔴 The newline-injection bypass flagged in the first review is still open. The prior review showed that embedding a \n inside a command string lets an attacker run an arbitrary second command past every whitelist check — DANGEROUS_SHELL_OPERATORS matches ; and && but not a newline, and shlex.split folds the newline into ordinary whitespace so _split_pipeline only ever inspects the first segment. Now that every command goes through a real shell (shell_session.py), the second line executes. This push did not add the control-character check; the vulnerability is unchanged.

The fix requested in the first review is still needed — reject \n, \r, and 0円 at the top of _validate_shell_command (before the operator check), and add the same guard to skill_grant_covers_call where it has its own copy of the operator search.

@kovtcharov-amd — the first review's security section still applies; nothing here closes it.

if any(ch in command for ch in ("\n", "\r", "0円")):
 return (
 {
 "status": "error",
 "error": "A command may not span multiple lines.",
 "has_errors": True,
 "hint": "Run one command per call.",
 },
 [],
 )

🔴 The newline-bypass security hole flagged in the first review is still open.

The first review showed that a command containing a literal newline character passes DANGEROUS_SHELL_OPERATORS (which lists ;, &&, |, etc., but not \n or \r), because shlex.split folds the newline into whitespace so the validator sees one segment — but session.run writes the original string to a file that /bin/sh sources, and the shell splits on newlines. echo hello\nid -un runs both halves; the second never went past the whitelist. skill_grant_covers_call has the same gap and would pre-authorise a folded command.

Neither _validate_shell_command nor skill_grant_covers_call has been updated to reject \n, \r, or 0円 in this push. The fix is one guard at the top of _validate_shell_command (and a matching one in skill_grant_covers_call).

@kovtcharov-amd — this remains unresolved since the first review.

if any(ch in command for ch in ("\n", "\r", "0円")):
 return (
 {
 "status": "error",
 "error": "A command may not span multiple lines.",
 "has_errors": True,
 "hint": "Run one command per call.",
 },
 [],
 )

if any(ch in command for ch in ("\n", "\r", "0円")):
 return False

🔴 The newline injection hole flagged in the first review has not been fixed.

_validate_shell_command still checks only for ;, &&, | etc. — \n and \r are not in DANGEROUS_SHELL_OPERATORS, and shlex.split() folds them to whitespace, so the validator sees one command while the shell script sees two. Since commands now run through a shell script (the session writes them to a file and sources it), this is exploitable today: a command string of echo ok\nid -un passes every check and then runs id -un from the shell — no approval prompt, no whitelist entry.

The same gap is in skill_grant_covers_call: it calls DANGEROUS_SHELL_OPERATORS.search(command) with no newline guard, so a skill-granted binary invocation can fold in a second command that bypasses the no-modal path entirely.

@kovtcharov-amd — the fix was spelled out in the prior review but didn't land.

if any(ch in command for ch in ("\n", "\r", "0円")):
 return (
 {
 "status": "error",
 "error": "A command may not span multiple lines.",
 "has_errors": True,
 "hint": "Run one command per call.",
 },
 [],
 )