diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 9a9285451521..01d5ce1534d9 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -161,8 +161,10 @@ pub(crate) async fn run_pre_tool_use_hooks( hook_events, should_block, block_reason, + additional_contexts, } = hooks.run_pre_tool_use(request).await; emit_hook_completed_events(sess, turn_context, hook_events).await; + record_additional_contexts(sess, turn_context, additional_contexts).await; if should_block { block_reason.map(|reason| { diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index e1027c9fa907..87b36ff17e8b 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -458,7 +458,6 @@ impl ToolRegistry { outcome.additional_contexts.clone(), ) .await; - let replacement_text = if outcome.should_stop { Some( outcome diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 695e908ed815..48c36d933e4e 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -237,6 +237,22 @@ if mode == "json_deny": "permissionDecisionReason": reason }} }})) +elif mode == "context": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "additionalContext": reason + }} + }})) +elif mode == "json_deny_with_context": + print(json.dumps({{ + "hookSpecificOutput": {{ + "hookEventName": "PreToolUse", + "permissionDecision": "deny", + "permissionDecisionReason": reason, + "additionalContext": reason + }} + }})) elif mode == "exit_2": sys.stderr.write(reason + "\n") raise SystemExit(2) @@ -1831,6 +1847,158 @@ async fn pre_tool_use_blocks_shell_command_before_execution() -> Result<()> { Ok(()) } +#[tokio::test] +async fn pre_tool_use_records_additional_context_for_shell_command() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-shell-command-context"; + let command = "printf pre-tool-output".to_string(); + let args = serde_json::json!({ "command": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + call_id, + "shell_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "pre hook context observed"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let pre_context = "Remember the bash pre-tool note."; + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = + write_pre_tool_use_hook(home, Some("^Bash$"), "context", pre_context) + { + panic!("failed to write pre tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + test.submit_turn("run the shell command with pre hook") + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + assert!( + requests[1] + .message_input_texts("developer") + .contains(&pre_context.to_string()), + "follow-up request should include pre tool use additional context", + ); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("shell command output string"); + assert!( + output.contains("pre-tool-output"), + "shell command output should still reach the model", + ); + + Ok(()) +} + +#[tokio::test] +async fn blocked_pre_tool_use_records_additional_context_for_shell_command() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "pretooluse-shell-command-blocked-context"; + let marker = std::env::temp_dir().join("pretooluse-shell-command-blocked-context-marker"); + let command = format!("printf blocked > {}", marker.display()); + let args = serde_json::json!({ "command": command }); + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + call_id, + "shell_command", + &serde_json::to_string(&args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "blocked pre hook context observed"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + + let pre_context = "blocked by pre hook with context"; + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = + write_pre_tool_use_hook(home, Some("^Bash$"), "json_deny_with_context", pre_context) + { + panic!("failed to write pre tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + if marker.exists() { + fs::remove_file(&marker).context("remove leftover pre tool use marker")?; + } + + test.submit_turn_with_permission_profile( + "run the blocked shell command with pre hook context", + PermissionProfile::Disabled, + ) + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 2); + assert!( + requests[1] + .message_input_texts("developer") + .contains(&pre_context.to_string()), + "follow-up request should include blocked pre tool use additional context", + ); + let output_item = requests[1].function_call_output(call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("shell command output string"); + assert!( + output.contains("Command blocked by PreToolUse hook: blocked by pre hook with context"), + "blocked tool output should still surface the hook reason", + ); + assert!( + !marker.exists(), + "blocked command should not create marker file" + ); + + Ok(()) +} + #[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/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index 0a3a994e19da..464c7f86083e 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -16,6 +16,7 @@ pub(crate) struct SessionStartOutput { pub(crate) struct PreToolUseOutput { pub universal: UniversalOutput, pub block_reason: Option, + pub additional_context: Option, pub invalid_reason: Option, } @@ -92,11 +93,12 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option { } = parse_json(stdout)?; let universal = UniversalOutput::from(universal_wire); let hook_specific_output = hook_specific_output.as_ref(); + let additional_context = + hook_specific_output.and_then(|output| output.additional_context.clone()); let use_hook_specific_decision = hook_specific_output.is_some_and(|output| { output.permission_decision.is_some() || output.permission_decision_reason.is_some() || output.updated_input.is_some() - || output.additional_context.is_some() }); let invalid_reason = unsupported_pre_tool_use_universal(&universal).or_else(|| { if use_hook_specific_decision { @@ -127,6 +129,7 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option { Some(PreToolUseOutput { universal, block_reason, + additional_context, invalid_reason, }) } @@ -339,13 +342,6 @@ fn unsupported_pre_tool_use_hook_specific_output( ) -> Option { if output.updated_input.is_some() { Some("PreToolUse hook returned unsupported updatedInput".to_string()) - } else if output - .additional_context - .as_deref() - .and_then(trimmed_reason) - .is_some() - { - Some("PreToolUse hook returned unsupported additionalContext".to_string()) } else { match output.permission_decision { Some(PreToolUsePermissionDecisionWire::Allow) => { diff --git a/codex-rs/hooks/src/events/pre_tool_use.rs b/codex-rs/hooks/src/events/pre_tool_use.rs index 6fe1555229c9..39d0e6125813 100644 --- a/codex-rs/hooks/src/events/pre_tool_use.rs +++ b/codex-rs/hooks/src/events/pre_tool_use.rs @@ -37,12 +37,14 @@ pub struct PreToolUseOutcome { pub hook_events: Vec, pub should_block: bool, pub block_reason: Option, + pub additional_contexts: Vec, } #[derive(Debug, Default, PartialEq, Eq)] struct PreToolUseHandlerData { should_block: bool, block_reason: Option, + additional_contexts_for_model: Vec, } pub(crate) fn preview( @@ -78,6 +80,7 @@ pub(crate) async fn run( hook_events: Vec::new(), should_block: false, block_reason: None, + additional_contexts: Vec::new(), }; } @@ -108,6 +111,11 @@ pub(crate) async fn run( let block_reason = results .iter() .find_map(|result| result.data.block_reason.clone()); + let additional_contexts = common::flatten_additional_contexts( + results + .iter() + .map(|result| result.data.additional_contexts_for_model.as_slice()), + ); PreToolUseOutcome { hook_events: results @@ -118,6 +126,7 @@ pub(crate) async fn run( .collect(), should_block, block_reason, + additional_contexts, } } @@ -151,6 +160,7 @@ fn parse_completed( let mut status = HookRunStatus::Completed; let mut should_block = false; let mut block_reason = None; + let mut additional_contexts_for_model = Vec::new(); match run_result.error.as_deref() { Some(error) => { @@ -177,14 +187,23 @@ fn parse_completed( kind: HookOutputEntryKind::Error, text: invalid_reason, }); - } else if let Some(reason) = parsed.block_reason { - status = HookRunStatus::Blocked; - should_block = true; - block_reason = Some(reason.clone()); - entries.push(HookOutputEntry { - kind: HookOutputEntryKind::Feedback, - text: reason, - }); + } else { + if let Some(additional_context) = parsed.additional_context { + common::append_additional_context( + &mut entries, + &mut additional_contexts_for_model, + additional_context, + ); + } + if let Some(reason) = parsed.block_reason { + status = HookRunStatus::Blocked; + should_block = true; + block_reason = Some(reason.clone()); + entries.push(HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: reason, + }); + } } } else if trimmed_stdout.starts_with('{') || trimmed_stdout.starts_with('[') { status = HookRunStatus::Failed; @@ -238,6 +257,7 @@ fn parse_completed( data: PreToolUseHandlerData { should_block, block_reason, + additional_contexts_for_model, }, } } @@ -247,6 +267,7 @@ fn serialization_failure_outcome(hook_events: Vec) -> PreToo hook_events, should_block: false, block_reason: None, + additional_contexts: Vec::new(), } } @@ -298,6 +319,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + additional_contexts_for_model: Vec::new(), } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); @@ -327,6 +349,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("do not run that".to_string()), + additional_contexts_for_model: Vec::new(), } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); @@ -339,6 +362,42 @@ mod tests { ); } + #[test] + fn deprecated_block_decision_with_additional_context_blocks_processing() { + let parsed = parse_completed( + &handler(), + run_result( + Some(0), + r#"{"decision":"block","reason":"do not run that","hookSpecificOutput":{"hookEventName":"PreToolUse","additionalContext":"remember this"}}"#, + "", + ), + Some("turn-1".to_string()), + ); + + assert_eq!( + parsed.data, + PreToolUseHandlerData { + should_block: true, + block_reason: Some("do not run that".to_string()), + additional_contexts_for_model: vec!["remember this".to_string()], + } + ); + assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); + assert_eq!( + parsed.completed.run.entries, + vec![ + HookOutputEntry { + kind: HookOutputEntryKind::Context, + text: "remember this".to_string(), + }, + HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: "do not run that".to_string(), + }, + ] + ); + } + #[test] fn unsupported_permission_decision_fails_open() { let parsed = parse_completed( @@ -356,6 +415,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + additional_contexts_for_model: Vec::new(), } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -381,6 +441,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + additional_contexts_for_model: Vec::new(), } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -394,7 +455,7 @@ mod tests { } #[test] - fn unsupported_additional_context_fails_open() { + fn additional_context_is_recorded() { let parsed = parse_completed( &handler(), run_result( @@ -408,17 +469,24 @@ mod tests { assert_eq!( parsed.data, PreToolUseHandlerData { - should_block: false, - block_reason: None, + should_block: true, + block_reason: Some("do not run that".to_string()), + additional_contexts_for_model: vec!["nope".to_string()], } ); - assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); + assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked); assert_eq!( parsed.completed.run.entries, - vec![HookOutputEntry { - kind: HookOutputEntryKind::Error, - text: "PreToolUse hook returned unsupported additionalContext".to_string(), - }] + vec![ + HookOutputEntry { + kind: HookOutputEntryKind::Context, + text: "nope".to_string(), + }, + HookOutputEntry { + kind: HookOutputEntryKind::Feedback, + text: "do not run that".to_string(), + }, + ] ); } @@ -435,6 +503,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + additional_contexts_for_model: Vec::new(), } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Completed); @@ -454,6 +523,7 @@ mod tests { PreToolUseHandlerData { should_block: false, block_reason: None, + additional_contexts_for_model: Vec::new(), } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Failed); @@ -479,6 +549,7 @@ mod tests { PreToolUseHandlerData { should_block: true, block_reason: Some("blocked by policy".to_string()), + additional_contexts_for_model: Vec::new(), } ); assert_eq!(parsed.completed.run.status, HookRunStatus::Blocked);