Run exec-server fs operations through sandbox helper#17294
Run exec-server fs operations through sandbox helper#17294starr-openai merged 23 commits intomainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41f0e9c405
ℹ️ 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".
Discover exec-server arg0 helper paths from PATH for the default public server entrypoint, preserving sandboxed filesystem support when callers do not pass runtime paths explicitly. Co-authored-by: Codex <noreply@openai.com>
| pub codex_self_exe: Option<PathBuf>, | ||
| /// Path to the `codex-fs` helper alias used by exec-server filesystem | ||
| /// sandboxing. | ||
| pub codex_fs_exe: Option<PathBuf>, |
There was a problem hiding this comment.
isn't it the same as codex_self_exe ?
|
|
||
| #[cfg(windows)] | ||
| { | ||
| let arg1 = if *filename == CODEX_FS_HELPER_ARG0 { |
There was a problem hiding this comment.
this seems wrong. we are not writing scripts for what's being invoked
we are writing scripts that allow us to invoke apply_patch
I'm also not sure we need a script for fs-helper at all.
| codex_fs_exe: { | ||
| #[cfg(windows)] | ||
| { | ||
| Some(path.join(format!("{CODEX_FS_HELPER_ARG0}.bat"))) |
There was a problem hiding this comment.
we don't need for helper to be invocable via shell, we can create the correct command line directly. can we avoid the entire bat business?
| // Allow a bit more time to accommodate async startup work (e.g. config IO, tool discovery) | ||
| let ev = timeout(wait_time.max(Duration::from_secs(10)), codex.next_event()) | ||
| // Allow extra time to accommodate async startup work (e.g. config IO, | ||
| // tool discovery). Windows CI can take longer to launch helper |
There was a problem hiding this comment.
this probably shouldn't be done in this pr.
| Some(sandbox_policy) => { | ||
| self.run_sandboxed( | ||
| sandbox_policy, | ||
| enforce_read_access(¶ms.path, Some(sandbox_policy)), |
There was a problem hiding this comment.
why do we still have enforce_read_access?
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(tag = "operation", rename_all = "camelCase")] | ||
| pub(crate) enum FsHelperRequest { |
There was a problem hiding this comment.
can we reuse the same requests and responses we use for trait?
There was a problem hiding this comment.
FsWriteFileParams/FsWriteFileResponse
| sandbox_policy, | ||
| enforce_read_access(¶ms.path, Some(sandbox_policy)), | ||
| FsHelperRequest::ReadFile { path: params.path }, | ||
| "readFile", |
There was a problem hiding this comment.
can we reuse op names?
| message.contains("is not permitted") | ||
| || message.contains("Operation not permitted") | ||
| || message.contains("Permission denied") | ||
| || message.contains("No such file"), |
There was a problem hiding this comment.
no such file is a bit sus
|
|
||
| use crate::ExecServerRuntimePaths; | ||
|
|
||
| pub async fn run_main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> { |
There was a problem hiding this comment.
we should probably drop all overloads that don't take runtime_paths to ensure they are always passed in correctly.
| } | ||
| } | ||
|
|
||
| fn find_program_in_path(path_env: Option<&OsString>, program: &str) -> Option<PathBuf> { |
There was a problem hiding this comment.
this is wayy to much reimplementation that we don't do for any other commands.
| } | ||
|
|
||
| fn enforce_read_access( | ||
| pub(crate) fn enforce_read_access( |
|
|
||
| pub(crate) async fn run( | ||
| &self, | ||
| sandbox_policy: &SandboxPolicy, |
There was a problem hiding this comment.
is SandboxPolicy enough? for apply patch we include a bunch of other permission
codex/codex-rs/core/src/tools/handlers/apply_patch.rs
Lines 203 to 213 in 04fc208
| &helper_sandbox_policy, | ||
| cwd.as_path(), | ||
| ); | ||
| let network_policy = NetworkSandboxPolicy::from(&helper_sandbox_policy); |
There was a problem hiding this comment.
network is always disabled
| } | ||
|
|
||
| fn helper_program(&self) -> Result<PathBuf, JSONRPCErrorError> { | ||
| let helper = self.runtime_paths.codex_fs_exe.clone().ok_or_else(|| { |
There was a problem hiding this comment.
can codex_fs_exe be non optional?
| .to_string(), | ||
| ) | ||
| })?; | ||
| if !helper.is_absolute() { |
There was a problem hiding this comment.
and use absolute path type?
| Ok(helper) | ||
| } | ||
|
|
||
| fn helper_read_permissions(&self, helper_path: &Path) -> PermissionProfile { |
There was a problem hiding this comment.
this logic must exist somwhere already, otherwise how would apply_patch work today?
| file_system_policy, | ||
| network_policy, | ||
| SandboxablePreference::Auto, | ||
| WindowsSandboxLevel::Disabled, |
pakrym-oai
left a comment
There was a problem hiding this comment.
Directionally correct. Few things:
- Can we reuse more of existing request/response types/ops
- Remove "manual" sandboxing
- Reduce amount of custom sandbox configuration logic and check whether we need more parameters to fully migrate something like apply_patch
- Remove .bat support and ability to start fs-helper directly via shell.
|
Updated this PR to Focused devbox validation passing:
I am digging into the remaining unfiltered |
|
Follow-up validation: the unfiltered core aggregate passes on the devbox when Bazel receives the correct test environment. Passing command: cd /tmp/codex-worktrees/exec-server-fs-sandbox-arg0-20260409/codex-rs && \
export PATH=$HOME/code/openai/project/dotslash-gen/bin:$HOME/.local/bin:$PATH && \
bazel test --bes_backend= --bes_results_url= --test_output=errors \
--test_env=CODEX_JS_REPL_NODE_PATH=/home/dev-user/.nvm/versions/node/v25.9.0/bin/node \
--test_env=PATH \
//codex-rs/core:core-all-test --test_arg=--test-threads=1Result: The earlier broad failure was caused by Bazel test actions resolving |
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(tag = "operation", content = "params")] | ||
| pub(crate) enum FsHelperRequest { | ||
| #[serde(rename = "fs/readFile")] |
There was a problem hiding this comment.
super nit: reuse consts?
| .map_or_else(|| self.cwd.clone(), |path| self.cwd.join(path)) | ||
| } | ||
|
|
||
| pub(crate) fn file_system_sandbox_context( |
There was a problem hiding this comment.
codex/codex-rs/core/src/tools/handlers/apply_patch.rs
Lines 95 to 96 in 04fc208
ok to punt until we convert the main apply patch path
| "view_image is unavailable in this session".to_string(), | ||
| )); | ||
| }; | ||
| let sandbox = environment |
There was a problem hiding this comment.
why only for remote? local too, right?
There was a problem hiding this comment.
eh, let's keep it remote only so we don't break existing usage.
There was a problem hiding this comment.
eh, let's keep it remote only so we don't break existing usage.
| &self.runtime_paths.codex_self_exe | ||
| } | ||
|
|
||
| fn helper_permissions( |
There was a problem hiding this comment.
why doesn't apply_patch need to do this?
| .map_err(|err| invalid_request(format!("failed to prepare fs sandbox: {err}"))) | ||
| } | ||
|
|
||
| fn helper_program(&self) -> &AbsolutePathBuf { |
Discover exec-server arg0 helper paths from PATH for the default public server entrypoint, preserving sandboxed filesystem support when callers do not pass runtime paths explicitly. Co-authored-by: Codex <noreply@openai.com>
2a0e0ff to
d0f6dce
Compare
96279d4 to
1a252f0
Compare
Route sandboxed exec-server filesystem operations through the codex-fs arg0 helper over stdin/stdout, while keeping direct local execution for policies that do not require platform sandboxing. Co-authored-by: Codex <noreply@openai.com>
Discover exec-server arg0 helper paths from PATH for the default public server entrypoint, preserving sandboxed filesystem support when callers do not pass runtime paths explicitly. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Windows CI can spend more than ten seconds launching helper processes inside the large core aggregate test binary, which makes apply-patch harness tests time out while the turn is still running. Keep the existing non-Windows floor and only raise the Windows floor. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Collapse ExecutorFileSystem onto one optional-sandbox method per filesystem operation and update unsandboxed callers to pass an explicit None. Also replace helper response into_* demux helpers with expect_* variants that report the unexpected operation directly. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
1a252f0 to
1059905
Compare
Co-authored-by: Codex <noreply@openai.com>
1059905 to
707c7cc
Compare
Strip the implicit cwd grant from request-side filesystem preflight so absolute fs API calls cannot escape explicit readable roots only because Bazel/TMPDIR place test tempdirs under the process cwd. Add regression coverage for the cwd/TMPDIR overlap case that previously returned NotFound instead of an upfront sandbox denial. Co-authored-by: Codex <noreply@openai.com>
Back out the request-side preflight split until the failing absolute-path regression is reworked around the actual AbsolutePathBuf semantics. Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Summary
codex-fsarg0 helper over stdin/stdoutDangerFullAccessand external sandbox policiesValidation
just fmtgit diff --checkcd codex-rs && bazel test --bes_backend= --bes_results_url= //codex-rs/exec-server:all(6/6 passed)