[codex] Add unsandboxed process exec API#19040
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a269ff272
ℹ️ 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".
9a269ff to
2654ece
Compare
cf963c7 to
efd8792
Compare
32f8f49 to
15e265b
Compare
| Stderr, | ||
| } | ||
|
|
||
| /// PTY size in character cells for `process/spawn` PTY sessions. |
There was a problem hiding this comment.
Incidentally, we should probably start to break up this file...
If we need the content defined in this file, we could use #[path = "process.rs"] though ideally we would just use the more standard mod process?
There was a problem hiding this comment.
agreed... let's punt to a separate PR that mechanically splits up v2.rs?
| fn terminal_size_from_protocol( | ||
| size: ProcessTerminalSize, | ||
| ) -> Result<TerminalSize, JSONRPCErrorError> { | ||
| if size.rows == 0 || size.cols == 0 { |
There was a problem hiding this comment.
Consider std::num::NonZero?
There was a problem hiding this comment.
IMO let's keep as is - so that it's a simple number in the API schema but we validate on the server
| if size.is_some() && !tty { | ||
| return Err(invalid_params("process/spawn size requires tty: true")); | ||
| } | ||
| let mut env = std::env::vars().collect::<HashMap<_, _>>(); |
There was a problem hiding this comment.
Does this build on std::env? The caller shouldn't have to know the PATH on the app server host, right?
Why
App-server clients sometimes need argv-based local process execution while sandbox policy is controlled outside Codex. Those environments can reject sandbox-disabling paths before a command ever starts, even when the caller intentionally wants unsandboxed execution.
This PR adds a distinct
process/*API for that use case instead of extendingcommand/execwith another sandbox-disabling shape. Keeping the new surface separate also makes the future removal ofcommand/execsimpler: clients that need explicit process lifecycle control can move to the newer handle-based API without depending oncommand/execbusiness logic.What changed
process/spawn,process/writeStdin,process/resizePty, andprocess/kill.process/outputDeltafor streamed stdout/stderr chunks andprocess/exitedfor final exit status and buffered output.process/spawnintentionally unsandboxed and omitted sandbox-selection fields such assandboxPolicyandpermissionProfile.processHandlevalues for follow-up control requests and notification routing.request_processors/app-server layout, with process-handle request serialization for follow-up control calls.codex-rs/app-server/README.md.codex-rs/app-server/tests/suite/v2/process_exec.rsfor spawn acknowledgement before exit, buffered output caps, and process termination.Verification
cargo test -p codex-app-server-protocolcargo test -p codex-app-server