openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-18 17:18:52 +00:00
Code Issues Packages Projects Releases Wiki Activity

Propagate approval rejection reasons (#34400)

Browse Source 
## What changed

- Change `ReviewDecision::Denied` to carry a rejection string and update the generated protocol schemas.
- Preserve specific rejection reasons through command, patch, network, MCP, delegated, and automatic approval flows so tool results can return them to the model.
- Distinguish invalid approval responses from user declines and truncate oversized rejection messages before adding them to model context.

## Testing

- Add coverage for denial serialization, invalid app-server approval responses, dropped automatic reviews, network rejection propagation, and rejection-message truncation.

GitOrigin-RevId: d48a55643aa03c7ecfa08a8e4b669913eec5464a
This commit is contained in:
Dylan Hurd
2026-07-20 19:23:21 +00:00
committed by copyberry
parent 8c41ed33ce
commit e52c35b000
36 changed files with 499 additions and 452 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json
Generated
+17 -2
View File
@@ -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.",
codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json
Generated
+17 -2
View File
@@ -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.",
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json
Generated
+17 -2
View File
@@ -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.",
codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts
Generated
+1 -1
View File
@@ -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";
codex-rs/app-server-protocol/src/protocol/v2/item.rs
+1 -1
View File
@@ -94,7 +94,7 @@ impl From<CoreReviewDecision> 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,
}
}
codex-rs/app-server/src/bespoke_event_handling.rs
+61 -59
View File
@@ -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::<FileChangeRequestApprovalResponse>(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::<FileChangeRequestApprovalResponse>(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::<CommandExecutionRequestApprovalResponse>(value)
.unwrap_or_else(|err| {
error!("failed to deserialize CommandExecutionRequestApprovalResponse: {err}");
CommandExecutionRequestApprovalResponse {
decision: CommandExecutionApprovalDecision::Decline,
match serde_json::from_value::<CommandExecutionRequestApprovalResponse>(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),
)
}
};
codex-rs/app-server/tests/suite/v2/turn_start.rs
+72 -15
View File
@@ -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"
codex-rs/core/src/codex_delegate.rs
+15 -5
View File
@@ -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>) -> 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<F>(
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
}
codex-rs/core/src/codex_delegate_tests.rs
+12
View File
@@ -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);
codex-rs/core/src/guardian/mod.rs
-8
View File
@@ -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<String, GuardianRejectionCircuitBreakerTurn>,
codex-rs/core/src/guardian/review.rs
+34 -54
View File
@@ -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<ReviewDecision> {
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
}
codex-rs/core/src/guardian/tests.rs
+9 -14
View File
@@ -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(())
}
codex-rs/core/src/mcp_tool_call.rs
+5 -11
View File
@@ -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 },
}
codex-rs/core/src/mcp_tool_call_tests.rs
+8 -36
View File
@@ -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 }
);
}
codex-rs/core/src/session/mcp.rs
+5 -23
View File
@@ -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<String>,
) -> 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())
}
codex-rs/core/src/session/mcp_tests.rs
+6 -16
View File
@@ -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,
codex-rs/core/src/session/mod.rs
+10 -8
View File
@@ -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,
codex-rs/core/src/session/session.rs
-1
View File
@@ -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,
codex-rs/core/src/session/tests.rs
-2
View File
@@ -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,
codex-rs/core/src/state/service.rs
-3
View File
@@ -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<ApprovalStore>,
pub(crate) guardian_rejections: Mutex<HashMap<String, GuardianRejection>>,
pub(crate) guardian_rejection_circuit_breaker: Mutex<GuardianRejectionCircuitBreaker>,
pub(crate) runtime_handle: Handle,
pub(crate) skills_service: Arc<SkillsService>,
codex-rs/core/src/tools/approvals.rs
+35 -69
View File
@@ -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<String>,
source: ApprovalResolutionSource,
}
impl ApprovalResolution {
fn into_tool_result(self) -> Result<ReviewDecision, ToolError> {
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<Session>,
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,
codex-rs/core/src/tools/approvals_tests.rs
+18
View File
@@ -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"))
codex-rs/core/src/tools/events.rs
+8
View File
@@ -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,
codex-rs/core/src/tools/network_approval.rs
+42 -24
View File
@@ -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<NetworkApprovalOutcome>,
) -> 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;
}
codex-rs/core/src/tools/network_approval_tests.rs
+28 -14
View File
@@ -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]
codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
+15 -52
View File
@@ -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<String>,
rejection_message: Option<String>,
}
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<AdditionalPermissionProfile>,
) -> anyhow::Result<PromptDecision> {
) -> anyhow::Result<ReviewDecision> {
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()))
}
codex-rs/core/tests/suite/approvals.rs
+14 -14
View File
@@ -1005,11 +1005,11 @@ fn scenarios() -> Vec<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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<ScenarioSpec> {
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;
codex-rs/core/tests/suite/codex_delegate.rs
+1 -1
View File
@@ -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");
codex-rs/core/tests/suite/exec_policy.rs
+1 -1
View File
@@ -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| {
codex-rs/core/tests/suite/network_approval.rs
+8 -3
View File
@@ -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(
codex-rs/core/tests/suite/otel.rs
+2 -2
View File
@@ -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();
codex-rs/core/tests/suite/request_permissions.rs
+1 -1
View File
@@ -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;
codex-rs/ext/extension-api/tests/registry.rs
+4 -1
View File
@@ -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,
codex-rs/mcp-server/src/exec_approval.rs
+1 -1
View File
@@ -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"),
}
});
codex-rs/mcp-server/src/patch_approval.rs
+2 -2
View File
@@ -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::<PatchApprovalResponse>(value).unwrap_or_else(|err| {
error!("failed to deserialize PatchApprovalResponse: {err}");
PatchApprovalResponse {
decision: ReviewDecision::Denied,
decision: ReviewDecision::denied("approval request failed"),
}
});
codex-rs/protocol/src/protocol.rs
+29 -4
View File
@@ -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<String>) -> 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::<ReviewDecision>(value)?, decision);
Ok(())
}
#[test]
fn feature_thread_source_serializes_as_its_app_owned_label() -> Result<()> {
let source = ThreadSource::Feature("automation".to_string());