From a134bdde49d5839fc4d4c8bb55e2d672441da669 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 4 May 2025 11:39:10 -0700 Subject: [PATCH 1/2] fix: is_inside_git_repo should take the directory as a param (#809) https://github.com/openai/codex/pull/800 made `cwd` a property of `Config` and made it so the `cwd` is not necessarily `std::env::current_dir()`. As such, `is_inside_git_repo()` should check `Config.cwd` rather than `std::env::current_dir()`. This PR updates `is_inside_git_repo()` to take `Config` instead of an arbitrary `PathBuf` to force the check to operate on a `Config` where `cwd` has been resolved to what the user specified. --- codex-rs/core/src/util.rs | 24 ++++++++++-------------- codex-rs/exec/src/lib.rs | 35 ++++++++++++++++++----------------- codex-rs/tui/src/lib.rs | 2 +- 3 files changed, 29 insertions(+), 32 deletions(-) diff --git a/codex-rs/core/src/util.rs b/codex-rs/core/src/util.rs index 14bcc16d51..a7c1485273 100644 --- a/codex-rs/core/src/util.rs +++ b/codex-rs/core/src/util.rs @@ -5,6 +5,8 @@ use rand::Rng; use tokio::sync::Notify; use tracing::debug; +use crate::config::Config; + const INITIAL_DELAY_MS: u64 = 200; const BACKOFF_FACTOR: f64 = 1.3; @@ -33,26 +35,20 @@ pub(crate) fn backoff(attempt: u64) -> Duration { Duration::from_millis((base as f64 * jitter) as u64) } -/// Return `true` if the current working directory is inside a Git repository. +/// Return `true` if the project folder specified by the `Config` is inside a +/// Git repository. /// -/// The check walks up the directory hierarchy looking for a `.git` folder. This +/// The check walks up the directory hierarchy looking for a `.git` file or +/// directory (note `.git` can be a file that contains a `gitdir` entry). This /// approach does **not** require the `git` binary or the `git2` crate and is -/// therefore fairly lightweight. It intentionally only looks for the -/// presence of a *directory* named `.git` – this is good enough for regular -/// work‑trees and bare repos that live inside a work‑tree (common for -/// developers running Codex locally). +/// therefore fairly lightweight. /// /// Note that this does **not** detect *work‑trees* created with /// `git worktree add` where the checkout lives outside the main repository -/// directory. If you need Codex to work from such a checkout simply pass the +/// directory. If you need Codex to work from such a checkout simply pass the /// `--allow-no-git-exec` CLI flag that disables the repo requirement. -pub fn is_inside_git_repo() -> bool { - // Best‑effort: any IO error is treated as "not a repo" – the caller can - // decide what to do with the result. - let mut dir = match std::env::current_dir() { - Ok(d) => d, - Err(_) => return false, - }; +pub fn is_inside_git_repo(config: &Config) -> bool { + let mut dir = config.cwd.to_path_buf(); loop { if dir.join(".git").exists() { diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 4f9c94b5a7..1bd5069eed 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -47,23 +47,6 @@ pub async fn run_main(cli: Cli) -> anyhow::Result<()> { assert_api_key(stderr_with_ansi); - if !skip_git_repo_check && !is_inside_git_repo() { - eprintln!("Not inside a Git repo and --skip-git-repo-check was not specified."); - std::process::exit(1); - } - - // TODO(mbolin): Take a more thoughtful approach to logging. - let default_level = "error"; - let _ = tracing_subscriber::fmt() - .with_env_filter( - EnvFilter::try_from_default_env() - .or_else(|_| EnvFilter::try_new(default_level)) - .unwrap(), - ) - .with_ansi(stderr_with_ansi) - .with_writer(std::io::stderr) - .try_init(); - let sandbox_policy = if full_auto { Some(SandboxPolicy::new_full_auto_policy()) } else { @@ -85,6 +68,24 @@ pub async fn run_main(cli: Cli) -> anyhow::Result<()> { cwd: cwd.map(|p| p.canonicalize().unwrap_or(p)), }; let config = Config::load_with_overrides(overrides)?; + + if !skip_git_repo_check && !is_inside_git_repo(&config) { + eprintln!("Not inside a Git repo and --skip-git-repo-check was not specified."); + std::process::exit(1); + } + + // TODO(mbolin): Take a more thoughtful approach to logging. + let default_level = "error"; + let _ = tracing_subscriber::fmt() + .with_env_filter( + EnvFilter::try_from_default_env() + .or_else(|_| EnvFilter::try_new(default_level)) + .unwrap(), + ) + .with_ansi(stderr_with_ansi) + .with_writer(std::io::stderr) + .try_init(); + let (codex_wrapper, event, ctrl_c) = codex_wrapper::init_codex(config).await?; let codex = Arc::new(codex_wrapper); info!("Codex initialized with event: {event:?}"); diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 4c4f4e9165..0117135b49 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -114,7 +114,7 @@ pub fn run_main(cli: Cli) -> std::io::Result<()> { // modal. The flag is shown when the current working directory is *not* // inside a Git repository **and** the user did *not* pass the // `--allow-no-git-exec` flag. - let show_git_warning = !cli.skip_git_repo_check && !is_inside_git_repo(); + let show_git_warning = !cli.skip_git_repo_check && !is_inside_git_repo(&config); try_run_ratatui_app(cli, config, show_git_warning, log_rx); Ok(()) From f60e43a101d00fac78311cf0609f1bc90be51759 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 4 May 2025 11:39:57 -0700 Subject: [PATCH 2/2] fix: ensure apply_patch resolves relative paths against workdir or project cwd --- codex-rs/apply-patch/src/lib.rs | 45 +++++++-- codex-rs/core/src/codex.rs | 156 ++++++++++++++------------------ codex-rs/core/src/safety.rs | 25 ++--- 3 files changed, 115 insertions(+), 111 deletions(-) diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 090eab18f1..fef7d4f389 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -95,7 +95,7 @@ pub enum ApplyPatchFileChange { pub enum MaybeApplyPatchVerified { /// `argv` corresponded to an `apply_patch` invocation, and these are the /// resulting proposed file changes. - Body(HashMap), + Body(ApplyPatchAction), /// `argv` could not be parsed to determine whether it corresponds to an /// `apply_patch` invocation. ShellParseError(Error), @@ -106,7 +106,38 @@ pub enum MaybeApplyPatchVerified { 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, +} + +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 { + &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) { MaybeApplyPatch::Body(hunks) => { let mut changes = HashMap::new(); @@ -114,14 +145,14 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerif match hunk { Hunk::AddFile { path, contents } => { changes.insert( - path, + cwd.join(path), ApplyPatchFileChange::Add { content: contents.clone(), }, ); } Hunk::DeleteFile { path } => { - changes.insert(path, ApplyPatchFileChange::Delete); + changes.insert(cwd.join(path), ApplyPatchFileChange::Delete); } Hunk::UpdateFile { path, @@ -138,17 +169,17 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String]) -> MaybeApplyPatchVerif } }; changes.insert( - path.clone(), + cwd.join(path), ApplyPatchFileChange::Update { unified_diff, - move_path, + move_path: move_path.map(|p| cwd.join(p)), new_content: contents, }, ); } } } - MaybeApplyPatchVerified::Body(changes) + MaybeApplyPatchVerified::Body(ApplyPatchAction { changes }) } MaybeApplyPatch::ShellParseError(e) => MaybeApplyPatchVerified::ShellParseError(e), MaybeApplyPatch::PatchParseError(e) => MaybeApplyPatchVerified::CorrectnessError(e.into()), diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 8f3420ac28..a55ac423f0 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -12,6 +12,7 @@ use async_channel::Sender; use codex_apply_patch::maybe_parse_apply_patch_verified; use codex_apply_patch::print_summary; use codex_apply_patch::AffectedPaths; +use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use codex_apply_patch::MaybeApplyPatchVerified; use fs_err as fs; @@ -271,7 +272,7 @@ impl Session { pub async fn request_patch_approval( &self, sub_id: String, - changes: &HashMap, + action: &ApplyPatchAction, reason: Option, grant_root: Option, ) -> oneshot::Receiver { @@ -279,7 +280,7 @@ impl Session { let event = Event { id: sub_id.clone(), msg: EventMsg::ApplyPatchApprovalRequest { - changes: convert_apply_patch_to_protocol(changes), + changes: convert_apply_patch_to_protocol(action), reason, grant_root, }, @@ -304,19 +305,13 @@ impl Session { state.approved_commands.insert(cmd); } - async fn notify_exec_command_begin( - &self, - sub_id: &str, - call_id: &str, - command: Vec, - cwd: PathBuf, - ) { + async fn notify_exec_command_begin(&self, sub_id: &str, call_id: &str, params: &ExecParams) { let event = Event { id: sub_id.to_string(), msg: EventMsg::ExecCommandBegin { call_id: call_id.to_string(), - command, - cwd, + command: params.command.clone(), + cwd: params.cwd.clone(), }, }; let _ = self.tx_event.send(event).await; @@ -886,8 +881,12 @@ async fn handle_function_call( match name.as_str() { "container.exec" | "shell" => { // parse command - let params = match serde_json::from_str::(&arguments) { - Ok(v) => v, + let params: ExecParams = match serde_json::from_str::(&arguments) { + 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) => { // allow model to re-sample let output = ResponseInputItem::FunctionCallOutput { @@ -902,7 +901,7 @@ async fn handle_function_call( }; // check if this was a patch, and apply it if so - match maybe_parse_apply_patch_verified(¶ms.command) { + match maybe_parse_apply_patch_verified(¶ms.command, ¶ms.cwd) { MaybeApplyPatchVerified::Body(changes) => { return apply_patch(sess, sub_id, call_id, changes).await; } @@ -924,9 +923,6 @@ async fn handle_function_call( MaybeApplyPatchVerified::NotApplyPatch => (), } - // this was not a valid patch, execute command - let workdir = sess.resolve_path(params.workdir.clone()); - // safety checks let safety = { let state = sess.state.lock().unwrap(); @@ -944,7 +940,7 @@ async fn handle_function_call( .request_command_approval( sub_id.clone(), params.command.clone(), - workdir.clone(), + params.cwd.clone(), None, ) .await; @@ -980,20 +976,11 @@ async fn handle_function_call( } }; - sess.notify_exec_command_begin( - &sub_id, - &call_id, - params.command.clone(), - workdir.clone(), - ) - .await; + sess.notify_exec_command_begin(&sub_id, &call_id, ¶ms) + .await; let output_result = process_exec_tool_call( - ExecParams { - command: params.command.clone(), - cwd: workdir.clone(), - timeout_ms: params.timeout_ms, - }, + params.clone(), sandbox_type, sess.ctrl_c.clone(), &sess.sandbox_policy, @@ -1050,7 +1037,7 @@ async fn handle_function_call( .request_command_approval( sub_id.clone(), params.command.clone(), - workdir, + params.cwd.clone(), Some("command failed; retry without sandbox?".to_string()), ) .await; @@ -1071,23 +1058,13 @@ async fn handle_function_call( // Emit a fresh Begin event so progress bars reset. 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.command.clone(), - cwd.clone(), - ) - .await; + sess.notify_exec_command_begin(&sub_id, &retry_call_id, ¶ms) + .await; // This is an escalated retry; the policy will not be // examined and the sandbox has been set to `None`. let retry_output_result = process_exec_tool_call( - ExecParams { - command: params.command.clone(), - cwd: cwd.clone(), - timeout_ms: params.timeout_ms, - }, + params, SandboxType::None, sess.ctrl_c.clone(), &sess.sandbox_policy, @@ -1180,7 +1157,7 @@ async fn apply_patch( sess: &Session, sub_id: String, call_id: String, - changes: HashMap, + action: ApplyPatchAction, ) -> ResponseInputItem { let writable_roots_snapshot = { let guard = sess.writable_roots.lock().unwrap(); @@ -1188,7 +1165,7 @@ async fn apply_patch( }; let auto_approved = match assess_patch_safety( - &changes, + &action, sess.approval_policy, &writable_roots_snapshot, &sess.cwd, @@ -1198,7 +1175,7 @@ async fn apply_patch( // Compute a readable summary of path changes to include in the // approval request so the user can make an informed decision. let rx_approve = sess - .request_patch_approval(sub_id.clone(), &changes, None, None) + .request_patch_approval(sub_id.clone(), &action, None, None) .await; match rx_approve.await.unwrap_or_default() { ReviewDecision::Approved | ReviewDecision::ApprovedForSession => false, @@ -1227,7 +1204,7 @@ async fn apply_patch( // Verify write permissions before touching the filesystem. 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 reason = Some(format!( @@ -1236,7 +1213,7 @@ async fn apply_patch( )); 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; if !matches!( @@ -1263,7 +1240,7 @@ async fn apply_patch( msg: EventMsg::PatchApplyBegin { call_id: call_id.clone(), auto_approved, - changes: convert_apply_patch_to_protocol(&changes), + changes: convert_apply_patch_to_protocol(&action), }, }) .await; @@ -1272,37 +1249,43 @@ async fn apply_patch( let mut stderr = Vec::new(); // Enforce writable roots. If a write is blocked, collect offending root // 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 err.kind() == std::io::ErrorKind::PermissionDenied { // Determine first offending path. - let offending_opt = changes.iter().find_map(|(path, change)| { - let path_ref = match change { - ApplyPatchFileChange::Add { .. } => path, - ApplyPatchFileChange::Delete => path, - ApplyPatchFileChange::Update { .. } => path, - }; + let offending_opt = action + .changes() + .iter() + .flat_map(|(path, change)| match change { + 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| { + // Reuse safety normalization logic: treat absolute path. + if !path.is_absolute() { + panic!("apply_patch invariant failed: path is not absolute: {path:?}"); + } - // Reuse safety normalization logic: treat absolute path. - let abs = if path_ref.is_absolute() { - path_ref.clone() - } else { - // TODO(mbolin): If workdir was supplied with apply_patch call, - // relative paths should be resolved against it. - sess.cwd.join(path_ref) - }; - - 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()) - } - }); + let writable = { + let roots = sess.writable_roots.lock().unwrap(); + roots.iter().any(|root| path.starts_with(root)) + }; + if writable { + None + } else { + Some(path.to_path_buf()) + } + }); if let Some(offending) = offending_opt { let root = offending.parent().unwrap_or(&offending).to_path_buf(); @@ -1314,7 +1297,7 @@ async fn apply_patch( let rx = sess .request_patch_approval( sub_id.clone(), - &changes, + &action, reason.clone(), Some(root.clone()), ) @@ -1328,7 +1311,7 @@ async fn apply_patch( stdout.clear(); stderr.clear(); result = apply_changes_from_apply_patch_and_report( - &changes, + &action, &mut stdout, &mut stderr, ); @@ -1374,10 +1357,11 @@ async fn apply_patch( /// `writable_roots` (after normalising). If all paths are acceptable, /// returns None. fn first_offending_path( - changes: &HashMap, + action: &ApplyPatchAction, writable_roots: &[PathBuf], cwd: &Path, ) -> Option { + let changes = action.changes(); for (path, change) in changes { let candidate = match change { ApplyPatchFileChange::Add { .. } => path, @@ -1411,9 +1395,8 @@ fn first_offending_path( None } -fn convert_apply_patch_to_protocol( - changes: &HashMap, -) -> HashMap { +fn convert_apply_patch_to_protocol(action: &ApplyPatchAction) -> HashMap { + let changes = action.changes(); let mut result = HashMap::with_capacity(changes.len()); for (path, change) in changes { let protocol_change = match change { @@ -1436,11 +1419,11 @@ fn convert_apply_patch_to_protocol( } fn apply_changes_from_apply_patch_and_report( - changes: &HashMap, + action: &ApplyPatchAction, stdout: &mut impl std::io::Write, stderr: &mut impl std::io::Write, ) -> std::io::Result<()> { - match apply_changes_from_apply_patch(changes) { + match apply_changes_from_apply_patch(action) { Ok(affected_paths) => { print_summary(&affected_paths, stdout)?; } @@ -1452,13 +1435,12 @@ fn apply_changes_from_apply_patch_and_report( Ok(()) } -fn apply_changes_from_apply_patch( - changes: &HashMap, -) -> anyhow::Result { +fn apply_changes_from_apply_patch(action: &ApplyPatchAction) -> anyhow::Result { let mut added: Vec = Vec::new(); let mut modified: Vec = Vec::new(); let mut deleted: Vec = Vec::new(); + let changes = action.changes(); for (path, change) in changes { match change { ApplyPatchFileChange::Add { content } => { diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index 3d98be6ccd..ac1b30a6d8 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -1,9 +1,9 @@ -use std::collections::HashMap; use std::collections::HashSet; use std::path::Component; use std::path::Path; use std::path::PathBuf; +use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; use crate::exec::SandboxType; @@ -19,12 +19,12 @@ pub enum SafetyCheck { } pub fn assess_patch_safety( - changes: &HashMap, + action: &ApplyPatchAction, policy: AskForApproval, writable_roots: &[PathBuf], cwd: &Path, ) -> SafetyCheck { - if changes.is_empty() { + if action.is_empty() { return SafetyCheck::Reject { 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 { sandbox_type: SandboxType::None, } @@ -114,7 +114,7 @@ pub fn get_platform_sandbox() -> Option { } fn is_write_patch_constrained_to_writable_paths( - changes: &HashMap, + action: &ApplyPatchAction, writable_roots: &[PathBuf], cwd: &Path, ) -> 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 { ApplyPatchFileChange::Add { .. } | ApplyPatchFileChange::Delete => { if !is_path_writable(path) { @@ -198,18 +198,9 @@ mod tests { // Helper to build a single‑entry map representing a patch that adds a // file at `p`. - let make_add_change = |p: PathBuf| { - let mut m = HashMap::new(); - m.insert( - p.clone(), - ApplyPatchFileChange::Add { - content: String::new(), - }, - ); - m - }; + let make_add_change = |p: PathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string()); - 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")); assert!(is_write_patch_constrained_to_writable_paths(