diff --git a/codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json b/codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json index 84c36edf10..c47cf09f2d 100644 --- a/codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json +++ b/codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json @@ -88,11 +88,26 @@ "type": "object" }, { + "additionalProperties": false, "description": "User has denied this command and the agent should not execute it, but it should continue the session and try something else.", - "enum": [ + "properties": { + "denied": { + "properties": { + "rejection": { + "type": "string" + } + }, + "required": [ + "rejection" + ], + "type": "object" + } + }, + "required": [ "denied" ], - "type": "string" + "title": "DeniedReviewDecision", + "type": "object" }, { "description": "Automatic approval review timed out before reaching a decision.", diff --git a/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json b/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json index 477109e2b0..7a78661926 100644 --- a/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json +++ b/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json @@ -88,11 +88,26 @@ "type": "object" }, { + "additionalProperties": false, "description": "User has denied this command and the agent should not execute it, but it should continue the session and try something else.", - "enum": [ + "properties": { + "denied": { + "properties": { + "rejection": { + "type": "string" + } + }, + "required": [ + "rejection" + ], + "type": "object" + } + }, + "required": [ "denied" ], - "type": "string" + "title": "DeniedReviewDecision", + "type": "object" }, { "description": "Automatic approval review timed out before reaching a decision.", diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 6691e30019..b1981a4cf2 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -4129,11 +4129,26 @@ "type": "object" }, { + "additionalProperties": false, "description": "User has denied this command and the agent should not execute it, but it should continue the session and try something else.", - "enum": [ + "properties": { + "denied": { + "properties": { + "rejection": { + "type": "string" + } + }, + "required": [ + "rejection" + ], + "type": "object" + } + }, + "required": [ "denied" ], - "type": "string" + "title": "DeniedReviewDecision", + "type": "object" }, { "description": "Automatic approval review timed out before reaching a decision.", diff --git a/codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts b/codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts index 109f72929c..22c09a24e2 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts @@ -7,4 +7,4 @@ import type { NetworkPolicyAmendment } from "./NetworkPolicyAmendment"; /** * User's decision in response to an ExecApprovalRequest. */ -export type ReviewDecision = "approved" | { "approved_execpolicy_amendment": { proposed_execpolicy_amendment: ExecPolicyAmendment, } } | "approved_for_session" | { "network_policy_amendment": { network_policy_amendment: NetworkPolicyAmendment, } } | "denied" | "timed_out" | "abort"; +export type ReviewDecision = "approved" | { "approved_execpolicy_amendment": { proposed_execpolicy_amendment: ExecPolicyAmendment, } } | "approved_for_session" | { "network_policy_amendment": { network_policy_amendment: NetworkPolicyAmendment, } } | { "denied": { rejection: string, } } | "timed_out" | "abort"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index 2f38a723f3..e26fa35baa 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -94,7 +94,7 @@ impl From for CommandExecutionApprovalDecision { network_policy_amendment: network_policy_amendment.into(), }, CoreReviewDecision::Abort => Self::Cancel, - CoreReviewDecision::Denied => Self::Decline, + CoreReviewDecision::Denied { .. } => Self::Decline, CoreReviewDecision::TimedOut => Self::Decline, } } diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 86555017f1..c51fd7ffd8 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -1886,7 +1886,7 @@ fn map_file_change_approval_decision(decision: FileChangeApprovalDecision) -> Re match decision { FileChangeApprovalDecision::Accept => ReviewDecision::Approved, FileChangeApprovalDecision::AcceptForSession => ReviewDecision::ApprovedForSession, - FileChangeApprovalDecision::Decline => ReviewDecision::Denied, + FileChangeApprovalDecision::Decline => ReviewDecision::denied("rejected by user"), FileChangeApprovalDecision::Cancel => ReviewDecision::Abort, } } @@ -1904,25 +1904,21 @@ async fn on_file_change_request_approval_response( resolve_server_request_on_thread_listener(&thread_state, pending_request_id).await; drop(permission_guard); let decision = match response { - Ok(Ok(value)) => { - let response = serde_json::from_value::(value) - .unwrap_or_else(|err| { - error!("failed to deserialize FileChangeRequestApprovalResponse: {err}"); - FileChangeRequestApprovalResponse { - decision: FileChangeApprovalDecision::Decline, - } - }); - - map_file_change_approval_decision(response.decision) - } + Ok(Ok(value)) => match serde_json::from_value::(value) { + Ok(response) => map_file_change_approval_decision(response.decision), + Err(err) => { + error!("failed to deserialize FileChangeRequestApprovalResponse: {err}"); + ReviewDecision::denied("approval request failed") + } + }, Ok(Err(err)) if is_turn_transition_server_request_error(&err) => return, Ok(Err(err)) => { error!("request failed with client error: {err:?}"); - ReviewDecision::Denied + ReviewDecision::denied("approval request failed") } Err(err) => { error!("request failed: {err:?}"); - ReviewDecision::Denied + ReviewDecision::denied("approval request failed") } }; @@ -1956,62 +1952,68 @@ async fn on_command_execution_request_approval_response( drop(permission_guard); let (decision, completion_status) = match response { Ok(Ok(value)) => { - let response = serde_json::from_value::(value) - .unwrap_or_else(|err| { - error!("failed to deserialize CommandExecutionRequestApprovalResponse: {err}"); - CommandExecutionRequestApprovalResponse { - decision: CommandExecutionApprovalDecision::Decline, + match serde_json::from_value::(value) { + Ok(response) => match response.decision { + CommandExecutionApprovalDecision::Accept => (ReviewDecision::Approved, None), + CommandExecutionApprovalDecision::AcceptForSession => { + (ReviewDecision::ApprovedForSession, None) } - }); - - let decision = response.decision; - - let (decision, completion_status) = match decision { - CommandExecutionApprovalDecision::Accept => (ReviewDecision::Approved, None), - CommandExecutionApprovalDecision::AcceptForSession => { - (ReviewDecision::ApprovedForSession, None) - } - CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment { - execpolicy_amendment, - } => ( - ReviewDecision::ApprovedExecpolicyAmendment { - proposed_execpolicy_amendment: execpolicy_amendment.into_core(), - }, - None, - ), - CommandExecutionApprovalDecision::ApplyNetworkPolicyAmendment { - network_policy_amendment, - } => { - let completion_status = match network_policy_amendment.action { - V2NetworkPolicyRuleAction::Allow => None, - V2NetworkPolicyRuleAction::Deny => Some(CommandExecutionStatus::Declined), - }; - ( - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment: network_policy_amendment.into_core(), + CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment { + execpolicy_amendment, + } => ( + ReviewDecision::ApprovedExecpolicyAmendment { + proposed_execpolicy_amendment: execpolicy_amendment.into_core(), }, - completion_status, + None, + ), + CommandExecutionApprovalDecision::ApplyNetworkPolicyAmendment { + network_policy_amendment, + } => { + let completion_status = match network_policy_amendment.action { + V2NetworkPolicyRuleAction::Allow => None, + V2NetworkPolicyRuleAction::Deny => { + Some(CommandExecutionStatus::Declined) + } + }; + ( + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: network_policy_amendment.into_core(), + }, + completion_status, + ) + } + CommandExecutionApprovalDecision::Decline => ( + ReviewDecision::denied("rejected by user"), + Some(CommandExecutionStatus::Declined), + ), + CommandExecutionApprovalDecision::Cancel => ( + ReviewDecision::Abort, + Some(CommandExecutionStatus::Declined), + ), + }, + Err(err) => { + error!("failed to deserialize CommandExecutionRequestApprovalResponse: {err}"); + ( + ReviewDecision::denied("approval request failed"), + Some(CommandExecutionStatus::Failed), ) } - CommandExecutionApprovalDecision::Decline => ( - ReviewDecision::Denied, - Some(CommandExecutionStatus::Declined), - ), - CommandExecutionApprovalDecision::Cancel => ( - ReviewDecision::Abort, - Some(CommandExecutionStatus::Declined), - ), - }; - (decision, completion_status) + } } Ok(Err(err)) if is_turn_transition_server_request_error(&err) => return, Ok(Err(err)) => { error!("request failed with client error: {err:?}"); - (ReviewDecision::Denied, Some(CommandExecutionStatus::Failed)) + ( + ReviewDecision::denied("approval request failed"), + Some(CommandExecutionStatus::Failed), + ) } Err(err) => { error!("request failed: {err:?}"); - (ReviewDecision::Denied, Some(CommandExecutionStatus::Failed)) + ( + ReviewDecision::denied("approval request failed"), + Some(CommandExecutionStatus::Failed), + ) } }; diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index bbf92c37c0..15994e7e22 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -2388,6 +2388,31 @@ async fn turn_start_exec_approval_toggle_v2() -> Result<()> { #[tokio::test] async fn turn_start_exec_approval_decline_v2() -> Result<()> { + run_turn_start_exec_approval_rejection_v2( + serde_json::to_value(CommandExecutionRequestApprovalResponse { + decision: CommandExecutionApprovalDecision::Decline, + })?, + CommandExecutionStatus::Declined, + "rejected by user", + ) + .await +} + +#[tokio::test] +async fn turn_start_exec_approval_invalid_response_v2() -> Result<()> { + run_turn_start_exec_approval_rejection_v2( + json!({ "unexpected": "response" }), + CommandExecutionStatus::Failed, + "approval request failed", + ) + .await +} + +async fn run_turn_start_exec_approval_rejection_v2( + approval_response: Value, + expected_status: CommandExecutionStatus, + expected_rejection: &str, +) -> Result<()> { // TODO(anp): Remove after command approval routing accepts target-native Windows cwd. skip_if_wine_exec!( Ok(()), @@ -2487,13 +2512,7 @@ async fn turn_start_exec_approval_decline_v2() -> Result<()> { assert_eq!(params.thread_id, thread.id); assert_eq!(params.turn_id, turn.id); - mcp.send_response( - request_id, - serde_json::to_value(CommandExecutionRequestApprovalResponse { - decision: CommandExecutionApprovalDecision::Decline, - })?, - ) - .await?; + mcp.send_response(request_id, approval_response).await?; let completed_command_execution = timeout(DEFAULT_READ_TIMEOUT, async { loop { @@ -2523,7 +2542,7 @@ async fn turn_start_exec_approval_decline_v2() -> Result<()> { unreachable!("loop ensures we break on command execution items"); }; assert_eq!(id, "call-decline"); - assert_eq!(status, CommandExecutionStatus::Declined); + assert_eq!(status, expected_status); assert!(exit_code.is_none()); assert!(aggregated_output.is_none()); @@ -2533,6 +2552,17 @@ async fn turn_start_exec_approval_decline_v2() -> Result<()> { ) .await??; + let requests = server + .received_requests() + .await + .context("failed to fetch received requests")?; + assert!( + requests.iter().any(|request| { + request.url.path().ends_with("/responses") && body_contains(request, expected_rejection) + }), + "model request should include approval rejection: {expected_rejection}" + ); + Ok(()) } @@ -4255,6 +4285,28 @@ async fn turn_start_file_change_approval_accept_for_session_persists_v2() -> Res #[tokio::test] async fn turn_start_file_change_approval_decline_v2() -> Result<()> { + run_turn_start_file_change_approval_rejection_v2( + serde_json::to_value(FileChangeRequestApprovalResponse { + decision: FileChangeApprovalDecision::Decline, + })?, + "rejected by user", + ) + .await +} + +#[tokio::test] +async fn turn_start_file_change_approval_invalid_response_v2() -> Result<()> { + run_turn_start_file_change_approval_rejection_v2( + json!({ "unexpected": "response" }), + "approval request failed", + ) + .await +} + +async fn run_turn_start_file_change_approval_rejection_v2( + approval_response: Value, + expected_rejection: &str, +) -> Result<()> { // TODO(anp): Materialize apply-patch workspaces in the selected remote environment. skip_if_remote!( Ok(()), @@ -4371,13 +4423,7 @@ async fn turn_start_file_change_approval_decline_v2() -> Result<()> { }] ); - mcp.send_response( - request_id, - serde_json::to_value(FileChangeRequestApprovalResponse { - decision: FileChangeApprovalDecision::Decline, - })?, - ) - .await?; + mcp.send_response(request_id, approval_response).await?; let completed_file_change = timeout(DEFAULT_READ_TIMEOUT, async { loop { @@ -4408,6 +4454,17 @@ async fn turn_start_file_change_approval_decline_v2() -> Result<()> { ) .await??; + let requests = server + .received_requests() + .await + .context("failed to fetch received requests")?; + assert!( + requests.iter().any(|request| { + request.url.path().ends_with("/responses") && body_contains(request, expected_rejection) + }), + "model request should include approval rejection: {expected_rejection}" + ); + assert!( !expected_readme_path.exists(), "declined patch should not be applied" diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 4d2953f3e9..eba2bc6f8b 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -28,6 +28,7 @@ use codex_protocol::user_input::UserInput; use serde_json::Value; use std::time::Duration; use tokio::sync::Mutex; +use tokio::sync::oneshot; use tokio::time::timeout; use tokio_util::sync::CancellationToken; @@ -534,7 +535,7 @@ async fn handle_exec_approval( review_cancel.clone(), ); await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, + receive_approval_review(review_rx), parent_session, &approval_id_for_op, cancel_token, @@ -643,7 +644,7 @@ async fn handle_patch_approval( ); Some( await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, + receive_approval_review(review_rx), parent_session, &approval_id, cancel_token, @@ -758,7 +759,7 @@ async fn maybe_auto_review_mcp_request_user_input( review_cancel.clone(), ); let decision = await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, + receive_approval_review(review_rx), parent_session, &event.call_id, cancel_token, @@ -779,7 +780,7 @@ async fn maybe_auto_review_mcp_request_user_input( ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } | ReviewDecision::NetworkPolicyAmendment { .. } => MCP_TOOL_APPROVAL_ACCEPT.to_string(), - ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => { + ReviewDecision::Denied { .. } | ReviewDecision::TimedOut | ReviewDecision::Abort => { MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string() } }; @@ -884,6 +885,12 @@ where } } +async fn receive_approval_review(review_rx: oneshot::Receiver) -> ReviewDecision { + review_rx + .await + .unwrap_or_else(|_| ReviewDecision::denied("automatic approval review could not complete")) +} + /// Await an approval decision, aborting on cancellation. async fn await_approval_with_cancel( fut: F, @@ -902,7 +909,10 @@ where review_cancel_token.cancel(); } parent_session - .notify_approval(approval_id, codex_protocol::protocol::ReviewDecision::Abort) + .notify_approval( + approval_id, + codex_protocol::protocol::ReviewDecision::Abort, + ) .await; codex_protocol::protocol::ReviewDecision::Abort } diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 6f67902aa1..98adcfd385 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -34,9 +34,21 @@ use pretty_assertions::assert_eq; use std::collections::HashMap; use std::sync::Arc; use tokio::sync::Mutex; +use tokio::sync::oneshot; use tokio::sync::watch; use tokio::time::timeout; +#[tokio::test] +async fn dropped_approval_review_fails_closed() { + let (tx, rx) = oneshot::channel(); + drop(tx); + + assert_eq!( + receive_approval_review(rx).await, + ReviewDecision::denied("automatic approval review could not complete") + ); +} + #[tokio::test] async fn forward_events_filters_private_events_before_blocked_send_is_cancelled() { let (tx_events, rx_events) = bounded(SUBMISSION_CHANNEL_CAPACITY); diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 5402afeaf9..65faad5a60 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -19,7 +19,6 @@ mod review_session; use std::time::Duration; -use codex_protocol::protocol::GuardianAssessmentDecisionSource; use codex_protocol::protocol::GuardianAssessmentOutcome; use serde::Deserialize; use serde::Serialize; @@ -29,7 +28,6 @@ pub(crate) use approval_request::GuardianMcpAnnotations; pub(crate) use approval_request::GuardianNetworkAccessTrigger; #[cfg(test)] pub(crate) use approval_request::guardian_approval_request_to_json; -pub(crate) use review::guardian_rejection_message; pub(crate) use review::guardian_timeout_message; pub(crate) use review::is_guardian_reviewer_source; pub(crate) use review::new_guardian_review_id; @@ -68,12 +66,6 @@ pub(crate) struct GuardianAssessment { pub(crate) rationale: String, } -#[derive(Debug, Clone, PartialEq, Eq)] -pub(crate) struct GuardianRejection { - pub(crate) rationale: String, - pub(crate) source: GuardianAssessmentDecisionSource, -} - #[derive(Debug, Default)] pub(crate) struct GuardianRejectionCircuitBreaker { turns: std::collections::HashMap, diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index c3b1f62cf7..9db4bac06d 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -35,7 +35,6 @@ use super::GUARDIAN_REVIEWER_NAME; use super::GuardianApprovalRequest; use super::GuardianAssessment; use super::GuardianAssessmentOutcome; -use super::GuardianRejection; use super::GuardianRejectionCircuitBreakerAction; use super::approval_request::guardian_assessment_action; use super::approval_request::guardian_request_target_item_id; @@ -68,27 +67,6 @@ pub(crate) fn new_guardian_review_id() -> String { uuid::Uuid::new_v4().to_string() } -pub(crate) async fn guardian_rejection_message(session: &Session, review_id: &str) -> String { - let rejection = session - .services - .guardian_rejections - .lock() - .await - .remove(review_id) - .filter(|rejection| !rejection.rationale.trim().is_empty()) - .unwrap_or_else(|| GuardianRejection { - rationale: "Auto-reviewer denied the action without a specific rationale.".to_string(), - source: GuardianAssessmentDecisionSource::Agent, - }); - match rejection.source { - GuardianAssessmentDecisionSource::Agent => format!( - "This action was rejected due to unacceptable risk.\nReason: {}\n{}", - rejection.rationale.trim(), - GUARDIAN_REJECTION_INSTRUCTIONS - ), - } -} - pub(crate) fn guardian_timeout_message() -> String { GUARDIAN_TIMEOUT_INSTRUCTIONS.to_string() } @@ -551,18 +529,6 @@ async fn run_guardian_review( } else { GuardianAssessmentStatus::Denied }; - { - let mut rationales = session.services.guardian_rejections.lock().await; - if approved { - rationales.remove(&review_id); - } else { - let rejection = GuardianRejection { - rationale: assessment.rationale.clone(), - source: GuardianAssessmentDecisionSource::Agent, - }; - rationales.insert(review_id.clone(), rejection); - } - } session .send_event( turn.as_ref(), @@ -591,7 +557,14 @@ async fn run_guardian_review( if approved { ReviewDecision::Approved } else { - ReviewDecision::Denied + let rationale = if assessment.rationale.trim().is_empty() { + "Auto-reviewer denied the action without a specific rationale." + } else { + assessment.rationale.trim() + }; + ReviewDecision::denied(format!( + "This action was rejected due to unacceptable risk.\nReason: {rationale}\n{GUARDIAN_REJECTION_INSTRUCTIONS}" + )) } } @@ -648,25 +621,32 @@ pub(crate) fn spawn_approval_request_review( cancel_token: CancellationToken, ) -> oneshot::Receiver { let (tx, rx) = oneshot::channel(); - std::thread::spawn(move || { - let Ok(runtime) = tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - else { - let _ = tx.send(ReviewDecision::Denied); - return; - }; - let decision = runtime.block_on(review_approval_request_with_cancel( - &session, - &turn, - review_id, - request, - retry_reason, - approval_request_source, - cancel_token, - )); - let _ = tx.send(decision); - }); + let spawn_result = std::thread::Builder::new() + .name("codex-approval-review".to_string()) + .spawn(move || { + let Ok(runtime) = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + else { + let _ = tx.send(ReviewDecision::denied( + "automatic approval review could not complete", + )); + return; + }; + let decision = runtime.block_on(review_approval_request_with_cancel( + &session, + &turn, + review_id, + request, + retry_reason, + approval_request_source, + cancel_token, + )); + let _ = tx.send(decision); + }); + if let Err(err) = spawn_result { + tracing::error!(%err, "failed to spawn automatic approval review worker"); + } rx } diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index bc8eaf8b69..c622d68541 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -2337,7 +2337,9 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> ) .await; - assert_eq!(decision, ReviewDecision::Denied); + let ReviewDecision::Denied { rejection } = decision else { + panic!("guardian error should deny the approval"); + }; assert_eq!(request_log.requests().len(), 1); let mut warnings = Vec::new(); @@ -2373,17 +2375,10 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> }), "denial rationale should not fall back to the generic missing payload error" ); - { - let rationales = session.services.guardian_rejections.lock().await; - assert!(rationales.contains_key("review-shell-guardian-error")); - assert!(!rationales.contains_key("shell-guardian-error")); - } - let rejection_message = - guardian_rejection_message(session.as_ref(), "review-shell-guardian-error").await; assert!( - rejection_message.contains("Reason: Automatic approval review failed:") - && rejection_message.contains(error_message), - "rejection message should include guardian rationale: {rejection_message}" + rejection.contains("Reason: Automatic approval review failed:") + && rejection.contains(error_message), + "rejection message should include guardian rationale: {rejection}" ); Ok(()) @@ -2470,7 +2465,7 @@ async fn guardian_review_does_not_retry_missing_assessment_payload() -> anyhow:: ) .await; - assert_eq!(decision, ReviewDecision::Denied); + assert!(matches!(decision, ReviewDecision::Denied { .. })); assert_eq!(request_log.requests().len(), 1); Ok(()) } @@ -2574,7 +2569,7 @@ async fn guardian_review_exhausts_three_failures_with_one_terminal_event() -> an ) .await; - assert_eq!(decision, ReviewDecision::Denied); + assert!(matches!(decision, ReviewDecision::Denied { .. })); assert_eq!(request_log.requests().len(), 3); let mut statuses = Vec::new(); while let Ok(event) = rx.try_recv() { @@ -2625,7 +2620,7 @@ async fn guardian_review_does_not_retry_valid_denial() -> anyhow::Result<()> { ) .await; - assert_eq!(decision, ReviewDecision::Denied); + assert!(matches!(decision, ReviewDecision::Denied { .. })); assert_eq!(request_log.requests().len(), 1); Ok(()) } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 876f114431..692a85a479 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -8,8 +8,6 @@ use crate::config::edit::ConfigEditsBuilder; use crate::connectors; use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianMcpAnnotations; -use crate::guardian::guardian_rejection_message; -use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian_with_reviewer; @@ -1292,7 +1290,7 @@ async fn maybe_request_mcp_tool_approval( /*retry_reason*/ None, ) .await; - let decision = mcp_tool_approval_decision_from_guardian(sess, &review_id, decision).await; + let decision = mcp_tool_approval_decision_from_guardian(decision); apply_mcp_tool_approval_decision( sess, turn_context, @@ -1464,21 +1462,17 @@ pub(crate) fn build_guardian_mcp_tool_review_request( } } -async fn mcp_tool_approval_decision_from_guardian( - sess: &Session, - review_id: &str, - decision: ReviewDecision, -) -> McpToolApprovalDecision { +fn mcp_tool_approval_decision_from_guardian(decision: ReviewDecision) -> McpToolApprovalDecision { match decision { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } | ReviewDecision::NetworkPolicyAmendment { .. } => McpToolApprovalDecision::Accept, ReviewDecision::ApprovedForSession => McpToolApprovalDecision::AcceptForSession, - ReviewDecision::Denied => McpToolApprovalDecision::Decline { - message: Some(guardian_rejection_message(sess, review_id).await), + ReviewDecision::Denied { rejection } => McpToolApprovalDecision::Decline { + message: Some(rejection), }, ReviewDecision::TimedOut => McpToolApprovalDecision::Decline { - message: Some(guardian_timeout_message()), + message: Some(crate::guardian::guardian_timeout_message()), }, ReviewDecision::Abort => McpToolApprovalDecision::Decline { message: None }, } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index c27d51de0b..2699d6057d 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1901,33 +1901,15 @@ fn guardian_mcp_review_request_ignores_untrusted_connected_account_email() { ); } -#[tokio::test(flavor = "current_thread")] -async fn guardian_review_decision_maps_to_mcp_tool_decision() { - let (session, _) = make_session_and_context().await; - let session = Arc::new(session); - +#[test] +fn guardian_review_decision_maps_to_mcp_tool_decision() { assert_eq!( - mcp_tool_approval_decision_from_guardian( - session.as_ref(), - "review-id", - ReviewDecision::Approved - ) - .await, + mcp_tool_approval_decision_from_guardian(ReviewDecision::Approved), McpToolApprovalDecision::Accept ); - session.services.guardian_rejections.lock().await.insert( - "review-id".to_string(), - crate::guardian::GuardianRejection { - rationale: "too risky".to_string(), - source: codex_protocol::protocol::GuardianAssessmentDecisionSource::Agent, - }, - ); - let denial = mcp_tool_approval_decision_from_guardian( - session.as_ref(), - "review-id", - ReviewDecision::Denied, - ) - .await; + let denial = mcp_tool_approval_decision_from_guardian(ReviewDecision::denied( + "This action was rejected due to unacceptable risk.\nReason: too risky\nThe agent must not attempt to achieve the same outcome", + )); let McpToolApprovalDecision::Decline { message: Some(message), } = denial @@ -1936,12 +1918,7 @@ async fn guardian_review_decision_maps_to_mcp_tool_decision() { }; assert!(message.contains("Reason: too risky")); assert!(message.contains("The agent must not attempt to achieve the same outcome")); - let timeout = mcp_tool_approval_decision_from_guardian( - session.as_ref(), - "review-id", - ReviewDecision::TimedOut, - ) - .await; + let timeout = mcp_tool_approval_decision_from_guardian(ReviewDecision::TimedOut); let McpToolApprovalDecision::Decline { message: Some(message), } = timeout @@ -1951,12 +1928,7 @@ async fn guardian_review_decision_maps_to_mcp_tool_decision() { assert!(message.contains("did not finish before its deadline")); assert!(!message.contains("unacceptable risk")); assert_eq!( - mcp_tool_approval_decision_from_guardian( - session.as_ref(), - "review-id", - ReviewDecision::Abort - ) - .await, + mcp_tool_approval_decision_from_guardian(ReviewDecision::Abort), McpToolApprovalDecision::Decline { message: None } ); } diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index f4bbc7d650..157391c390 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -735,10 +735,9 @@ async fn review_guardian_mcp_elicitation( /*retry_reason*/ None, ) .await; - Ok(Some( - mcp_elicitation_response_from_guardian_decision(session.as_ref(), &review_id, decision) - .await, - )) + Ok(Some(mcp_elicitation_response_from_guardian_decision( + decision, + ))) } fn guardian_elicitation_review_request( @@ -882,23 +881,8 @@ fn mcp_elicitation_request_id(id: &RequestId) -> String { } } -async fn mcp_elicitation_response_from_guardian_decision( - session: &Session, - review_id: &str, +fn mcp_elicitation_response_from_guardian_decision( decision: ReviewDecision, -) -> ElicitationResponse { - let denial_message = match decision { - ReviewDecision::Denied => { - Some(crate::guardian::guardian_rejection_message(session, review_id).await) - } - _ => None, - }; - mcp_elicitation_response_from_guardian_decision_parts(decision, denial_message) -} - -fn mcp_elicitation_response_from_guardian_decision_parts( - decision: ReviewDecision, - denial_message: Option, ) -> ElicitationResponse { match decision { ReviewDecision::Approved @@ -909,9 +893,7 @@ fn mcp_elicitation_response_from_guardian_decision_parts( content: Some(serde_json::json!({})), meta: Some(mcp_elicitation_auto_meta()), }, - ReviewDecision::Denied => mcp_elicitation_decline_with_message( - denial_message.unwrap_or_else(|| "Guardian denied this request.".to_string()), - ), + ReviewDecision::Denied { rejection } => mcp_elicitation_decline_with_message(rejection), ReviewDecision::TimedOut => { mcp_elicitation_decline_with_message(crate::guardian::guardian_timeout_message()) } diff --git a/codex-rs/core/src/session/mcp_tests.rs b/codex-rs/core/src/session/mcp_tests.rs index 5e85b92f47..1dd941d7ef 100644 --- a/codex-rs/core/src/session/mcp_tests.rs +++ b/codex-rs/core/src/session/mcp_tests.rs @@ -221,10 +221,7 @@ fn guardian_elicitation_review_request_declines_unsupported_opt_in_shapes() { #[test] fn guardian_decisions_map_to_elicitation_responses_without_session_state() { assert_eq!( - mcp_elicitation_response_from_guardian_decision_parts( - ReviewDecision::Approved, - /*denial_message*/ None, - ), + mcp_elicitation_response_from_guardian_decision(ReviewDecision::Approved), ElicitationResponse { action: ElicitationAction::Accept, content: Some(json!({})), @@ -234,10 +231,9 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() { } ); assert_eq!( - mcp_elicitation_response_from_guardian_decision_parts( - ReviewDecision::Denied, - Some("Denied by Guardian".to_string()), - ), + mcp_elicitation_response_from_guardian_decision(ReviewDecision::denied( + "Denied by Guardian", + )), ElicitationResponse { action: ElicitationAction::Decline, content: None, @@ -248,10 +244,7 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() { } ); assert_eq!( - mcp_elicitation_response_from_guardian_decision_parts( - ReviewDecision::TimedOut, - /*denial_message*/ None, - ), + mcp_elicitation_response_from_guardian_decision(ReviewDecision::TimedOut), ElicitationResponse { action: ElicitationAction::Decline, content: None, @@ -262,10 +255,7 @@ fn guardian_decisions_map_to_elicitation_responses_without_session_state() { } ); assert_eq!( - mcp_elicitation_response_from_guardian_decision_parts( - ReviewDecision::Abort, - /*denial_message*/ None, - ), + mcp_elicitation_response_from_guardian_decision(ReviewDecision::Abort), ElicitationResponse { action: ElicitationAction::Cancel, content: None, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index a3c714207a..2f6dcf520a 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2371,7 +2371,9 @@ impl Session { let decision = tokio::select! { biased; _ = cancellation_token.cancelled() => return None, - decision = review_rx => decision.unwrap_or(ReviewDecision::Denied), + decision = review_rx => decision.unwrap_or_else(|_| { + ReviewDecision::denied("automatic approval review could not complete") + }), }; let response = match decision { ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { @@ -2400,13 +2402,13 @@ impl Session { strict_auto_review: false, }, }, - ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => { - RequestPermissionsResponse { - permissions: RequestPermissionProfile::default(), - scope: PermissionGrantScope::Turn, - strict_auto_review: false, - } - } + ReviewDecision::Abort + | ReviewDecision::Denied { .. } + | ReviewDecision::TimedOut => RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }, }; let response = Self::normalize_request_permissions_response( requested_permissions, diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 04aaba64b6..2c81b59720 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1080,7 +1080,6 @@ impl Session { session_telemetry, models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - guardian_rejections: Mutex::new(HashMap::new()), guardian_rejection_circuit_breaker: Mutex::new(Default::default()), runtime_handle: tokio::runtime::Handle::current(), skills_service, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 6d1c20f71b..19fbf0cc18 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -5339,7 +5339,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { session_telemetry: session_telemetry.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - guardian_rejections: Mutex::new(std::collections::HashMap::new()), guardian_rejection_circuit_breaker: Mutex::new(Default::default()), runtime_handle: tokio::runtime::Handle::current(), skills_service, @@ -7497,7 +7496,6 @@ where session_telemetry: session_telemetry.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - guardian_rejections: Mutex::new(std::collections::HashMap::new()), guardian_rejection_circuit_breaker: Mutex::new(Default::default()), runtime_handle: tokio::runtime::Handle::current(), skills_service, diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 33c0a1b087..53043a1d42 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use std::sync::Arc; use std::sync::atomic::AtomicBool; @@ -13,7 +12,6 @@ use crate::current_time::TimeProvider; use crate::elicitation::ElicitationService; use crate::environment_selection::ThreadEnvironments; use crate::exec_policy::ExecPolicyManager; -use crate::guardian::GuardianRejection; use crate::guardian::GuardianRejectionCircuitBreaker; use crate::mcp::McpManager; use crate::session::McpRuntimeSnapshot; @@ -72,7 +70,6 @@ pub(crate) struct SessionServices { pub(crate) models_manager: SharedModelsManager, pub(crate) session_telemetry: SessionTelemetry, pub(crate) tool_approvals: Mutex, - pub(crate) guardian_rejections: Mutex>, pub(crate) guardian_rejection_circuit_breaker: Mutex, pub(crate) runtime_handle: Handle, pub(crate) skills_service: Arc, diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 1c6fd1037d..6b4bedd00d 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -1,15 +1,11 @@ //! Central approval policy-stage execution and reviewer routing. -use std::sync::Arc; - -use crate::guardian::guardian_rejection_message; use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian_with_reviewer; use crate::hook_runtime::run_permission_request_hooks; use crate::sandboxing::SandboxPermissions; -use crate::session::session::Session; use crate::session::turn_context::TurnContext; use crate::tools::flat_tool_name; use crate::tools::sandboxing::ApprovalCtx; @@ -163,16 +159,31 @@ enum ApprovalResolutionSource { #[derive(Clone, Debug, PartialEq, Eq)] struct ApprovalResolution { decision: ReviewDecision, - rejection: Option, source: ApprovalResolutionSource, } impl ApprovalResolution { fn into_tool_result(self) -> Result { - if let Some(rejection) = self.rejection { - Err(ToolError::Rejected(rejection)) - } else { - Ok(self.decision) + let source = self.source; + match self.decision { + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + } if network_policy_amendment.action == NetworkPolicyRuleAction::Deny => { + let rejection = match source { + ApprovalResolutionSource::Hook => "rejected by configuration", + ApprovalResolutionSource::Guardian => { + "automatic approval review denied the action" + } + ApprovalResolutionSource::User => "rejected by user", + }; + Err(ToolError::Rejected(rejection.to_string())) + } + ReviewDecision::Denied { rejection } => Err(ToolError::Rejected(rejection)), + ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())), + ReviewDecision::Abort => { + Err(ToolError::Rejected("approval request aborted".to_string())) + } + decision => Ok(decision), } } } @@ -201,7 +212,6 @@ where Some(PermissionRequestDecision::Allow) => { let resolution = ApprovalResolution { decision: ReviewDecision::Approved, - rejection: None, source: ApprovalResolutionSource::Hook, }; record_resolution(otel, tool_ctx, &resolution); @@ -209,8 +219,7 @@ where } Some(PermissionRequestDecision::Deny { message }) => { let resolution = ApprovalResolution { - decision: ReviewDecision::Denied, - rejection: Some(message), + decision: ReviewDecision::denied(message), source: ApprovalResolutionSource::Hook, }; record_resolution(otel, tool_ctx, &resolution); @@ -231,9 +240,8 @@ where Err(err) => { tracing::error!(%err, "failed to build automatic approval action"); let resolution = ApprovalResolution { - decision: ReviewDecision::Abort, - rejection: Some( - "automatic approval review could not prepare the action".to_string(), + decision: ReviewDecision::denied( + "automatic approval review could not prepare the action", ), source: ApprovalResolutionSource::Guardian, }; @@ -241,71 +249,29 @@ where return resolution.into_tool_result(); } }; - let decision = review_approval_request( + review_approval_request( ctx.session, ctx.turn, - review_id.clone(), + review_id, action, ctx.retry_reason.clone(), ) - .await; - normalize_guardian(ctx.session, review_id, decision).await + .await } - ApprovalReviewer::User => ApprovalResolution { - decision: tool.start_approval_async(req, ctx.clone()).await, - rejection: None, - source: ApprovalResolutionSource::User, - }, + ApprovalReviewer::User => tool.start_approval_async(req, ctx.clone()).await, + }; + let source = match reviewer { + ApprovalReviewer::Guardian => ApprovalResolutionSource::Guardian, + ApprovalReviewer::User => ApprovalResolutionSource::User, + }; + let resolution = ApprovalResolution { + decision: resolution, + source, }; - let resolution = normalize_user_rejection(resolution); record_resolution(otel, tool_ctx, &resolution); resolution.into_tool_result() } -async fn normalize_guardian( - session: &Arc, - review_id: String, - decision: ReviewDecision, -) -> ApprovalResolution { - let rejection = match &decision { - ReviewDecision::Approved - | ReviewDecision::ApprovedForSession - | ReviewDecision::ApprovedExecpolicyAmendment { .. } => None, - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } if network_policy_amendment.action == NetworkPolicyRuleAction::Allow => None, - ReviewDecision::TimedOut => Some(guardian_timeout_message()), - ReviewDecision::NetworkPolicyAmendment { .. } - | ReviewDecision::Denied - | ReviewDecision::Abort => { - Some(guardian_rejection_message(session.as_ref(), &review_id).await) - } - }; - ApprovalResolution { - decision, - rejection, - source: ApprovalResolutionSource::Guardian, - } -} - -fn normalize_user_rejection(mut resolution: ApprovalResolution) -> ApprovalResolution { - if resolution.source == ApprovalResolutionSource::User { - resolution.rejection = match &resolution.decision { - ReviewDecision::Approved - | ReviewDecision::ApprovedForSession - | ReviewDecision::ApprovedExecpolicyAmendment { .. } => None, - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } if network_policy_amendment.action == NetworkPolicyRuleAction::Allow => None, - ReviewDecision::NetworkPolicyAmendment { .. } - | ReviewDecision::Denied - | ReviewDecision::Abort => Some("rejected by user".to_string()), - ReviewDecision::TimedOut => Some("approval request timed out".to_string()), - }; - } - resolution -} - fn record_resolution( otel: &codex_otel::SessionTelemetry, tool_ctx: &ToolCtx, diff --git a/codex-rs/core/src/tools/approvals_tests.rs b/codex-rs/core/src/tools/approvals_tests.rs index 9cf066e093..4ab5daadc9 100644 --- a/codex-rs/core/src/tools/approvals_tests.rs +++ b/codex-rs/core/src/tools/approvals_tests.rs @@ -1,6 +1,24 @@ use super::*; +use codex_protocol::approvals::NetworkPolicyAmendment; use pretty_assertions::assert_eq; +#[test] +fn approval_resolution_rejects_denied_network_policy_amendment() { + let resolution = ApprovalResolution { + decision: ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment: NetworkPolicyAmendment { + host: "denied.example.com".to_string(), + action: NetworkPolicyRuleAction::Deny, + }, + }, + source: ApprovalResolutionSource::User, + }; + assert!(matches!( + resolution.into_tool_result(), + Err(ToolError::Rejected(rejection)) if rejection == "rejected by user" + )); +} + #[test] fn guardian_cwd_preserves_drive_shaped_local_posix_path() { let native_cwd = AbsolutePathBuf::try_from(std::path::PathBuf::from("/C:/workspace")) diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index 628e6ce9bc..d67ee115e1 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -21,12 +21,19 @@ use codex_protocol::protocol::TurnDiffEvent; use codex_shell_command::parse_command::parse_command; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use codex_utils_string::truncate_middle_with_token_budget; use std::collections::HashMap; use std::path::PathBuf; use std::time::Duration; use super::format_exec_output_str; +const REJECTION_MESSAGE_MAX_TOKENS: usize = 900; + +pub(super) fn truncate_rejection_message(message: &str) -> String { + truncate_middle_with_token_budget(message, REJECTION_MESSAGE_MAX_TOKENS).0 +} + #[derive(Clone, Copy)] pub(crate) struct ToolEventCtx<'a> { pub session: &'a Session, @@ -422,6 +429,7 @@ impl ToolEmitter { } else { msg }; + let normalized = truncate_rejection_message(&normalized); let event = ToolEventStage::Failure(ToolEventFailure::Rejected { message: normalized.clone(), applied_patch_delta, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index a6eb1d55a7..05ad9f9675 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -1,13 +1,12 @@ use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianNetworkAccessTrigger; -use crate::guardian::guardian_rejection_message; -use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; use crate::hook_runtime::run_permission_request_hooks; use crate::network_policy_decision::denied_network_policy_message; use crate::session::session::Session; +use crate::tools::events::truncate_rejection_message; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::ToolError; use codex_hooks::PermissionRequestDecision; @@ -169,7 +168,7 @@ enum PendingApprovalDecision { #[derive(Clone, Debug, Eq, PartialEq)] enum NetworkApprovalOutcome { - DeniedByUser, + DeniedByApproval(String), DeniedByPolicy(String), } @@ -177,10 +176,12 @@ fn network_approval_outcome_to_result( outcome: Option, ) -> Result<(), ToolError> { match outcome { - Some(NetworkApprovalOutcome::DeniedByUser) => { - Err(ToolError::Rejected("rejected by user".to_string())) + Some(NetworkApprovalOutcome::DeniedByApproval(rejection)) => { + Err(ToolError::Rejected(truncate_rejection_message(&rejection))) + } + Some(NetworkApprovalOutcome::DeniedByPolicy(message)) => { + Err(ToolError::Rejected(truncate_rejection_message(&message))) } - Some(NetworkApprovalOutcome::DeniedByPolicy(message)) => Err(ToolError::Rejected(message)), None => Ok(()), } } @@ -427,7 +428,7 @@ impl NetworkApprovalService { }; if matches!( calls.call_outcomes.get(registration_id), - Some(NetworkApprovalOutcome::DeniedByUser) + Some(NetworkApprovalOutcome::DeniedByApproval(_)) ) { return; } @@ -760,7 +761,9 @@ impl NetworkApprovalService { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByUser, + NetworkApprovalOutcome::DeniedByApproval( + "rejected by user".to_string(), + ), ) .await; } @@ -768,22 +771,15 @@ impl NetworkApprovalService { PendingApprovalDecision::Deny } }, - ReviewDecision::Denied | ReviewDecision::Abort => { - if let Some(review_id) = guardian_review_id.as_deref() { - if let Some(owner_call) = owner_call.as_ref() { - let message = guardian_rejection_message(session.as_ref(), review_id).await; - self.record_call_outcome( - &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy(message), - ) + ReviewDecision::Denied { rejection } => { + if let Some(owner_call) = owner_call.as_ref() { + let outcome = if use_guardian { + NetworkApprovalOutcome::DeniedByPolicy(rejection) + } else { + NetworkApprovalOutcome::DeniedByApproval(rejection) + }; + self.record_call_outcome(&owner_call.registration_id, outcome) .await; - } - } else if let Some(owner_call) = owner_call.as_ref() { - self.record_call_outcome( - &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByUser, - ) - .await; } PendingApprovalDecision::Deny } @@ -791,7 +787,29 @@ impl NetworkApprovalService { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, - NetworkApprovalOutcome::DeniedByPolicy(guardian_timeout_message()), + NetworkApprovalOutcome::DeniedByPolicy( + crate::guardian::guardian_timeout_message(), + ), + ) + .await; + } + PendingApprovalDecision::Deny + } + ReviewDecision::Abort => { + if use_guardian { + if let Some(owner_call) = owner_call.as_ref() { + self.record_call_outcome( + &owner_call.registration_id, + NetworkApprovalOutcome::DeniedByPolicy( + "automatic approval review was cancelled".to_string(), + ), + ) + .await; + } + } else if let Some(owner_call) = owner_call.as_ref() { + self.record_call_outcome( + &owner_call.registration_id, + NetworkApprovalOutcome::DeniedByApproval("rejected by user".to_string()), ) .await; } diff --git a/codex-rs/core/src/tools/network_approval_tests.rs b/codex-rs/core/src/tools/network_approval_tests.rs index 38f1cbb9b1..51fa3626b7 100644 --- a/codex-rs/core/src/tools/network_approval_tests.rs +++ b/codex-rs/core/src/tools/network_approval_tests.rs @@ -390,29 +390,29 @@ async fn record_blocked_request_sets_policy_outcome_for_owner_call() { async fn blocked_request_does_not_override_recorded_approval_outcome() { let service = NetworkApprovalService::default(); register_call_with_default_shell_trigger(&service, "registration-1").await; + let rejection = "approval client unavailable"; service .record_call_outcome( "registration-1", - NetworkApprovalOutcome::DeniedByPolicy("custom approval rejection".to_string()), + NetworkApprovalOutcome::DeniedByApproval(rejection.to_string()), ) .await; service .record_blocked_request(denied_blocked_request("example.com")) .await; - assert_eq!( - service.take_call_outcome("registration-1").await, - Some(NetworkApprovalOutcome::DeniedByPolicy( - "custom approval rejection".to_string() - )) - ); + let error = + network_approval_outcome_to_result(service.take_call_outcome("registration-1").await) + .expect_err("approval denial should remain an error"); + assert!(matches!(error, ToolError::Rejected(message) if message == rejection)); } #[tokio::test] async fn specific_approval_outcome_replaces_earlier_blocked_request() { let service = NetworkApprovalService::default(); register_call_with_default_shell_trigger(&service, "registration-1").await; + let rejection = "specific approval rejection"; service .record_blocked_request(denied_blocked_request("example.com")) @@ -420,16 +420,30 @@ async fn specific_approval_outcome_replaces_earlier_blocked_request() { service .record_call_outcome( "registration-1", - NetworkApprovalOutcome::DeniedByPolicy("specific approval rejection".to_string()), + NetworkApprovalOutcome::DeniedByApproval(rejection.to_string()), ) .await; - assert_eq!( - service.take_call_outcome("registration-1").await, - Some(NetworkApprovalOutcome::DeniedByPolicy( - "specific approval rejection".to_string() - )) - ); + let error = + network_approval_outcome_to_result(service.take_call_outcome("registration-1").await) + .expect_err("specific approval denial should replace blocked policy denial"); + assert!(matches!(error, ToolError::Rejected(message) if message == rejection)); +} + +#[test] +fn approval_denial_messages_are_bounded_for_model_context() { + let rejection = "x".repeat(40_000); + + let error = network_approval_outcome_to_result(Some(NetworkApprovalOutcome::DeniedByApproval( + rejection, + ))) + .expect_err("approval denial should remain an error"); + let ToolError::Rejected(message) = error else { + panic!("approval denial should produce a rejected tool error"); + }; + + assert!(codex_utils_string::approx_token_count(&message) < 1_000); + assert!(message.contains("tokens truncated")); } #[tokio::test] diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 42ffd3e9aa..5a8e0d3c8c 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -4,8 +4,6 @@ use crate::exec::ExecExpiration; use crate::exec::cancel_when_either; use crate::exec::is_likely_sandbox_denied; use crate::guardian::GuardianApprovalRequest; -use crate::guardian::guardian_rejection_message; -use crate::guardian::guardian_timeout_message; use crate::guardian::new_guardian_review_id; use crate::guardian::review_approval_request; use crate::guardian::routes_approval_to_guardian; @@ -375,12 +373,6 @@ enum DecisionSource { UnmatchedCommandFallback, } -struct PromptDecision { - decision: ReviewDecision, - guardian_review_id: Option, - rejection_message: Option, -} - fn execve_prompt_is_rejected_by_policy( approval_policy: AskForApproval, decision_source: &DecisionSource, @@ -447,7 +439,7 @@ impl CoreShellActionProvider { workdir: &AbsolutePathBuf, stopwatch: &Stopwatch, additional_permissions: Option, - ) -> anyhow::Result { + ) -> anyhow::Result { let command = join_program_and_argv(program, argv); let workdir = workdir.clone(); let session = self.session.clone(); @@ -474,28 +466,20 @@ impl CoreShellActionProvider { .await { Some(PermissionRequestDecision::Allow) => { - return PromptDecision { - decision: ReviewDecision::Approved, - guardian_review_id: None, - rejection_message: None, - }; + return ReviewDecision::Approved; } Some(PermissionRequestDecision::Deny { message }) => { - return PromptDecision { - decision: ReviewDecision::Denied, - guardian_review_id: None, - rejection_message: Some(message), - }; + return ReviewDecision::denied(message); } None => {} } // 2) Route to Guardian if configured - if let Some(review_id) = guardian_review_id.clone() { - let decision = review_approval_request( + if let Some(review_id) = guardian_review_id { + return review_approval_request( &session, &turn, - review_id.clone(), + review_id, GuardianApprovalRequest::Execve { id: call_id.clone(), source, @@ -507,15 +491,10 @@ impl CoreShellActionProvider { /*retry_reason*/ None, ) .await; - return PromptDecision { - decision, - guardian_review_id, - rejection_message: None, - }; } // 3) Fall back to regular user prompt - let decision = session + session .request_command_approval( &turn, call_id, @@ -529,12 +508,7 @@ impl CoreShellActionProvider { additional_permissions, Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), ) - .await; - PromptDecision { - decision, - guardian_review_id: None, - rejection_message: None, - } + .await }) .await) } @@ -561,10 +535,10 @@ impl CoreShellActionProvider { { EscalationDecision::deny(Some("Execution forbidden by policy".to_string())) } else { - let prompt_decision = self + let decision = self .prompt(program, argv, workdir, &self.stopwatch, prompt_permissions) .await?; - match prompt_decision.decision { + match decision { ReviewDecision::Approved | ReviewDecision::ApprovedForSession | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { @@ -588,23 +562,12 @@ impl CoreShellActionProvider { EscalationDecision::deny(Some("User denied execution".to_string())) } }, - ReviewDecision::Denied => { - let message = if let Some(message) = - prompt_decision.rejection_message.clone() - { - message - } else if let Some(review_id) = - prompt_decision.guardian_review_id.as_deref() - { - guardian_rejection_message(self.session.as_ref(), review_id).await - } else { - "User denied execution".to_string() - }; - EscalationDecision::deny(Some(message)) - } - ReviewDecision::TimedOut => { - EscalationDecision::deny(Some(guardian_timeout_message())) + ReviewDecision::Denied { rejection } => { + EscalationDecision::deny(Some(rejection)) } + ReviewDecision::TimedOut => EscalationDecision::deny(Some( + crate::guardian::guardian_timeout_message(), + )), ReviewDecision::Abort => { EscalationDecision::deny(Some("User cancelled execution".to_string())) } diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 571acd086a..d6cd44c8c8 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1005,11 +1005,11 @@ fn scenarios() -> Vec { features: vec![], model_override: Some("gpt-5.2"), outcome: Outcome::ExecApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("blocked by distinctive approval policy"), expected_reason: None, }, expectation: Expectation::CommandFailure { - output_contains: "rejected by user", + output_contains: "blocked by distinctive approval policy", }, }, ScenarioSpec { @@ -1023,7 +1023,7 @@ fn scenarios() -> Vec { features: vec![], model_override: Some("gpt-5.2"), outcome: Outcome::ExecApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, }, expectation: Expectation::CommandFailure { @@ -1041,7 +1041,7 @@ fn scenarios() -> Vec { features: vec![], model_override: Some("gpt-5.2"), outcome: Outcome::ExecApprovalWithAmendment { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, expected_execpolicy_amendment: Some(&["echo", "known-safe-escalation"]), }, @@ -1084,7 +1084,7 @@ fn scenarios() -> Vec { features: vec![], model_override: Some("gpt-5.2"), outcome: Outcome::ExecApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, }, expectation: Expectation::CommandFailure { @@ -1105,7 +1105,7 @@ fn scenarios() -> Vec { features: vec![], model_override: Some("gpt-5.2"), outcome: Outcome::ExecApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, }, expectation: Expectation::CommandFailure { @@ -1126,7 +1126,7 @@ fn scenarios() -> Vec { features: vec![], model_override: Some("gpt-5.2"), outcome: Outcome::ExecApprovalWithAmendment { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, expected_execpolicy_amendment: None, }, @@ -1304,7 +1304,7 @@ fn scenarios() -> Vec { features: vec![], model_override: None, outcome: Outcome::ExecApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, }, expectation: Expectation::FileNotCreated { @@ -1436,7 +1436,7 @@ fn scenarios() -> Vec { features: vec![], model_override: Some("gpt-5.4"), outcome: Outcome::PatchApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, }, expectation: Expectation::FileNotCreated { @@ -1742,7 +1742,7 @@ fn scenarios() -> Vec { features: vec![Feature::UnifiedExec], model_override: None, outcome: Outcome::ExecApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, }, expectation: Expectation::CommandFailure { @@ -1761,7 +1761,7 @@ fn scenarios() -> Vec { features: vec![Feature::UnifiedExec], model_override: None, outcome: Outcome::ExecApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, }, expectation: Expectation::CommandFailure { @@ -1780,7 +1780,7 @@ fn scenarios() -> Vec { features: vec![Feature::UnifiedExec], model_override: None, outcome: Outcome::ExecApproval { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), expected_reason: None, }, expectation: Expectation::CommandFailure { @@ -3865,7 +3865,7 @@ allow_local_binding = true .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), }) .await?; wait_for_completion(&test).await; @@ -3937,7 +3937,7 @@ async fn compound_command_with_one_safe_command_still_requires_approval() -> Res .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), }) .await?; wait_for_completion(&test).await; diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index f5db9856df..98d9548e0e 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -184,7 +184,7 @@ async fn codex_delegate_forwards_patch_approval_and_proceeds_on_decision() { test.codex .submit(Op::PatchApproval { id: approval.call_id, - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), }) .await .expect("submit patch approval"); diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index d2783b7a02..83dda23eef 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -272,7 +272,7 @@ async fn granular_complex_forced_rm_requests_approval_when_allowed() -> Result<( .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), }) .await?; wait_for_event(&test.codex, |event| { diff --git a/codex-rs/core/tests/suite/network_approval.rs b/codex-rs/core/tests/suite/network_approval.rs index 56656d7199..03aebffb13 100644 --- a/codex-rs/core/tests/suite/network_approval.rs +++ b/codex-rs/core/tests/suite/network_approval.rs @@ -519,7 +519,7 @@ async fn user_network_approval_once_session_and_denial_semantics() -> Result<()> .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: Some(approval.turn_id), - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), }) .await?; wait_for_turn_complete(&test).await; @@ -1096,7 +1096,7 @@ async fn approved_network_host_for_one_environment_still_prompts_in_another() -> .await?; wait_for_turn_complete(&test).await; - mount_exec_network_turn( + let remote_responses = mount_exec_network_turn( &server, "resp-network-remote", "exec-network-remote", @@ -1112,14 +1112,19 @@ async fn approved_network_host_for_one_environment_still_prompts_in_another() -> ) .await?; let approval = expect_network_approval(&test, REMOTE_ENVIRONMENT_ID).await?; + let rejection = "approval request failed because the client disconnected"; test.codex .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied(rejection), }) .await?; wait_for_turn_complete(&test).await; + assert_eq!( + remote_responses.function_call_output_text("exec-network-remote"), + Some(rejection.to_string()) + ); test.fs() .remove( diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index 3d65de61d0..1051d32805 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -1565,7 +1565,7 @@ async fn handle_shell_command_user_denies_records_tool_decision() { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), }) .await .unwrap(); @@ -1706,7 +1706,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index bc0da01b96..af2f32d864 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -1017,7 +1017,7 @@ async fn with_additional_permissions_denied_approval_blocks_execution() -> Resul .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("rejected by user"), }) .await?; wait_for_completion(&test).await; diff --git a/codex-rs/ext/extension-api/tests/registry.rs b/codex-rs/ext/extension-api/tests/registry.rs index dac259bf60..de9cdf1539 100644 --- a/codex-rs/ext/extension-api/tests/registry.rs +++ b/codex-rs/ext/extension-api/tests/registry.rs @@ -314,7 +314,10 @@ async fn approval_review_returns_first_claim_and_short_circuits() { for (name, decision) in [ ("first", None), ("second", Some(ReviewDecision::Approved)), - ("third", Some(ReviewDecision::Denied)), + ( + "third", + Some(ReviewDecision::denied("rejected by extension")), + ), ] { builder.approval_review_contributor(Arc::new(RecordingApprovalContributor { name, diff --git a/codex-rs/mcp-server/src/exec_approval.rs b/codex-rs/mcp-server/src/exec_approval.rs index 3b5ef87cdb..c1427a0578 100644 --- a/codex-rs/mcp-server/src/exec_approval.rs +++ b/codex-rs/mcp-server/src/exec_approval.rs @@ -130,7 +130,7 @@ async fn on_exec_approval_response( // If we cannot deserialize the response, we deny the request to be // conservative. ExecApprovalResponse { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("approval request failed"), } }); diff --git a/codex-rs/mcp-server/src/patch_approval.rs b/codex-rs/mcp-server/src/patch_approval.rs index e392694d3a..56eca276b3 100644 --- a/codex-rs/mcp-server/src/patch_approval.rs +++ b/codex-rs/mcp-server/src/patch_approval.rs @@ -113,7 +113,7 @@ pub(crate) async fn on_patch_approval_response( if let Err(submit_err) = codex .submit(Op::PatchApproval { id: approval_id.clone(), - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("approval request failed"), }) .await { @@ -126,7 +126,7 @@ pub(crate) async fn on_patch_approval_response( let response = serde_json::from_value::(value).unwrap_or_else(|err| { error!("failed to deserialize PatchApprovalResponse: {err}"); PatchApprovalResponse { - decision: ReviewDecision::Denied, + decision: ReviewDecision::denied("approval request failed"), } }); diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 292326a4e0..0c16bf66ee 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -4101,7 +4101,7 @@ pub struct ThreadGoalUpdatedEvent { } /// User's decision in response to an ExecApprovalRequest. -#[derive(Debug, Default, Clone, Deserialize, Serialize, PartialEq, Eq, Display, JsonSchema, TS)] +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, Display, JsonSchema, TS)] #[serde(rename_all = "snake_case")] pub enum ReviewDecision { /// User has approved this command and the agent should execute it. @@ -4126,8 +4126,7 @@ pub enum ReviewDecision { /// User has denied this command and the agent should not execute it, but /// it should continue the session and try something else. - #[default] - Denied, + Denied { rejection: String }, /// Automatic approval review timed out before reaching a decision. TimedOut, @@ -4137,7 +4136,21 @@ pub enum ReviewDecision { Abort, } +impl Default for ReviewDecision { + fn default() -> Self { + Self::Denied { + rejection: "denied".to_string(), + } + } +} + impl ReviewDecision { + pub fn denied(rejection: impl Into) -> Self { + Self::Denied { + rejection: rejection.into(), + } + } + /// Returns an opaque version of the decision without PII. We can't use an ignored flag /// on `serde` because the serialization is required by some surfaces. pub fn to_opaque_string(&self) -> &'static str { @@ -4151,7 +4164,7 @@ impl ReviewDecision { NetworkPolicyRuleAction::Allow => "approved_with_network_policy_allow", NetworkPolicyRuleAction::Deny => "denied_with_network_policy_deny", }, - ReviewDecision::Denied => "denied", + ReviewDecision::Denied { .. } => "denied", ReviewDecision::TimedOut => "timed_out", ReviewDecision::Abort => "abort", } @@ -4470,6 +4483,18 @@ mod tests { use tempfile::NamedTempFile; use tempfile::TempDir; + #[test] + fn review_decision_denied_round_trip() -> Result<()> { + let decision = ReviewDecision::Denied { + rejection: "denied reason".to_string(), + }; + let value = json!({"denied": {"rejection": "denied reason"}}); + + assert_eq!(serde_json::to_value(&decision)?, value); + assert_eq!(serde_json::from_value::(value)?, decision); + Ok(()) + } + #[test] fn feature_thread_source_serializes_as_its_app_owned_label() -> Result<()> { let source = ThreadSource::Feature("automation".to_string());