Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions codex-rs/core/src/guardian/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ pub(crate) use approval_request::GuardianApprovalRequest;
pub(crate) use approval_request::GuardianMcpAnnotations;
pub(crate) use approval_request::guardian_approval_request_to_json;
pub(crate) use review::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;
pub(crate) use review::review_approval_request;
Expand Down
51 changes: 42 additions & 9 deletions codex-rs/core/src/guardian/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ const GUARDIAN_REJECTION_INSTRUCTIONS: &str = concat!(
"Otherwise, stop and request user input.",
);

const GUARDIAN_TIMEOUT_INSTRUCTIONS: &str = concat!(
"The automatic approval review did not finish before its deadline. ",
"Do not assume the action is unsafe based on the timeout alone. ",
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on what the instruction shuold be?

"You may retry once with a narrower or simpler request, or ask the user for guidance or explicit approval.",
);

pub(crate) fn new_guardian_review_id() -> String {
uuid::Uuid::new_v4().to_string()
}
Expand All @@ -63,6 +69,10 @@ pub(crate) async fn guardian_rejection_message(session: &Session, review_id: &st
}
}

pub(crate) fn guardian_timeout_message() -> String {
GUARDIAN_TIMEOUT_INSTRUCTIONS.to_string()
}

#[derive(Debug)]
pub(super) enum GuardianReviewOutcome {
Completed(anyhow::Result<GuardianAssessment>),
Expand Down Expand Up @@ -97,8 +107,9 @@ pub(crate) fn is_guardian_reviewer_source(
)
}

