mirror of
https://github.com/openai/codex.git
synced 2026-09-09 15:58:47 +00:00
Fix permission request hook precedence and retry context
This commit is contained in:
@@ -11,6 +11,7 @@ use codex_hooks::PreToolUseRequest;
|
||||
use codex_hooks::SessionStartOutcome;
|
||||
use codex_hooks::UserPromptSubmitOutcome;
|
||||
use codex_hooks::UserPromptSubmitRequest;
|
||||
use codex_protocol::approvals::NetworkApprovalContext;
|
||||
use codex_protocol::items::TurnItem;
|
||||
use codex_protocol::models::DeveloperInstructions;
|
||||
use codex_protocol::models::ResponseInputItem;
|
||||
@@ -154,6 +155,9 @@ pub(crate) async fn run_permission_request_hooks(
|
||||
turn_context: &Arc<TurnContext>,
|
||||
run_id_suffix: String,
|
||||
payload: PermissionRequestPayload,
|
||||
approval_attempt: String,
|
||||
retry_reason: Option<String>,
|
||||
network_approval_context: Option<NetworkApprovalContext>,
|
||||
) -> Option<PermissionRequestDecision> {
|
||||
let request = PermissionRequestRequest {
|
||||
session_id: sess.conversation_id,
|
||||
@@ -168,6 +172,9 @@ pub(crate) async fn run_permission_request_hooks(
|
||||
sandbox_permissions: payload.sandbox_permissions,
|
||||
additional_permissions: payload.additional_permissions,
|
||||
justification: payload.justification,
|
||||
approval_attempt,
|
||||
retry_reason,
|
||||
network_approval_context,
|
||||
};
|
||||
let preview_runs = sess.hooks().preview_permission_request(&request);
|
||||
emit_hook_started_events(sess, turn_context, preview_runs).await;
|
||||
|
||||
@@ -415,15 +415,24 @@ impl ToolOrchestrator {
|
||||
T: ToolRuntime<Rq, Out>,
|
||||
{
|
||||
if let Some(permission_request) = tool.permission_request_payload(req) {
|
||||
let run_id_suffix = match approval_attempt {
|
||||
ApprovalAttempt::Initial => format!("{}:initial", approval_ctx.call_id),
|
||||
ApprovalAttempt::Retry => format!("{}:retry", approval_ctx.call_id),
|
||||
let (approval_attempt_label, run_id_suffix) = match approval_attempt {
|
||||
ApprovalAttempt::Initial => (
|
||||
"initial".to_string(),
|
||||
format!("{}:initial", approval_ctx.call_id),
|
||||
),
|
||||
ApprovalAttempt::Retry => (
|
||||
"retry".to_string(),
|
||||
format!("{}:retry", approval_ctx.call_id),
|
||||
),
|
||||
};
|
||||
match run_permission_request_hooks(
|
||||
approval_ctx.session,
|
||||
approval_ctx.turn,
|
||||
run_id_suffix,
|
||||
permission_request,
|
||||
approval_attempt_label,
|
||||
approval_ctx.retry_reason.clone(),
|
||||
approval_ctx.network_approval_context.clone(),
|
||||
)
|
||||
.await
|
||||
{
|
||||
|
||||
@@ -1165,6 +1165,9 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
|
||||
"sandbox_permissions": "use_default",
|
||||
"additional_permissions": null,
|
||||
"justification": null,
|
||||
"approval_attempt": "initial",
|
||||
"retry_reason": null,
|
||||
"network_approval_context": null,
|
||||
})
|
||||
);
|
||||
assert!(
|
||||
@@ -1180,6 +1183,92 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() ->
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn permission_request_hook_sees_retry_context_after_sandbox_denial() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let call_id = "permissionrequest-retry-shell-command";
|
||||
let marker = "permissionrequest_retry_marker.txt";
|
||||
let command = format!("printf retry > {marker}");
|
||||
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", "permission request hook allowed retry"),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_pre_build_hook(|home| {
|
||||
if let Err(error) = write_permission_request_hook(
|
||||
home,
|
||||
Some("^Bash$"),
|
||||
"allow",
|
||||
"should not be used for allow",
|
||||
) {
|
||||
panic!("failed to write permission request 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?;
|
||||
let marker_path = test.workspace_path(marker);
|
||||
let _ = fs::remove_file(&marker_path);
|
||||
|
||||
test.submit_turn_with_policies(
|
||||
"retry the shell command after sandbox denial",
|
||||
AskForApproval::OnFailure,
|
||||
codex_protocol::protocol::SandboxPolicy::new_read_only_policy(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 2);
|
||||
requests[1].function_call_output(call_id);
|
||||
assert_eq!(
|
||||
fs::read_to_string(&marker_path).context("read retry marker")?,
|
||||
"retry"
|
||||
);
|
||||
|
||||
let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?;
|
||||
assert_eq!(hook_inputs.len(), 1);
|
||||
assert_eq!(hook_inputs[0]["hook_event_name"], "PermissionRequest");
|
||||
assert_eq!(hook_inputs[0]["tool_input"]["command"], command);
|
||||
assert_eq!(
|
||||
hook_inputs[0]["approval_context"],
|
||||
serde_json::json!({
|
||||
"sandbox_permissions": "use_default",
|
||||
"additional_permissions": null,
|
||||
"justification": null,
|
||||
"approval_attempt": "retry",
|
||||
"retry_reason": "command failed; retry without sandbox?",
|
||||
"network_approval_context": null,
|
||||
})
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn pre_tool_use_blocks_shell_command_before_execution() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -23,6 +23,30 @@
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"NetworkApprovalContext": {
|
||||
"properties": {
|
||||
"host": {
|
||||
"type": "string"
|
||||
},
|
||||
"protocol": {
|
||||
"$ref": "#/definitions/NetworkApprovalProtocol"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"host",
|
||||
"protocol"
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"NetworkApprovalProtocol": {
|
||||
"enum": [
|
||||
"http",
|
||||
"https",
|
||||
"socks5_tcp",
|
||||
"socks5_udp"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
"NetworkPermissions": {
|
||||
"properties": {
|
||||
"enabled": {
|
||||
@@ -54,14 +78,28 @@
|
||||
"additional_permissions": {
|
||||
"$ref": "#/definitions/PermissionProfile"
|
||||
},
|
||||
"approval_attempt": {
|
||||
"enum": [
|
||||
"initial",
|
||||
"retry"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
"justification": {
|
||||
"type": "string"
|
||||
},
|
||||
"network_approval_context": {
|
||||
"$ref": "#/definitions/NetworkApprovalContext"
|
||||
},
|
||||
"retry_reason": {
|
||||
"type": "string"
|
||||
},
|
||||
"sandbox_permissions": {
|
||||
"$ref": "#/definitions/SandboxPermissions"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"approval_attempt",
|
||||
"sandbox_permissions"
|
||||
],
|
||||
"type": "object"
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use std::path::PathBuf;
|
||||
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::approvals::NetworkApprovalContext;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::SandboxPermissions;
|
||||
use codex_protocol::protocol::HookCompletedEvent;
|
||||
@@ -34,6 +35,9 @@ pub struct PermissionRequestRequest {
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub additional_permissions: Option<PermissionProfile>,
|
||||
pub justification: Option<String>,
|
||||
pub approval_attempt: String,
|
||||
pub retry_reason: Option<String>,
|
||||
pub network_approval_context: Option<NetworkApprovalContext>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -115,9 +119,11 @@ pub(crate) async fn run(
|
||||
)
|
||||
.await;
|
||||
|
||||
let decision = results
|
||||
.iter()
|
||||
.find_map(|result| result.data.decision.clone());
|
||||
let decision = resolve_permission_request_decision(
|
||||
results
|
||||
.iter()
|
||||
.filter_map(|result| result.data.decision.as_ref()),
|
||||
);
|
||||
|
||||
PermissionRequestOutcome {
|
||||
hook_events: results
|
||||
@@ -130,6 +136,30 @@ pub(crate) async fn run(
|
||||
}
|
||||
}
|
||||
|
||||
fn resolve_permission_request_decision<'a>(
|
||||
decisions: impl IntoIterator<Item = &'a PermissionRequestDecision>,
|
||||
) -> Option<PermissionRequestDecision> {
|
||||
// Hooks are discovered in increasing precedence order, so later handlers are
|
||||
// more specific than earlier ones. Permission requests should stay
|
||||
// conservative across layers: any matching deny wins immediately, while
|
||||
// allows only take effect if no handler denied the request. When multiple
|
||||
// allows match, use the highest-precedence one (the latest allow we saw).
|
||||
let mut resolved_allow = None;
|
||||
for decision in decisions {
|
||||
match decision {
|
||||
PermissionRequestDecision::Allow => {
|
||||
resolved_allow = Some(PermissionRequestDecision::Allow);
|
||||
}
|
||||
PermissionRequestDecision::Deny { message } => {
|
||||
return Some(PermissionRequestDecision::Deny {
|
||||
message: message.clone(),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
resolved_allow
|
||||
}
|
||||
|
||||
fn build_command_input(request: &PermissionRequestRequest) -> PermissionRequestCommandInput {
|
||||
PermissionRequestCommandInput {
|
||||
session_id: request.session_id.to_string(),
|
||||
@@ -147,6 +177,9 @@ fn build_command_input(request: &PermissionRequestRequest) -> PermissionRequestC
|
||||
sandbox_permissions: request.sandbox_permissions,
|
||||
additional_permissions: request.additional_permissions.clone(),
|
||||
justification: request.justification.clone(),
|
||||
approval_attempt: request.approval_attempt.clone(),
|
||||
retry_reason: request.retry_reason.clone(),
|
||||
network_approval_context: request.network_approval_context.clone(),
|
||||
},
|
||||
}
|
||||
}
|
||||
@@ -253,3 +286,48 @@ fn parse_completed(
|
||||
data: PermissionRequestHandlerData { decision },
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
use super::PermissionRequestDecision;
|
||||
use super::resolve_permission_request_decision;
|
||||
|
||||
#[test]
|
||||
fn permission_request_deny_overrides_earlier_allow() {
|
||||
let decisions = vec![
|
||||
PermissionRequestDecision::Allow,
|
||||
PermissionRequestDecision::Deny {
|
||||
message: "repo deny".to_string(),
|
||||
},
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
resolve_permission_request_decision(decisions.iter()),
|
||||
Some(PermissionRequestDecision::Deny {
|
||||
message: "repo deny".to_string(),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn permission_request_returns_allow_when_no_handler_denies() {
|
||||
let decisions = vec![
|
||||
PermissionRequestDecision::Allow,
|
||||
PermissionRequestDecision::Allow,
|
||||
];
|
||||
|
||||
assert_eq!(
|
||||
resolve_permission_request_decision(decisions.iter()),
|
||||
Some(PermissionRequestDecision::Allow)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn permission_request_returns_none_when_no_handler_decides() {
|
||||
let decisions = Vec::<PermissionRequestDecision>::new();
|
||||
|
||||
assert_eq!(resolve_permission_request_decision(decisions.iter()), None);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -12,6 +12,7 @@ use serde_json::Value;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use codex_protocol::approvals::NetworkApprovalContext;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::SandboxPermissions;
|
||||
|
||||
@@ -244,6 +245,10 @@ pub(crate) struct PermissionRequestApprovalContext {
|
||||
pub sandbox_permissions: SandboxPermissions,
|
||||
pub additional_permissions: Option<PermissionProfile>,
|
||||
pub justification: Option<String>,
|
||||
#[schemars(schema_with = "permission_request_approval_attempt_schema")]
|
||||
pub approval_attempt: String,
|
||||
pub retry_reason: Option<String>,
|
||||
pub network_approval_context: Option<NetworkApprovalContext>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, JsonSchema)]
|
||||
@@ -566,6 +571,10 @@ fn permission_request_tool_name_schema(_gen: &mut SchemaGenerator) -> Schema {
|
||||
string_const_schema("Bash")
|
||||
}
|
||||
|
||||
fn permission_request_approval_attempt_schema(_gen: &mut SchemaGenerator) -> Schema {
|
||||
string_enum_schema(&["initial", "retry"])
|
||||
}
|
||||
|
||||
fn user_prompt_submit_hook_event_name_schema(_gen: &mut SchemaGenerator) -> Schema {
|
||||
string_const_schema("UserPromptSubmit")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user