mirror of
https://github.com/openai/codex.git
synced 2026-10-21 17:48:44 +00:00
Ignore PreToolUse allow decisions
This commit is contained in:
@@ -1171,7 +1171,7 @@ async fn maybe_request_mcp_tool_approval(
|
||||
);
|
||||
let pre_tool_use_reason = match &pre_tool_use_permission_decision {
|
||||
Some(PreToolUsePermissionDecision::Ask { reason }) => reason.as_deref(),
|
||||
Some(PreToolUsePermissionDecision::Allow { .. }) | None => None,
|
||||
None => None,
|
||||
};
|
||||
let prompt_reason = monitor_reason.as_deref().or(pre_tool_use_reason);
|
||||
question.question = mcp_tool_approval_question_text(question.question, prompt_reason);
|
||||
|
||||
@@ -2070,75 +2070,6 @@ async fn permission_request_hook_allows_mcp_tool_call() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn pre_tool_use_allow_does_not_skip_mcp_permission_request_hook() {
|
||||
let (mut session, turn_context) = make_session_and_context().await;
|
||||
let log_path = install_mcp_permission_request_hook(
|
||||
&mut session,
|
||||
&turn_context,
|
||||
"mcp__memory__.*",
|
||||
&serde_json::json!({
|
||||
"hookSpecificOutput": {
|
||||
"hookEventName": "PermissionRequest",
|
||||
"decision": {
|
||||
"behavior": "deny",
|
||||
"message": "denied by permission request hook"
|
||||
}
|
||||
}
|
||||
}),
|
||||
);
|
||||
turn_context.record_pre_tool_use_permission_decision(
|
||||
"call-mcp-pretool-allow".to_string(),
|
||||
PreToolUsePermissionDecision::Allow {
|
||||
reason: Some("approved by pre tool use hook".to_string()),
|
||||
},
|
||||
);
|
||||
let session = Arc::new(session);
|
||||
let turn_context = Arc::new(turn_context);
|
||||
let invocation = McpInvocation {
|
||||
server: "memory".to_string(),
|
||||
tool: "create_entities".to_string(),
|
||||
arguments: Some(serde_json::json!({ "entities": [] })),
|
||||
};
|
||||
let metadata = McpToolApprovalMetadata {
|
||||
annotations: Some(annotations(
|
||||
Some(false),
|
||||
Some(true),
|
||||
/*open_world*/ None,
|
||||
)),
|
||||
connector_id: None,
|
||||
connector_name: None,
|
||||
connector_description: None,
|
||||
tool_title: Some("Create entities".to_string()),
|
||||
tool_description: None,
|
||||
mcp_app_resource_uri: None,
|
||||
codex_apps_meta: None,
|
||||
openai_file_input_params: None,
|
||||
};
|
||||
|
||||
let decision = maybe_request_mcp_tool_approval(
|
||||
&session,
|
||||
&turn_context,
|
||||
"call-mcp-pretool-allow",
|
||||
&invocation,
|
||||
"mcp__memory__create_entities",
|
||||
Some(&metadata),
|
||||
AppToolApproval::Auto,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
decision,
|
||||
Some(McpToolApprovalDecision::Decline {
|
||||
message: Some("denied by permission request hook".to_string()),
|
||||
})
|
||||
);
|
||||
assert!(
|
||||
log_path.exists(),
|
||||
"pre tool use allow should not skip PermissionRequest hooks"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn permission_request_hook_uses_hook_tool_name_without_metadata() {
|
||||
let (mut session, turn_context) = make_session_and_context().await;
|
||||
|
||||
@@ -544,9 +544,7 @@ impl NetworkApprovalService {
|
||||
Some(PreToolUsePermissionDecision::Ask { reason }) => {
|
||||
reason.clone().or(Some(prompt_reason))
|
||||
}
|
||||
Some(PreToolUsePermissionDecision::Allow { .. }) | None => {
|
||||
Some(prompt_reason)
|
||||
}
|
||||
None => Some(prompt_reason),
|
||||
},
|
||||
Some(network_approval_context.clone()),
|
||||
/*proposed_execpolicy_amendment*/ None,
|
||||
|
||||
@@ -148,7 +148,7 @@ impl ToolOrchestrator {
|
||||
);
|
||||
let pre_tool_use_reason = match &pre_tool_use_permission_decision {
|
||||
Some(PreToolUsePermissionDecision::Ask { reason }) => reason.clone(),
|
||||
Some(PreToolUsePermissionDecision::Allow { .. }) | None => None,
|
||||
None => None,
|
||||
};
|
||||
let use_guardian = (routes_approval_to_guardian(turn_ctx) || strict_auto_review)
|
||||
&& !pre_tool_use_asks_user;
|
||||
|
||||
@@ -483,7 +483,7 @@ impl CoreShellActionProvider {
|
||||
workdir.clone(),
|
||||
match pre_tool_use_permission_decision {
|
||||
Some(PreToolUsePermissionDecision::Ask { reason }) => reason,
|
||||
Some(PreToolUsePermissionDecision::Allow { .. }) | None => None,
|
||||
None => None,
|
||||
},
|
||||
/*network_approval_context*/ None,
|
||||
/*proposed_execpolicy_amendment*/ None,
|
||||
|
||||
@@ -55,11 +55,17 @@
|
||||
"oneOf": [
|
||||
{
|
||||
"enum": [
|
||||
"allow",
|
||||
"deny"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "Accepted for compatibility.",
|
||||
"enum": [
|
||||
"allow"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "Always requests explicit human-user confirmation.",
|
||||
"enum": [
|
||||
|
||||
@@ -23,7 +23,6 @@ pub(crate) struct PreToolUseOutput {
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub(crate) enum PreToolUsePermissionDecision {
|
||||
Allow { reason: Option<String> },
|
||||
Ask { reason: Option<String> },
|
||||
}
|
||||
|
||||
@@ -357,9 +356,9 @@ fn unsupported_pre_tool_use_hook_specific_output(
|
||||
Some("PreToolUse hook returned unsupported updatedInput".to_string())
|
||||
} else {
|
||||
match output.permission_decision.as_ref() {
|
||||
Some(PreToolUsePermissionDecisionWire::Allow) => None,
|
||||
Some(
|
||||
decision @ (PreToolUsePermissionDecisionWire::Allow
|
||||
| PreToolUsePermissionDecisionWire::Ask
|
||||
decision @ (PreToolUsePermissionDecisionWire::Ask
|
||||
| PreToolUsePermissionDecisionWire::Deny),
|
||||
) => {
|
||||
if output
|
||||
@@ -428,14 +427,7 @@ fn pre_tool_use_permission_decision(
|
||||
output: &crate::schema::PreToolUseHookSpecificOutputWire,
|
||||
) -> Option<PreToolUsePermissionDecision> {
|
||||
match output.permission_decision {
|
||||
Some(PreToolUsePermissionDecisionWire::Allow) => {
|
||||
Some(PreToolUsePermissionDecision::Allow {
|
||||
reason: output
|
||||
.permission_decision_reason
|
||||
.as_deref()
|
||||
.and_then(trimmed_reason),
|
||||
})
|
||||
}
|
||||
Some(PreToolUsePermissionDecisionWire::Allow) => None,
|
||||
Some(PreToolUsePermissionDecisionWire::Ask) => Some(PreToolUsePermissionDecision::Ask {
|
||||
reason: output
|
||||
.permission_decision_reason
|
||||
@@ -535,7 +527,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn pre_tool_use_permission_decisions_require_reason() {
|
||||
for decision in ["allow", "ask", "deny"] {
|
||||
for decision in ["ask", "deny"] {
|
||||
let parsed = parse_pre_tool_use(
|
||||
&json!({
|
||||
"hookSpecificOutput": {
|
||||
@@ -555,4 +547,30 @@ mod tests {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pre_tool_use_allow_is_ignored() {
|
||||
for stdout in [
|
||||
json!({
|
||||
"hookSpecificOutput": {
|
||||
"hookEventName": "PreToolUse",
|
||||
"permissionDecision": "allow"
|
||||
}
|
||||
})
|
||||
.to_string(),
|
||||
json!({
|
||||
"hookSpecificOutput": {
|
||||
"hookEventName": "PreToolUse",
|
||||
"permissionDecision": "allow",
|
||||
"permissionDecisionReason": "approved by hook"
|
||||
}
|
||||
})
|
||||
.to_string(),
|
||||
] {
|
||||
let parsed =
|
||||
parse_pre_tool_use(&stdout).expect("pre tool use hook output should parse");
|
||||
assert_eq!(parsed.invalid_reason, None);
|
||||
assert_eq!(parsed.permission_decision, None);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -46,8 +46,6 @@ pub struct PreToolUseOutcome {
|
||||
/// boundaries for the same tool call.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum PreToolUsePermissionDecision {
|
||||
/// Record hook-level approval without waiving later concrete permission checks.
|
||||
Allow { reason: Option<String> },
|
||||
/// Require explicit human-user confirmation.
|
||||
Ask { reason: Option<String> },
|
||||
}
|
||||
@@ -153,23 +151,7 @@ pub(crate) async fn run(
|
||||
fn resolve_permission_decision<'a>(
|
||||
decisions: impl IntoIterator<Item = &'a PreToolUsePermissionDecision>,
|
||||
) -> Option<PreToolUsePermissionDecision> {
|
||||
let mut resolved_allow = None;
|
||||
let mut resolved_ask = None;
|
||||
for decision in decisions {
|
||||
match decision {
|
||||
PreToolUsePermissionDecision::Allow { reason } => {
|
||||
resolved_allow = Some(PreToolUsePermissionDecision::Allow {
|
||||
reason: reason.clone(),
|
||||
});
|
||||
}
|
||||
PreToolUsePermissionDecision::Ask { reason } => {
|
||||
resolved_ask = Some(PreToolUsePermissionDecision::Ask {
|
||||
reason: reason.clone(),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
resolved_ask.or(resolved_allow)
|
||||
decisions.into_iter().last().cloned()
|
||||
}
|
||||
|
||||
/// Serializes command stdin for a selected `PreToolUse` hook.
|
||||
@@ -248,19 +230,9 @@ fn parse_completed(
|
||||
});
|
||||
}
|
||||
if let Some(parsed_decision) = parsed.permission_decision {
|
||||
let (decision, reason) = match parsed_decision {
|
||||
ParsedPermissionDecision::Allow { reason } => (
|
||||
PreToolUsePermissionDecision::Allow {
|
||||
reason: reason.clone(),
|
||||
},
|
||||
reason,
|
||||
),
|
||||
ParsedPermissionDecision::Ask { reason } => (
|
||||
PreToolUsePermissionDecision::Ask {
|
||||
reason: reason.clone(),
|
||||
},
|
||||
reason,
|
||||
),
|
||||
let ParsedPermissionDecision::Ask { reason } = parsed_decision;
|
||||
let decision = PreToolUsePermissionDecision::Ask {
|
||||
reason: reason.clone(),
|
||||
};
|
||||
if let Some(reason) = reason {
|
||||
entries.push(HookOutputEntry {
|
||||
@@ -504,7 +476,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn permission_decision_allow_is_user_visible_without_model_context() {
|
||||
fn permission_decision_allow_is_ignored() {
|
||||
let parsed = parse_completed(
|
||||
&handler(),
|
||||
run_result(
|
||||
@@ -520,20 +492,12 @@ mod tests {
|
||||
PreToolUseHandlerData {
|
||||
should_block: false,
|
||||
block_reason: None,
|
||||
permission_decision: Some(PreToolUsePermissionDecision::Allow {
|
||||
reason: Some("approved by policy".to_string()),
|
||||
}),
|
||||
permission_decision: None,
|
||||
additional_contexts_for_model: Vec::new(),
|
||||
}
|
||||
);
|
||||
assert_eq!(parsed.completed.run.status, HookRunStatus::Completed);
|
||||
assert_eq!(
|
||||
parsed.completed.run.entries,
|
||||
vec![HookOutputEntry {
|
||||
kind: HookOutputEntryKind::Reason,
|
||||
text: "approved by policy".to_string(),
|
||||
}]
|
||||
);
|
||||
assert_eq!(parsed.completed.run.entries, Vec::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -194,6 +194,7 @@ pub(crate) struct PreToolUseHookSpecificOutputWire {
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq, Eq)]
|
||||
pub(crate) enum PreToolUsePermissionDecisionWire {
|
||||
/// Accepted for compatibility.
|
||||
#[serde(rename = "allow")]
|
||||
Allow,
|
||||
#[serde(rename = "deny")]
|
||||
|
||||
Reference in New Issue
Block a user