Add Windows sandbox unified exec runtime support by iceweasel-oai · Pull Request #15578 · openai/codex

iceweasel-oai · 2026-03-23T23:09:41Z

Summary

This is the runtime/foundation half of the Windows sandbox unified-exec work.

add Windows sandbox unified_exec session support in windows-sandbox-rs for both:
- the legacy restricted-token backend
- the elevated runner backend
extend the PTY/process runtime so driver-backed sessions can support:
- stdin streaming
- stdout/stderr separation
- exit propagation
- PTY resize hooks
add Windows sandbox runtime coverage in codex-windows-sandbox / codex-utils-pty

This PR does not enable Windows sandbox UnifiedExec for product callers yet because hooking this up to app-server comes in the next PR.

Windows sandbox advertising is intentionally kept aligned with main, so sandboxed Windows callers still fall back to ShellCommand.

This PR isolates the runtime/session layer so it can be reviewed independently from product-surface enablement.

iceweasel-oai · 2026-03-23T23:43:52Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 969be7c0ad

ℹ️ 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".

iceweasel-oai · 2026-03-24T17:07:54Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 48bab9be43

ℹ️ 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".

iceweasel-oai · 2026-03-24T17:43:46Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d486e3c5c

ℹ️ 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".

iceweasel-oai · 2026-03-24T18:20:27Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd873f727d

ℹ️ 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".

chatgpt-codex-connector · 2026-03-24T18:30:57Z

