diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index ed3758cfbd..241e121aa9 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -1,6 +1,7 @@ use std::future::Future; use std::sync::Arc; +use codex_hooks::PermissionRequestApprovalAttempt; use codex_hooks::PermissionRequestDecision; use codex_hooks::PermissionRequestOutcome; use codex_hooks::PermissionRequestRequest; @@ -158,7 +159,7 @@ pub(crate) async fn run_permission_request_hooks( turn_context: &Arc, run_id_suffix: String, payload: PermissionRequestPayload, - approval_attempt: String, + approval_attempt: PermissionRequestApprovalAttempt, retry_reason: Option, network_approval_context: Option, ) -> Option { diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index b7fc708e5b..fa421e3eaa 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -25,6 +25,7 @@ use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::default_exec_approval_requirement; +use codex_hooks::PermissionRequestApprovalAttempt; use codex_hooks::PermissionRequestDecision; use codex_otel::SessionTelemetry; use codex_otel::ToolDecisionSource; @@ -46,12 +47,6 @@ pub(crate) struct OrchestratorRunResult { pub deferred_network_approval: Option, } -#[derive(Clone, Copy)] -enum ApprovalAttempt { - Initial, - Retry, -} - struct ApprovalTelemetry<'a> { otel: &'a SessionTelemetry, tool_name: &'a str, @@ -162,7 +157,7 @@ impl ToolOrchestrator { let decision = Self::request_approval( tool, req, - ApprovalAttempt::Initial, + PermissionRequestApprovalAttempt::Initial, approval_ctx, turn_ctx, ApprovalTelemetry { @@ -172,31 +167,12 @@ impl ToolOrchestrator { }, ) .await?; - - match decision { - ReviewDecision::Denied | ReviewDecision::Abort => { - let reason = if let Some(review_id) = guardian_review_id.as_deref() { - guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await - } else { - "rejected by user".to_string() - }; - return Err(ToolError::Rejected(reason)); - } - ReviewDecision::TimedOut => { - return Err(ToolError::Rejected(guardian_timeout_message())); - } - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession => {} - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - NetworkPolicyRuleAction::Allow => {} - NetworkPolicyRuleAction::Deny => { - return Err(ToolError::Rejected("rejected by user".to_string())); - } - }, - } + Self::enforce_approval_decision( + tool_ctx.session.as_ref(), + guardian_review_id.as_deref(), + decision, + ) + .await?; already_approved = true; } } @@ -326,7 +302,7 @@ impl ToolOrchestrator { let decision = Self::request_approval( tool, req, - ApprovalAttempt::Retry, + PermissionRequestApprovalAttempt::Retry, approval_ctx, turn_ctx, ApprovalTelemetry { @@ -336,32 +312,12 @@ impl ToolOrchestrator { }, ) .await?; - - match decision { - ReviewDecision::Denied | ReviewDecision::Abort => { - let reason = if let Some(review_id) = guardian_review_id.as_deref() { - guardian_rejection_message(tool_ctx.session.as_ref(), review_id) - .await - } else { - "rejected by user".to_string() - }; - return Err(ToolError::Rejected(reason)); - } - ReviewDecision::TimedOut => { - return Err(ToolError::Rejected(guardian_timeout_message())); - } - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession => {} - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - NetworkPolicyRuleAction::Allow => {} - NetworkPolicyRuleAction::Deny => { - return Err(ToolError::Rejected("rejected by user".to_string())); - } - }, - } + Self::enforce_approval_decision( + tool_ctx.session.as_ref(), + guardian_review_id.as_deref(), + decision, + ) + .await?; } let escalated_attempt = SandboxAttempt { @@ -405,7 +361,7 @@ impl ToolOrchestrator { async fn request_approval( tool: &mut T, req: &Rq, - approval_attempt: ApprovalAttempt, + approval_attempt: PermissionRequestApprovalAttempt, approval_ctx: ApprovalCtx<'_>, turn_ctx: &crate::codex::TurnContext, telemetry: ApprovalTelemetry<'_>, @@ -414,22 +370,16 @@ impl ToolOrchestrator { T: ToolRuntime, { if let Some(permission_request) = tool.permission_request_payload(req) { - 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, + format!( + "{}:{}", + approval_ctx.call_id, + approval_attempt.as_wire_value() + ), permission_request, - approval_attempt_label, + approval_attempt, approval_ctx.retry_reason.clone(), approval_ctx.network_approval_context.clone(), ) @@ -471,6 +421,38 @@ impl ToolOrchestrator { ); Ok(decision) } + + // Normalizes approval outcomes from hooks, guardian, and user prompts into + // the orchestrator's `Result` flow so both initial and retry approvals use + // the same rejection and timeout handling. + async fn enforce_approval_decision( + session: &crate::codex::Session, + guardian_review_id: Option<&str>, + decision: ReviewDecision, + ) -> Result<(), ToolError> { + match decision { + ReviewDecision::Denied | ReviewDecision::Abort => { + let reason = if let Some(review_id) = guardian_review_id { + guardian_rejection_message(session, review_id).await + } else { + "rejected by user".to_string() + }; + Err(ToolError::Rejected(reason)) + } + ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())), + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::ApprovedForSession => Ok(()), + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + } => match network_policy_amendment.action { + NetworkPolicyRuleAction::Allow => Ok(()), + NetworkPolicyRuleAction::Deny => { + Err(ToolError::Rejected("rejected by user".to_string())) + } + }, + } + } } fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String { 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 6bf432acea..e6ebc2c496 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 @@ -72,6 +72,13 @@ }, "type": "object" }, + "PermissionRequestApprovalAttempt": { + "enum": [ + "initial", + "retry" + ], + "type": "string" + }, "PermissionRequestApprovalContext": { "additionalProperties": false, "properties": { @@ -79,11 +86,7 @@ "$ref": "#/definitions/PermissionProfile" }, "approval_attempt": { - "enum": [ - "initial", - "retry" - ], - "type": "string" + "$ref": "#/definitions/PermissionRequestApprovalAttempt" }, "justification": { "type": "string" diff --git a/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json b/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json index d603dd0f09..21d45382b0 100644 --- a/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json +++ b/codex-rs/hooks/schema/generated/permission-request.command.output.schema.json @@ -28,6 +28,7 @@ }, "interrupt": { "default": false, + "description": "Reserved for future short-circuiting semantics.\n\nPermissionRequest hooks currently fail closed if this field is `true`.", "type": "boolean" }, "message": { @@ -35,10 +36,12 @@ "type": "string" }, "updatedInput": { - "default": null + "default": null, + "description": "Reserved for a future input-rewrite capability.\n\nPermissionRequest hooks currently fail closed if this field is present." }, "updatedPermissions": { - "default": null + "default": null, + "description": "Reserved for a future permission-rewrite capability.\n\nPermissionRequest hooks currently fail closed if this field is present." } }, "required": [ diff --git a/codex-rs/hooks/src/engine/output_parser.rs b/codex-rs/hooks/src/engine/output_parser.rs index a95f57309b..0a3a994e19 100644 --- a/codex-rs/hooks/src/engine/output_parser.rs +++ b/codex-rs/hooks/src/engine/output_parser.rs @@ -415,3 +415,80 @@ fn trimmed_reason(reason: &str) -> Option { Some(trimmed.to_string()) } } + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + use serde_json::json; + + use super::parse_permission_request; + + #[test] + fn permission_request_rejects_reserved_updated_input_field() { + let parsed = parse_permission_request( + &json!({ + "continue": true, + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "allow", + "updatedInput": {} + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + + assert_eq!( + parsed.invalid_reason, + Some("PermissionRequest hook returned unsupported updatedInput".to_string()) + ); + } + + #[test] + fn permission_request_rejects_reserved_updated_permissions_field() { + let parsed = parse_permission_request( + &json!({ + "continue": true, + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "allow", + "updatedPermissions": {} + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + + assert_eq!( + parsed.invalid_reason, + Some("PermissionRequest hook returned unsupported updatedPermissions".to_string()) + ); + } + + #[test] + fn permission_request_rejects_reserved_interrupt_field() { + let parsed = parse_permission_request( + &json!({ + "continue": true, + "hookSpecificOutput": { + "hookEventName": "PermissionRequest", + "decision": { + "behavior": "allow", + "interrupt": true + } + } + }) + .to_string(), + ) + .expect("permission request hook output should parse"); + + assert_eq!( + parsed.invalid_reason, + Some("PermissionRequest hook returned unsupported interrupt:true".to_string()) + ); + } +} diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 2cf7d90536..24123d5083 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -25,6 +25,9 @@ use codex_protocol::protocol::HookOutputEntry; use codex_protocol::protocol::HookOutputEntryKind; use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::HookRunSummary; +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; use super::common; use crate::engine::CommandShell; @@ -50,11 +53,27 @@ pub struct PermissionRequestRequest { pub sandbox_permissions: SandboxPermissions, pub additional_permissions: Option, pub justification: Option, - pub approval_attempt: String, + pub approval_attempt: PermissionRequestApprovalAttempt, pub retry_reason: Option, pub network_approval_context: Option, } +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum PermissionRequestApprovalAttempt { + Initial, + Retry, +} + +impl PermissionRequestApprovalAttempt { + pub const fn as_wire_value(self) -> &'static str { + match self { + Self::Initial => "initial", + Self::Retry => "retry", + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum PermissionRequestDecision { Allow, diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index c8358c678c..e21d09a332 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -5,6 +5,7 @@ mod registry; mod schema; mod types; +pub use events::permission_request::PermissionRequestApprovalAttempt; pub use events::permission_request::PermissionRequestDecision; pub use events::permission_request::PermissionRequestOutcome; pub use events::permission_request::PermissionRequestRequest; diff --git a/codex-rs/hooks/src/schema.rs b/codex-rs/hooks/src/schema.rs index 2c8ed458c0..6a06027414 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -16,6 +16,8 @@ use codex_protocol::approvals::NetworkApprovalContext; use codex_protocol::models::PermissionProfile; use codex_protocol::models::SandboxPermissions; +use crate::events::permission_request::PermissionRequestApprovalAttempt; + const GENERATED_DIR: &str = "generated"; const POST_TOOL_USE_INPUT_FIXTURE: &str = "post-tool-use.command.input.schema.json"; const POST_TOOL_USE_OUTPUT_FIXTURE: &str = "post-tool-use.command.output.schema.json"; @@ -142,12 +144,21 @@ pub(crate) struct PermissionRequestHookSpecificOutputWire { #[serde(deny_unknown_fields)] pub(crate) struct PermissionRequestDecisionWire { pub behavior: PermissionRequestBehaviorWire, + /// Reserved for a future input-rewrite capability. + /// + /// PermissionRequest hooks currently fail closed if this field is present. #[serde(default)] pub updated_input: Option, + /// Reserved for a future permission-rewrite capability. + /// + /// PermissionRequest hooks currently fail closed if this field is present. #[serde(default)] pub updated_permissions: Option, #[serde(default)] pub message: Option, + /// Reserved for future short-circuiting semantics. + /// + /// PermissionRequest hooks currently fail closed if this field is `true`. #[serde(default)] pub interrupt: bool, } @@ -245,8 +256,7 @@ 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 approval_attempt: PermissionRequestApprovalAttempt, pub retry_reason: Option, pub network_approval_context: Option, } @@ -571,10 +581,6 @@ 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") }