From 1df9263bf1f0748bb0de8348ff7e6b8c14aa1ea5 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Thu, 7 May 2026 20:40:05 -0700 Subject: [PATCH] codex: fix apply_patch self-review follow-ups Co-authored-by: Codex --- codex-rs/core/src/guardian/approval_request.rs | 3 ++- codex-rs/core/src/guardian/tests.rs | 18 ++++++++++++++++++ .../core/src/tools/handlers/apply_patch.rs | 6 +++--- .../src/tools/handlers/apply_patch_tests.rs | 4 ++-- codex-rs/protocol/src/approvals.rs | 1 + .../tui/src/bottom_pane/approval_overlay.rs | 6 +++++- 6 files changed, 31 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 040e30f6b0..da9f6d348d 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -322,7 +322,8 @@ pub(crate) fn guardian_approval_request_to_json( "files": files, "patch": patch, }); - if environment_id != codex_exec_server::LOCAL_ENVIRONMENT_ID + if !environment_id.is_empty() + && environment_id != codex_exec_server::LOCAL_ENVIRONMENT_ID && let Value::Object(object) = &mut value { object.insert( diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 28b899b3ff..6879953292 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -719,6 +719,24 @@ fn format_guardian_action_pretty_reports_no_truncation_for_small_payload() -> se Ok(()) } +#[test] +fn format_guardian_action_pretty_omits_empty_apply_patch_environment_id() -> serde_json::Result<()> +{ + let action = GuardianApprovalRequest::ApplyPatch { + id: "patch-1".to_string(), + environment_id: String::new(), + cwd: test_path_buf("/tmp").abs(), + files: Vec::new(), + patch: "line\n".to_string(), + }; + + let rendered = format_guardian_action_pretty(&action)?; + + assert!(rendered.text.contains("\"tool\": \"apply_patch\"")); + assert!(!rendered.text.contains("environment_id")); + Ok(()) +} + #[test] fn guardian_approval_request_to_json_renders_mcp_tool_call_shape() -> serde_json::Result<()> { let action = GuardianApprovalRequest::McpToolCall { diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index ab51dbadef..e487d71c04 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -329,10 +329,10 @@ fn apply_patch_hook_tool_input( Ok(None) => return Some((tool_input, None)), Err(_) => return None, }; - if selected_environment.environment_id == codex_exec_server::LOCAL_ENVIRONMENT_ID { - return Some((tool_input, None)); - } let cwd = Some(selected_environment.cwd.clone()); + if selected_environment.environment_id == codex_exec_server::LOCAL_ENVIRONMENT_ID { + return Some((tool_input, cwd)); + } if let Some(object) = tool_input.as_object_mut() { object.insert( 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 dbb7cf0a81..6f1d20be42 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -122,7 +122,7 @@ async fn pre_tool_use_payload_omits_local_environment_context_for_multi_environm handler.pre_tool_use_payload(&invocation), Some(PreToolUsePayload { tool_name: HookToolName::apply_patch(), - cwd: None, + cwd: Some(invocation.turn.cwd.clone()), tool_input: json!({ "command": patch }), }) ); @@ -182,7 +182,7 @@ async fn pre_tool_use_payload_uses_freeform_remote_environment_header() { 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/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 555d272697..a665ad388f 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -447,6 +447,7 @@ mod tests { fn apply_patch_approval_request_deserializes_legacy_shape_without_cwd() { let event: ApplyPatchApprovalRequestEvent = serde_json::from_value(serde_json::json!({ "call_id": "call-1", + "started_at_ms": 0, "changes": {}, })) .expect("legacy apply patch approval request should deserialize"); diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 03e5531e3d..2b2e83a7a1 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -1122,6 +1122,10 @@ mod tests { [ (absolute_path("/tmp/readme.txt"), "/tmp/readme.txt"), (absolute_path("/tmp/out.txt"), "/tmp/out.txt"), + ( + absolute_path("/tmp/remote-worktree"), + "/tmp/remote-worktree", + ), ] .into_iter() .fold(rendered, |rendered, (path, normalized)| { @@ -2043,7 +2047,7 @@ mod tests { keymap.approval, keymap.list, ); - let rendered = render_overlay_lines(&view, /*width*/ 120); + let rendered = normalize_snapshot_paths(render_overlay_lines(&view, /*width*/ 120)); assert_snapshot!( "approval_overlay_apply_patch_remote_prompt", rendered.as_str()