diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index 16e74ef165..fa3e73e2d8 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -640,11 +640,18 @@ impl From for GuardianApprovalReviewAction { environment_id, cwd, files, - } => Self::ApplyPatch { - environment_id: Some(environment_id), - cwd, - files, - }, + } => { + let environment_id = if environment_id.is_empty() { + None + } else { + Some(environment_id) + }; + Self::ApplyPatch { + environment_id, + cwd, + files, + } + } CoreGuardianAssessmentAction::NetworkAccess { target, host, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index 47925b16ce..ceab904831 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -2160,6 +2160,51 @@ fn guardian_approval_review_action_round_trips_command_shape() { ); } +#[test] +fn guardian_approval_review_action_round_trips_apply_patch_environment() { + let value = json!({ + "type": "applyPatch", + "environmentId": "remote", + "cwd": absolute_path_string("tmp"), + "files": [absolute_path_string("tmp/example.txt")], + }); + let action: GuardianApprovalReviewAction = + serde_json::from_value(value.clone()).expect("guardian review action"); + + assert_eq!( + action, + GuardianApprovalReviewAction::ApplyPatch { + environment_id: Some("remote".to_string()), + cwd: absolute_path("tmp"), + files: vec![absolute_path("tmp/example.txt")], + } + ); + assert_eq!( + serde_json::to_value(&action).expect("serialize guardian review action"), + value + ); +} + +#[test] +fn guardian_approval_review_action_accepts_legacy_apply_patch_environment() { + let value = json!({ + "type": "applyPatch", + "cwd": absolute_path_string("tmp"), + "files": [absolute_path_string("tmp/example.txt")], + }); + let action: GuardianApprovalReviewAction = + serde_json::from_value(value).expect("guardian review action"); + + assert_eq!( + action, + GuardianApprovalReviewAction::ApplyPatch { + environment_id: None, + cwd: absolute_path("tmp"), + files: vec![absolute_path("tmp/example.txt")], + } + ); +} + #[test] fn network_requirements_deserializes_legacy_fields() { let requirements: NetworkRequirements = serde_json::from_value(json!({ diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 11ed1cb0bb..7b32ff7b2e 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -506,16 +506,18 @@ pub(crate) async fn apply_bespoke_event_handling( .note_permission_requested(&conversation_id.to_string()) .await; let item_id = event.call_id.clone(); + 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(), turn_id: event.turn_id.clone(), item_id: item_id.clone(), started_at_ms: event.started_at_ms, - environment_id: (!event.environment_id.is_empty() - && event.environment_id != codex_exec_server::LOCAL_ENVIRONMENT_ID) - .then_some(event.environment_id.clone()), - cwd: event.cwd.clone(), + environment_id, + cwd, reason: event.reason.clone(), grant_root: event.grant_root.clone(), }; diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index a005a464dc..040e30f6b0 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -315,13 +315,23 @@ pub(crate) fn guardian_approval_request_to_json( cwd, files, patch, - } => Ok(serde_json::json!({ - "tool": "apply_patch", - "environment_id": environment_id, - "cwd": cwd, - "files": files, - "patch": patch, - })), + } => { + let mut value = serde_json::json!({ + "tool": "apply_patch", + "cwd": cwd, + "files": files, + "patch": patch, + }); + if environment_id != codex_exec_server::LOCAL_ENVIRONMENT_ID + && let Value::Object(object) = &mut value + { + object.insert( + "environment_id".to_string(), + Value::String(environment_id.clone()), + ); + } + Ok(value) + } GuardianApprovalRequest::NetworkAccess { id: _, turn_id: _, @@ -404,7 +414,11 @@ pub(crate) fn guardian_assessment_action( files, .. } => GuardianAssessmentAction::ApplyPatch { - environment_id: environment_id.clone(), + environment_id: if environment_id == codex_exec_server::LOCAL_ENVIRONMENT_ID { + String::new() + } else { + environment_id.clone() + }, cwd: cwd.clone(), files: files.clone(), }, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 6a738a08bb..28b899b3ff 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -890,7 +890,25 @@ fn guardian_assessment_action_redacts_apply_patch_patch_text() { serde_json::to_value(guardian_assessment_action(&action)).expect("serialize action"), serde_json::json!({ "type": "apply_patch", - "environment_id": "local", + "cwd": cwd, + "files": [file], + }), + ); + + let remote_action = GuardianApprovalRequest::ApplyPatch { + id: "patch-2".to_string(), + environment_id: "remote".to_string(), + cwd: cwd.clone(), + files: vec![file.clone()], + patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+secret\n*** End Patch" + .to_string(), + }; + + assert_eq!( + serde_json::to_value(guardian_assessment_action(&remote_action)).expect("serialize action"), + serde_json::json!({ + "type": "apply_patch", + "environment_id": "remote", "cwd": cwd, "files": [file], }), diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 768fbf1a5b..555d272697 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -147,7 +147,7 @@ pub enum GuardianAssessmentAction { cwd: AbsolutePathBuf, }, ApplyPatch { - #[serde(default)] + #[serde(default, skip_serializing_if = "String::is_empty")] environment_id: String, cwd: AbsolutePathBuf, files: Vec, diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_apply_patch_remote_prompt.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_apply_patch_remote_prompt.snap index 2cd8990d19..91990f9dbf 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_apply_patch_remote_prompt.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_apply_patch_remote_prompt.snap @@ -9,6 +9,7 @@ expression: rendered.as_str() Working directory: /tmp/remote-worktree › 1. Yes, proceed (y) - 2. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for these files (a) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel