Repository navigation
Conversation
A foreground isolated run sends its script, user code followed by
`echo <marker> $?`, on the persistent shell's stdin. A command in the
user code that reads stdin (read, cat, any program reading stdin)
consumed the rest of the script instead: the marker line became input,
its text could come back as output with `$?` unparsed so the run
reported exit code 0, and `cat` kept running and echoed the next run's
script as that run's output.
Run the user code with </dev/null, in a `{ }` group for no-envs runs so
exports still persist and in the existing subshell for envs runs, the
same way background runs already detach stdin. The shell now reads the
whole group before running it, so the failed-write test simulates a
shell that stops reading with a closed pipe instead of exec'ing away
mid-script.
github-actions
Bot
added
component/execd
size/S
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 187278e392
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // keep cross-run exports working. Either way the user code reads | ||
| // /dev/null: the script arrives on the shell's stdin, so a command that | ||
| // reads stdin would otherwise consume the end marker below. | ||
| openGroup, closeGroup := "{\n", "} </dev/null\n" |
There was a problem hiding this comment.
With the default Bash shell and envs empty, this brace group is parsed in full before any body command executes. Thus valid input such as shopt -s extglob followed by a case using +(...) now hits a parse error before the option is enabled; Bash exits without the marker, so the run fails and the long-lived session is lost (aliases defined and used later in the same request similarly stop expanding). Preserve top-level incremental parsing while detaching fd 0 rather than wrapping the whole request in one compound command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, thanks. Fixed in c7610cf: the code is no longer wrapped in a { } group. It is passed to eval as one quoted word with </dev/null, and eval parses and runs it one command at a time like top-level input, so shopt -s extglob and aliases still apply to later lines. The end marker stays a separate top-level command, so user code still cannot read it. Added TestRunInIsolatedSession_ParsesCodeIncrementally, which fails with the group. A side effect with bash: a syntax error now ends the run with status 2 instead of killing the session.
| // /dev/null: the script arrives on the shell's stdin, so a command that | ||
| // reads stdin would otherwise consume the end marker below. |
There was a problem hiding this comment.
For every foreground run whose program reads standard input, this redirect now produces immediate EOF, but neither specs/execd-api.yaml's IsolatedRunRequest.code contract nor the Limitations section of docs/guides/isolation-sessions.md says that run stdin is unavailable. Since the request has no stdin field, callers otherwise discover this behavior only at execution time; document the /dev/null contract and the supported explicit-redirection/file alternatives together with this user-visible change.
AGENTS.md reference: AGENTS.md:L45-L46
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, done in c7610cf. IsolatedRunRequest.code in the execd spec now says its stdin is /dev/null and to pass input through a file or a here-document, and the Python and JavaScript SDK descriptions are regenerated. The Limitations section of the isolation guide says the same, for foreground and background runs.
Wrapping the run in a { } group made the shell parse the whole request
before running any of it, so `shopt -s extglob` or an alias defined
early in the code no longer applied to the lines after it. Pass the code
to eval instead, with stdin on /dev/null: eval parses and runs it one
command at a time like top-level input, and user commands still cannot
read the end marker. With bash, a syntax error now ends the run with
status 2 instead of killing the session.
Document the /dev/null stdin in the execd spec and the isolation guide,
and regenerate the IsolatedRunRequest descriptions in the Python and
JavaScript SDKs.
github-actions
Bot
added
documentation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7610cf731
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !strings.HasSuffix(code, "\n") { | ||
| script.WriteString("\n") | ||
| } | ||
| script.WriteString(fmt.Sprintf("eval %s </dev/null\n", shellescape(code))) |
There was a problem hiding this comment.
When a no-envs run defines a function or expanded alias named eval (or runs enable -n eval), that shell state persists, so this unqualified call in the next request resolves to the user's override instead of the builtin; for example, eval() { :; } makes every later request report success without executing its code. The previous top-level injection did not reserve eval, so invoke the builtin through a path that ordinary user functions and aliases cannot shadow and cover this with a two-run regression test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, reproduced with both a function and an alias. Fixed in 4c151e2: the runner now calls \command eval. command skips function lookup and the leading backslash prevents alias expansion, which works the same in bash and dash. State the code sets (functions, cd, variables) still persists across runs. Added TestRunInIsolatedSession_IgnoresEvalOverrides, which leaves eval() { :; } or alias eval=true in one run and checks that the next run still executes and reports its exit code. It fails with plain eval.
The session shell persists between runs, so a function or alias named
eval defined by one run (for example `eval() { :; }`) took over the
runner's eval in every later run, which then reported success without
running its code.
Call `\command eval`: `command` skips function lookup and the backslash
prevents alias expansion, in bash and dash alike. In dash it also keeps
a syntax error in the code from ending the session.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c151e28d2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !strings.HasSuffix(code, "\n") { | ||
| script.WriteString("\n") | ||
| } | ||
| script.WriteString(fmt.Sprintf("\\command eval %s </dev/null\n", shellescape(code))) |
There was a problem hiding this comment.
Unlike the eval override covered by the new test, a no-env run can define command() { :; }, and that function persists into the next request. The backslash here suppresses alias expansion but not function lookup, so both Bash and dash call the user function, never reach eval, and then emit the marker with status 0 even when all submitted code (for example false) was skipped. Protect this dispatch from a user-defined command and add the corresponding two-run regression test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks. I am leaving this one as is, because it is a property of the persistent session rather than of this dispatch. Every word the runner relies on can be redefined as a function by an earlier no-env run: command here, but also builtin, unset, trap, and the echo that prints the end marker. That last one was already true before this PR. On current main, a run that defines echo() { builtin echo "$1" 0; } makes every later run report status 0, false included. Guarding against a deliberate override would take a different protocol, for example having the runner report the status through a descriptor the user code cannot reach. That is a bigger change than this fix. \command eval covers the accidental case, a function or alias that happens to be named eval. The session state belongs to the code that runs in it, so a run that redefines the shell's own commands only affects its own session.
A foreground isolated run writes its whole script to the persistent shell's stdin:
trap : INT, the user code, thenecho <marker> $?. The user code runs with that same stdin. So a command in the code that reads stdin (read,cat,python script.pyreading input, etc.) consumes the rest of the script, including the end-marker line.On current main:
read x; echo "got:$x"; falsereturns success.readtakes the lineecho __ISOLATED_RUN_END___<uuid> $?, the nextechoprints it, the scanner finds the marker in that output, and$?doesn't parse, so the exit code defaults to 0. The output is["got:echo "].catreports success right away whilecatkeeps running, and the next run (echo next) returns["trap : INT", "echo next", "echo "], becausecatechoes that run's script back. The session stays like this until a run is cancelled.This also contradicts the comment that the per-run UUID makes the end marker impossible to forge. Background runs already avoid it by running the script with
</dev/null.Summary
\command evalas one quoted word, with</dev/nullas stdin.\commandreaches the builtin even if an earlier run defined a function or alias namedeval.evalparses and runs the code one command at a time, like top-level input, soshopt -s extglob, aliases and similar settings still apply to the lines after them.( )subshell.TestRunInIsolatedSession_ReportsFailedScriptWriteusedexec sleep 300to replace the shell partway through a 150 KiB script. The shell now reads the wholeevalword before running any of it, so the test gives the run a stdin pipe whose read end is closed, a shell that no longer reads. It still checks that the run reportswrite stdinpromptly instead of waiting for its deadline. The comment on the concurrent write is updated to match./dev/nullstdin is documented onIsolatedRunRequest.codein the execd spec (with the Python and JavaScript SDK descriptions regenerated) and in the Limitations section of the isolation guide.Testing
Not run (explain why)
Unit tests
Integration tests
e2e / manual verification
New
TestRunInIsolatedSession_StdinDetached, with and without envs:read x; echo "got:$x"; falsemust fail with exit code 1 and printgot:, and after acatrun the next run must print exactlynext. Every assertion fails on current main. Dropping the</dev/nullfails it again.New
TestRunInIsolatedSession_IgnoresEvalOverrides: after a run that leaveseval() { :; }oralias eval=true, the next run must still execute and report its exit code. It fails with a plaineval.New
TestRunInIsolatedSession_ParsesCodeIncrementally:shopt -s extglobfollowed by a+(a)pattern must match. It fails if the code is wrapped in a{ }group.Removing the
cancelRun()after a failed write makes the adapted write-failure test wait for its deadline.All execd packages pass (
GOOS=linux go test ./...), including the interrupt tests and-raceon the isolated tests.golangci-lintis clean, andgo vetpasses for darwin and windows.Breaking Changes
Code in a foreground isolated run can no longer read the session's stdin. It only ever got the rest of the run's own script that way, so nothing useful depended on it.
Checklist