valknar/llmx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: ensure apply_patch resolves relative paths against workdir or project cwd (#810)

Browse Source 
https://github.com/openai/codex/pull/800 kicked off some work to be more
disciplined about honoring the `cwd` param passed in rather than
assuming `std::env::current_dir()` as the `cwd`. As part of this, we
need to ensure `apply_patch` calls honor the appropriate `cwd` as well,
which is significant if the paths in the `apply_patch` arg are not
absolute paths themselves. Failing that:

- The `apply_patch` function call can contain an optional`workdir`
param, so:
- If specified and is an absolute path, it should be used to resolve
relative paths
- If specified and is a relative path, should be resolved against
`Config.cwd` and then any relative paths will be resolved against the
result
- If `workdir` is not specified on the function call, relative paths
should be resolved against `Config.cwd`

Note that we had a similar issue in the TypeScript CLI that was fixed in
https://github.com/openai/codex/pull/556.

As part of the fix, this PR introduces `ApplyPatchAction` so clients can
deal with that instead of the raw `HashMap<PathBuf,
ApplyPatchFileChange>`. This enables us to enforce, by construction,
that all paths contained in the `ApplyPatchAction` are absolute paths.
This commit is contained in:
Michael Bolin
2025-05-04 12:32:51 -07:00
committed by GitHub
parent a134bdde49
commit 5d924d44cf
3 changed files with 115 additions and 111 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
codex-rs/apply-patch/src/lib.rs
View File

@@ -95,7 +95,7 @@ pub enum ApplyPatchFileChange {
pub enum MaybeApplyPatchVerified { pub enum MaybeApplyPatchVerified {
/// `argv` corresponded to an `apply_patch` invocation, and these are the /// `argv` corresponded to an `apply_patch` invocation, and these are the
/// resulting proposed file changes. /// resulting proposed file changes.
Body(HashMap<PathBuf, ApplyPatchFileChange>), Body(ApplyPatchAction),
/// `argv` could not be parsed to determine whether it corresponds to an /// `argv` could not be parsed to determine whether it corresponds to an
/// `apply_patch` invocation. /// `apply_patch` invocation.
ShellParseError(Error), ShellParseError(Error),
@@ -106,7 +106,38 @@ pub enum MaybeApplyPatchVerified {
NotApplyPatch, NotApplyPatch,
} }
pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerified { #[derive(Debug)]
/// ApplyPatchAction is the result of parsing an `apply_patch` command. By
/// construction, all paths should be absolute paths.
pub struct ApplyPatchAction {
changes: HashMap<PathBuf, ApplyPatchFileChange>,
}
impl ApplyPatchAction {
pub fn is_empty(&self) -> bool {
self.changes.is_empty()
}
/// Returns the changes that would be made by applying the patch.
pub fn changes(&self) -> &HashMap<PathBuf, ApplyPatchFileChange> {
&self.changes
}
/// Should be used exclusively for testing. (Not worth the overhead of
/// creating a feature flag for this.)
pub fn new_add_for_test(path: &Path, content: String) -> Self {
if !path.is_absolute() {
panic!("path must be absolute");
}
let changes = HashMap::from([(path.to_path_buf(), ApplyPatchFileChange::Add { content })]);
Self { changes }
}
}
/// cwd must be an absolute path so that we can resolve relative paths in the
/// patch.
pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified {
match maybe_parse_apply_patch(argv) { match maybe_parse_apply_patch(argv) {
MaybeApplyPatch::Body(hunks) => { MaybeApplyPatch::Body(hunks) => {
let mut changes = HashMap::new(); let mut changes = HashMap::new();
@@ -114,14 +145,14 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerif
match hunk { match hunk {
Hunk::AddFile { path, contents } => { Hunk::AddFile { path, contents } => {
changes.insert( changes.insert(
path, cwd.join(path),
ApplyPatchFileChange::Add { ApplyPatchFileChange::Add {
content: contents.clone(), content: contents.clone(),
}, },
); );
} }
Hunk::DeleteFile { path } => { Hunk::DeleteFile { path } => {
changes.insert(path, ApplyPatchFileChange::Delete); changes.insert(cwd.join(path), ApplyPatchFileChange::Delete);
} }
Hunk::UpdateFile { Hunk::UpdateFile {
path, path,
@@ -138,17 +169,17 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerif
} }
}; };
changes.insert( changes.insert(
path.clone(), cwd.join(path),
ApplyPatchFileChange::Update { ApplyPatchFileChange::Update {
unified_diff, unified_diff,
move_path, move_path: move_path.map(|p| cwd.join(p)),
new_content: contents, new_content: contents,
}, },
); );
} }
} }
} }
MaybeApplyPatchVerified::Body(changes) MaybeApplyPatchVerified::Body(ApplyPatchAction { changes })
} }
MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e), MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e),
MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()), MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()),

