mirror of
https://github.com/openai/codex.git
synced 2026-06-01 19:02:59 +00:00
codex: fix apply_patch review regressions
Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
})
|
||||
|
||||
@@ -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,
|
||||
})
|
||||
|
||||
@@ -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,
|
||||
}),
|
||||
|
||||
@@ -233,7 +233,7 @@ impl Approvable<ApplyPatchRequest> 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,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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]
|
||||
|
||||
Reference in New Issue
Block a user