From e59b01ebd312a995fc40d836001ee0b36a4118fc Mon Sep 17 00:00:00 2001 From: starr-openai Date: Thu, 7 May 2026 20:32:06 -0700 Subject: [PATCH] codex: fix apply_patch review regressions Co-authored-by: Codex --- .../app-server/src/bespoke_event_handling.rs | 3 +-- .../core/src/tools/handlers/apply_patch.rs | 22 ++++++++++++------- .../src/tools/handlers/apply_patch_spec.rs | 4 ++-- .../tools/handlers/apply_patch_spec_tests.rs | 4 ++-- .../src/tools/handlers/apply_patch_tests.rs | 2 +- .../core/src/tools/runtimes/apply_patch.rs | 2 +- .../src/tools/runtimes/apply_patch_tests.rs | 3 ++- codex-rs/mcp-server/src/patch_approval.rs | 2 +- codex-rs/mcp-server/tests/suite/codex_tool.rs | 7 +++++- 9 files changed, 30 insertions(+), 19 deletions(-) diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 7b32ff7b2e..2a9409426f 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -509,7 +509,6 @@ pub(crate) async fn apply_bespoke_event_handling( let environment_id = (!event.environment_id.is_empty() && event.environment_id != codex_exec_server::LOCAL_ENVIRONMENT_ID) .then_some(event.environment_id.clone()); - let cwd = environment_id.as_ref().map(|_| event.cwd.clone()); let params = FileChangeRequestApprovalParams { thread_id: conversation_id.to_string(), @@ -517,7 +516,7 @@ pub(crate) async fn apply_bespoke_event_handling( item_id: item_id.clone(), started_at_ms: event.started_at_ms, environment_id, - cwd, + cwd: event.cwd.clone(), reason: event.reason.clone(), grant_root: event.grant_root.clone(), }; diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index c8c7046b64..ab51dbadef 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -278,7 +278,7 @@ fn write_permissions_for_paths( /// /// The apply_patch tool can arrive as the older JSON/function shape or as a /// freeform custom tool call. Both represent the same file edit operation, so -/// hooks see the raw patch body in `tool_input.command` either way. +/// hooks see the patch body in `tool_input.command` either way. fn apply_patch_hook_tool_input( invocation: &ToolInvocation, allow_environment_id: bool, @@ -292,9 +292,8 @@ fn apply_patch_hook_tool_input( _ => None, }?; - let mut tool_input = serde_json::json!({ "command": command }); if !allow_environment_id { - return Some((tool_input, None)); + return Some((serde_json::json!({ "command": command }), None)); } let function_environment_id = function_environment_id.as_deref(); if let Some(environment_id) = function_environment_id @@ -303,20 +302,27 @@ fn apply_patch_hook_tool_input( return None; } - let parsed_environment_id = codex_apply_patch::parse_patch_with_options( + let parsed_args = codex_apply_patch::parse_patch_with_options( &command, ParseOptions { allow_environment_id: true, }, ) - .ok() - .and_then(|args| args.environment_id); - let requested_environment_id = match (parsed_environment_id.as_deref(), function_environment_id) - { + .ok(); + let parsed_environment_id = parsed_args + .as_ref() + .and_then(|args| args.environment_id.as_deref()); + let requested_environment_id = match (parsed_environment_id, function_environment_id) { (Some(parsed), Some(function)) if parsed != function => return None, (Some(environment_id), _) | (_, Some(environment_id)) => Some(environment_id), (None, None) => None, }; + let command = parsed_args + .as_ref() + .filter(|args| args.environment_id.is_some()) + .map(|args| args.patch.clone()) + .unwrap_or(command); + let mut tool_input = serde_json::json!({ "command": command }); let selected_environment = match resolve_tool_environment(invocation.turn.as_ref(), requested_environment_id) { Ok(Some(environment)) => environment, diff --git a/codex-rs/core/src/tools/handlers/apply_patch_spec.rs b/codex-rs/core/src/tools/handlers/apply_patch_spec.rs index 1b1b0bfbb2..ee89ab79f0 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_spec.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_spec.rs @@ -154,8 +154,8 @@ pub fn create_apply_patch_json_tool(multi_environment: bool) -> ToolSpec { defer_loading: None, parameters: JsonSchema::object( properties, - Some(vec!["input".to_string()]), - Some(false.into()), + /*required*/ Some(vec!["input".to_string()]), + /*additional_properties*/ Some(false.into()), ), output_schema: None, }) diff --git a/codex-rs/core/src/tools/handlers/apply_patch_spec_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_spec_tests.rs index 07f0c54b29..40226c6798 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_spec_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_spec_tests.rs @@ -66,8 +66,8 @@ fn create_apply_patch_json_tool_matches_expected_spec() { ),), ) ]), - Some(vec!["input".to_string()]), - Some(false.into()) + /*required*/ Some(vec!["input".to_string()]), + /*additional_properties*/ Some(false.into()) ), output_schema: None, }) diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 46fad36197..dbb7cf0a81 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -151,7 +151,7 @@ async fn pre_tool_use_payload_includes_remote_environment_context() { tool_name: HookToolName::apply_patch(), cwd: Some(environment.cwd.clone()), tool_input: json!({ - "command": patch, + "command": sample_patch(), "environment_id": &environment.environment_id, "cwd": &environment.cwd, }), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 77de96cca9..d247268527 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -233,7 +233,7 @@ impl Approvable for ApplyPatchRuntime { } Some(PermissionRequestPayload { tool_name: HookToolName::apply_patch(), - cwd: include_environment_context.then(|| req.action.cwd.clone()), + cwd: Some(req.action.cwd.clone()), tool_input, }) } diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index 6f9c700df8..705eab8caa 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -137,6 +137,7 @@ fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() { .join("apply-patch-permission-request-payload.txt") .abs(); let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); + let expected_cwd = action.cwd.clone(); let expected_patch = action.patch.clone(); let req = ApplyPatchRequest { action, @@ -164,7 +165,7 @@ fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() { payload.tool_input, serde_json::json!({ "command": expected_patch }) ); - assert_eq!(payload.cwd, None); + assert_eq!(payload.cwd, Some(expected_cwd)); } #[test] diff --git a/codex-rs/mcp-server/src/patch_approval.rs b/codex-rs/mcp-server/src/patch_approval.rs index e4a73e1855..3cb19a7397 100644 --- a/codex-rs/mcp-server/src/patch_approval.rs +++ b/codex-rs/mcp-server/src/patch_approval.rs @@ -67,7 +67,7 @@ pub(crate) async fn handle_patch_approval_request( let include_environment_context = !environment_id.is_empty() && environment_id != codex_exec_server::LOCAL_ENVIRONMENT_ID; let codex_environment_id = include_environment_context.then_some(environment_id); - let codex_cwd = include_environment_context.then_some(cwd).flatten(); + let codex_cwd = cwd; if let Some(environment_id) = codex_environment_id.as_deref() { message_lines.push(format!("Environment: {environment_id}")); } diff --git a/codex-rs/mcp-server/tests/suite/codex_tool.rs b/codex-rs/mcp-server/tests/suite/codex_tool.rs index 3b222a3edf..add57fa5e2 100644 --- a/codex-rs/mcp-server/tests/suite/codex_tool.rs +++ b/codex-rs/mcp-server/tests/suite/codex_tool.rs @@ -297,6 +297,7 @@ async fn patch_approval_triggers_elicitation() -> anyhow::Result<()> { expected_changes, /*grant_root*/ None, // No grant_root expected /*reason*/ None, // No reason expected + /*codex_cwd*/ Some(cwd.path().to_path_buf()), codex_request_id.to_string(), params.codex_event_id.clone(), params.thread_id, @@ -445,6 +446,7 @@ fn create_expected_patch_approval_elicitation_request_params( changes: HashMap, grant_root: Option, reason: Option, + codex_cwd: Option, codex_mcp_tool_call_id: String, codex_event_id: String, thread_id: codex_protocol::ThreadId, @@ -453,6 +455,9 @@ fn create_expected_patch_approval_elicitation_request_params( if let Some(r) = &reason { message_lines.push(r.clone()); } + if let Some(cwd) = codex_cwd.as_ref() { + message_lines.push(format!("Working directory: {}", cwd.display())); + } message_lines.push("Allow Codex to apply proposed code changes?".to_string()); let params_json = serde_json::to_value(PatchApprovalElicitRequestParams { message: message_lines.join("\n"), @@ -464,7 +469,7 @@ fn create_expected_patch_approval_elicitation_request_params( codex_reason: reason, codex_grant_root: grant_root, codex_environment_id: None, - codex_cwd: None, + codex_cwd, codex_changes: changes, codex_call_id: "call1234".to_string(), })?;