mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Support PreToolUse additionalContext (#20692)
# Why `PreToolUse` already exposes `hookSpecificOutput.additionalContext` in the generated hook schema, but the runtime still rejected it as unsupported. That leaves `PreToolUse` out of step with the other context-injecting hooks and prevents hook authors from attaching model-visible guidance to a pending tool call before it runs. # What - Parse `PreToolUse.additionalContext` and carry it through the hook event pipeline. - Record `PreToolUse` context at the hook boundary so successful context is preserved for both allowed and blocked calls without widening the tool registry surface. - Preserve existing deny behavior when context is combined with either `permissionDecision: "deny"` or the legacy `decision: "block"` shape.
This commit is contained in:
committed by
GitHub
Unverified
parent
f35285dc78
commit
af86be529c
@@ -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| {
|
||||
|
||||
@@ -458,7 +458,6 @@ impl ToolRegistry {
|
||||
outcome.additional_contexts.clone(),
|
||||
)
|
||||
.await;
|
||||
|
||||
let replacement_text = if outcome.should_stop {
|
||||
Some(
|
||||
outcome
|
||||
|
||||
@@ -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)
|
||||
@@ -1973,6 +1989,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(()));
|
||||
|
||||
@@ -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>,
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|| 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<PreToolUseOutput> {
|
||||
Some(PreToolUseOutput {
|
||||
universal,
|
||||
block_reason,
|
||||
additional_context,
|
||||
invalid_reason,
|
||||
})
|
||||
}
|
||||
@@ -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) => {
|
||||
|
||||
@@ -37,12 +37,14 @@ pub struct PreToolUseOutcome {
|
||||
pub hook_events: Vec<HookCompletedEvent>,
|
||||
pub should_block: bool,
|
||||
pub block_reason: Option<String>,
|
||||
pub additional_contexts: Vec<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Default, PartialEq, Eq)]
|
||||
struct PreToolUseHandlerData {
|
||||
should_block: bool,
|
||||
block_reason: Option<String>,
|
||||
additional_contexts_for_model: Vec<String>,
|
||||
}
|
||||
|
||||
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<HookCompletedEvent>) -> 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);
|
||||
|
||||
Reference in New Issue
Block a user