Conversation
![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: a02c347a59
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(stdin_task) = stdin_task { | ||
| match stdin_task.await { | ||
| Ok(Ok(())) => {} | ||
| Ok(Err(err)) => return Err(CodexErr::Io(err)), | ||
| Err(join_err) => return Err(CodexErr::Io(io::Error::other(join_err))), |
There was a problem hiding this comment.
When ExecStdin::Bytes is used with the direct exec() path, commands that stop reading before draining all input (for example head -c 1, parsers that only consume a header, or a child that exits immediately) now come back as I/O errors even if the process itself exits successfully. In that case child_stdin.write_all() returns BrokenPipe, and this block converts it into CodexErr::Io after we've already collected the child's exit status, so partial stdin consumers become unusable through the new API.
Useful? React with 👍 / 👎.
| mut env, | ||
| expiration, | ||
| network, | ||
| stdin: _stdin, |
There was a problem hiding this comment.
build_exec_request() drops the new ExecParams.stdin field here, and SandboxManager::transform() still initializes ExecRequest.stdin to ExecStdin::Closed in core/src/sandboxing/mod.rs. That means any caller that goes through process_exec_tool_call/build_exec_request will always send EOF, so the stdin support added in this commit only works for the direct exec() path covered by the new unit test.
Useful? React with 👍 / 👎.
| if let Some(stdin_bytes) = stdin_bytes { | ||
| write_frame( | ||
| &mut pipe_write, | ||
| &FramedMessage { | ||
| version: 1, | ||
| message: Message::Stdin { | ||
| payload: crate::ipc_framed::StdinPayload { | ||
| data_b64: crate::ipc_framed::encode_bytes(&stdin_bytes), |
There was a problem hiding this comment.
On the elevated Windows backend this sends the entire stdin buffer as one Message::Stdin frame. windows-sandbox-rs/src/elevated/ipc_framed.rs caps a frame at 8 MiB, and base64 expansion means the raw stdin limit is only about 6 MiB. Any larger sandboxed write will therefore fail deterministically on elevated Windows even though the helper itself is streaming stdin to disk.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr14989
branch
4 times, most recently
from
29d2549 to
2e1d275
Compare
1 participant
Why
The first PR introduces a read-only sandbox-backed filesystem helper so
view_imagecan stop reading files in-process. We still need the corresponding write/stdin plumbing so future filesystem operations can go through the same sandbox boundary instead of falling back to host-sidestd::fscalls.What changed
codex-fs-opswith awritecommand and write-path testsExecStdinplumbing throughcore/src/exec.rs,core/src/spawn.rs, andcore/src/sandboxing/mod.rssandboxed_fs::write_file()on top of that shared stdin pathTesting
cargo test -p codex-fs-opscargo test -p codex-core --lib exec::tests::exec_passes_stdin_bytes_to_child -- --exactcargo test -p codex-core --test all view_imageStack created with Sapling. Best reviewed with ReviewStack.