From f6517fa6a2e85bbdce7de199b107216ee0e4f226 Mon Sep 17 00:00:00 2001 From: Abhinav Vedmala Date: Sun, 12 Apr 2026 13:17:13 -0700 Subject: [PATCH] Fix permission request hook precedence and retry context --- codex-rs/core/src/hook_runtime.rs | 7 ++ codex-rs/core/src/tools/orchestrator.rs | 15 +++- codex-rs/core/tests/suite/hooks.rs | 89 +++++++++++++++++++ ...rmission-request.command.input.schema.json | 38 ++++++++ .../hooks/src/events/permission_request.rs | 84 ++++++++++++++++- codex-rs/hooks/src/schema.rs | 9 ++ 6 files changed, 236 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 4393bf4039..f6319084b2 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -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, run_id_suffix: String, payload: PermissionRequestPayload, + approval_attempt: String, + retry_reason: Option, + network_approval_context: Option, ) -> Option { 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; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index eb3005411a..b1273e5248 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -415,15 +415,24 @@ impl ToolOrchestrator { T: ToolRuntime, { 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 { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index b09c3db98e..cda757f9f2 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -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(())); diff --git a/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json b/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json index 0e4e1f58c0..6bf432acea 100644 --- a/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json +++ b/codex-rs/hooks/schema/generated/permission-request.command.input.schema.json @@ -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" diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index c7d5deec42..591a57413a 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -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, pub justification: Option, + pub approval_attempt: String, + pub retry_reason: Option, + pub network_approval_context: Option, } #[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, +) -> Option { + // 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::::new(); + + assert_eq!(resolve_permission_request_decision(decisions.iter()), None); + } +} diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 27c1e2cc91..2c8ed458c0 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -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, pub justification: Option, + #[schemars(schema_with = "permission_request_approval_attempt_schema")] + pub approval_attempt: String, + pub retry_reason: Option, + pub network_approval_context: Option, } #[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") }