mirror of
https://github.com/openai/codex.git
synced 2026-10-23 22:16:45 +00:00
I was looking at the implementation of `Session::get_writable_roots()`, which did not seem right, as it was a copy of writable roots, which is not guaranteed to be in sync with the `sandbox_policy` field. I looked at who was calling `get_writable_roots()` and its only call site was `apply_patch()` in `codex-rs/core/src/apply_patch.rs`, which took the roots and forwarded them to `assess_patch_safety()` in `safety.rs`. I updated `assess_patch_safety()` to take `sandbox_policy: &SandboxPolicy` instead of `writable_roots: &[PathBuf]` (and replaced `Session::get_writable_roots()` with `Session::get_sandbox_policy()`). Within `safety.rs`, it was fairly easy to update `is_write_patch_constrained_to_writable_paths()` to work with `SandboxPolicy`, and in particular, it is far more accurate because, for better or worse, `SandboxPolicy::get_writable_roots_with_cwd()` _returns an empty vec_ for `SandboxPolicy::DangerFullAccess`, suggesting that _nothing_ is writable when in reality _everything_ is writable. With this PR, `is_write_patch_constrained_to_writable_paths()` now does the right thing for each variant of `SandboxPolicy`. I thought this would be the end of the story, but it turned out that `test_writable_roots_constraint()` in `safety.rs` needed to be updated, as well. In particular, the test was writing to `std::env::current_dir()` instead of a `TempDir`, which I suspect was a holdover from earlier when `SandboxPolicy::WorkspaceWrite` would always make `TMPDIR` writable on macOS, which made it hard to write tests to verify `SandboxPolicy` in `TMPDIR`. Fortunately, we now have `exclude_tmpdir_env_var` as an option on `SandboxPolicy::WorkspaceWrite`, so I was able to update the test to preserve the existing behavior, but to no longer write to `std::env::current_dir()`. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/2338). * #2345 * #2329 * #2343 * #2340 * __->__ #2338
124 lines
4.6 KiB
Rust
124 lines
4.6 KiB
Rust
use crate::codex::Session;
|
|
use crate::models::FunctionCallOutputPayload;
|
|
use crate::models::ResponseInputItem;
|
|
use crate::protocol::FileChange;
|
|
use crate::protocol::ReviewDecision;
|
|
use crate::safety::SafetyCheck;
|
|
use crate::safety::assess_patch_safety;
|
|
use codex_apply_patch::ApplyPatchAction;
|
|
use codex_apply_patch::ApplyPatchFileChange;
|
|
use std::collections::HashMap;
|
|
use std::path::PathBuf;
|
|
|
|
pub const CODEX_APPLY_PATCH_ARG1: &str = "--codex-run-as-apply-patch";
|
|
|
|
pub(crate) enum InternalApplyPatchInvocation {
|
|
/// The `apply_patch` call was handled programmatically, without any sort
|
|
/// of sandbox, because the user explicitly approved it. This is the
|
|
/// result to use with the `shell` function call that contained `apply_patch`.
|
|
Output(ResponseInputItem),
|
|
|
|
/// The `apply_patch` call was approved, either automatically because it
|
|
/// appears that it should be allowed based on the user's sandbox policy
|
|
/// *or* because the user explicitly approved it. In either case, we use
|
|
/// exec with [`CODEX_APPLY_PATCH_ARG1`] to realize the `apply_patch` call,
|
|
/// but [`ApplyPatchExec::auto_approved`] is used to determine the sandbox
|
|
/// used with the `exec()`.
|
|
DelegateToExec(ApplyPatchExec),
|
|
}
|
|
|
|
pub(crate) struct ApplyPatchExec {
|
|
pub(crate) action: ApplyPatchAction,
|
|
pub(crate) user_explicitly_approved_this_action: bool,
|
|
}
|
|
|
|
impl From<ResponseInputItem> for InternalApplyPatchInvocation {
|
|
fn from(item: ResponseInputItem) -> Self {
|
|
InternalApplyPatchInvocation::Output(item)
|
|
}
|
|
}
|
|
|
|
pub(crate) async fn apply_patch(
|
|
sess: &Session,
|
|
sub_id: &str,
|
|
call_id: &str,
|
|
action: ApplyPatchAction,
|
|
) -> InternalApplyPatchInvocation {
|
|
match assess_patch_safety(
|
|
&action,
|
|
sess.get_approval_policy(),
|
|
sess.get_sandbox_policy(),
|
|
sess.get_cwd(),
|
|
) {
|
|
SafetyCheck::AutoApprove { .. } => {
|
|
InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
|
|
action,
|
|
user_explicitly_approved_this_action: false,
|
|
})
|
|
}
|
|
SafetyCheck::AskUser => {
|
|
// Compute a readable summary of path changes to include in the
|
|
// approval request so the user can make an informed decision.
|
|
//
|
|
// Note that it might be worth expanding this approval request to
|
|
// give the user the option to expand the set of writable roots so
|
|
// that similar patches can be auto-approved in the future during
|
|
// this session.
|
|
let rx_approve = sess
|
|
.request_patch_approval(sub_id.to_owned(), call_id.to_owned(), &action, None, None)
|
|
.await;
|
|
match rx_approve.await.unwrap_or_default() {
|
|
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {
|
|
InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
|
|
action,
|
|
user_explicitly_approved_this_action: true,
|
|
})
|
|
}
|
|
ReviewDecision::Denied | ReviewDecision::Abort => {
|
|
ResponseInputItem::FunctionCallOutput {
|
|
call_id: call_id.to_owned(),
|
|
output: FunctionCallOutputPayload {
|
|
content: "patch rejected by user".to_string(),
|
|
success: Some(false),
|
|
},
|
|
}
|
|
.into()
|
|
}
|
|
}
|
|
}
|
|
SafetyCheck::Reject { reason } => ResponseInputItem::FunctionCallOutput {
|
|
call_id: call_id.to_owned(),
|
|
output: FunctionCallOutputPayload {
|
|
content: format!("patch rejected: {reason}"),
|
|
success: Some(false),
|
|
},
|
|
}
|
|
.into(),
|
|
}
|
|
}
|
|
|
|
pub(crate) fn convert_apply_patch_to_protocol(
|
|
action: &ApplyPatchAction,
|
|
) -> HashMap<PathBuf, FileChange> {
|
|
let changes = action.changes();
|
|
let mut result = HashMap::with_capacity(changes.len());
|
|
for (path, change) in changes {
|
|
let protocol_change = match change {
|
|
ApplyPatchFileChange::Add { content } => FileChange::Add {
|
|
content: content.clone(),
|
|
},
|
|
ApplyPatchFileChange::Delete => FileChange::Delete,
|
|
ApplyPatchFileChange::Update {
|
|
unified_diff,
|
|
move_path,
|
|
new_content: _new_content,
|
|
} => FileChange::Update {
|
|
unified_diff: unified_diff.clone(),
|
|
move_path: move_path.clone(),
|
|
},
|
|
};
|
|
result.insert(path.clone(), protocol_change);
|
|
}
|
|
result
|
|
}
|