Conversation
💡 Codex Reviewcodex/codex-rs/windows-sandbox-rs/src/setup.rs Lines 1086 to 1087 in 8851476  Pass workspace-roots slice to setup refresh helper
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
## Why The Windows sandbox runner still carried the old `SandboxPolicy` compatibility path even though core now computes `PermissionProfile`. That meant Windows command-runner execution could only see the legacy projection, so profile-only filesystem rules such as deny globs were not part of the runner input. ## What Changed - Removed the Windows-local `SandboxPolicy` parser/export and deleted `windows-sandbox-rs/src/policy.rs`. - Changed restricted-token capture/session setup, elevated setup, world-writable audit, read-root grant, and command-runner session APIs to accept `PermissionProfile` plus the profile cwd. - Bumped the elevated command-runner IPC protocol to version 2 because `SpawnRequest` now carries `permission_profile` / `permission_profile_cwd` instead of the legacy `policy_json_or_preset` / `sandbox_policy_cwd` fields. - Updated core exec, unified exec, debug-sandbox, TUI setup/grant flows, and app-server setup to pass the actual effective `PermissionProfile`. - Left regression coverage asserting the old IPC policy fields are absent and the runner serializes tagged `PermissionProfile` JSON. ## Verification - `cargo test -p codex-windows-sandbox` - `cargo test -p codex-core windows_sandbox` - `cargo test -p codex-app-server request_processors::windows_sandbox_processor` - `just fix -p codex-windows-sandbox -p codex-core -p codex-app-server -p codex-cli -p codex-tui` - `just fix -p codex-cli -p codex-tui` - `just fix -p codex-windows-sandbox -p codex-tui` - `rg "\\bSandboxPolicy\\b" codex-rs/windows-sandbox-rs` returned no matches. Note: `cargo test -p codex-cli` was attempted but did not reach crate tests because local disk filled while compiling dependencies (`No space left on device`). The targeted clippy pass compiled the affected CLI/TUI surfaces afterward. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/23813). * #24108 * __->__ #23813
bolinfest
force-pushed
the
pr24108
branch
2 times, most recently
from
7e648d7 to
d3ed08c
Compare
|
Addressed in d3ed08c: the setup refresh test now builds a workspace-roots slice with |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
3 participants
Why
#23813 switches the Windows sandbox runner path to
PermissionProfile, but it still left one runtime anchor for resolving symbolic:workspace_rootsentries. That is not enough once a turn has multiple effective workspace roots: exact entries and deny globs under:workspace_rootsneed to be materialized for every runtime root before the command runner chooses token mode or builds ACL plans.What Changed
permission_profile_cwdplumbing withworkspace_roots: Vec<AbsolutePathBuf>.PermissionProfiledata withmaterialize_project_roots_with_workspace_roots(...)instead of the single-cwd helper.Config::effective_workspace_roots()through core execution, unified exec, TUI setup/read-grant flows, app-server setup, app-servercommand/exec, anddebug sandboxon Windows.sandbox_policy_cwd.ExecRequest::new(...)and the remainingbuild_exec_request(...)helper path takewindows_sandbox_workspace_rootsexplicitly so new call sites cannot silently fall back tovec![cwd].debug sandboxnon-Windows comment: remaining cwd-dependent resolution still usessandbox_policy_cwd, while:workspace_rootsentries are already materialized from config roots.SpawnRequestto sendworkspace_rootsand bumps the framed IPC protocol version to3for the payload shape change.:workspace_rootsentries across multiple roots, plus core helper coverage proving explicit roots are preserved.Verification
cargo check -p codex-windows-sandbox -p codex-core -p codex-tui -p codex-cli -p codex-app-servercargo test -p codex-windows-sandboxcargo test -p codex-core windows_sandboxcargo test -p codex-core unix_escalationcargo test -p codex-app-server windows_sandboxcargo test -p codex-tui windows_sandboxcargo test -p codex-cli debug_sandboxjust test -p codex-core unified_execjust test -p codex-core build_exec_request_preserves_windows_workspace_rootsenv -u CODEX_NETWORK_PROXY_ACTIVE -u CODEX_NETWORK_ALLOW_LOCAL_BINDING just test -p codex-app-server --lib command_execjust test -p codex-windows-sandboxjust test -p codex-exec sandboxjust fix -p codex-core -p codex-app-server -p codex-windows-sandboxA local macOS cross-check with
cargo check --target x86_64-pc-windows-msvc ...did not reach crate Rust code because native dependencies require Windows SDK headers (windows.h/assert.h) in this environment; Windows CI remains the real target validation.Two local targeted filters compile but do not run assertions on macOS:
env -u CODEX_NETWORK_PROXY_ACTIVE -u CODEX_NETWORK_ALLOW_LOCAL_BINDING just test -p codex-app-server --lib command_exec_processormatched zero tests, andjust test -p codex-linux-sandbox landlockmatched zero tests because the landlock suite is Linux-only.