+
+    if !guards.is_empty() {
+        if let Some(cap_sid) = cap_sid {
+            if let Some(sid) = unsafe { convert_string_sid_to_sid(&cap_sid) } {


Free SID returned by ConvertStringSidToSidW

convert_string_sid_to_sid allocates SID memory that must be released with LocalFree (see token.rs). In finalize_exit, the SID created for ACE revocation is never freed, so every read-only session leaks heap memory. Long-lived agents that open many sessions will accumulate unreclaimed allocations.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-03-24T18:30:57Z

+    ) = unsafe {
+        match policy {
+            SandboxPolicy::ReadOnly { .. } => {
+                let psid = convert_string_sid_to_sid(&caps.readonly).unwrap();


Release legacy session SID pointers after use

prepare_legacy_session_security stores raw SID pointers from convert_string_sid_to_sid in LegacySessionSecurity, but there is no corresponding LocalFree on success or cleanup paths. Each spawned legacy session leaks one or more SID allocations, causing unbounded memory growth over many sessions.

Useful? React with 👍 / 👎.

jif-oai

I think we transfer a bit too much of the windows specificities from the pty lib to the unified_exec implementation. IMO this will reduce maintainability over time

jif-oai · 2026-03-26T16:29:26Z

    ) -> Result<UnifiedExecProcess, UnifiedExecError> {
        let (program, args) = env
            .command
            .split_first()
            .ok_or(UnifiedExecError::MissingCommandLine)?;
        let inherited_fds = spawn_lifecycle.inherited_fds();

+        #[cfg(target_os = "windows")]
+        if env.sandbox == crate::exec::SandboxType::WindowsRestrictedToken {


this seems a bit odd to only load, and potentially error, at the call site of the tool
This should have been done upfront

jif-oai · 2026-03-26T16:42:32Z

+        stdin_open,
+        use_private_desktop,
+    };
+    send_spawn_request(&mut pipe_write, spawn_request)?;


We should bound the spawn_ready handshake. We don't want infinite hang in case of any handshake issue

jif-oai · 2026-03-26T16:44:15Z

+    let user_w = to_wide(&sandbox_creds.username);
+    let domain_w = to_wide(".");
+    let password_w = to_wide(&sandbox_creds.password);
+    let _ = unsafe { SetErrorMode(0x0001 | 0x0002) };


Maybe I'm wrong but shouldn't we revert this later?

jif-oai

I think some previous comments have been addressed yet

Global, I'm a bit scared by the stability of this code and this PR is becoming gigantic. How do you plan to test it deeply before shipping this?

jif-oai · 2026-04-01T13:21:40Z

+    ) = unsafe {
+        match policy {
+            SandboxPolicy::ReadOnly { .. } => {
+                let psid = convert_string_sid_to_sid(&caps.readonly).unwrap();


I think you have a leak here

jif-oai · 2026-04-01T13:21:59Z

+    let caps = load_or_create_cap_sids(codex_home)?;
+    let (psid_to_use, cap_sids) = match &common.policy {
+        SandboxPolicy::ReadOnly { .. } => (
+            unsafe { convert_string_sid_to_sid(&caps.readonly).unwrap() },


I think you have another leak here

jif-oai · 2026-04-01T13:22:34Z

sd is never freed

jif-oai · 2026-04-01T13:25:02Z

+        &command,
+    )?;
+
+    let mut transport = spawn_runner_transport(


this is blocking.. this can stale a tokio worker

jif-oai · 2026-04-01T13:25:34Z

+
+        match msg.message {
+            Message::Output { payload } => {
+                if let Ok(data) = decode_bytes(&payload.data_b64) {


we should log the potential decode issues...

dylan-hurd-oai · 2026-04-01T09:46:48Z

@@ -549,6 +549,59 @@ impl UnifiedExecProcessManager {
            .ok_or(UnifiedExecError::MissingCommandLine)?;
        let inherited_fds = spawn_lifecycle.inherited_fds();

+        #[cfg(target_os = "windows")]
+        if env.sandbox == crate::exec::SandboxType::WindowsRestrictedToken {
+            let policy_json = serde_json::to_string(&env.sandbox_policy).map_err(|err| {


Out of scope for this PR, but would be great to start using PermissionProfile instead of a json serialization here.

dylan-hurd-oai · 2026-04-01T09:51:25Z

@@ -205,6 +226,20 @@ impl Drop for ProcessHandle {
    }
 }

+/// Adapts a closure into a `ChildTerminator` implementation.
+struct ClosureTerminator {


ultra-nit / non-blocking - macro for this?

Co-authored-by: Codex <noreply@openai.com>

jif-oai

I think the happy path looks good but the failure path can still be racy
I approve as this is not for prod yet and it will be easier to review smaller PRs containing fixes and robustification

jif-oai · 2026-04-21T09:45:00Z

+    }
+
+    let connect_result = (|| -> Result<()> {
+        connect_pipe(h_pipe_in)?;


This is still unbounded no?
So if the runner dies before opening one of the named pipe, we will hang forever

jif-oai · 2026-04-21T09:49:30Z

+    })
+}
+
+pub(crate) fn start_runner_stdout_reader(


the runner stdout reader is detached and not owned by ProcessHandle... but the matching pipe writer thread is also detached so terminate() can't reliably reclaim a wedged elevanted session. This will create a leak

jif-oai · 2026-04-21T09:51:29Z

+                loop {
+                    let recv_result = if process_exited {
+                        match tokio::time::timeout(
+                            std::time::Duration::from_millis(200),


From my experience, some process can signal exist before the final flush of their STDOUT. So here if we have an exist before 200ms and the flush comes after, we will loose the tail

In general, those things are extremely racy so please ask Codex multiple time to look and identify every possible races. Process handling is a very difficult job

jif-oai · 2026-04-21T09:52:17Z

+
+            match msg.message {
+                Message::Output { payload } => {
+                    if let Ok(data) = decode_bytes(&payload.data_b64) {


What if not ok ? Are we 100% sure this will never happen?

chatgpt-codex-connector Bot reviewed Mar 23, 2026

View reviewed changes

Comment thread codex-rs/windows-sandbox-rs/src/unified_exec/session.rs Outdated

Comment thread codex-rs/windows-sandbox-rs/src/spawn_prep.rs

Comment thread codex-rs/windows-sandbox-rs/src/winutil.rs Outdated

chatgpt-codex-connector Bot reviewed Mar 24, 2026

View reviewed changes

Comment thread codex-rs/windows-sandbox-rs/src/unified_exec/session.rs Outdated

Comment thread codex-rs/windows-sandbox-rs/src/unified_exec/session.rs Outdated

chatgpt-codex-connector Bot reviewed Mar 24, 2026

View reviewed changes

Comment thread codex-rs/windows-sandbox-rs/src/winutil.rs Outdated

Comment thread codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs Outdated

chatgpt-codex-connector Bot reviewed Mar 24, 2026

View reviewed changes

iceweasel-oai marked this pull request as ready for review March 24, 2026 18:41

jif-oai reviewed Mar 26, 2026

View reviewed changes

iceweasel-oai requested a review from jif-oai March 31, 2026 20:50

jif-oai reviewed Apr 1, 2026

View reviewed changes

dylan-hurd-oai reviewed Apr 1, 2026

View reviewed changes

iceweasel-oai force-pushed the windows-sandbox-unified-exec-sessions branch 2 times, most recently from 7bc7db8 to 1ec4660 Compare April 10, 2026 20:17

iceweasel-oai added 15 commits April 20, 2026 14:52

Add Windows sandbox unified exec runtime support

b5ad53c

Drop temporary Windows spawn matrix doc

0400378

Fix Windows sandbox session formatting and syntax

46e6236

review fixes

fdcedc2

do not leak desktops

8d425cf

do not prematurely drop desktop handle

0814f67

fix escaping for commands with spaces, and newline boundary issue

fb97fdd

fix compile issues

315f2b1

review comments

9c2805d

review comments

f5f5583

review comments

5dc5903

Refactor Windows sandbox unified exec sessions

dcaad56

centralize sandbox policy/cwd in ExecRequest

797c70e

address review comments.

6255767

Fix Windows sandbox rebase fallout

fd2e48a

iceweasel-oai added 4 commits April 20, 2026 14:53

Fix ExecRequest policy cwd plumbing

96e0eda

Fix Windows sandbox clippy lints

59c02dd

Fix PR3 CI lints

cd660aa

Fix elevated spawn prep after main rebase

8b36fb4

iceweasel-oai force-pushed the windows-sandbox-unified-exec-sessions branch from 1ec4660 to 8b36fb4 Compare April 20, 2026 21:53

iceweasel-oai requested a review from a team as a code owner April 20, 2026 21:53

jif-oai and others added 3 commits April 20, 2026 23:39

Merge remote-tracking branch 'origin/main' into pr-15578

9fe61eb

Fix core test after main merge

5406738

Co-authored-by: Codex <noreply@openai.com>

Merge branch 'main' into windows-sandbox-unified-exec-sessions

93b85b2

jif-oai approved these changes Apr 21, 2026

View reviewed changes

Restore Windows workspace ACL helpers

c82057d

iceweasel-oai merged commit 8612714 into main Apr 21, 2026
24 of 25 checks passed

iceweasel-oai deleted the windows-sandbox-unified-exec-sessions branch April 21, 2026 17:44

github-actions Bot locked and limited conversation to collaborators Apr 21, 2026

Conversation

iceweasel-oai commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

iceweasel-oai commented Mar 23, 2026

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

iceweasel-oai commented Mar 24, 2026

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

iceweasel-oai commented Mar 24, 2026

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

iceweasel-oai commented Mar 24, 2026

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

chatgpt-codex-connector Bot Mar 24, 2026

Choose a reason for hiding this comment

chatgpt-codex-connector Bot Mar 24, 2026

Choose a reason for hiding this comment

jif-oai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jif-oai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jif-oai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Labels

3 participants

iceweasel-oai commented Mar 23, 2026 •

edited

Loading