diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 3d3bf788c5..9a30b3941b 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -24,10 +24,14 @@ use serde::Serialize; pub(crate) use approval_request::GuardianApprovalRequest; pub(crate) use approval_request::GuardianMcpAnnotations; pub(crate) use approval_request::guardian_approval_request_to_json; +pub(crate) use review::GuardianApprovalReview; +pub(crate) use review::GuardianApprovalReviewResult; +pub(crate) use review::GuardianApprovalReviewStatus; pub(crate) use review::guardian_rejection_message; pub(crate) use review::is_guardian_reviewer_source; pub(crate) use review::review_approval_request; pub(crate) use review::review_approval_request_with_cancel; +pub(crate) use review::review_approval_request_with_review; pub(crate) use review::routes_approval_to_guardian; pub(crate) use review_session::GuardianReviewSessionManager; diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 320ef8efec..18ed8a60cd 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -59,6 +59,30 @@ pub(super) enum GuardianReviewOutcome { Aborted, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct GuardianApprovalReviewResult { + pub(crate) decision: ReviewDecision, + pub(crate) review: GuardianApprovalReview, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct GuardianApprovalReview { + pub(crate) status: GuardianApprovalReviewStatus, + pub(crate) decision: Option, + pub(crate) risk_level: Option, + pub(crate) user_authorization: Option, + pub(crate) rationale: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum GuardianApprovalReviewStatus { + Approved, + Denied, + Aborted, + Failed, + TimedOut, +} + fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str { match level { GuardianRiskLevel::Low => "low", @@ -94,7 +118,7 @@ async fn run_guardian_review( request: GuardianApprovalRequest, retry_reason: Option, external_cancel: Option, -) -> ReviewDecision { +) -> GuardianApprovalReviewResult { let assessment_id = guardian_request_id(&request).to_string(); let assessment_turn_id = guardian_request_turn_id(&request, &turn.sub_id).to_string(); let action_summary = guardian_assessment_action(&request); @@ -131,7 +155,16 @@ async fn run_guardian_review( }), ) .await; - return ReviewDecision::Abort; + return GuardianApprovalReviewResult { + decision: ReviewDecision::Abort, + review: GuardianApprovalReview { + status: GuardianApprovalReviewStatus::Aborted, + decision: None, + risk_level: None, + user_authorization: None, + rationale: None, + }, + }; } let schema = guardian_output_schema(); @@ -146,22 +179,28 @@ async fn run_guardian_review( ) .await; - let assessment = match outcome { - GuardianReviewOutcome::Completed(Ok(assessment)) => assessment, - GuardianReviewOutcome::Completed(Err(err)) => GuardianAssessment { - risk_level: GuardianRiskLevel::High, - user_authorization: GuardianUserAuthorization::Unknown, - outcome: GuardianAssessmentOutcome::Deny, - rationale: format!("Automatic approval review failed: {err}"), - }, - GuardianReviewOutcome::TimedOut => GuardianAssessment { - risk_level: GuardianRiskLevel::High, - user_authorization: GuardianUserAuthorization::Unknown, - outcome: GuardianAssessmentOutcome::Deny, - rationale: - "Automatic approval review timed out while evaluating the requested approval." - .to_string(), - }, + let (assessment, advisory_status) = match outcome { + GuardianReviewOutcome::Completed(Ok(assessment)) => (assessment, None), + GuardianReviewOutcome::Completed(Err(err)) => ( + GuardianAssessment { + risk_level: GuardianRiskLevel::High, + user_authorization: GuardianUserAuthorization::Unknown, + outcome: GuardianAssessmentOutcome::Deny, + rationale: format!("Automatic approval review failed: {err}"), + }, + Some(GuardianApprovalReviewStatus::Failed), + ), + GuardianReviewOutcome::TimedOut => ( + GuardianAssessment { + risk_level: GuardianRiskLevel::High, + user_authorization: GuardianUserAuthorization::Unknown, + outcome: GuardianAssessmentOutcome::Deny, + rationale: + "Automatic approval review timed out while evaluating the requested approval." + .to_string(), + }, + Some(GuardianApprovalReviewStatus::TimedOut), + ), GuardianReviewOutcome::Aborted => { session .send_event( @@ -177,7 +216,16 @@ async fn run_guardian_review( }), ) .await; - return ReviewDecision::Abort; + return GuardianApprovalReviewResult { + decision: ReviewDecision::Abort, + review: GuardianApprovalReview { + status: GuardianApprovalReviewStatus::Aborted, + decision: None, + risk_level: None, + user_authorization: None, + rationale: None, + }, + }; } }; @@ -208,6 +256,23 @@ async fn run_guardian_review( } else { GuardianAssessmentStatus::Denied }; + let advisory_status = advisory_status.unwrap_or(if approved { + GuardianApprovalReviewStatus::Approved + } else { + GuardianApprovalReviewStatus::Denied + }); + let decision = if approved { + ReviewDecision::Approved + } else { + ReviewDecision::Denied + }; + let review = GuardianApprovalReview { + status: advisory_status, + decision: Some(assessment.outcome), + risk_level: Some(assessment.risk_level), + user_authorization: Some(assessment.user_authorization), + rationale: Some(assessment.rationale.clone()), + }; { let mut rationales = session.services.guardian_rejection_rationales.lock().await; if approved { @@ -231,11 +296,24 @@ async fn run_guardian_review( ) .await; - if approved { - ReviewDecision::Approved - } else { - ReviewDecision::Denied - } + GuardianApprovalReviewResult { decision, review } +} + +/// Runs guardian and returns both the approval decision and hook-visible review data. +pub(crate) async fn review_approval_request_with_review( + session: &Arc, + turn: &Arc, + request: GuardianApprovalRequest, + retry_reason: Option, +) -> GuardianApprovalReviewResult { + run_guardian_review( + Arc::clone(session), + Arc::clone(turn), + request, + retry_reason, + /*external_cancel*/ None, + ) + .await } /// Public entrypoint for approval requests that should be reviewed by guardian. @@ -245,14 +323,9 @@ pub(crate) async fn review_approval_request( request: GuardianApprovalRequest, retry_reason: Option, ) -> ReviewDecision { - run_guardian_review( - Arc::clone(session), - Arc::clone(turn), - request, - retry_reason, - /*external_cancel*/ None, - ) - .await + review_approval_request_with_review(session, turn, request, retry_reason) + .await + .decision } pub(crate) async fn review_approval_request_with_cancel( @@ -270,6 +343,7 @@ pub(crate) async fn review_approval_request_with_cancel( Some(cancel_token), ) .await + .decision } /// Runs the guardian in a locked-down reusable review session. diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 6ce74467e5..984232bb82 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::PermissionRequestApprovalReview; use codex_hooks::PermissionRequestDecision; use codex_hooks::PermissionRequestOutcome; use codex_hooks::PermissionRequestRequest; @@ -154,6 +155,7 @@ pub(crate) async fn run_permission_request_hooks( run_id_suffix: String, tool_name: String, command: String, + approval_review: Option, ) -> Option { let request = PermissionRequestRequest { session_id: sess.conversation_id, @@ -165,6 +167,7 @@ pub(crate) async fn run_permission_request_hooks( tool_name, run_id_suffix, command, + approval_review, }; 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 2473919b3f..b8ffaac73b 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -6,7 +6,12 @@ simple sequence for any ToolRuntime: approval → select sandbox → attempt → retry with an escalated sandbox strategy on denial (no re‑approval thanks to caching). */ +use crate::guardian::GuardianApprovalReview; +use crate::guardian::GuardianApprovalReviewResult; +use crate::guardian::GuardianApprovalReviewStatus; +use crate::guardian::GuardianAssessmentOutcome; use crate::guardian::guardian_rejection_message; +use crate::guardian::review_approval_request_with_review; use crate::guardian::routes_approval_to_guardian; use crate::hook_runtime::run_permission_request_hooks; use crate::network_policy_decision::network_approval_context_from_payload; @@ -23,6 +28,11 @@ 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::PermissionRequestApprovalReview; +use codex_hooks::PermissionRequestApprovalReviewDecision; +use codex_hooks::PermissionRequestApprovalReviewRiskLevel; +use codex_hooks::PermissionRequestApprovalReviewStatus as HookApprovalReviewStatus; +use codex_hooks::PermissionRequestApprovalReviewUserAuthorization; use codex_hooks::PermissionRequestDecision; use codex_otel::SessionTelemetry; use codex_otel::ToolDecisionSource; @@ -371,10 +381,12 @@ impl ToolOrchestrator { } } - // PermissionRequest hooks get the first chance to answer approval prompts - // for tools that expose a hook payload. Today this is Bash-only; if no - // matching hook returns a decision, fall back to the normal user or guardian - // approval path. + // Centralize one approval prompt for three possible decision makers. If + // this prompt would normally route to guardian, run guardian first and pass + // its result to PermissionRequest hooks as advisory context. The hook can + // still answer the prompt; if it stays quiet, reuse the guardian decision + // instead of asking guardian again. Without a hook or reusable guardian + // result, fall back to the runtime's normal approval path. async fn request_approval( tool: &mut T, req: &Rq, @@ -387,6 +399,23 @@ impl ToolOrchestrator { where T: ToolRuntime, { + let guardian_review = if routes_approval_to_guardian(turn_ctx) { + match tool.guardian_approval_request(req, &approval_ctx) { + Some(request) => Some( + review_approval_request_with_review( + approval_ctx.session, + approval_ctx.turn, + request, + approval_ctx.retry_reason.clone(), + ) + .await, + ), + None => None, + } + } else { + None + }; + if let Some(permission_request) = tool.permission_request_payload(req) { match run_permission_request_hooks( approval_ctx.session, @@ -394,6 +423,9 @@ impl ToolOrchestrator { approval_ctx.call_id.to_string(), permission_request.tool_name, permission_request.command, + guardian_review + .as_ref() + .map(|review| permission_request_approval_review(review.review.clone())), ) .await { @@ -419,6 +451,16 @@ impl ToolOrchestrator { } } + if let Some(GuardianApprovalReviewResult { decision, .. }) = guardian_review { + otel.tool_decision( + otel_tn, + otel_ci, + &decision, + ToolDecisionSource::AutomatedReviewer, + ); + return Ok(decision); + } + let decision = tool.start_approval_async(req, approval_ctx).await; let otel_source = if routes_approval_to_guardian(turn_ctx) { ToolDecisionSource::AutomatedReviewer @@ -435,3 +477,52 @@ fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String { // output so we can evolve heuristics later without touching call sites. "command failed; retry without sandbox?".to_string() } + +fn permission_request_approval_review( + review: GuardianApprovalReview, +) -> PermissionRequestApprovalReview { + PermissionRequestApprovalReview { + status: match review.status { + GuardianApprovalReviewStatus::Approved => HookApprovalReviewStatus::Approved, + GuardianApprovalReviewStatus::Denied => HookApprovalReviewStatus::Denied, + GuardianApprovalReviewStatus::Aborted => HookApprovalReviewStatus::Aborted, + GuardianApprovalReviewStatus::Failed => HookApprovalReviewStatus::Failed, + GuardianApprovalReviewStatus::TimedOut => HookApprovalReviewStatus::TimedOut, + }, + decision: review.decision.map(|decision| match decision { + GuardianAssessmentOutcome::Allow => PermissionRequestApprovalReviewDecision::Allow, + GuardianAssessmentOutcome::Deny => PermissionRequestApprovalReviewDecision::Deny, + }), + risk_level: review.risk_level.map(|risk_level| match risk_level { + codex_protocol::protocol::GuardianRiskLevel::Low => { + PermissionRequestApprovalReviewRiskLevel::Low + } + codex_protocol::protocol::GuardianRiskLevel::Medium => { + PermissionRequestApprovalReviewRiskLevel::Medium + } + codex_protocol::protocol::GuardianRiskLevel::High => { + PermissionRequestApprovalReviewRiskLevel::High + } + codex_protocol::protocol::GuardianRiskLevel::Critical => { + PermissionRequestApprovalReviewRiskLevel::Critical + } + }), + user_authorization: review.user_authorization.map(|user_authorization| { + match user_authorization { + codex_protocol::protocol::GuardianUserAuthorization::Unknown => { + PermissionRequestApprovalReviewUserAuthorization::Unknown + } + codex_protocol::protocol::GuardianUserAuthorization::Low => { + PermissionRequestApprovalReviewUserAuthorization::Low + } + codex_protocol::protocol::GuardianUserAuthorization::Medium => { + PermissionRequestApprovalReviewUserAuthorization::Medium + } + codex_protocol::protocol::GuardianUserAuthorization::High => { + PermissionRequestApprovalReviewUserAuthorization::High + } + } + }), + rationale: review.rationale, + } +} diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 98a5b6d0c4..260ca2cc09 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -118,6 +118,17 @@ impl ShellRuntime { tx_event: ctx.session.get_tx_event(), }) } + + fn guardian_request(req: &ShellRequest, ctx: &ApprovalCtx<'_>) -> GuardianApprovalRequest { + GuardianApprovalRequest::Shell { + id: ctx.call_id.to_string(), + command: req.command.clone(), + cwd: req.cwd.to_path_buf(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + } + } } impl Sandboxable for ShellRuntime { @@ -147,6 +158,7 @@ impl Approvable for ShellRuntime { ctx: ApprovalCtx<'a>, ) -> BoxFuture<'a, ReviewDecision> { let keys = self.approval_keys(req); + let guardian_request = Self::guardian_request(req, &ctx); let command = req.command.clone(); let cwd = req.cwd.to_path_buf(); let retry_reason = ctx.retry_reason.clone(); @@ -156,20 +168,8 @@ impl Approvable for ShellRuntime { let call_id = ctx.call_id.to_string(); Box::pin(async move { if routes_approval_to_guardian(turn) { - return review_approval_request( - session, - turn, - GuardianApprovalRequest::Shell { - id: call_id, - command, - cwd, - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - }, - retry_reason, - ) - .await; + return review_approval_request(session, turn, guardian_request, retry_reason) + .await; } with_cached_approval(&session.services, "shell", keys, move || async move { let available_decisions = None; @@ -202,6 +202,14 @@ impl Approvable for ShellRuntime { Some(PermissionRequestPayload::bash(req.hook_command.clone())) } + fn guardian_approval_request( + &self, + req: &ShellRequest, + ctx: &ApprovalCtx<'_>, + ) -> Option { + Some(Self::guardian_request(req, ctx)) + } + fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride { sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement) } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 519728f576..03e1e72e31 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -92,6 +92,21 @@ impl<'a> UnifiedExecRuntime<'a> { shell_mode, } } + + fn guardian_request( + req: &UnifiedExecRequest, + ctx: &ApprovalCtx<'_>, + ) -> GuardianApprovalRequest { + GuardianApprovalRequest::ExecCommand { + id: ctx.call_id.to_string(), + command: req.command.clone(), + cwd: req.cwd.to_path_buf(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + tty: req.tty, + } + } } impl Sandboxable for UnifiedExecRuntime<'_> { @@ -123,6 +138,7 @@ impl Approvable for UnifiedExecRuntime<'_> { ctx: ApprovalCtx<'b>, ) -> BoxFuture<'b, ReviewDecision> { let keys = self.approval_keys(req); + let guardian_request = Self::guardian_request(req, &ctx); let session = ctx.session; let turn = ctx.turn; let call_id = ctx.call_id.to_string(); @@ -132,21 +148,8 @@ impl Approvable for UnifiedExecRuntime<'_> { let reason = retry_reason.clone().or_else(|| req.justification.clone()); Box::pin(async move { if routes_approval_to_guardian(turn) { - return review_approval_request( - session, - turn, - GuardianApprovalRequest::ExecCommand { - id: call_id, - command, - cwd, - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - tty: req.tty, - }, - retry_reason, - ) - .await; + return review_approval_request(session, turn, guardian_request, retry_reason) + .await; } with_cached_approval(&session.services, "unified_exec", keys, || async move { let available_decisions = None; @@ -185,6 +188,14 @@ impl Approvable for UnifiedExecRuntime<'_> { Some(PermissionRequestPayload::bash(req.hook_command.clone())) } + fn guardian_approval_request( + &self, + req: &UnifiedExecRequest, + ctx: &ApprovalCtx<'_>, + ) -> Option { + Some(Self::guardian_request(req, ctx)) + } + fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride { sandbox_override_for_first_attempt(req.sandbox_permissions, &req.exec_approval_requirement) } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index b3c37af4fc..19fd4d7bca 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -6,6 +6,7 @@ use crate::codex::Session; use crate::codex::TurnContext; +use crate::guardian::GuardianApprovalRequest; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::state::SessionServices; @@ -285,6 +286,19 @@ pub(crate) trait Approvable { None } + /// Build the guardian request that corresponds to this approval prompt. + /// + /// Runtimes that can route approvals through guardian should return the + /// same request they would pass to `review_approval_request`, so shared + /// orchestration can run guardian once and reuse that decision as fallback. + fn guardian_approval_request( + &self, + _req: &Req, + _ctx: &ApprovalCtx<'_>, + ) -> Option { + None + } + /// Decide we can request an approval for no-sandbox execution. fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool { match policy { diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 1825a44699..1b37924521 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -3,6 +3,7 @@ use std::path::Path; use anyhow::Context; use anyhow::Result; +use codex_config::types::ApprovalsReviewer; use codex_features::Feature; use codex_protocol::items::parse_hook_prompt_fragment; use codex_protocol::models::ContentItem; @@ -1169,6 +1170,7 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> assert_eq!(hook_inputs[0]["hook_event_name"], "PermissionRequest"); assert_eq!(hook_inputs[0]["tool_name"], "Bash"); assert_eq!(hook_inputs[0]["tool_input"]["command"], command); + assert_eq!(hook_inputs[0]["approval_review"], Value::Null); assert!( hook_inputs[0].get("tool_use_id").is_none(), "PermissionRequest input should not include a tool_use_id", @@ -1182,6 +1184,108 @@ async fn permission_request_hook_allows_shell_command_without_user_approval() -> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn permission_request_hook_receives_guardian_review_before_fallback() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "permissionrequest-guardian-review"; + let marker = std::env::temp_dir().join("permissionrequest-guardian-review-marker"); + let command = format!("printf guardian > {}", marker.display()); + let args = serde_json::json!({ "command": command }); + let guardian_assessment = serde_json::json!({ + "risk_level": "medium", + "user_authorization": "high", + "outcome": "allow", + "rationale": "The user asked to run this local marker command.", + }) + .to_string(); + 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-guardian"), + ev_assistant_message("msg-guardian", &guardian_assessment), + ev_completed("resp-guardian"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", "guardian fallback allowed it"), + 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$"), "quiet", "unused") + { + 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"); + config + .features + .enable(Feature::GuardianApproval) + .expect("test config should allow feature update"); + config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + }); + let test = builder.build(&server).await?; + + if marker.exists() { + fs::remove_file(&marker).context("remove leftover guardian review marker")?; + } + + test.submit_turn_with_policies( + "run the shell command after guardian review", + AskForApproval::OnRequest, + codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + ) + .await?; + + let requests = responses.requests(); + assert_eq!( + requests.len(), + 3, + "guardian decision should be reused instead of running a second review", + ); + assert!( + marker.exists(), + "guardian-approved fallback should create marker file", + ); + + let hook_inputs = read_permission_request_hook_inputs(test.codex_home_path())?; + assert_eq!(hook_inputs.len(), 1); + assert_eq!( + hook_inputs[0]["approval_review"], + serde_json::json!({ + "source": "guardian", + "status": "approved", + "decision": "allow", + "risk_level": "medium", + "user_authorization": "high", + "rationale": "The user asked to run this local marker command.", + }) + ); + + 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 9bdcacf0cb..49b2da2160 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 @@ -8,6 +8,98 @@ "null" ] }, + "PermissionRequestApprovalReviewDecisionWire": { + "enum": [ + "allow", + "deny" + ], + "type": "string" + }, + "PermissionRequestApprovalReviewRiskLevelWire": { + "enum": [ + "low", + "medium", + "high", + "critical" + ], + "type": "string" + }, + "PermissionRequestApprovalReviewStatusWire": { + "enum": [ + "approved", + "denied", + "aborted", + "failed", + "timed_out" + ], + "type": "string" + }, + "PermissionRequestApprovalReviewUserAuthorizationWire": { + "enum": [ + "unknown", + "low", + "medium", + "high" + ], + "type": "string" + }, + "PermissionRequestApprovalReviewWire": { + "additionalProperties": false, + "properties": { + "decision": { + "anyOf": [ + { + "$ref": "#/definitions/PermissionRequestApprovalReviewDecisionWire" + }, + { + "type": "null" + } + ] + }, + "rationale": { + "type": [ + "string", + "null" + ] + }, + "risk_level": { + "anyOf": [ + { + "$ref": "#/definitions/PermissionRequestApprovalReviewRiskLevelWire" + }, + { + "type": "null" + } + ] + }, + "source": { + "const": "guardian", + "type": "string" + }, + "status": { + "$ref": "#/definitions/PermissionRequestApprovalReviewStatusWire" + }, + "user_authorization": { + "anyOf": [ + { + "$ref": "#/definitions/PermissionRequestApprovalReviewUserAuthorizationWire" + }, + { + "type": "null" + } + ] + } + }, + "required": [ + "decision", + "rationale", + "risk_level", + "source", + "status", + "user_authorization" + ], + "type": "object" + }, "PermissionRequestToolInput": { "additionalProperties": false, "properties": { @@ -22,6 +114,16 @@ } }, "properties": { + "approval_review": { + "anyOf": [ + { + "$ref": "#/definitions/PermissionRequestApprovalReviewWire" + }, + { + "type": "null" + } + ] + }, "cwd": { "type": "string" }, @@ -61,6 +163,7 @@ } }, "required": [ + "approval_review", "cwd", "hook_event_name", "model", diff --git a/codex-rs/hooks/src/events/permission_request.rs b/codex-rs/hooks/src/events/permission_request.rs index 62a3265f7c..e7aff0e804 100644 --- a/codex-rs/hooks/src/events/permission_request.rs +++ b/codex-rs/hooks/src/events/permission_request.rs @@ -1,3 +1,10 @@ +//! PermissionRequest hook execution for approval prompts. +//! +//! This event is different from `PreToolUse`: it runs only when Codex is about +//! to ask for permission, and its decision answers that approval prompt rather +//! than blocking normal tool execution. A quiet hook is a no-op so callers can +//! fall back to the existing approval path. + use std::path::PathBuf; use codex_protocol::ThreadId; @@ -14,6 +21,11 @@ use crate::engine::ConfiguredHandler; use crate::engine::command_runner::CommandRunResult; use crate::engine::dispatcher; use crate::engine::output_parser; +use crate::schema::PermissionRequestApprovalReviewDecisionWire; +use crate::schema::PermissionRequestApprovalReviewRiskLevelWire; +use crate::schema::PermissionRequestApprovalReviewStatusWire; +use crate::schema::PermissionRequestApprovalReviewUserAuthorizationWire; +use crate::schema::PermissionRequestApprovalReviewWire; use crate::schema::PermissionRequestCommandInput; use crate::schema::PermissionRequestToolInput; @@ -26,8 +38,18 @@ pub struct PermissionRequestRequest { pub model: String, pub permission_mode: String, pub tool_name: String, + /// Suffix used only for hook run ids. + /// + /// Claude's PermissionRequest input does not include `tool_use_id`, but Codex + /// still needs stable begin/end ids for hook UI and transcript bookkeeping. pub run_id_suffix: String, pub command: String, + /// Advisory approval context from Codex's automated reviewer, when one ran. + /// + /// A hook can use this as another signal, but it is not bound by the + /// guardian's decision. The hook may allow, deny, or stay quiet; if it stays + /// quiet, the orchestrator falls back to the guardian's original decision. + pub approval_review: Option, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -36,6 +58,46 @@ pub enum PermissionRequestDecision { Deny { message: String }, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PermissionRequestApprovalReview { + pub status: PermissionRequestApprovalReviewStatus, + pub decision: Option, + pub risk_level: Option, + pub user_authorization: Option, + pub rationale: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PermissionRequestApprovalReviewStatus { + Approved, + Denied, + Aborted, + Failed, + TimedOut, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PermissionRequestApprovalReviewDecision { + Allow, + Deny, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PermissionRequestApprovalReviewRiskLevel { + Low, + Medium, + High, + Critical, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PermissionRequestApprovalReviewUserAuthorization { + Unknown, + Low, + Medium, + High, +} + #[derive(Debug)] pub struct PermissionRequestOutcome { pub hook_events: Vec, @@ -83,6 +145,10 @@ pub(crate) async fn run( }; } + // This first pass is Bash-only. Keep the wire input fixed to Claude's + // `Bash` shape even though the request carries `tool_name`, so later + // tool support has to choose its own explicit schema instead of + // accidentally inheriting Bash fields. let input_json = match serde_json::to_string(&PermissionRequestCommandInput { session_id: request.session_id.to_string(), turn_id: request.turn_id.clone(), @@ -95,6 +161,7 @@ pub(crate) async fn run( tool_input: PermissionRequestToolInput { command: request.command.clone(), }, + approval_review: request.approval_review.map(Into::into), }) { Ok(input_json) => input_json, Err(error) => { @@ -121,6 +188,9 @@ pub(crate) async fn run( ) .await; + // Multiple hooks may match the same approval prompt. For now, use the first + // explicit decision in declaration order and leave richer precedence rules + // to the follow-up work. let decision = results .iter() .find_map(|result| result.data.decision.clone()); @@ -188,6 +258,9 @@ fn parse_completed( } } } else if trimmed_stdout.starts_with('{') || trimmed_stdout.starts_with('[') { + // Invalid JSON-like output is treated as a hook failure, not an + // approval decision. That keeps malformed hooks fail-open: the + // orchestrator can still fall back to normal approval. status = HookRunStatus::Failed; entries.push(HookOutputEntry { kind: HookOutputEntryKind::Error, @@ -196,6 +269,8 @@ fn parse_completed( } } Some(2) => { + // Match Claude's blocking-hook convention: exit code 2 denies + // the approval prompt, with stderr as the denial message. if let Some(message) = common::trimmed_non_empty(&run_result.stderr) { status = HookRunStatus::Blocked; entries.push(HookOutputEntry { @@ -238,3 +313,63 @@ fn parse_completed( data: PermissionRequestHandlerData { decision }, } } + +impl From for PermissionRequestApprovalReviewWire { + fn from(value: PermissionRequestApprovalReview) -> Self { + Self { + source: "guardian".to_string(), + status: value.status.into(), + decision: value.decision.map(Into::into), + risk_level: value.risk_level.map(Into::into), + user_authorization: value.user_authorization.map(Into::into), + rationale: value.rationale, + } + } +} + +impl From for PermissionRequestApprovalReviewStatusWire { + fn from(value: PermissionRequestApprovalReviewStatus) -> Self { + match value { + PermissionRequestApprovalReviewStatus::Approved => Self::Approved, + PermissionRequestApprovalReviewStatus::Denied => Self::Denied, + PermissionRequestApprovalReviewStatus::Aborted => Self::Aborted, + PermissionRequestApprovalReviewStatus::Failed => Self::Failed, + PermissionRequestApprovalReviewStatus::TimedOut => Self::TimedOut, + } + } +} + +impl From for PermissionRequestApprovalReviewDecisionWire { + fn from(value: PermissionRequestApprovalReviewDecision) -> Self { + match value { + PermissionRequestApprovalReviewDecision::Allow => Self::Allow, + PermissionRequestApprovalReviewDecision::Deny => Self::Deny, + } + } +} + +impl From + for PermissionRequestApprovalReviewRiskLevelWire +{ + fn from(value: PermissionRequestApprovalReviewRiskLevel) -> Self { + match value { + PermissionRequestApprovalReviewRiskLevel::Low => Self::Low, + PermissionRequestApprovalReviewRiskLevel::Medium => Self::Medium, + PermissionRequestApprovalReviewRiskLevel::High => Self::High, + PermissionRequestApprovalReviewRiskLevel::Critical => Self::Critical, + } + } +} + +impl From + for PermissionRequestApprovalReviewUserAuthorizationWire +{ + fn from(value: PermissionRequestApprovalReviewUserAuthorization) -> Self { + match value { + PermissionRequestApprovalReviewUserAuthorization::Unknown => Self::Unknown, + PermissionRequestApprovalReviewUserAuthorization::Low => Self::Low, + PermissionRequestApprovalReviewUserAuthorization::Medium => Self::Medium, + PermissionRequestApprovalReviewUserAuthorization::High => Self::High, + } + } +} diff --git a/codex-rs/hooks/src/lib.rs b/codex-rs/hooks/src/lib.rs index c8358c678c..c94bf2c221 100644 --- a/codex-rs/hooks/src/lib.rs +++ b/codex-rs/hooks/src/lib.rs @@ -5,6 +5,11 @@ mod registry; mod schema; mod types; +pub use events::permission_request::PermissionRequestApprovalReview; +pub use events::permission_request::PermissionRequestApprovalReviewDecision; +pub use events::permission_request::PermissionRequestApprovalReviewRiskLevel; +pub use events::permission_request::PermissionRequestApprovalReviewStatus; +pub use events::permission_request::PermissionRequestApprovalReviewUserAuthorization; 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 51a635f761..11e44d5151 100644 --- a/codex-rs/hooks/src/schema.rs +++ b/codex-rs/hooks/src/schema.rs @@ -5,6 +5,7 @@ use schemars::schema::InstanceType; use schemars::schema::RootSchema; use schemars::schema::Schema; use schemars::schema::SchemaObject; +use schemars::schema::SubschemaValidation; use serde::Deserialize; use serde::Serialize; use serde_json::Map; @@ -235,6 +236,59 @@ pub(crate) struct PermissionRequestToolInput { pub command: String, } +#[derive(Debug, Clone, Serialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub(crate) struct PermissionRequestApprovalReviewWire { + #[schemars(schema_with = "permission_request_approval_review_source_schema")] + pub source: String, + pub status: PermissionRequestApprovalReviewStatusWire, + #[schemars(schema_with = "nullable_permission_request_approval_review_decision_schema")] + pub decision: Option, + #[schemars(schema_with = "nullable_permission_request_approval_review_risk_level_schema")] + pub risk_level: Option, + #[schemars( + schema_with = "nullable_permission_request_approval_review_user_authorization_schema" + )] + pub user_authorization: Option, + #[schemars(schema_with = "nullable_string_schema")] + pub rationale: Option, +} + +#[derive(Debug, Clone, Serialize, JsonSchema, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub(crate) enum PermissionRequestApprovalReviewStatusWire { + Approved, + Denied, + Aborted, + Failed, + TimedOut, +} + +#[derive(Debug, Clone, Serialize, JsonSchema, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub(crate) enum PermissionRequestApprovalReviewDecisionWire { + Allow, + Deny, +} + +#[derive(Debug, Clone, Serialize, JsonSchema, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub(crate) enum PermissionRequestApprovalReviewRiskLevelWire { + Low, + Medium, + High, + Critical, +} + +#[derive(Debug, Clone, Serialize, JsonSchema, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub(crate) enum PermissionRequestApprovalReviewUserAuthorizationWire { + Unknown, + Low, + Medium, + High, +} + #[derive(Debug, Clone, Serialize, JsonSchema)] #[serde(deny_unknown_fields)] #[schemars(rename = "permission-request.command.input")] @@ -252,6 +306,8 @@ pub(crate) struct PermissionRequestCommandInput { #[schemars(schema_with = "permission_request_tool_name_schema")] pub tool_name: String, pub tool_input: PermissionRequestToolInput, + #[schemars(schema_with = "nullable_permission_request_approval_review_schema")] + pub approval_review: Option, } #[derive(Debug, Clone, Serialize, JsonSchema)] @@ -554,6 +610,54 @@ fn permission_request_tool_name_schema(_gen: &mut SchemaGenerator) -> Schema { string_const_schema("Bash") } +fn permission_request_approval_review_source_schema(_gen: &mut SchemaGenerator) -> Schema { + string_const_schema("guardian") +} + +fn nullable_permission_request_approval_review_schema(generator: &mut SchemaGenerator) -> Schema { + nullable_schema(generator.subschema_for::()) +} + +fn nullable_permission_request_approval_review_decision_schema( + generator: &mut SchemaGenerator, +) -> Schema { + nullable_schema(generator.subschema_for::()) +} + +fn nullable_permission_request_approval_review_risk_level_schema( + generator: &mut SchemaGenerator, +) -> Schema { + nullable_schema(generator.subschema_for::()) +} + +fn nullable_permission_request_approval_review_user_authorization_schema( + generator: &mut SchemaGenerator, +) -> Schema { + nullable_schema( + generator.subschema_for::(), + ) +} + +fn nullable_string_schema(generator: &mut SchemaGenerator) -> Schema { + NullableString::json_schema(generator) +} + +fn nullable_schema(schema: Schema) -> Schema { + Schema::Object(SchemaObject { + subschemas: Some(Box::new(SubschemaValidation { + any_of: Some(vec![ + schema, + Schema::Object(SchemaObject { + instance_type: Some(InstanceType::Null.into()), + ..Default::default() + }), + ]), + ..Default::default() + })), + ..Default::default() + }) +} + fn user_prompt_submit_hook_event_name_schema(_gen: &mut SchemaGenerator) -> Schema { string_const_schema("UserPromptSubmit") }