use long-lived sessions for codex sandbox windows#18953
Conversation
| ) | ||
| } | ||
| }) | ||
| run_command_under_windows_session( |
There was a problem hiding this comment.
this is just pulling the windows part into its own function because it's kinda big
![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: faa44d3aca
ℹ️ 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".
| spawn_windows_sandbox_session_elevated( | ||
| policy_str.as_str(), | ||
| sandbox_policy_cwd.as_path(), | ||
| config.codex_home.as_path(), | ||
| command, | ||
| cwd.as_path(), | ||
| env, | ||
| None, | ||
| /*tty*/ false, | ||
| /*stdin_open*/ true, | ||
| config.permissions.windows_sandbox_private_desktop, | ||
| ) | ||
| .await | ||
| } else { | ||
| spawn_windows_sandbox_session_legacy( |
There was a problem hiding this comment.
mostly these spawn_windows_sandbox_session_* changes are the key part. They use the same backend as unified_exec will use instead of the previous "one and done" spawn approach
| }; | ||
|
|
||
| stdin_close_task.abort(); | ||
| let _ = tokio::time::timeout(output_drain_timeout, async { |
There was a problem hiding this comment.
this part is just waiting for stdout/stderr from the child process to drain after we receive its exit code
| add_git_safe_directory, | ||
| )?; | ||
| if should_apply_network_block(&common.policy) { | ||
| apply_no_network_to_env(env_map)?; |
There was a problem hiding this comment.
apply the "no network" changes to the env here, only in the legacy function, not the elevated one
aaronl-openai
left a comment
aaronl-openai
left a comment
There was a problem hiding this comment.
thank you!
when I have my windows machine tomorrow I'll test this against my changes to make sure everything works together
2 participants
codex sandbox windowspreviously did a one-shot spawn for all commands.This change uses the
unified_execsession to spawn long-lived processes instead, and implements a simple bridge to forward stdin to the spawned session and stdout/stderr from the spawned session back to the caller.It also fixes a bug with the new shared spawn context code where the "no-network env" was being applied to both elevated and unelevated sandbox spawns. It should only be applied for the unelevated sandbox because the elevated one uses firewall rules instead of an env-based network suppression strategy.