mirror of
https://github.com/openai/codex.git
synced 2026-04-30 17:36:40 +00:00
fix(core): emit hooks for apply_patch edits (#18391)
Fixes https://github.com/openai/codex/issues/16732. ## Why `apply_patch` is Codex's primary file edit path, but it was not emitting `PreToolUse` or `PostToolUse` hook events. That meant hook-based policy, auditing, and write coordination could observe shell commands while missing the actual file mutation performed by `apply_patch`. The issue also exposed that the hook runtime serialized command hook payloads with `tool_name: "Bash"` unconditionally. Even if `apply_patch` supplied hook payloads, hooks would either fail to match it directly or receive misleading stdin that identified the edit as a Bash tool call. ## What Changed - Added `PreToolUse` and `PostToolUse` payload support to `ApplyPatchHandler`. - Exposed the raw patch body as `tool_input.command` for both JSON/function and freeform `apply_patch` calls. - Taught tool hook payloads to carry a handler-supplied hook-facing `tool_name`. - Preserved existing shell compatibility by continuing to emit `Bash` for shell-like tools. - Serialized the selected hook `tool_name` into hook stdin instead of hardcoding `Bash`. - Relaxed the generated hook command input schema so `tool_name` can represent tools other than `Bash`. ## Verification Added focused handler coverage for: - JSON/function `apply_patch` calls producing a `PreToolUse` payload. - Freeform `apply_patch` calls producing a `PreToolUse` payload. - Successful `apply_patch` output producing a `PostToolUse` payload. - Shell and `exec_command` handlers continuing to expose `Bash`. Added end-to-end hook coverage for: - A `PreToolUse` hook matching `^apply_patch$` blocking the patch before the target file is created. - A `PostToolUse` hook matching `^apply_patch$` receiving the patch input and tool response, then adding context to the follow-up model request. - Non-participating tools such as the plan tool continuing not to emit `PreToolUse`/`PostToolUse` hook events. Also validated manually with a live `codex exec` smoke test using an isolated temp workspace and temp `CODEX_HOME`. The smoke test confirmed that a real `apply_patch` edit emits `PreToolUse`/`PostToolUse` with `tool_name: "apply_patch"`, a shell command still emits `tool_name: "Bash"`, and a denying `PreToolUse` hook prevents the blocked patch file from being created.
This commit is contained in:
@@ -1,6 +1,7 @@
|
||||
#![allow(clippy::unwrap_used, clippy::expect_used)]
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_core::CodexThread;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_core::config_loader::ConfigLayerStack;
|
||||
@@ -1809,6 +1810,7 @@ async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file()
|
||||
.with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
|
||||
config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config);
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
|
||||
@@ -21,6 +21,7 @@ use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::RolloutLine;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::responses::ev_apply_patch_function_call;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_function_call;
|
||||
@@ -505,11 +506,12 @@ fn read_permission_request_hook_inputs(home: &Path) -> Result<Vec<serde_json::Va
|
||||
|
||||
fn assert_permission_request_hook_input(
|
||||
hook_input: &Value,
|
||||
tool_name: &str,
|
||||
command: &str,
|
||||
description: Option<&str>,
|
||||
) {
|
||||
assert_eq!(hook_input["hook_event_name"], "PermissionRequest");
|
||||
assert_eq!(hook_input["tool_name"], "Bash");
|
||||
assert_eq!(hook_input["tool_name"], tool_name);
|
||||
assert_eq!(hook_input["tool_input"]["command"], command);
|
||||
assert_eq!(
|
||||
hook_input["tool_input"]["description"],
|
||||
@@ -527,10 +529,19 @@ fn assert_single_permission_request_hook_input(
|
||||
home: &Path,
|
||||
command: &str,
|
||||
description: Option<&str>,
|
||||
) -> Result<Vec<serde_json::Value>> {
|
||||
assert_single_permission_request_hook_input_for_tool(home, "Bash", command, description)
|
||||
}
|
||||
|
||||
fn assert_single_permission_request_hook_input_for_tool(
|
||||
home: &Path,
|
||||
tool_name: &str,
|
||||
command: &str,
|
||||
description: Option<&str>,
|
||||
) -> Result<Vec<serde_json::Value>> {
|
||||
let hook_inputs = read_permission_request_hook_inputs(home)?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
assert_permission_request_hook_input(&hook_inputs[0], command, description);
|
||||
assert_permission_request_hook_input(&hook_inputs[0], tool_name, command, description);
|
||||
Ok(hook_inputs)
|
||||
}
|
||||
|
||||
@@ -1223,6 +1234,89 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn permission_request_hook_allows_apply_patch_with_write_alias() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let call_id = "permissionrequest-apply-patch";
|
||||
let file_name = "permission_request_apply_patch.txt";
|
||||
let patch_path = format!("../{file_name}");
|
||||
let patch = format!(
|
||||
r#"*** Begin Patch
|
||||
*** Add File: {patch_path}
|
||||
+approved
|
||||
*** End Patch"#
|
||||
);
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_apply_patch_function_call(call_id, &patch),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "permission request hook allowed apply_patch"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
if let Err(error) = write_permission_request_hook(
|
||||
home,
|
||||
Some("^Write$"),
|
||||
"allow",
|
||||
PERMISSION_REQUEST_ALLOW_REASON,
|
||||
) {
|
||||
panic!("failed to write permission request hook test fixture: {error}");
|
||||
}
|
||||
})
|
||||
.with_config(|config| {
|
||||
config.include_apply_patch_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodexHooks)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
let target_path = test.workspace_path(&patch_path);
|
||||
|
||||
test.submit_turn_with_policies(
|
||||
"apply the patch after hook approval",
|
||||
AskForApproval::OnRequest,
|
||||
SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
read_only_access: Default::default(),
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
},
|
||||
)
|
||||
.await?;
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
requests[1].function_call_output(call_id);
|
||||
assert!(
|
||||
target_path.exists(),
|
||||
"approved apply_patch should create the out-of-workspace file"
|
||||
);
|
||||
|
||||
assert_single_permission_request_hook_input_for_tool(
|
||||
test.codex_home_path(),
|
||||
"apply_patch",
|
||||
&patch,
|
||||
/*description*/ None,
|
||||
)?;
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn permission_request_hook_sees_raw_exec_command_input() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
@@ -1814,7 +1908,159 @@ async fn pre_tool_use_blocks_exec_command_before_execution() -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn pre_tool_use_does_not_fire_for_non_shell_tools() -> Result<()> {
|
||||
async fn pre_tool_use_blocks_apply_patch_before_execution() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let call_id = "pretooluse-apply-patch";
|
||||
let file_name = "pre_tool_use_apply_patch.txt";
|
||||
let patch = format!(
|
||||
r#"*** Begin Patch
|
||||
*** Add File: {file_name}
|
||||
+blocked
|
||||
*** End Patch"#
|
||||
);
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_apply_patch_function_call(call_id, &patch),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "apply_patch blocked"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
if let Err(error) = write_pre_tool_use_hook(
|
||||
home,
|
||||
Some("^apply_patch$"),
|
||||
"json_deny",
|
||||
"blocked apply_patch",
|
||||
) {
|
||||
panic!("failed to write pre tool use hook test fixture: {error}");
|
||||
}
|
||||
})
|
||||
.with_config(|config| {
|
||||
config.include_apply_patch_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodexHooks)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
test.submit_turn("apply the blocked patch").await?;
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
let output_item = requests[1].function_call_output(call_id);
|
||||
let output = output_item
|
||||
.get("output")
|
||||
.and_then(Value::as_str)
|
||||
.expect("apply_patch output string");
|
||||
assert!(
|
||||
output.contains("Command blocked by PreToolUse hook: blocked apply_patch"),
|
||||
"blocked apply_patch output should surface the hook reason",
|
||||
);
|
||||
assert!(
|
||||
!test.workspace_path(file_name).exists(),
|
||||
"blocked apply_patch should not create the file"
|
||||
);
|
||||
|
||||
let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
assert_eq!(hook_inputs[0]["tool_name"], "apply_patch");
|
||||
assert_eq!(hook_inputs[0]["tool_use_id"], call_id);
|
||||
assert_eq!(hook_inputs[0]["tool_input"]["command"], patch);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn pre_tool_use_blocks_apply_patch_with_write_alias() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let call_id = "pretooluse-apply-patch-write";
|
||||
let file_name = "pre_tool_use_apply_patch_write.txt";
|
||||
let patch = format!(
|
||||
r#"*** Begin Patch
|
||||
*** Add File: {file_name}
|
||||
+blocked
|
||||
*** End Patch"#
|
||||
);
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_apply_patch_function_call(call_id, &patch),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "apply_patch blocked by Write alias"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
if let Err(error) =
|
||||
write_pre_tool_use_hook(home, Some("^Write$"), "json_deny", "blocked write alias")
|
||||
{
|
||||
panic!("failed to write pre tool use hook test fixture: {error}");
|
||||
}
|
||||
})
|
||||
.with_config(|config| {
|
||||
config.include_apply_patch_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodexHooks)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
test.submit_turn("apply the patch blocked by Write alias")
|
||||
.await?;
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
let output_item = requests[1].function_call_output(call_id);
|
||||
let output = output_item
|
||||
.get("output")
|
||||
.and_then(Value::as_str)
|
||||
.expect("apply_patch output string");
|
||||
assert!(
|
||||
output.contains("Command blocked by PreToolUse hook: blocked write alias"),
|
||||
"blocked apply_patch output should surface the hook reason",
|
||||
);
|
||||
assert!(
|
||||
!test.workspace_path(file_name).exists(),
|
||||
"blocked apply_patch should not create the file"
|
||||
);
|
||||
|
||||
let hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
assert_eq!(hook_inputs[0]["tool_name"], "apply_patch");
|
||||
assert_eq!(hook_inputs[0]["tool_use_id"], call_id);
|
||||
assert_eq!(hook_inputs[0]["tool_input"]["command"], patch);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn pre_tool_use_does_not_fire_for_plan_tool() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
@@ -1879,7 +2125,7 @@ async fn pre_tool_use_does_not_fire_for_non_shell_tools() -> Result<()> {
|
||||
let hook_log_path = test.codex_home_path().join("pre_tool_use_hook_log.jsonl");
|
||||
assert!(
|
||||
!hook_log_path.exists(),
|
||||
"non-shell tools should not trigger pre tool use hooks",
|
||||
"plan tool should not trigger pre tool use hooks",
|
||||
);
|
||||
|
||||
Ok(())
|
||||
@@ -2269,7 +2515,171 @@ async fn post_tool_use_exit_two_replaces_one_shot_exec_command_output_with_feedb
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn post_tool_use_does_not_fire_for_non_shell_tools() -> Result<()> {
|
||||
async fn post_tool_use_records_additional_context_for_apply_patch() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let call_id = "posttooluse-apply-patch";
|
||||
let file_name = "post_tool_use_apply_patch.txt";
|
||||
let patch = format!(
|
||||
r#"*** Begin Patch
|
||||
*** Add File: {file_name}
|
||||
+patched
|
||||
*** End Patch"#
|
||||
);
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_apply_patch_function_call(call_id, &patch),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "apply_patch post hook context observed"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let post_context = "Remember the apply_patch post-tool note.";
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
if let Err(error) =
|
||||
write_post_tool_use_hook(home, Some("^apply_patch$"), "context", post_context)
|
||||
{
|
||||
panic!("failed to write post tool use hook test fixture: {error}");
|
||||
}
|
||||
})
|
||||
.with_config(|config| {
|
||||
config.include_apply_patch_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodexHooks)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
test.submit_turn("apply the patch with post hook").await?;
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
assert!(
|
||||
requests[1]
|
||||
.message_input_texts("developer")
|
||||
.contains(&post_context.to_string()),
|
||||
"follow-up request should include apply_patch post tool use context",
|
||||
);
|
||||
assert!(
|
||||
test.workspace_path(file_name).exists(),
|
||||
"apply_patch should create the file"
|
||||
);
|
||||
|
||||
let hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
assert_eq!(hook_inputs[0]["tool_name"], "apply_patch");
|
||||
assert_eq!(hook_inputs[0]["tool_use_id"], call_id);
|
||||
assert_eq!(hook_inputs[0]["tool_input"]["command"], patch);
|
||||
let tool_response = hook_inputs[0]["tool_response"]
|
||||
.as_str()
|
||||
.context("apply_patch tool_response should be a string")?;
|
||||
let mut parsed_tool_response = serde_json::from_str::<Value>(tool_response)?;
|
||||
if let Some(metadata) = parsed_tool_response
|
||||
.get_mut("metadata")
|
||||
.and_then(Value::as_object_mut)
|
||||
{
|
||||
let _ = metadata.remove("duration_seconds");
|
||||
}
|
||||
assert_eq!(
|
||||
parsed_tool_response,
|
||||
serde_json::json!({
|
||||
"output": "Success. Updated the following files:\nA post_tool_use_apply_patch.txt\n",
|
||||
"metadata": {
|
||||
"exit_code": 0,
|
||||
},
|
||||
})
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn post_tool_use_records_apply_patch_context_with_edit_alias() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let call_id = "posttooluse-apply-patch-edit";
|
||||
let file_name = "post_tool_use_apply_patch_edit.txt";
|
||||
let patch = format!(
|
||||
r#"*** Begin Patch
|
||||
*** Add File: {file_name}
|
||||
+patched
|
||||
*** End Patch"#
|
||||
);
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_apply_patch_function_call(call_id, &patch),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-2"),
|
||||
ev_assistant_message("msg-1", "apply_patch edit hook context observed"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let post_context = "Remember the edit alias post-tool note.";
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
if let Err(error) =
|
||||
write_post_tool_use_hook(home, Some("^Edit$"), "context", post_context)
|
||||
{
|
||||
panic!("failed to write post tool use hook test fixture: {error}");
|
||||
}
|
||||
})
|
||||
.with_config(|config| {
|
||||
config.include_apply_patch_tool = true;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodexHooks)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
test.submit_turn("apply the patch with edit alias post hook")
|
||||
.await?;
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
assert!(
|
||||
requests[1]
|
||||
.message_input_texts("developer")
|
||||
.contains(&post_context.to_string()),
|
||||
"follow-up request should include apply_patch post tool use context",
|
||||
);
|
||||
assert!(
|
||||
test.workspace_path(file_name).exists(),
|
||||
"apply_patch should create the file"
|
||||
);
|
||||
|
||||
let hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
assert_eq!(hook_inputs[0]["tool_name"], "apply_patch");
|
||||
assert_eq!(hook_inputs[0]["tool_use_id"], call_id);
|
||||
assert_eq!(hook_inputs[0]["tool_input"]["command"], patch);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn post_tool_use_does_not_fire_for_plan_tool() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
@@ -2337,7 +2747,7 @@ async fn post_tool_use_does_not_fire_for_non_shell_tools() -> Result<()> {
|
||||
let hook_log_path = test.codex_home_path().join("post_tool_use_hook_log.jsonl");
|
||||
assert!(
|
||||
!hook_log_path.exists(),
|
||||
"non-shell tools should not trigger post tool use hooks",
|
||||
"plan tool should not trigger post tool use hooks",
|
||||
);
|
||||
|
||||
Ok(())
|
||||
|
||||
Reference in New Issue
Block a user