chore: sanbox extraction #4286
chore: sanbox extraction #4286
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ 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 |
|
This PR can use some integration tests for areas that are being touched. |
| Err(e) => Err(FunctionCallError::RespondToModel(format!( | ||
| "execution error: {e:?}" | ||
| Err(ExecError::Function(err)) => Err(err), | ||
| Err(ExecError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output }))) => Err( |
There was a problem hiding this comment.
Is it worth having more explicit members on ExecError ?
There was a problem hiding this comment.
I kind of like this error nesting schema. We could collapse it but it would add redundance and it makes it easy to keep the structure. wdyt ?
There was a problem hiding this comment.
Long nested error make it hard to know which meaningful scenarios there is to handle.
In addition we're leaking internals of executor here. All that consumer of executor needs to know is whether a timeout error happened or not. Today we force them to understand the executor-internal CodexErr::Sandbox + SandboxErr::Timeout
|
|
||
| #[derive(Clone, Debug)] | ||
| pub(crate) struct ExecutorConfig { | ||
| pub(crate) sandbox_policy: SandboxPolicy, |
There was a problem hiding this comment.
should approval_policy be here as well?
There was a problem hiding this comment.
It can change per turn. I think it would make things more complicate
There was a problem hiding this comment.
So does sandbox policy
codex-rs/core/src/codex.rs
Outdated
| stdout_stream, | ||
| }; | ||
|
|
||
| sess.executor.update_environment( |
There was a problem hiding this comment.
feels a bit risky to do this inline, in theory we shouldn't need this.
There was a problem hiding this comment.
Risky to do this inline ? I'm not sure I understand this point
|
|
||
| match decision { | ||
| ReviewDecision::Approved => Ok(SandboxDecision::user_override(false)), | ||
| ReviewDecision::ApprovedForSession => Ok(SandboxDecision::user_override(true)), |
There was a problem hiding this comment.
Any reason not to record approval right here?
There was a problem hiding this comment.
Not really, both are equivalent IMO. Ownership of the mutability of the approval_cache is slightly better defined with the current implementation IMO
# Conflicts: # codex-rs/app-server/tests/suite/codex_message_processor_flow.rs
Extract and Centralize Sandboxing