Move unified-exec sandbox launch to exec-server#16736
Move unified-exec sandbox launch to exec-server#16736starr-openai wants to merge 28 commits intomainfrom
Conversation
![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: 13c8fde3cf
ℹ️ 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".
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
13c8fde to
f5d15d0
Compare
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
f5d15d0 to
da8e023
Compare
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
- Reduce complexity in codex-sandboxing if possible
- Use codex_arg0 to figure out the linux sandbox path
- Use single code path in unified exec, fail when using zsh/additional permissions with remote.
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
ce6f444 to
0695fe1
Compare
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
c02a60d to
c5942d9
Compare
Use /bin/sh for the PTY/stdin and write-denial probes so the tests do not depend on /usr/bin/python3 being usable on macOS Bazel runners. Co-authored-by: Codex <noreply@openai.com>
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
5681137 to
0e02bcc
Compare
| run_main_with_listen_url(DEFAULT_LISTEN_URL).await | ||
| } | ||
|
|
||
| pub async fn run_main_with_listen_url( |
There was a problem hiding this comment.
Done in 8cf18dc. I removed the unused run_main_with_listen_url() wrapper/export, updated run_main() to call run_main_with_runtime(DEFAULT_LISTEN_URL, ...) directly, and fixed the README export list to point at run_main_with_runtime().
There was a problem hiding this comment.
doesn't actually look like it's removed
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
0e02bcc to
8cf18dc
Compare
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
8cf18dc to
66b06cc
Compare
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
66b06cc to
c2596cc
Compare
Prepare sandbox env inside the sandbox transform result so core and exec-server do not have to mutate the request at their spawn boundaries. Co-authored-by: Codex <noreply@openai.com>
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
c2596cc to
61500ee
Compare
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
My main q is whether we can maintain a single execution path like we do today (even if some features like network don't work over the remote executor).
Then there are also some non obvious changes to sandboxing logic like moving where env vars are set and how the preference is used.
|
Responding to the latest review summary: I pushed 61500ee to make the sandboxing pieces less surprising. Normal core/exec-server launches now pass On the single execution path question: this PR now shares the sandbox transform path via |
Co-authored-by: Codex <noreply@openai.com>
starr-openai
force-pushed
the
exec-server-sandbox-0403
branch
from
8ddfba8 to
59cf717
Compare
Co-authored-by: Codex <noreply@openai.com>
| // Safety: [`run_main`] never returns. | ||
| codex_linux_sandbox::run_main(); | ||
| } else if exe_name == APPLY_PATCH_ARG0 || exe_name == MISSPELLED_APPLY_PATCH_ARG0 { | ||
| codex_linux_sandbox::dispatch_if_requested(); |
There was a problem hiding this comment.
why did this have to change?
There was a problem hiding this comment.
this doesn't strictly need to change - the main thing was to pull codex_linux_sandbox::dispatch_if_requested() so we can call that from the exec-server main, and thi sjust re-uses the same function.
There was a problem hiding this comment.
why can't we use the entire arg0_dispatch_or_else as every other entrypoint does?
There was a problem hiding this comment.
I think we need to split exec-server bin and lib to avoid the cicrular dependency. And use the existing pattern.
| .turn | ||
| .environment | ||
| .as_ref() | ||
| .filter(|environment| environment.exec_server_url().is_some()) |
There was a problem hiding this comment.
we already have this fork in
I don't think we should add another fork and another entry point to remote exec
| } | ||
|
|
||
| impl<'a> SandboxAttempt<'a> { | ||
| pub(crate) fn sandbox_preference(&self) -> SandboxablePreference { |
There was a problem hiding this comment.
same comment as before, we select sandbox type based on preference, feels a bit sus to do the reverse. Are you sure having htis method makes sense?
There was a problem hiding this comment.
Should SandboxAttempt store preference directly instead?
| env: Default::default(), | ||
| tty: false, | ||
| arg0: None, | ||
| sandbox: SandboxLaunchConfig::no_sandbox( |
There was a problem hiding this comment.
do you agree with codex that special SandboxLaunchConfig is better than Option<SandboxLaunchConfig> pattern that we used for Fs abstraction?
| } | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| pub fn dispatch_if_requested() { |
There was a problem hiding this comment.
we should break the circular dependency in a separte pr and keep arg0/sandboxing code as is.
| pub windows_sandbox_level: WindowsSandboxLevel, | ||
| pub windows_sandbox_private_desktop: bool, | ||
| impl SandboxExecRequest { | ||
| pub fn prepare_env_for_spawn(&mut self) { |
There was a problem hiding this comment.
What is the reason this had to move?
3 participants
Summary
SandboxLaunchConfigthat can be serialized across the exec-server protocolcodex-exec-serverValidation
just fmtcodex-applied-devbox:cargo test -p codex-exec-servercodex-applied-devbox:cargo test -p codex-sandboxingcodex-applied-devbox:cargo test -p codex-core unified_execsandbox=None->SandboxType::None,autoselectsLinuxSeccompwhen policy requires it, andrequireforcesLinuxSeccompacross env boundary (restrictive auto case in Docker still depends on host kernel userns support)