Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions codex-rs/core/src/hook_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this uncapped? maybe an existing/broader concern (though PreToolUse is probably a high volume hook), but allowing injection of an uncapped amount of context directly into a dev message seems like an easy way for users to unknowingly blow up their own context window


if should_block {
block_reason.map(|reason| {
Expand Down
1 change: 0 additions & 1 deletion codex-rs/core/src/tools/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,6 @@ impl ToolRegistry {
outcome.additional_contexts.clone(),
)
.await;

let replacement_text = if outcome.should_stop {
Some(
outcome
Expand Down
168 changes: 168 additions & 0 deletions codex-rs/core/tests/suite/hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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(()));
Expand Down
12 changes: 4 additions & 8 deletions codex-rs/hooks/src/engine/output_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub(crate) struct SessionStartOutput {
pub(crate) struct PreToolUseOutput {
pub universal: UniversalOutput,
pub block_reason: Option<String>,
pub additional_context: Option<String>,
pub invalid_reason: Option<String>,
}

Expand Down Expand Up @@ -92,11 +93,12 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option<PreToolUseOutput> {
} = 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()
Comment thread
abhinav-oai marked this conversation as resolved.
|| 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 {
Expand Down Expand Up @@ -127,6 +129,7 @@ pub(crate) fn parse_pre_tool_use(stdout: &str) -> Option<PreToolUseOutput> {
Some(PreToolUseOutput {
universal,
block_reason,
additional_context,
invalid_reason,
})
}
Expand Down Expand Up @@ -339,13 +342,6 @@ fn unsupported_pre_tool_use_hook_specific_output(
) -> Option<String> {
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) => {
Expand Down
Loading
Loading