Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3d50256f6
ℹ️ 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".
codex-rs/core/src/sandboxing/mod.rs
Outdated
| env, | ||
| network: network.cloned(), | ||
| expiration: spec.expiration, | ||
| capture_policy: ExecCapturePolicy::ShellTool, |
There was a problem hiding this comment.
Preserve
capture_policy through sandbox transformation
Checked the process_exec_tool_call -> build_exec_request -> SandboxManager::transform path: the new ExecCapturePolicy::FullBuffer is dropped before execution and every transformed ExecRequest is forced back to ShellTool. That means any caller that sets capture_policy: FullBuffer on ExecParams will still get the old truncation/timeout behavior, so the feature added in this patch only works in the direct exec() helper used by the new unit test, not in the normal execution path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d886f18afeb4.
capture_policy now survives the full process_exec_tool_call -> build_exec_request -> SandboxManager::transform path:
ExecParamsthreadscapture_policyintoCommandSpecincore/src/exec.rsCommandSpecandExecRequestboth carrycapture_policyincore/src/sandboxing/mod.rsSandboxManager::transform()now copiesspec.capture_policyinto the transformedExecRequestinstead of forcingShellTool
I also replaced the earlier field-copy style check with process_exec_tool_call_preserves_full_buffer_capture_policy() in core/src/exec_tests.rs, which exercises the normal request-building path and proves FullBuffer still disables exec expiration and output caps after sandbox transformation.
| let stdout = await_output(&mut stdout_handle, capture_policy.io_drain_timeout()).await?; | ||
| let stderr = await_output(&mut stderr_handle, capture_policy.io_drain_timeout()).await?; |
There was a problem hiding this comment.
Keep an I/O drain guard for
FullBuffer execs
In consume_output, FullBuffer passes None here, so the post-exit pipe drain no longer has the timeout that the comment above IO_DRAIN_TIMEOUT_MS says is needed when grandchildren inherit stdout/stderr. In that scenario the direct child can exit successfully while a background child keeps the pipe open, and a FullBuffer caller will hang forever waiting for EOF instead of returning the captured output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d886f18afeb4.
FullBuffer no longer disables the post-exit pipe drain guard: ExecCapturePolicy::io_drain_timeout() now always returns IO_DRAIN_TIMEOUT_MS, while uses_expiration() remains the switch that disables exec expiration for FullBuffer.
I also added a regression test in core/src/exec_tests.rs that runs a FullBuffer exec where a background descendant inherits stdout/stderr and keeps the pipe open after the direct child exits. The test wraps the call in an outer timeout and now proves we return once the drain guard fires instead of hanging forever.
d886f18 to
5626b76
Compare
Why
Upcoming sandbox-backed filesystem helpers (see #14989) need to run through
exec()so the sandbox remains the enforcement boundary. The existing exec path is intentionally tuned for shell-style tool calls: it caps retained output and enforces exec expiration. That behavior is correct forshell, but it is the wrong default for trusted internal helpers that need the full child output buffered in memory.At the same time, those helpers still need the existing post-exit stdout/stderr drain guard. If a child exits after spawning a descendant that inherited its pipes, dropping that guard can hang the caller forever while it waits for EOF.
What Changed
ExecCapturePolicyto the exec request/params flow so output capture semantics are explicit instead of being implied by the call pathExecCapturePolicy::ShellTool, and addExecCapturePolicy::FullBufferfor internal callers that need uncapped buffered output without exec expirationexec()and the Windows sandbox capture path to honor the capture policy when applying retained-byte caps and exec-expiration handlingIO_DRAIN_TIMEOUT_MSguard forFullBufferexecs so inherited stdout/stderr pipes cannot hang the caller forever after child exit