156
codex-rs/core/src/codex.rs
View File

@@ -12,6 +12,7 @@ use async_channel::Sender;
use codex_apply_patch::maybe_parse_apply_patch_verified; use codex_apply_patch::maybe_parse_apply_patch_verified;
use codex_apply_patch::print_summary; use codex_apply_patch::print_summary;
use codex_apply_patch::AffectedPaths; use codex_apply_patch::AffectedPaths;
use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::ApplyPatchFileChange; use codex_apply_patch::ApplyPatchFileChange;
use codex_apply_patch::MaybeApplyPatchVerified; use codex_apply_patch::MaybeApplyPatchVerified;
use fs_err as fs; use fs_err as fs;
@@ -271,7 +272,7 @@ impl Session {
pub async fn request_patch_approval( pub async fn request_patch_approval(
&self, &self,
sub_id: String, sub_id: String,
changes: &HashMap<PathBuf, ApplyPatchFileChange>, action: &ApplyPatchAction,
reason: Option<String>, reason: Option<String>,
grant_root: Option<PathBuf>, grant_root: Option<PathBuf>,
) -> oneshot::Receiver<ReviewDecision> { ) -> oneshot::Receiver<ReviewDecision> {
@@ -279,7 +280,7 @@ impl Session {
let event = Event { let event = Event {
id: sub_id.clone(), id: sub_id.clone(),
msg: EventMsg::ApplyPatchApprovalRequest { msg: EventMsg::ApplyPatchApprovalRequest {
changes: convert_apply_patch_to_protocol(changes), changes: convert_apply_patch_to_protocol(action),
reason, reason,
grant_root, grant_root,
}, },
@@ -304,19 +305,13 @@ impl Session {
state.approved_commands.insert(cmd); state.approved_commands.insert(cmd);
} }
async fn notify_exec_command_begin( async fn notify_exec_command_begin(&self, sub_id: &str, call_id: &str, params: &ExecParams) {
&self,
sub_id: &str,
call_id: &str,
command: Vec<String>,
cwd: PathBuf,
) {
let event = Event { let event = Event {
id: sub_id.to_string(), id: sub_id.to_string(),
msg: EventMsg::ExecCommandBegin { msg: EventMsg::ExecCommandBegin {
call_id: call_id.to_string(), call_id: call_id.to_string(),
command, command: params.command.clone(),
cwd, cwd: params.cwd.clone(),
}, },
}; };
let _ = self.tx_event.send(event).await; let _ = self.tx_event.send(event).await;
@@ -886,8 +881,12 @@ async fn handle_function_call(
match name.as_str() { match name.as_str() {
"container.exec" | "shell" => { "container.exec" | "shell" => {
// parse command // parse command
let params = match serde_json::from_str::<ShellToolCallParams>(&arguments) { let params: ExecParams = match serde_json::from_str::<ShellToolCallParams>(&arguments) {
Ok(v) => v, Ok(shell_tool_call_params) => ExecParams {
command: shell_tool_call_params.command,
cwd: sess.resolve_path(shell_tool_call_params.workdir.clone()),
timeout_ms: shell_tool_call_params.timeout_ms,
},
Err(e) => { Err(e) => {
// allow model to re-sample // allow model to re-sample
let output = ResponseInputItem::FunctionCallOutput { let output = ResponseInputItem::FunctionCallOutput {
@@ -902,7 +901,7 @@ async fn handle_function_call(
}; };
// check if this was a patch, and apply it if so // check if this was a patch, and apply it if so
match maybe_parse_apply_patch_verified(&params.command) { match maybe_parse_apply_patch_verified(&params.command, &params.cwd) {
MaybeApplyPatchVerified::Body(changes) => { MaybeApplyPatchVerified::Body(changes) => {
return apply_patch(sess, sub_id, call_id, changes).await; return apply_patch(sess, sub_id, call_id, changes).await;
} }
@@ -924,9 +923,6 @@ async fn handle_function_call(
MaybeApplyPatchVerified::NotApplyPatch => (), MaybeApplyPatchVerified::NotApplyPatch => (),
} }
// this was not a valid patch, execute command
let workdir = sess.resolve_path(params.workdir.clone());
// safety checks // safety checks
let safety = { let safety = {
let state = sess.state.lock().unwrap(); let state = sess.state.lock().unwrap();
@@ -944,7 +940,7 @@ async fn handle_function_call(
.request_command_approval( .request_command_approval(
sub_id.clone(), sub_id.clone(),
params.command.clone(), params.command.clone(),
workdir.clone(), params.cwd.clone(),
None, None,
) )
.await; .await;
@@ -980,20 +976,11 @@ async fn handle_function_call(
} }
}; };
sess.notify_exec_command_begin( sess.notify_exec_command_begin(&sub_id, &call_id, &params)
&sub_id, .await;
&call_id,
params.command.clone(),
workdir.clone(),
)
.await;
let output_result = process_exec_tool_call( let output_result = process_exec_tool_call(
ExecParams { params.clone(),
command: params.command.clone(),
cwd: workdir.clone(),
timeout_ms: params.timeout_ms,
},
sandbox_type, sandbox_type,
sess.ctrl_c.clone(), sess.ctrl_c.clone(),
&sess.sandbox_policy, &sess.sandbox_policy,
@@ -1050,7 +1037,7 @@ async fn handle_function_call(
.request_command_approval( .request_command_approval(
sub_id.clone(), sub_id.clone(),
params.command.clone(), params.command.clone(),
workdir, params.cwd.clone(),
Some("command failed; retry without sandbox?".to_string()), Some("command failed; retry without sandbox?".to_string()),
) )
.await; .await;
@@ -1071,23 +1058,13 @@ async fn handle_function_call(
// Emit a fresh Begin event so progress bars reset. // Emit a fresh Begin event so progress bars reset.
let retry_call_id = format!("{call_id}-retry"); let retry_call_id = format!("{call_id}-retry");
let cwd = sess.resolve_path(params.workdir.clone()); sess.notify_exec_command_begin(&sub_id, &retry_call_id, &params)
sess.notify_exec_command_begin( .await;
&sub_id,
&retry_call_id,
params.command.clone(),
cwd.clone(),
)
.await;
// This is an escalated retry; the policy will not be // This is an escalated retry; the policy will not be
// examined and the sandbox has been set to `None`. // examined and the sandbox has been set to `None`.
let retry_output_result = process_exec_tool_call( let retry_output_result = process_exec_tool_call(
ExecParams { params,
command: params.command.clone(),
cwd: cwd.clone(),
timeout_ms: params.timeout_ms,
},
SandboxType::None, SandboxType::None,
sess.ctrl_c.clone(), sess.ctrl_c.clone(),
&sess.sandbox_policy, &sess.sandbox_policy,
@@ -1180,7 +1157,7 @@ async fn apply_patch(
sess: &Session, sess: &Session,
sub_id: String, sub_id: String,
call_id: String, call_id: String,
changes: HashMap<PathBuf, ApplyPatchFileChange>, action: ApplyPatchAction,
) -> ResponseInputItem { ) -> ResponseInputItem {
let writable_roots_snapshot = { let writable_roots_snapshot = {
let guard = sess.writable_roots.lock().unwrap(); let guard = sess.writable_roots.lock().unwrap();
@@ -1188,7 +1165,7 @@ async fn apply_patch(
}; };
let auto_approved = match assess_patch_safety( let auto_approved = match assess_patch_safety(
&changes, &action,
sess.approval_policy, sess.approval_policy,
&writable_roots_snapshot, &writable_roots_snapshot,
&sess.cwd, &sess.cwd,
@@ -1198,7 +1175,7 @@ async fn apply_patch(
// Compute a readable summary of path changes to include in the // Compute a readable summary of path changes to include in the
// approval request so the user can make an informed decision. // approval request so the user can make an informed decision.
let rx_approve = sess let rx_approve = sess
.request_patch_approval(sub_id.clone(), &changes, None, None) .request_patch_approval(sub_id.clone(), &action, None, None)
.await; .await;
match rx_approve.await.unwrap_or_default() { match rx_approve.await.unwrap_or_default() {
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false,
@@ -1227,7 +1204,7 @@ async fn apply_patch(
// Verify write permissions before touching the filesystem. // Verify write permissions before touching the filesystem.
let writable_snapshot = { sess.writable_roots.lock().unwrap().clone() }; let writable_snapshot = { sess.writable_roots.lock().unwrap().clone() };
if let Some(offending) = first_offending_path(&changes, &writable_snapshot, &sess.cwd) { if let Some(offending) = first_offending_path(&action, &writable_snapshot, &sess.cwd) {
let root = offending.parent().unwrap_or(&offending).to_path_buf(); let root = offending.parent().unwrap_or(&offending).to_path_buf();
let reason = Some(format!( let reason = Some(format!(
@@ -1236,7 +1213,7 @@ async fn apply_patch(
)); ));
let rx = sess let rx = sess
.request_patch_approval(sub_id.clone(), &changes, reason.clone(), Some(root.clone())) .request_patch_approval(sub_id.clone(), &action, reason.clone(), Some(root.clone()))
.await; .await;
if !matches!( if !matches!(
@@ -1263,7 +1240,7 @@ async fn apply_patch(
msg: EventMsg::PatchApplyBegin { msg: EventMsg::PatchApplyBegin {
call_id: call_id.clone(), call_id: call_id.clone(),
auto_approved, auto_approved,
changes: convert_apply_patch_to_protocol(&changes), changes: convert_apply_patch_to_protocol(&action),
}, },
}) })
.await; .await;
@@ -1272,37 +1249,43 @@ async fn apply_patch(
let mut stderr = Vec::new(); let mut stderr = Vec::new();
// Enforce writable roots. If a write is blocked, collect offending root // Enforce writable roots. If a write is blocked, collect offending root
// and prompt the user to extend permissions. // and prompt the user to extend permissions.
let mut result = apply_changes_from_apply_patch_and_report(&changes, &mut stdout, &mut stderr); let mut result = apply_changes_from_apply_patch_and_report(&action, &mut stdout, &mut stderr);
if let Err(err) = &result { if let Err(err) = &result {
if err.kind() == std::io::ErrorKind::PermissionDenied { if err.kind() == std::io::ErrorKind::PermissionDenied {
// Determine first offending path. // Determine first offending path.
let offending_opt = changes.iter().find_map(|(path, change)| { let offending_opt = action
let path_ref = match change { .changes()
ApplyPatchFileChange::Add { .. } => path, .iter()
ApplyPatchFileChange::Delete => path, .flat_map(|(path, change)| match change {
ApplyPatchFileChange::Update { .. } => path, ApplyPatchFileChange::Add { .. } => vec![path.as_ref()],
}; ApplyPatchFileChange::Delete => vec![path.as_ref()],
ApplyPatchFileChange::Update {
move_path: Some(move_path),
..
} => {
vec![path.as_ref(), move_path.as_ref()]
}
ApplyPatchFileChange::Update {
move_path: None, ..
} => vec![path.as_ref()],
})
.find_map(|path: &Path| {
// ApplyPatchAction promises to guarantee absolute paths.
if !path.is_absolute() {
panic!("apply_patch invariant failed: path is not absolute: {path:?}");
}
// Reuse safety normalization logic: treat absolute path. let writable = {
let abs = if path_ref.is_absolute() { let roots = sess.writable_roots.lock().unwrap();
path_ref.clone() roots.iter().any(|root| path.starts_with(root))
} else { };
// TODO(mbolin): If workdir was supplied with apply_patch call, if writable {
// relative paths should be resolved against it. None
sess.cwd.join(path_ref) } else {
}; Some(path.to_path_buf())
}
let writable = { });
let roots = sess.writable_roots.lock().unwrap();
roots.iter().any(|root| abs.starts_with(root))
};
if writable {
None
} else {
Some(path_ref.clone())
}
});
if let Some(offending) = offending_opt { if let Some(offending) = offending_opt {
let root = offending.parent().unwrap_or(&offending).to_path_buf(); let root = offending.parent().unwrap_or(&offending).to_path_buf();
@@ -1314,7 +1297,7 @@ async fn apply_patch(
let rx = sess let rx = sess
.request_patch_approval( .request_patch_approval(
sub_id.clone(), sub_id.clone(),
&changes, &action,
reason.clone(), reason.clone(),
Some(root.clone()), Some(root.clone()),
) )
@@ -1328,7 +1311,7 @@ async fn apply_patch(
stdout.clear(); stdout.clear();
stderr.clear(); stderr.clear();
result = apply_changes_from_apply_patch_and_report( result = apply_changes_from_apply_patch_and_report(
&changes, &action,
&mut stdout, &mut stdout,
&mut stderr, &mut stderr,
); );
@@ -1374,10 +1357,11 @@ async fn apply_patch(
/// `writable_roots` (after normalising). If all paths are acceptable, /// `writable_roots` (after normalising). If all paths are acceptable,
/// returns None. /// returns None.
fn first_offending_path( fn first_offending_path(
changes: &HashMap<PathBuf, ApplyPatchFileChange>, action: &ApplyPatchAction,
writable_roots: &[PathBuf], writable_roots: &[PathBuf],
cwd: &Path, cwd: &Path,
) -> Option<PathBuf> { ) -> Option<PathBuf> {
let changes = action.changes();
for (path, change) in changes { for (path, change) in changes {
let candidate = match change { let candidate = match change {
ApplyPatchFileChange::Add { .. } => path, ApplyPatchFileChange::Add { .. } => path,
@@ -1411,9 +1395,8 @@ fn first_offending_path(
None None
} }
fn convert_apply_patch_to_protocol( fn convert_apply_patch_to_protocol(action: &ApplyPatchAction) -> HashMap<PathBuf, FileChange> {
changes: &HashMap<PathBuf, ApplyPatchFileChange>, let changes = action.changes();
) -> HashMap<PathBuf, FileChange> {
let mut result = HashMap::with_capacity(changes.len()); let mut result = HashMap::with_capacity(changes.len());
for (path, change) in changes { for (path, change) in changes {
let protocol_change = match change { let protocol_change = match change {
@@ -1436,11 +1419,11 @@ fn convert_apply_patch_to_protocol(
} }
fn apply_changes_from_apply_patch_and_report( fn apply_changes_from_apply_patch_and_report(
changes: &HashMap<PathBuf, ApplyPatchFileChange>, action: &ApplyPatchAction,
stdout: &mut impl std::io::Write, stdout: &mut impl std::io::Write,
stderr: &mut impl std::io::Write, stderr: &mut impl std::io::Write,
) -> std::io::Result<()> { ) -> std::io::Result<()> {
match apply_changes_from_apply_patch(changes) { match apply_changes_from_apply_patch(action) {
Ok(affected_paths) => { Ok(affected_paths) => {
print_summary(&affected_paths, stdout)?; print_summary(&affected_paths, stdout)?;
} }
@@ -1452,13 +1435,12 @@ fn apply_changes_from_apply_patch_and_report(
Ok(()) Ok(())
} }
fn apply_changes_from_apply_patch( fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result<AffectedPaths> {
changes: &HashMap<PathBuf, ApplyPatchFileChange>,
) -> anyhow::Result<AffectedPaths> {
let mut added: Vec<PathBuf> = Vec::new(); let mut added: Vec<PathBuf> = Vec::new();
let mut modified: Vec<PathBuf> = Vec::new(); let mut modified: Vec<PathBuf> = Vec::new();
let mut deleted: Vec<PathBuf> = Vec::new(); let mut deleted: Vec<PathBuf> = Vec::new();
let changes = action.changes();
for (path, change) in changes { for (path, change) in changes {
match change { match change {
ApplyPatchFileChange::Add { content } => { ApplyPatchFileChange::Add { content } => {

25
codex-rs/core/src/safety.rs
View File

@@ -1,9 +1,9 @@
use std::collections::HashMap;
use std::collections::HashSet; use std::collections::HashSet;
use std::path::Component; use std::path::Component;
use std::path::Path; use std::path::Path;
use std::path::PathBuf; use std::path::PathBuf;
use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::ApplyPatchFileChange; use codex_apply_patch::ApplyPatchFileChange;
use crate::exec::SandboxType; use crate::exec::SandboxType;
@@ -19,12 +19,12 @@ pub enum SafetyCheck {
} }
pub fn assess_patch_safety( pub fn assess_patch_safety(
changes: &HashMap<PathBuf, ApplyPatchFileChange>, action: &ApplyPatchAction,
policy: AskForApproval, policy: AskForApproval,
writable_roots: &[PathBuf], writable_roots: &[PathBuf],
cwd: &Path, cwd: &Path,
) -> SafetyCheck { ) -> SafetyCheck {
if changes.is_empty() { if action.is_empty() {
return SafetyCheck::Reject { return SafetyCheck::Reject {
reason: "empty patch".to_string(), reason: "empty patch".to_string(),
}; };
@@ -41,7 +41,7 @@ pub fn assess_patch_safety(
} }
} }
if is_write_patch_constrained_to_writable_paths(changes, writable_roots, cwd) { if is_write_patch_constrained_to_writable_paths(action, writable_roots, cwd) {
SafetyCheck::AutoApprove { SafetyCheck::AutoApprove {
sandbox_type: SandboxType::None, sandbox_type: SandboxType::None,
} }
@@ -114,7 +114,7 @@ pub fn get_platform_sandbox() -> Option<SandboxType> {
} }
fn is_write_patch_constrained_to_writable_paths( fn is_write_patch_constrained_to_writable_paths(
changes: &HashMap<PathBuf, ApplyPatchFileChange>, action: &ApplyPatchAction,
writable_roots: &[PathBuf], writable_roots: &[PathBuf],
cwd: &Path, cwd: &Path,
) -> bool { ) -> bool {
@@ -164,7 +164,7 @@ fn is_write_patch_constrained_to_writable_paths(
}) })
}; };
for (path, change) in changes { for (path, change) in action.changes() {
match change { match change {
ApplyPatchFileChange::Add { .. } | ApplyPatchFileChange::Delete => { ApplyPatchFileChange::Add { .. } | ApplyPatchFileChange::Delete => {
if !is_path_writable(path) { if !is_path_writable(path) {
@@ -198,18 +198,9 @@ mod tests {
// Helper to build a single‑entry map representing a patch that adds a // Helper to build a single‑entry map representing a patch that adds a
// file at `p`. // file at `p`.
let make_add_change = |p: PathBuf| { let make_add_change = |p: PathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string());
let mut m = HashMap::new();
m.insert(
p.clone(),
ApplyPatchFileChange::Add {
content: String::new(),
},
);
m
};
let add_inside = make_add_change(PathBuf::from("inner.txt")); let add_inside = make_add_change(cwd.join("inner.txt"));
let add_outside = make_add_change(parent.join("outside.txt")); let add_outside = make_add_change(parent.join("outside.txt"));
assert!(is_write_patch_constrained_to_writable_paths( assert!(is_write_patch_constrained_to_writable_paths(