/// This function always fails closed: any timeout, review-session failure, or
/// parse failure is treated as a high-risk denial.
/// This function always fails closed: timeouts, review-session failures, and
/// parse failures all block execution, but timeouts are still surfaced to the
/// caller as distinct from explicit guardian denials.
async fn run_guardian_review(
session: Arc<Session>,
turn: Arc<TurnContext>,
Expand Down Expand Up @@ -170,14 +181,36 @@ async fn run_guardian_review(
outcome: GuardianAssessmentOutcome::Deny,
rationale: format!("Automatic approval review failed: {err}"),
},
GuardianReviewOutcome::TimedOut => GuardianAssessment {
risk_level: GuardianRiskLevel::High,
user_authorization: GuardianUserAuthorization::Unknown,
outcome: GuardianAssessmentOutcome::Deny,
rationale:
GuardianReviewOutcome::TimedOut => {
let rationale =
"Automatic approval review timed out while evaluating the requested approval."
.to_string(),
},
.to_string();
session
.send_event(
turn.as_ref(),
EventMsg::Warning(WarningEvent {
message: rationale.clone(),
}),
)
.await;
session
.send_event(
turn.as_ref(),
EventMsg::GuardianAssessment(GuardianAssessmentEvent {
id: review_id,
target_item_id,
turn_id: assessment_turn_id,
status: GuardianAssessmentStatus::TimedOut,
risk_level: None,
user_authorization: None,
rationale: Some(rationale),
decision_source: Some(GuardianAssessmentDecisionSource::Agent),
action: terminal_action,
}),
)
.await;
return ReviewDecision::TimedOut;
}
GuardianReviewOutcome::Aborted => {
session
.send_event(
Expand Down
8 changes: 8 additions & 0 deletions codex-rs/core/src/guardian/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,14 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
assert!(warnings.is_empty());
}

#[test]
fn guardian_timeout_message_distinguishes_timeout_from_policy_denial() {
let message = guardian_timeout_message();
assert!(message.contains("did not finish before its deadline"));
assert!(message.contains("retry once"));
assert!(!message.contains("unacceptable risk"));
}

#[tokio::test]
async fn routes_approval_to_guardian_requires_auto_only_review_policy() {
let (_session, mut turn) = crate::codex::make_session_and_context().await;
Expand Down
6 changes: 5 additions & 1 deletion codex-rs/core/src/mcp_tool_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use crate::guardian::GuardianApprovalRequest;
use crate::guardian::GuardianMcpAnnotations;
use crate::guardian::guardian_approval_request_to_json;
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;
Expand Down Expand Up @@ -980,9 +981,12 @@ async fn mcp_tool_approval_decision_from_guardian(
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::NetworkPolicyAmendment { .. } => McpToolApprovalDecision::Accept,
ReviewDecision::ApprovedForSession => McpToolApprovalDecision::AcceptForSession,
ReviewDecision::Denied | ReviewDecision::TimedOut => McpToolApprovalDecision::Decline {
ReviewDecision::Denied => McpToolApprovalDecision::Decline {
message: Some(guardian_rejection_message(sess, review_id).await),
},
ReviewDecision::TimedOut => McpToolApprovalDecision::Decline {
message: Some(guardian_timeout_message()),
},
ReviewDecision::Abort => McpToolApprovalDecision::Decline { message: None },
}
}
Expand Down
14 changes: 14 additions & 0 deletions codex-rs/core/src/mcp_tool_call_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,20 @@ 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 McpToolApprovalDecision::Decline {
message: Some(message),
} = timeout
else {
panic!("guardian timeout should carry a timeout message");
};
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(),
Expand Down
13 changes: 12 additions & 1 deletion codex-rs/core/src/tools/network_approval.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::codex::Session;
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;
Expand Down Expand Up @@ -488,7 +489,7 @@ impl NetworkApprovalService {
PendingApprovalDecision::Deny
}
},
ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => {
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;
Expand All @@ -507,6 +508,16 @@ impl NetworkApprovalService {
}
PendingApprovalDecision::Deny
}
ReviewDecision::TimedOut => {
if let Some(owner_call) = owner_call.as_ref() {
self.record_call_outcome(
&owner_call.registration_id,
NetworkApprovalOutcome::DeniedByPolicy(guardian_timeout_message()),
)
.await;
}
PendingApprovalDecision::Deny
}
};

if matches!(resolved, PendingApprovalDecision::AllowForSession) {
Expand Down
13 changes: 9 additions & 4 deletions codex-rs/core/src/tools/orchestrator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ retry with an escalated sandbox strategy on denial (no re‑approval thanks to
caching).
*/
use crate::guardian::guardian_rejection_message;
use crate::guardian::guardian_timeout_message;
use crate::guardian::new_guardian_review_id;
use crate::guardian::routes_approval_to_guardian;
use crate::network_policy_decision::network_approval_context_from_payload;
Expand Down Expand Up @@ -151,14 +152,17 @@ impl ToolOrchestrator {
otel.tool_decision(otel_tn, otel_ci, &decision, otel_source);

match decision {
ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => {
ReviewDecision::Denied | ReviewDecision::Abort => {
let reason = if let Some(review_id) = guardian_review_id.as_deref() {
guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await
} else {
"rejected by user".to_string()
};
return Err(ToolError::Rejected(reason));
}
ReviewDecision::TimedOut => {
return Err(ToolError::Rejected(guardian_timeout_message()));
}
ReviewDecision::Approved
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => {}
Expand Down Expand Up @@ -306,9 +310,7 @@ impl ToolOrchestrator {
otel.tool_decision(otel_tn, otel_ci, &decision, otel_source);

match decision {
ReviewDecision::Denied
| ReviewDecision::TimedOut
| ReviewDecision::Abort => {
ReviewDecision::Denied | ReviewDecision::Abort => {
let reason = if let Some(review_id) = guardian_review_id.as_deref() {
guardian_rejection_message(tool_ctx.session.as_ref(), review_id)
.await
Expand All @@ -317,6 +319,9 @@ impl ToolOrchestrator {
};
return Err(ToolError::Rejected(reason));
}
ReviewDecision::TimedOut => {
return Err(ToolError::Rejected(guardian_timeout_message()));
}
ReviewDecision::Approved
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => {}
Expand Down
6 changes: 5 additions & 1 deletion codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::exec::ExecExpiration;
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;
Expand Down Expand Up @@ -485,7 +486,7 @@ impl CoreShellActionProvider {
EscalationDecision::deny(Some("User denied execution".to_string()))
}
},
ReviewDecision::Denied | ReviewDecision::TimedOut => {
ReviewDecision::Denied => {
let message = if let Some(review_id) =
prompt_decision.guardian_review_id.as_deref()
{
Expand All @@ -495,6 +496,9 @@ impl CoreShellActionProvider {
};
EscalationDecision::deny(Some(message))
}
ReviewDecision::TimedOut => {
EscalationDecision::deny(Some(guardian_timeout_message()))
}
ReviewDecision::Abort => {
EscalationDecision::deny(Some("User cancelled execution".to_string()))
}
Expand Down
Loading