diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index c027bd16b..23294e694 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -594,7 +594,6 @@ impl ToolRegistry { } else { None }; - if let Some(outcome) = &post_tool_use_outcome { record_additional_contexts( &invocation.session, @@ -602,32 +601,9 @@ impl ToolRegistry { outcome.additional_contexts.clone(), ) .await; - let replacement_text = if outcome.should_stop { - Some( - outcome - .feedback_message - .clone() - .or_else(|| outcome.stop_reason.clone()) - .unwrap_or_else(|| "PostToolUse hook stopped execution".to_string()), - ) - } else { - outcome.feedback_message.clone() - }; - if let Some(replacement_text) = replacement_text { - let mut guard = response_cell.lock().await; - if let Some(mut result) = guard.take() { - result.result = Box::new(PostToolUseFeedbackOutput { - original: result.result, - model_visible: FunctionToolOutput::from_text( - replacement_text, - /*success*/ None, - ), - }); - *guard = Some(result); - } - } } + // A PostToolUse block rejects the result, not the already-completed tool execution. let lifecycle_outcome = match &result { Ok(_) => { let guard = response_cell.lock().await; @@ -654,9 +630,28 @@ impl ToolRegistry { match result { Ok(_) => { let mut guard = response_cell.lock().await; - let result = guard.take().ok_or_else(|| { + let mut result = guard.take().ok_or_else(|| { FunctionCallError::Fatal("tool produced no output".to_string()) })?; + if let Some(outcome) = post_tool_use_outcome { + if outcome.should_block { + let message = outcome.feedback_message.unwrap_or_else(|| { + "PostToolUse hook blocked the tool result".to_string() + }); + let err = FunctionCallError::RespondToModel(message); + dispatch_trace.record_failed(&err); + return Err(err); + } + if let Some(feedback_message) = outcome.feedback_message { + result.result = Box::new(PostToolUseFeedbackOutput { + original: result.result, + model_visible: FunctionToolOutput::from_text( + feedback_message, + /*success*/ None, + ), + }); + } + } dispatch_trace.record_completed( &invocation, &result.call_id, diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 31e54c5e7..0127dd74b 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -77,6 +77,23 @@ fn network_workspace_write_profile() -> PermissionProfile { ) } +fn code_mode_custom_tool_output_text(output_item: &Value) -> String { + match output_item.get("output") { + Some(Value::String(text)) => text.clone(), + Some(Value::Array(items)) => items + .iter() + .filter_map(|item| item.get("text").and_then(Value::as_str)) + .collect::>() + .join("\n"), + Some(Value::Object(output)) => output + .get("content") + .and_then(Value::as_str) + .unwrap_or_default() + .to_string(), + output => panic!("unexpected code mode custom tool output: {output:?}"), + } +} + fn non_openai_model_provider(server: &wiremock::MockServer) -> ModelProviderInfo { let mut provider = built_in_model_providers(/* openai_base_url */ /*openai_base_url*/ None)["openai"].clone(); @@ -2738,10 +2755,17 @@ async fn pre_tool_use_rewrites_code_mode_nested_exec_command_before_execution() let server = start_mock_server().await; let call_id = "pretooluse-code-mode-rewrite"; - let original_marker = std::env::temp_dir().join("pretooluse-code-mode-original-marker"); - let rewritten_marker = std::env::temp_dir().join("pretooluse-code-mode-rewritten-marker"); - let original_command = format!("printf original > {}", original_marker.display()); - let rewritten_command = format!("printf rewritten > {}", rewritten_marker.display()); + let marker_dir = TempDir::new().context("create pre tool rewrite marker directory")?; + let original_marker = marker_dir.path().join("original"); + let rewritten_marker = marker_dir.path().join("rewritten"); + let original_command = format!( + "printf original > {}; printf original-result", + original_marker.display() + ); + let rewritten_command = format!( + "printf rewritten > {}; printf rewritten-result", + rewritten_marker.display() + ); let original_command_json = serde_json::to_string(&original_command).context("serialize original command")?; let code = format!( @@ -2781,13 +2805,6 @@ text(output.output); }); let test = builder.build(&server).await?; - if original_marker.exists() { - fs::remove_file(&original_marker).context("remove stale original pre tool marker")?; - } - if rewritten_marker.exists() { - fs::remove_file(&rewritten_marker).context("remove stale rewritten pre tool marker")?; - } - test.submit_turn_with_permission_profile( "run the rewritten shell command from code mode", PermissionProfile::Disabled, @@ -2796,7 +2813,16 @@ text(output.output); let requests = responses.requests(); assert_eq!(requests.len(), 2); - requests[1].custom_tool_call_output(call_id); + let output_item = requests[1].custom_tool_call_output(call_id); + let output = code_mode_custom_tool_output_text(&output_item); + assert!( + output.contains("rewritten-result"), + "code mode should receive the rewritten command result" + ); + assert!( + !output.contains("original-result"), + "code mode should not receive the original command result" + ); assert!( !original_marker.exists(), "original nested shell command should not execute after rewrite" @@ -2814,6 +2840,186 @@ text(output.output); Ok(()) } +#[tokio::test] +async fn pre_tool_use_block_rejects_code_mode_tool_promise_before_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-code-mode-block"; + let marker_dir = TempDir::new().context("create pre tool block marker directory")?; + let marker = marker_dir.path().join("blocked"); + let command = format!("printf blocked > {}", marker.display()); + let command_json = serde_json::to_string(&command).context("serialize blocked command")?; + let code = format!( + r#" +try {{ + const result = await tools.exec_command({{ cmd: {command_json} }}); + text(JSON.stringify({{ kind: "unexpected-success", result }})); +}} catch (error) {{ + text(JSON.stringify({{ kind: "caught", error: String(error) }})); +}} +"# + ); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_custom_tool_call(call_id, "exec", &code), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "pre hook block observed"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let reason = "blocked nested command"; + let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") + .with_pre_build_hook(move |home| { + if let Err(error) = write_pre_tool_use_hook(home, Some("^Bash$"), "json_deny", reason) { + panic!("failed to write blocking pre tool use hook fixture: {error}"); + } + }) + .with_config(|config| { + let _ = config.features.enable(Feature::CodeMode); + trust_discovered_hooks(config); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_permission_profile( + "run the blocked shell command from code mode", + PermissionProfile::Disabled, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].custom_tool_call_output(call_id); + let output = code_mode_custom_tool_output_text(&output_item); + assert!(output.contains(r#""kind":"caught""#)); + assert!(output.contains(reason)); + assert!(!output.contains("unexpected-success")); + assert!( + !marker.exists(), + "PreToolUse-blocked nested command should not execute" + ); + + 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_input"]["command"], command); + + Ok(()) +} + +async fn assert_post_tool_use_blocks_code_mode_tool_result( + hook_mode: &'static str, + reason: &'static str, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = format!("posttooluse-code-mode-{hook_mode}"); + let marker_dir = TempDir::new().context("create post tool block marker directory")?; + let marker = marker_dir.path().join(hook_mode); + let command = format!( + "printf executed > {}; printf original-post-tool-result", + marker.display() + ); + let command_json = serde_json::to_string(&command).context("serialize post hook command")?; + let code = format!( + r#" +try {{ + const result = await tools.exec_command({{ cmd: {command_json} }}); + text(JSON.stringify({{ kind: "unexpected-success", result }})); +}} catch (error) {{ + text(JSON.stringify({{ kind: "caught", error: String(error) }})); +}} +"# + ); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_custom_tool_call(&call_id, "exec", &code), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "post hook block observed"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_model("test-gpt-5.1-codex") + .with_pre_build_hook(move |home| { + if let Err(error) = write_post_tool_use_hook(home, Some("^Bash$"), hook_mode, reason) { + panic!("failed to write blocking post tool use hook fixture: {error}"); + } + }) + .with_config(|config| { + let _ = config.features.enable(Feature::CodeMode); + trust_discovered_hooks(config); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_permission_profile( + "run the shell command blocked after execution from code mode", + PermissionProfile::Disabled, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + let output_item = requests[1].custom_tool_call_output(&call_id); + let output = code_mode_custom_tool_output_text(&output_item); + assert!(output.contains(r#""kind":"caught""#)); + assert!(output.contains(reason)); + assert!(!output.contains("unexpected-success")); + assert!( + !output.contains("original-post-tool-result"), + "blocked post tool result should not reach code mode" + ); + assert_eq!( + fs::read_to_string(&marker).context("read blocking post tool marker")?, + "executed", + "PostToolUse should run after the nested command executes" + ); + + 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_input"]["command"], command); + assert_eq!( + hook_inputs[0]["tool_response"], + Value::String("original-post-tool-result".to_string()) + ); + + Ok(()) +} + +#[tokio::test] +async fn post_tool_use_block_decision_rejects_code_mode_tool_promise() -> Result<()> { + assert_post_tool_use_blocks_code_mode_tool_result( + "decision_block", + "blocked nested result by decision", + ) + .await +} + +#[tokio::test] +async fn post_tool_use_exit_two_rejects_code_mode_tool_promise() -> Result<()> { + assert_post_tool_use_blocks_code_mode_tool_result("exit_2", "blocked nested result by exit two") + .await +} + #[tokio::test] async fn plugin_pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/src/events/post_tool_use.rs b/codex-rs/hooks/src/events/post_tool_use.rs index f096de011..71adb11cd 100644 --- a/codex-rs/hooks/src/events/post_tool_use.rs +++ b/codex-rs/hooks/src/events/post_tool_use.rs @@ -38,16 +38,14 @@ pub struct PostToolUseRequest { #[derive(Debug)] pub struct PostToolUseOutcome { pub hook_events: Vec, - pub should_stop: bool, - pub stop_reason: Option, + pub should_block: bool, pub additional_contexts: Vec, pub feedback_message: Option, } #[derive(Debug, Default, PartialEq, Eq)] struct PostToolUseHandlerData { - should_stop: bool, - stop_reason: Option, + should_block: bool, additional_contexts_for_model: Vec, feedback_messages_for_model: Vec, } @@ -83,8 +81,7 @@ pub(crate) async fn run( if matched.is_empty() { return PostToolUseOutcome { hook_events: Vec::new(), - should_stop: false, - stop_reason: None, + should_block: false, additional_contexts: Vec::new(), feedback_message: None, }; @@ -118,10 +115,7 @@ pub(crate) async fn run( .iter() .map(|result| result.data.additional_contexts_for_model.as_slice()), ); - let should_stop = results.iter().any(|result| result.data.should_stop); - let stop_reason = results - .iter() - .find_map(|result| result.data.stop_reason.clone()); + let should_block = results.iter().any(|result| result.data.should_block); let feedback_message = common::join_text_chunks( results .iter() @@ -136,8 +130,7 @@ pub(crate) async fn run( common::hook_completed_for_tool_use(result.completed, &request.tool_use_id) }) .collect(), - should_stop, - stop_reason, + should_block, additional_contexts, feedback_message, } @@ -175,8 +168,7 @@ fn parse_completed( ) -> dispatcher::ParsedHandler { let mut entries = Vec::new(); let mut status = HookRunStatus::Completed; - let mut should_stop = false; - let mut stop_reason = None; + let mut should_block = false; let mut additional_contexts_for_model = Vec::new(); let mut feedback_messages_for_model = Vec::new(); @@ -212,8 +204,6 @@ fn parse_completed( } if !parsed.universal.continue_processing { status = HookRunStatus::Stopped; - should_stop = true; - stop_reason = parsed.universal.stop_reason.clone(); let stop_text = parsed .universal .stop_reason @@ -242,6 +232,7 @@ fn parse_completed( }); } else if parsed.should_block { status = HookRunStatus::Blocked; + should_block = true; if let Some(reason) = parsed.reason { entries.push(HookOutputEntry { kind: HookOutputEntryKind::Feedback, @@ -260,6 +251,8 @@ fn parse_completed( } Some(2) => { if let Some(reason) = common::trimmed_non_empty(&run_result.stderr) { + status = HookRunStatus::Blocked; + should_block = true; entries.push(HookOutputEntry { kind: HookOutputEntryKind::Feedback, text: reason.clone(), @@ -298,8 +291,7 @@ fn parse_completed( dispatcher::ParsedHandler { completed, data: PostToolUseHandlerData { - should_stop, - stop_reason, + should_block, additional_contexts_for_model, feedback_messages_for_model, }, @@ -310,8 +302,7 @@ fn parse_completed( fn serialization_failure_outcome(hook_events: Vec) -> PostToolUseOutcome { PostToolUseOutcome { hook_events, - should_stop: false, - stop_reason: None, + should_block: false, additional_contexts: Vec::new(), feedback_message: None, } @@ -364,8 +355,7 @@ mod tests { assert_eq!( parsed.data, PostToolUseHandlerData { - should_stop: false, - stop_reason: None, + should_block: true, additional_contexts_for_model: Vec::new(), feedback_messages_for_model: vec!["bash output looked sketchy".to_string()], } @@ -388,8 +378,7 @@ mod tests { assert_eq!( parsed.data, PostToolUseHandlerData { - should_stop: false, - stop_reason: None, + should_block: false, additional_contexts_for_model: vec!["Remember the bash cleanup note.".to_string()], feedback_messages_for_model: Vec::new(), } @@ -418,8 +407,7 @@ mod tests { assert_eq!( parsed.data, PostToolUseHandlerData { - should_stop: false, - stop_reason: None, + should_block: false, additional_contexts_for_model: Vec::new(), feedback_messages_for_model: Vec::new(), } @@ -435,7 +423,7 @@ mod tests { } #[test] - fn exit_two_surfaces_feedback_to_model_without_blocking() { + fn exit_two_blocks_with_feedback() { let parsed = parse_completed( &handler(), run_result(Some(2), "", "post hook says pause"), @@ -445,13 +433,12 @@ mod tests { assert_eq!( parsed.data, PostToolUseHandlerData { - should_stop: false, - stop_reason: None, + should_block: true, additional_contexts_for_model: Vec::new(), feedback_messages_for_model: vec!["post hook says pause".to_string()], } ); - assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); + assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); } #[test] @@ -469,8 +456,7 @@ mod tests { assert_eq!( parsed.data, PostToolUseHandlerData { - should_stop: true, - stop_reason: Some("halt after bash output".to_string()), + should_block: false, additional_contexts_for_model: Vec::new(), feedback_messages_for_model: vec!["post-tool hook says stop".to_string()], } @@ -485,6 +471,32 @@ mod tests { ); } + #[test] + fn continue_false_without_reason_synthesizes_feedback() { + let parsed = parse_completed( + &handler(), + run_result(Some(0), r#"{"continue":false}"#, ""), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PostToolUseHandlerData { + should_block: false, + additional_contexts_for_model: Vec::new(), + feedback_messages_for_model: vec!["PostToolUse hook stopped execution".to_string()], + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Stopped); + assert_eq!( + parsed.completed.run.entries, + vec![HookOutputEntry { + kind: HookOutputEntryKind::Stop, + text: "PostToolUse hook stopped execution".to_string(), + }] + ); + } + #[test] fn plain_stdout_is_ignored_for_post_tool_use() { let parsed = parse_completed( @@ -496,8 +508,7 @@ mod tests { assert_eq!( parsed.data, PostToolUseHandlerData { - should_stop: false, - stop_reason: None, + should_block: false, additional_contexts_for_model: Vec::new(), feedback_messages_for_model: Vec::new(), }