clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-05-16 17:23:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

codex: address apply_patch review pass

Browse Source 
Fix remote apply_patch hook environment reconciliation so malformed environment requests do not run hooks against the primary environment. Preserve local/default hook payload shapes, align Guardian app-server optionality, and surface remote approval cwd in the TUI.

Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
starr-openai
2026-05-07 20:00:34 -07:00
parent 52b025f591
commit dd74dc860d
14 changed files with 322 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
codex-rs/app-server-protocol/schema/json/ServerNotification.json generated
View File

@@ -1530,8 +1530,11 @@
"$ref": "#/definitions/AbsolutePathBuf"
},
"environmentId": {
"default": "",
"type": "string"
"default": null,
"type": [
"string",
"null"
]
},
"files": {
"items": {

7
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json generated
View File

@@ -9271,8 +9271,11 @@
"$ref": "#/definitions/v2/AbsolutePathBuf"
},
"environmentId": {
"default": "",
"type": "string"
"default": null,
"type": [
"string",
"null"
]
},
"files": {
"items": {

7
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json generated
View File

@@ -5800,8 +5800,11 @@
"$ref": "#/definitions/AbsolutePathBuf"
},
"environmentId": {
"default": "",
"type": "string"
"default": null,
"type": [
"string",
"null"
]
},
"files": {
"items": {

7
codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewCompletedNotification.json generated
View File

@@ -370,8 +370,11 @@
"$ref": "#/definitions/AbsolutePathBuf"
},
"environmentId": {
"default": "",
"type": "string"
"default": null,
"type": [
"string",
"null"
]
},
"files": {
"items": {

7
codex-rs/app-server-protocol/schema/json/v2/ItemGuardianApprovalReviewStartedNotification.json generated
View File

@@ -363,8 +363,11 @@
"$ref": "#/definitions/AbsolutePathBuf"
},
"environmentId": {
"default": "",
"type": "string"
"default": null,
"type": [
"string",
"null"
]
},
"files": {
"items": {

2
codex-rs/app-server-protocol/schema/typescript/v2/GuardianApprovalReviewAction.ts generated
View File

@@ -6,4 +6,4 @@ import type { GuardianCommandSource } from "./GuardianCommandSource";
import type { NetworkApprovalProtocol } from "./NetworkApprovalProtocol";
import type { RequestPermissionProfile } from "./RequestPermissionProfile";
export type GuardianApprovalReviewAction = { "type": "command", source: GuardianCommandSource, command: string, cwd: AbsolutePathBuf, } | { "type": "execve", source: GuardianCommandSource, program: string, argv: Array<string>, cwd: AbsolutePathBuf, } | { "type": "applyPatch", environmentId: string, cwd: AbsolutePathBuf, files: Array<AbsolutePathBuf>, } | { "type": "networkAccess", target: string, host: string, protocol: NetworkApprovalProtocol, port: number, } | { "type": "mcpToolCall", server: string, toolName: string, connectorId: string | null, connectorName: string | null, toolTitle: string | null, } | { "type": "requestPermissions", reason: string | null, permissions: RequestPermissionProfile, };
export type GuardianApprovalReviewAction = { "type": "command", source: GuardianCommandSource, command: string, cwd: AbsolutePathBuf, } | { "type": "execve", source: GuardianCommandSource, program: string, argv: Array<string>, cwd: AbsolutePathBuf, } | { "type": "applyPatch", environmentId?: string | null, cwd: AbsolutePathBuf, files: Array<AbsolutePathBuf>, } | { "type": "networkAccess", target: string, host: string, protocol: NetworkApprovalProtocol, port: number, } | { "type": "mcpToolCall", server: string, toolName: string, connectorId: string | null, connectorName: string | null, toolTitle: string | null, } | { "type": "requestPermissions", reason: string | null, permissions: RequestPermissionProfile, };

7
codex-rs/app-server-protocol/src/protocol/v2/item.rs
View File

@@ -584,7 +584,8 @@ pub enum GuardianApprovalReviewAction {
#[ts(rename_all = "camelCase")]
ApplyPatch {
#[serde(default)]
environment_id: String,
#[ts(optional = nullable)]
environment_id: Option<String>,
cwd: AbsolutePathBuf,
files: Vec<AbsolutePathBuf>,
},
@@ -641,7 +642,7 @@ impl From<CoreGuardianAssessmentAction> for GuardianApprovalReviewAction {
cwd,
files,
} => Self::ApplyPatch {
environment_id,
environment_id: Some(environment_id),
cwd,
files,
},
@@ -708,7 +709,7 @@ impl From<GuardianApprovalReviewAction> for CoreGuardianAssessmentAction {
cwd,
files,
} => Self::ApplyPatch {
environment_id,
environment_id: environment_id.unwrap_or_default(),
cwd,
files,
},

34
codex-rs/core/src/tools/handlers/apply_patch.rs
View File

@@ -296,11 +296,11 @@ fn apply_patch_hook_tool_input(
if !allow_environment_id {
return Some((tool_input, None));
}
if function_environment_id
.as_deref()
.is_some_and(|environment_id| environment_id.trim().is_empty())
let function_environment_id = function_environment_id.as_deref();
if let Some(environment_id) = function_environment_id
&& environment_id.trim().is_empty()
{
return Some((tool_input, None));
return None;
}
let parsed_environment_id = codex_apply_patch::parse_patch_with_options(
@@ -311,31 +311,31 @@ fn apply_patch_hook_tool_input(
)
.ok()
.and_then(|args| args.environment_id);
let function_environment_id = function_environment_id
.as_deref()
.filter(|environment_id| !environment_id.trim().is_empty());
let requested_environment_id = match (parsed_environment_id.as_deref(), function_environment_id)
{
(Some(parsed), Some(function)) if parsed != function => None,
(Some(parsed), Some(function)) if parsed != function => return None,
(Some(environment_id), _) | (_, Some(environment_id)) => Some(environment_id),
(None, None) => None,
};
let selected_environment =
resolve_tool_environment(invocation.turn.as_ref(), requested_environment_id)
.ok()
.flatten();
let cwd = selected_environment.map(|environment| environment.cwd.clone());
match resolve_tool_environment(invocation.turn.as_ref(), requested_environment_id) {
Ok(Some(environment)) => environment,
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 let Some(object) = tool_input.as_object_mut()
&& let Some(environment) = selected_environment
{
if let Some(object) = tool_input.as_object_mut() {
object.insert(
"environment_id".to_string(),
serde_json::Value::String(environment.environment_id.clone()),
serde_json::Value::String(selected_environment.environment_id.clone()),
);
object.insert(
"cwd".to_string(),
serde_json::json!(environment.cwd.clone()),
serde_json::json!(selected_environment.cwd.clone()),
);
}
Some((tool_input, cwd))

141
codex-rs/core/src/tools/handlers/apply_patch_tests.rs
View File

@@ -15,6 +15,7 @@ use tempfile::TempDir;
use tokio::sync::Mutex;
use crate::session::tests::make_session_and_context;
use crate::session::turn_context::TurnEnvironment;
use crate::tools::context::ToolInvocation;
use crate::tools::hook_names::HookToolName;
use crate::tools::registry::PostToolUsePayload;
@@ -42,6 +43,27 @@ async fn invocation_for_payload(payload: ToolPayload) -> ToolInvocation {
}
}
fn add_remote_environment(invocation: &mut ToolInvocation) -> TurnEnvironment {
let primary = invocation
.turn
.environments
.primary()
.expect("primary env")
.clone();
let remote = TurnEnvironment {
environment_id: "remote".to_string(),
environment: primary.environment,
cwd: primary.cwd.join("remote"),
shell: primary.shell,
};
Arc::get_mut(&mut invocation.turn)
.expect("unique turn context")
.environments
.turn_environments
.push(remote.clone());
remote
}
#[tokio::test]
async fn pre_tool_use_payload_uses_json_patch_input() {
let patch = sample_patch();
@@ -81,7 +103,7 @@ async fn pre_tool_use_payload_uses_freeform_patch_input() {
}
#[tokio::test]
async fn pre_tool_use_payload_includes_environment_context_for_multi_environment() {
async fn pre_tool_use_payload_omits_local_environment_context_for_multi_environment() {
let patch = sample_patch();
let invocation = invocation_for_payload(ToolPayload::Function {
arguments: json!({
@@ -91,7 +113,33 @@ async fn pre_tool_use_payload_includes_environment_context_for_multi_environment
.to_string(),
})
.await;
let environment = invocation.turn.environments.primary().expect("primary env");
let handler = ApplyPatchHandler::new(
ApplyPatchToolType::Function,
/*multi_environment*/ true,
);
assert_eq!(
handler.pre_tool_use_payload(&invocation),
Some(PreToolUsePayload {
tool_name: HookToolName::apply_patch(),
cwd: None,
tool_input: json!({ "command": patch }),
})
);
}
#[tokio::test]
async fn pre_tool_use_payload_includes_remote_environment_context() {
let patch = sample_patch();
let mut invocation = invocation_for_payload(ToolPayload::Function {
arguments: json!({
"input": patch,
"environment_id": "remote",
})
.to_string(),
})
.await;
let environment = add_remote_environment(&mut invocation);
let handler = ApplyPatchHandler::new(
ApplyPatchToolType::Function,
/*multi_environment*/ true,
@@ -111,6 +159,61 @@ async fn pre_tool_use_payload_includes_environment_context_for_multi_environment
);
}
#[tokio::test]
async fn pre_tool_use_payload_uses_freeform_remote_environment_header() {
let patch = r#"*** Begin Patch
*** Environment ID: remote
*** Add File: hello.txt
+hello
*** End Patch"#;
let mut invocation = invocation_for_payload(ToolPayload::Custom {
input: patch.to_string(),
})
.await;
let environment = add_remote_environment(&mut invocation);
let handler = ApplyPatchHandler::new(
ApplyPatchToolType::Freeform,
/*multi_environment*/ true,
);
assert_eq!(
handler.pre_tool_use_payload(&invocation),
Some(PreToolUsePayload {
tool_name: HookToolName::apply_patch(),
cwd: Some(environment.cwd.clone()),
tool_input: json!({
"command": patch,
"environment_id": environment.environment_id.clone(),
"cwd": environment.cwd.clone(),
}),
})
);
}
#[tokio::test]
async fn pre_tool_use_payload_rejects_environment_mismatch() {
let patch = r#"*** Begin Patch
*** Environment ID: remote
*** Add File: hello.txt
+hello
*** End Patch"#;
let mut invocation = invocation_for_payload(ToolPayload::Function {
arguments: json!({
"input": patch,
"environment_id": "other",
})
.to_string(),
})
.await;
add_remote_environment(&mut invocation);
let handler = ApplyPatchHandler::new(
ApplyPatchToolType::Function,
/*multi_environment*/ true,
);
assert_eq!(handler.pre_tool_use_payload(&invocation), None);
}
#[tokio::test]
async fn post_tool_use_payload_uses_patch_input_and_tool_output() {
let patch = sample_patch();
@@ -133,6 +236,40 @@ async fn post_tool_use_payload_uses_patch_input_and_tool_output() {
);
}
#[tokio::test]
async fn post_tool_use_payload_includes_remote_environment_context() {
let patch = sample_patch();
let mut invocation = invocation_for_payload(ToolPayload::Function {
arguments: json!({
"input": patch,
"environment_id": "remote",
})
.to_string(),
})
.await;
let environment = add_remote_environment(&mut invocation);
let output = ApplyPatchToolOutput::from_text("Success. Updated files.".to_string());
let handler = ApplyPatchHandler::new(
ApplyPatchToolType::Function,
/*multi_environment*/ true,
);
assert_eq!(
handler.post_tool_use_payload(&invocation, &output),
Some(PostToolUsePayload {
tool_name: HookToolName::apply_patch(),
tool_use_id: "call-apply-patch".to_string(),
cwd: Some(environment.cwd.clone()),
tool_input: json!({
"command": patch,
"environment_id": environment.environment_id.clone(),
"cwd": environment.cwd.clone(),
}),
tool_response: json!("Success. Updated files."),
})
);
}
#[test]
fn diff_consumer_does_not_stream_json_tool_call_arguments() {
let mut consumer = ApplyPatchArgumentDiffConsumer::default();

26
codex-rs/core/src/tools/runtimes/apply_patch.rs
View File

@@ -217,19 +217,33 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
&self,
req: &ApplyPatchRequest,
) -> Option<PermissionRequestPayload> {
let include_environment_context = !req.environment_id.is_empty()
&& req.environment_id != codex_exec_server::LOCAL_ENVIRONMENT_ID;
let mut tool_input = serde_json::json!({
"command": req.action.patch.clone(),
});
if let Some(object) = tool_input.as_object_mut()
&& include_environment_context
{
object.insert(
"environment_id".to_string(),
serde_json::Value::String(req.environment_id.clone()),
);
object.insert("cwd".to_string(), serde_json::json!(req.action.cwd.clone()));
}
Some(PermissionRequestPayload {
tool_name: HookToolName::apply_patch(),
cwd: Some(req.action.cwd.clone()),
tool_input: serde_json::json!({
"command": req.action.patch.clone(),
"environment_id": req.environment_id.clone(),
"cwd": req.action.cwd.clone(),
}),
cwd: include_environment_context.then(|| req.action.cwd.clone()),
tool_input,
})
}
}
impl ToolRuntime<ApplyPatchRequest, ApplyPatchRuntimeOutput> for ApplyPatchRuntime {
fn sandbox_cwd<'a>(&self, req: &'a ApplyPatchRequest) -> Option<&'a AbsolutePathBuf> {
Some(&req.action.cwd)
}
async fn run(
&mut self,
req: &ApplyPatchRequest,

58
codex-rs/core/src/tools/runtimes/apply_patch_tests.rs
View File

@@ -108,6 +108,28 @@ fn approval_keys_include_environment_id() {
);
}
#[test]
fn sandbox_cwd_uses_patch_cwd() {
let runtime = ApplyPatchRuntime::new();
let path = std::env::temp_dir()
.join("apply-patch-sandbox-cwd.txt")
.abs();
let req = ApplyPatchRequest {
action: ApplyPatchAction::new_add_for_test(&path, "hello".to_string()),
environment_id: "remote".to_string(),
file_paths: vec![path],
changes: HashMap::new(),
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: None,
},
additional_permissions: None,
permissions_preapproved: false,
};
assert_eq!(runtime.sandbox_cwd(&req), Some(&req.action.cwd));
}
#[test]
fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() {
let runtime = ApplyPatchRuntime::new();
@@ -115,7 +137,6 @@ 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,
@@ -139,11 +160,44 @@ fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() {
payload.tool_name.matcher_aliases(),
&["Write".to_string(), "Edit".to_string()]
);
assert_eq!(
payload.tool_input,
serde_json::json!({ "command": expected_patch })
);
assert_eq!(payload.cwd, None);
}
#[test]
fn permission_request_payload_includes_remote_environment_context() {
let runtime = ApplyPatchRuntime::new();
let path = std::env::temp_dir()
.join("apply-patch-remote-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,
environment_id: "remote".to_string(),
file_paths: vec![path],
changes: HashMap::new(),
exec_approval_requirement: ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: None,
},
additional_permissions: None,
permissions_preapproved: false,
};
let payload = runtime
.permission_request_payload(&req)
.expect("permission request payload");
assert_eq!(
payload.tool_input,
serde_json::json!({
"command": expected_patch,
"environment_id": "local",
"environment_id": "remote",
"cwd": expected_cwd,
})
);

4
codex-rs/tui/src/app/thread_routing.rs
View File

@@ -259,7 +259,9 @@ impl App {
environment_id: params
.environment_id
.clone()
.filter(|id| id != codex_exec_server::LOCAL_ENVIRONMENT_ID),
.filter(|id| {
!id.is_empty() && id != codex_exec_server::LOCAL_ENVIRONMENT_ID
}),
reason: params.reason.clone(),
cwd,
changes: self

43
codex-rs/tui/src/bottom_pane/approval_overlay.rs
View File

@@ -695,6 +695,7 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
thread_label,
environment_id,
reason,
cwd,
..
} => {
let mut header: Vec<Box<dyn Renderable>> = Vec::new();
@@ -726,6 +727,10 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
"Environment: ".into(),
environment_id.clone().bold(),
])));
header.push(Box::new(Line::from(vec![
"Working directory: ".into(),
cwd.display().to_string().dim(),
])));
}
Box::new(ColumnRenderable::with(header))
}
@@ -2009,6 +2014,44 @@ mod tests {
assert!(!rendered.contains("$ apply_patch"));
}
#[test]
fn apply_patch_prompt_with_environment_shows_cwd() {
let (tx, _rx) = unbounded_channel::<AppEvent>();
let tx = AppEventSender::new(tx);
let mut changes = HashMap::new();
changes.insert(
PathBuf::from("bug1.txt"),
FileChange::Add {
content: "one\ntwo\nthree\n".to_string(),
},
);
let cwd = absolute_path("/tmp/remote-worktree");
let request = ApprovalRequest::ApplyPatch {
thread_id: ThreadId::new(),
thread_label: None,
id: "test".to_string(),
environment_id: Some("remote".to_string()),
reason: None,
cwd,
changes,
};
let keymap = crate::keymap::RuntimeKeymap::defaults();
let view = ApprovalOverlay::new(
request,
tx,
Features::with_defaults(),
keymap.approval,
keymap.list,
);
let rendered = render_overlay_lines(&view, /*width*/ 120);
assert_snapshot!(
"approval_overlay_apply_patch_remote_prompt",
rendered.clone()
);
assert!(rendered.contains("Environment: remote"));
assert!(rendered.contains("Working directory: /tmp/remote-worktree"));
}
#[test]
fn network_exec_prompt_title_includes_host() {
let (tx, _rx) = unbounded_channel::<AppEvent>();

14
codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_apply_patch_remote_prompt.snap Normal file
View File

@@ -0,0 +1,14 @@
---
source: tui/src/bottom_pane/approval_overlay.rs
expression: rendered.clone()
---
Would you like to make the following edits?
Environment: remote
Working directory: /tmp/remote-worktree
› 1. Yes, proceed (y)
2. No, and tell Codex what to do differently (esc)
Press enter to confirm or esc to cancel