use framed IPC for elevated command runner#14846
Conversation
iceweasel-oai
changed the title
|
@codex review |
![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: 8119d7e847
ℹ️ 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".
| // PIPE_ACCESS_DUPLEX is 0x00000003; not exposed in windows-sys 0.52, so use the value directly. | ||
| const PIPE_ACCESS_DUPLEX: u32 = 0x0000_0003; | ||
| const PIPE_ACCESS_INBOUND: u32 = 0x0000_0001; | ||
| const PIPE_ACCESS_OUTBOUND: u32 = 0x0000_0002; |
There was a problem hiding this comment.
sanity-check: PIPE_ACCESS_DUPLEX is not exposed in windows-sys 0.52, but are these?
There was a problem hiding this comment.
they also are not
| return Err(err.into()); | ||
| } | ||
|
|
||
| let result = (|| -> Result<CaptureResult> { |
There was a problem hiding this comment.
since we're doing a read loop in this block, should it be on its own thread? or are we planning to handle that in a follow-up unified exec PR?
There was a problem hiding this comment.
yep - this is for the non-pty/unified_exec version only
2 participants
Summary
This is PR 2 of the Windows sandbox runner split.
PR 1 introduced the framed IPC runner foundation and related Windows sandbox infrastructure without changing the active elevated one-shot execution path. This PR switches that elevated one-shot path over to the new runner IPC transport and removes the old request-file bootstrap that PR 1 intentionally left in place.
After this change, ordinary elevated Windows sandbox commands still behave as one-shot executions, but they now run as the simple case of the same helper/IPC transport that later unified_exec work will build on.
Why this is needed for unified_exec
Windows elevated sandboxed execution crosses a user boundary: the CLI launches a helper as the sandbox user and has to manage command execution from outside that security context. For one-shot commands, the old request-file/bootstrap flow was sufficient. For unified_exec, it is not.
Unified_exec needs a long-lived bidirectional channel so the parent can:
This PR does not add long-lived sessions yet. It converts the existing elevated one-shot path to use the same framed IPC transport so that PR 3 can add unified_exec session semantics on top of a transport that is already exercised by normal elevated command execution.
Scope
This PR:
windows-sandbox-rs/src/elevated_impl.rsto launch the runner with named pipes, send a framedSpawnRequest, wait forSpawnReady, and collect framedOutput/Exitmessages--request-file=...execution path fromwindows-sandbox-rs/src/elevated/command_runner_win.rsThis PR does not:
Why Windows needs this and Linux/macOS do not
On Linux and macOS, the existing sandbox/process model composes much more directly with long-lived process control. The parent can generally spawn and own the child process (or PTY) directly inside the sandbox model we already use.
Windows elevated sandboxing is different. The parent is not directly managing the sandboxed process in the same way; it launches across a different user/security context. That means long-lived control requires an explicit helper process plus IPC for spawn, output, exit, and later stdin/session control.
So the extra machinery here is not because unified_exec is conceptually different on Windows. It is because the elevated Windows sandbox boundary requires a helper-mediated transport to support it cleanly.
Validation
cargo test -p codex-windows-sandbox