mirror of
https://github.com/openai/codex.git
synced 2026-11-03 19:53:25 +00:00
Handle guardian review timeouts explicitly
Increase the guardian review timeout and surface reviewer timeouts separately from intentional rejections across tool approval paths, protocol notifications, and TUI rendering. Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
co-authored by
Codex
parent
b32d921cd9
commit
fc60875e65
@@ -1144,6 +1144,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
@@ -7956,6 +7956,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
@@ -4660,6 +4660,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
+1
@@ -43,6 +43,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
+1
@@ -43,6 +43,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
@@ -5,4 +5,4 @@
|
||||
/**
|
||||
* [UNSTABLE] Lifecycle state for a guardian approval review.
|
||||
*/
|
||||
export type GuardianApprovalReviewStatus = "inProgress" | "approved" | "denied" | "aborted";
|
||||
export type GuardianApprovalReviewStatus = "inProgress" | "approved" | "denied" | "timedOut" | "aborted";
|
||||
|
||||
@@ -4367,6 +4367,7 @@ pub enum GuardianApprovalReviewStatus {
|
||||
InProgress,
|
||||
Approved,
|
||||
Denied,
|
||||
TimedOut,
|
||||
Aborted,
|
||||
}
|
||||
|
||||
@@ -7584,6 +7585,26 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn automatic_approval_review_deserializes_timed_out_status() {
|
||||
let review: GuardianApprovalReview = serde_json::from_value(json!({
|
||||
"status": "timedOut",
|
||||
"riskScore": null,
|
||||
"riskLevel": null,
|
||||
"rationale": "review timed out"
|
||||
}))
|
||||
.expect("timed-out automatic review should deserialize");
|
||||
assert_eq!(
|
||||
review,
|
||||
GuardianApprovalReview {
|
||||
status: GuardianApprovalReviewStatus::TimedOut,
|
||||
risk_score: None,
|
||||
risk_level: None,
|
||||
rationale: Some("review timed out".to_string()),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn core_turn_item_into_thread_item_converts_supported_variants() {
|
||||
let user_item = TurnItem::UserMessage(UserMessageItem {
|
||||
|
||||
@@ -881,7 +881,7 @@ All items emit shared lifecycle events:
|
||||
- `item/autoApprovalReview/started` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, review, action?}` when guardian approval review begins. This shape is expected to change soon.
|
||||
- `item/autoApprovalReview/completed` — [UNSTABLE] temporary guardian notification carrying `{threadId, turnId, targetItemId, review, action?}` when guardian approval review resolves. This shape is expected to change soon.
|
||||
|
||||
`review` is [UNSTABLE] and currently has `{status, riskScore?, riskLevel?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, or `aborted`. `action` is the guardian action summary payload from core when available and is intended to support temporary standalone pending-review UI. These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the guardian app protocol is still being designed.
|
||||
`review` is [UNSTABLE] and currently has `{status, riskScore?, riskLevel?, rationale?}`, where `status` is one of `inProgress`, `approved`, `denied`, `timedOut`, or `aborted`. `action` is the guardian action summary payload from core when available and is intended to support temporary standalone pending-review UI. These notifications are separate from the target item's own `item/completed` lifecycle and are intentionally temporary while the guardian app protocol is still being designed.
|
||||
|
||||
There are additional item-specific events:
|
||||
|
||||
|
||||
@@ -216,6 +216,9 @@ fn guardian_auto_approval_review_notification(
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::Denied => {
|
||||
GuardianApprovalReviewStatus::Denied
|
||||
}
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::TimedOut => {
|
||||
GuardianApprovalReviewStatus::TimedOut
|
||||
}
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::Aborted => {
|
||||
GuardianApprovalReviewStatus::Aborted
|
||||
}
|
||||
@@ -238,6 +241,7 @@ fn guardian_auto_approval_review_notification(
|
||||
}
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::Approved
|
||||
| codex_protocol::protocol::GuardianAssessmentStatus::Denied
|
||||
| codex_protocol::protocol::GuardianAssessmentStatus::TimedOut
|
||||
| codex_protocol::protocol::GuardianAssessmentStatus::Aborted => {
|
||||
ServerNotification::ItemGuardianApprovalReviewCompleted(
|
||||
ItemGuardianApprovalReviewCompletedNotification {
|
||||
@@ -3020,6 +3024,48 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_assessment_timed_out_emits_completed_review_payload() {
|
||||
let conversation_id = ThreadId::new();
|
||||
let action = json!({
|
||||
"tool": "shell",
|
||||
"command": "git push",
|
||||
});
|
||||
let notification = guardian_auto_approval_review_notification(
|
||||
&conversation_id,
|
||||
"turn-from-event",
|
||||
&GuardianAssessmentEvent {
|
||||
id: "item-4".to_string(),
|
||||
turn_id: "turn-from-assessment".to_string(),
|
||||
status: codex_protocol::protocol::GuardianAssessmentStatus::TimedOut,
|
||||
risk_score: None,
|
||||
risk_level: None,
|
||||
rationale: Some("review timed out".to_string()),
|
||||
action: Some(action.clone()),
|
||||
},
|
||||
);
|
||||
|
||||
match notification {
|
||||
ServerNotification::ItemGuardianApprovalReviewCompleted(payload) => {
|
||||
assert_eq!(payload.thread_id, conversation_id.to_string());
|
||||
assert_eq!(payload.turn_id, "turn-from-assessment");
|
||||
assert_eq!(payload.target_item_id, "item-4");
|
||||
assert_eq!(
|
||||
payload.review.status,
|
||||
GuardianApprovalReviewStatus::TimedOut
|
||||
);
|
||||
assert_eq!(payload.review.risk_score, None);
|
||||
assert_eq!(payload.review.risk_level, None);
|
||||
assert_eq!(
|
||||
payload.review.rationale.as_deref(),
|
||||
Some("review timed out")
|
||||
);
|
||||
assert_eq!(payload.action, Some(action));
|
||||
}
|
||||
other => panic!("unexpected notification: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn file_change_accept_for_session_maps_to_approved_for_session() {
|
||||
let (decision, completion_status) =
|
||||
|
||||
@@ -1854,6 +1854,7 @@ impl Session {
|
||||
session_telemetry,
|
||||
models_manager: Arc::clone(&models_manager),
|
||||
tool_approvals: Mutex::new(ApprovalStore::default()),
|
||||
guardian_review_timeouts: Mutex::new(HashMap::new()),
|
||||
execve_session_approvals: RwLock::new(HashMap::new()),
|
||||
skills_manager,
|
||||
plugins_manager: Arc::clone(&plugins_manager),
|
||||
|
||||
@@ -2653,6 +2653,7 @@ 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_review_timeouts: Mutex::new(HashMap::new()),
|
||||
execve_session_approvals: RwLock::new(HashMap::new()),
|
||||
skills_manager,
|
||||
plugins_manager,
|
||||
@@ -3488,6 +3489,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
|
||||
session_telemetry: session_telemetry.clone(),
|
||||
models_manager: Arc::clone(&models_manager),
|
||||
tool_approvals: Mutex::new(ApprovalStore::default()),
|
||||
guardian_review_timeouts: Mutex::new(HashMap::new()),
|
||||
execve_session_approvals: RwLock::new(HashMap::new()),
|
||||
skills_manager,
|
||||
plugins_manager,
|
||||
|
||||
@@ -29,10 +29,11 @@ pub(crate) use review::is_guardian_reviewer_source;
|
||||
pub(crate) use review::review_approval_request;
|
||||
pub(crate) use review::review_approval_request_with_cancel;
|
||||
pub(crate) use review::routes_approval_to_guardian;
|
||||
pub(crate) use review::take_guardian_timeout_message;
|
||||
pub(crate) use review_session::GuardianReviewSessionManager;
|
||||
|
||||
const GUARDIAN_PREFERRED_MODEL: &str = "gpt-5.4";
|
||||
pub(crate) const GUARDIAN_REVIEW_TIMEOUT: Duration = Duration::from_secs(90);
|
||||
pub(crate) const GUARDIAN_REVIEW_TIMEOUT: Duration = Duration::from_secs(120);
|
||||
pub(crate) const GUARDIAN_REVIEWER_NAME: &str = "guardian";
|
||||
const GUARDIAN_MAX_MESSAGE_TRANSCRIPT_TOKENS: usize = 10_000;
|
||||
const GUARDIAN_MAX_TOOL_TRANSCRIPT_TOKENS: usize = 10_000;
|
||||
|
||||
@@ -37,6 +37,11 @@ pub(crate) const GUARDIAN_REJECTION_MESSAGE: &str = concat!(
|
||||
"Otherwise, stop and request user input.",
|
||||
);
|
||||
|
||||
pub(crate) const GUARDIAN_TIMEOUT_MESSAGE: &str = concat!(
|
||||
"Automatic approval review timed out before it could finish evaluating this action. ",
|
||||
"The action was not intentionally rejected, and a retry may succeed.",
|
||||
);
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(super) enum GuardianReviewOutcome {
|
||||
Completed(anyhow::Result<GuardianAssessment>),
|
||||
@@ -44,6 +49,18 @@ pub(super) enum GuardianReviewOutcome {
|
||||
Aborted,
|
||||
}
|
||||
|
||||
pub(crate) async fn take_guardian_timeout_message(
|
||||
session: &Session,
|
||||
request_id: &str,
|
||||
) -> Option<String> {
|
||||
session
|
||||
.services
|
||||
.guardian_review_timeouts
|
||||
.lock()
|
||||
.await
|
||||
.remove(request_id)
|
||||
}
|
||||
|
||||
fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str {
|
||||
match level {
|
||||
GuardianRiskLevel::Low => "low",
|
||||
@@ -82,6 +99,12 @@ async fn run_guardian_review(
|
||||
let assessment_id = guardian_request_id(&request).to_string();
|
||||
let assessment_turn_id = guardian_request_turn_id(&request, &turn.sub_id).to_string();
|
||||
let action_summary = guardian_assessment_action_value(&request);
|
||||
session
|
||||
.services
|
||||
.guardian_review_timeouts
|
||||
.lock()
|
||||
.await
|
||||
.remove(&assessment_id);
|
||||
session
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
@@ -134,22 +157,41 @@ async fn run_guardian_review(
|
||||
Err(err) => GuardianReviewOutcome::Completed(Err(err.into())),
|
||||
};
|
||||
|
||||
let assessment = match outcome {
|
||||
GuardianReviewOutcome::Completed(Ok(assessment)) => assessment,
|
||||
GuardianReviewOutcome::Completed(Err(err)) => GuardianAssessment {
|
||||
risk_level: GuardianRiskLevel::High,
|
||||
risk_score: 100,
|
||||
rationale: format!("Automatic approval review failed: {err}"),
|
||||
evidence: vec![],
|
||||
},
|
||||
GuardianReviewOutcome::TimedOut => GuardianAssessment {
|
||||
risk_level: GuardianRiskLevel::High,
|
||||
risk_score: 100,
|
||||
rationale:
|
||||
"Automatic approval review timed out while evaluating the requested approval."
|
||||
.to_string(),
|
||||
evidence: vec![],
|
||||
},
|
||||
let (assessment, status) = match outcome {
|
||||
GuardianReviewOutcome::Completed(Ok(assessment)) => {
|
||||
let status = if assessment.risk_score < GUARDIAN_APPROVAL_RISK_THRESHOLD {
|
||||
GuardianAssessmentStatus::Approved
|
||||
} else {
|
||||
GuardianAssessmentStatus::Denied
|
||||
};
|
||||
(assessment, status)
|
||||
}
|
||||
GuardianReviewOutcome::Completed(Err(err)) => (
|
||||
GuardianAssessment {
|
||||
risk_level: GuardianRiskLevel::High,
|
||||
risk_score: 100,
|
||||
rationale: format!("Automatic approval review failed: {err}"),
|
||||
evidence: vec![],
|
||||
},
|
||||
GuardianAssessmentStatus::Denied,
|
||||
),
|
||||
GuardianReviewOutcome::TimedOut => {
|
||||
session
|
||||
.services
|
||||
.guardian_review_timeouts
|
||||
.lock()
|
||||
.await
|
||||
.insert(assessment_id.clone(), GUARDIAN_TIMEOUT_MESSAGE.to_string());
|
||||
(
|
||||
GuardianAssessment {
|
||||
risk_level: GuardianRiskLevel::High,
|
||||
risk_score: 100,
|
||||
rationale: GUARDIAN_TIMEOUT_MESSAGE.to_string(),
|
||||
evidence: vec![],
|
||||
},
|
||||
GuardianAssessmentStatus::TimedOut,
|
||||
)
|
||||
}
|
||||
GuardianReviewOutcome::Aborted => {
|
||||
session
|
||||
.send_event(
|
||||
@@ -169,23 +211,34 @@ async fn run_guardian_review(
|
||||
}
|
||||
};
|
||||
|
||||
let approved = assessment.risk_score < GUARDIAN_APPROVAL_RISK_THRESHOLD;
|
||||
let verdict = if approved { "approved" } else { "denied" };
|
||||
let warning = format!(
|
||||
"Automatic approval review {verdict} (risk: {}): {}",
|
||||
guardian_risk_level_str(assessment.risk_level),
|
||||
assessment.rationale
|
||||
);
|
||||
let warning = match status {
|
||||
GuardianAssessmentStatus::TimedOut => assessment.rationale.clone(),
|
||||
GuardianAssessmentStatus::Approved | GuardianAssessmentStatus::Denied => {
|
||||
let verdict = if status == GuardianAssessmentStatus::Approved {
|
||||
"approved"
|
||||
} else {
|
||||
"denied"
|
||||
};
|
||||
format!(
|
||||
"Automatic approval review {verdict} (risk: {}): {}",
|
||||
guardian_risk_level_str(assessment.risk_level),
|
||||
assessment.rationale
|
||||
)
|
||||
}
|
||||
GuardianAssessmentStatus::InProgress | GuardianAssessmentStatus::Aborted => {
|
||||
unreachable!("guardian terminal warnings only use terminal statuses")
|
||||
}
|
||||
};
|
||||
session
|
||||
.send_event(
|
||||
turn.as_ref(),
|
||||
EventMsg::Warning(WarningEvent { message: warning }),
|
||||
)
|
||||
.await;
|
||||
let status = if approved {
|
||||
GuardianAssessmentStatus::Approved
|
||||
let (risk_score, risk_level) = if status == GuardianAssessmentStatus::TimedOut {
|
||||
(None, None)
|
||||
} else {
|
||||
GuardianAssessmentStatus::Denied
|
||||
(Some(assessment.risk_score), Some(assessment.risk_level))
|
||||
};
|
||||
session
|
||||
.send_event(
|
||||
@@ -194,15 +247,15 @@ async fn run_guardian_review(
|
||||
id: assessment_id,
|
||||
turn_id: assessment_turn_id,
|
||||
status,
|
||||
risk_score: Some(assessment.risk_score),
|
||||
risk_level: Some(assessment.risk_level),
|
||||
risk_score,
|
||||
risk_level,
|
||||
rationale: Some(assessment.rationale.clone()),
|
||||
action: Some(terminal_action),
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
|
||||
if approved {
|
||||
if status == GuardianAssessmentStatus::Approved {
|
||||
ReviewDecision::Approved
|
||||
} else {
|
||||
ReviewDecision::Denied
|
||||
|
||||
@@ -416,6 +416,29 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
|
||||
assert!(warnings.is_empty());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_timeout_message_is_taken_once() {
|
||||
let (session, _turn) = crate::codex::make_session_and_context().await;
|
||||
session
|
||||
.services
|
||||
.guardian_review_timeouts
|
||||
.lock()
|
||||
.await
|
||||
.insert("approval-1".to_string(), "review timed out".to_string());
|
||||
|
||||
assert_eq!(
|
||||
take_guardian_timeout_message(&session, "approval-1")
|
||||
.await
|
||||
.as_deref(),
|
||||
Some("review timed out")
|
||||
);
|
||||
assert!(
|
||||
take_guardian_timeout_message(&session, "approval-1")
|
||||
.await
|
||||
.is_none()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn routes_approval_to_guardian_requires_auto_only_review_policy() {
|
||||
let (_session, mut turn) = crate::codex::make_session_and_context().await;
|
||||
|
||||
@@ -24,6 +24,7 @@ use crate::guardian::GuardianMcpAnnotations;
|
||||
use crate::guardian::guardian_approval_request_to_json;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::take_guardian_timeout_message;
|
||||
use crate::mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam;
|
||||
use crate::mcp_tool_approval_templates::render_mcp_tool_approval_template;
|
||||
@@ -197,6 +198,17 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
)
|
||||
.await
|
||||
}
|
||||
McpToolApprovalDecision::TimedOut(message) => {
|
||||
notify_mcp_tool_call_skip(
|
||||
sess.as_ref(),
|
||||
turn_context.as_ref(),
|
||||
&call_id,
|
||||
invocation,
|
||||
message,
|
||||
/*already_started*/ true,
|
||||
)
|
||||
.await
|
||||
}
|
||||
McpToolApprovalDecision::Cancel => {
|
||||
let message = "user cancelled MCP tool call".to_string();
|
||||
notify_mcp_tool_call_skip(
|
||||
@@ -374,6 +386,7 @@ enum McpToolApprovalDecision {
|
||||
AcceptForSession,
|
||||
AcceptAndRemember,
|
||||
Decline,
|
||||
TimedOut(String),
|
||||
Cancel,
|
||||
BlockedBySafetyMonitor(String),
|
||||
}
|
||||
@@ -551,7 +564,13 @@ async fn maybe_request_mcp_tool_approval(
|
||||
monitor_reason.clone(),
|
||||
)
|
||||
.await;
|
||||
let decision = mcp_tool_approval_decision_from_guardian(decision);
|
||||
let decision = if (decision == ReviewDecision::Denied || decision == ReviewDecision::Abort)
|
||||
&& let Some(message) = take_guardian_timeout_message(sess.as_ref(), call_id).await
|
||||
{
|
||||
McpToolApprovalDecision::TimedOut(message)
|
||||
} else {
|
||||
mcp_tool_approval_decision_from_guardian(decision)
|
||||
};
|
||||
apply_mcp_tool_approval_decision(
|
||||
sess,
|
||||
turn_context,
|
||||
@@ -1245,6 +1264,7 @@ async fn apply_mcp_tool_approval_decision(
|
||||
}
|
||||
McpToolApprovalDecision::Accept
|
||||
| McpToolApprovalDecision::Decline
|
||||
| McpToolApprovalDecision::TimedOut(_)
|
||||
| McpToolApprovalDecision::Cancel
|
||||
| McpToolApprovalDecision::BlockedBySafetyMonitor(_) => {}
|
||||
}
|
||||
|
||||
@@ -49,6 +49,7 @@ pub(crate) struct SessionServices {
|
||||
pub(crate) models_manager: Arc<ModelsManager>,
|
||||
pub(crate) session_telemetry: SessionTelemetry,
|
||||
pub(crate) tool_approvals: Mutex<ApprovalStore>,
|
||||
pub(crate) guardian_review_timeouts: Mutex<HashMap<String, String>>,
|
||||
#[cfg_attr(not(unix), allow(dead_code))]
|
||||
pub(crate) execve_session_approvals: RwLock<HashMap<AbsolutePathBuf, ExecveSessionApproval>>,
|
||||
pub(crate) skills_manager: Arc<SkillsManager>,
|
||||
|
||||
@@ -333,6 +333,11 @@ impl ToolEmitter {
|
||||
let result = Err(FunctionCallError::RespondToModel(message));
|
||||
(event, result)
|
||||
}
|
||||
Err(ToolError::Message(msg)) => {
|
||||
let event = ToolEventStage::Failure(ToolEventFailure::Message(msg.clone()));
|
||||
let result = Err(FunctionCallError::RespondToModel(msg));
|
||||
(event, result)
|
||||
}
|
||||
Err(ToolError::Rejected(msg)) => {
|
||||
// Normalize common rejection messages for exec tools so tests and
|
||||
// users see a clear, consistent phrase.
|
||||
|
||||
@@ -3,6 +3,7 @@ use crate::guardian::GUARDIAN_REJECTION_MESSAGE;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::take_guardian_timeout_message;
|
||||
use crate::network_policy_decision::denied_network_policy_message;
|
||||
use crate::tools::sandboxing::ToolError;
|
||||
use codex_network_proxy::BlockedRequest;
|
||||
@@ -111,6 +112,7 @@ enum PendingApprovalDecision {
|
||||
enum NetworkApprovalOutcome {
|
||||
DeniedByUser,
|
||||
DeniedByPolicy(String),
|
||||
TimedOutByReviewer(String),
|
||||
}
|
||||
|
||||
/// Whether an allowlist miss may be reviewed instead of hard-denied.
|
||||
@@ -470,14 +472,21 @@ impl NetworkApprovalService {
|
||||
},
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
if routes_approval_to_guardian(&turn_context) {
|
||||
let timeout_message = take_guardian_timeout_message(
|
||||
session.as_ref(),
|
||||
&Self::approval_id_for_key(&key),
|
||||
)
|
||||
.await;
|
||||
if let Some(owner_call) = owner_call.as_ref() {
|
||||
self.record_call_outcome(
|
||||
&owner_call.registration_id,
|
||||
let outcome = if let Some(message) = timeout_message {
|
||||
NetworkApprovalOutcome::TimedOutByReviewer(message)
|
||||
} else {
|
||||
NetworkApprovalOutcome::DeniedByPolicy(
|
||||
GUARDIAN_REJECTION_MESSAGE.to_string(),
|
||||
),
|
||||
)
|
||||
.await;
|
||||
)
|
||||
};
|
||||
self.record_call_outcome(&owner_call.registration_id, outcome)
|
||||
.await;
|
||||
}
|
||||
} else if let Some(owner_call) = owner_call.as_ref() {
|
||||
self.record_call_outcome(
|
||||
@@ -594,6 +603,9 @@ pub(crate) async fn finish_immediate_network_approval(
|
||||
Err(ToolError::Rejected("rejected by user".to_string()))
|
||||
}
|
||||
Some(NetworkApprovalOutcome::DeniedByPolicy(message)) => Err(ToolError::Rejected(message)),
|
||||
Some(NetworkApprovalOutcome::TimedOutByReviewer(message)) => {
|
||||
Err(ToolError::Message(message))
|
||||
}
|
||||
None => Ok(()),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -11,6 +11,7 @@ use crate::error::SandboxErr;
|
||||
use crate::exec::ExecToolCallOutput;
|
||||
use crate::guardian::GUARDIAN_REJECTION_MESSAGE;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::take_guardian_timeout_message;
|
||||
use crate::network_policy_decision::network_approval_context_from_payload;
|
||||
use crate::tools::network_approval::DeferredNetworkApproval;
|
||||
use crate::tools::network_approval::NetworkApprovalMode;
|
||||
@@ -41,6 +42,25 @@ pub(crate) struct OrchestratorRunResult<Out> {
|
||||
pub deferred_network_approval: Option<DeferredNetworkApproval>,
|
||||
}
|
||||
|
||||
async fn approval_denial_tool_error(
|
||||
tool_ctx: &ToolCtx,
|
||||
turn_ctx: &crate::codex::TurnContext,
|
||||
) -> ToolError {
|
||||
if routes_approval_to_guardian(turn_ctx)
|
||||
&& let Some(message) =
|
||||
take_guardian_timeout_message(tool_ctx.session.as_ref(), &tool_ctx.call_id).await
|
||||
{
|
||||
ToolError::Message(message)
|
||||
} else {
|
||||
let reason = if routes_approval_to_guardian(turn_ctx) {
|
||||
GUARDIAN_REJECTION_MESSAGE.to_string()
|
||||
} else {
|
||||
"rejected by user".to_string()
|
||||
};
|
||||
ToolError::Rejected(reason)
|
||||
}
|
||||
}
|
||||
|
||||
impl ToolOrchestrator {
|
||||
pub fn new() -> Self {
|
||||
Self {
|
||||
@@ -148,12 +168,7 @@ impl ToolOrchestrator {
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if routes_approval_to_guardian(turn_ctx) {
|
||||
GUARDIAN_REJECTION_MESSAGE.to_string()
|
||||
} else {
|
||||
"rejected by user".to_string()
|
||||
};
|
||||
return Err(ToolError::Rejected(reason));
|
||||
return Err(approval_denial_tool_error(tool_ctx, turn_ctx).await);
|
||||
}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
@@ -301,12 +316,7 @@ impl ToolOrchestrator {
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
let reason = if routes_approval_to_guardian(turn_ctx) {
|
||||
GUARDIAN_REJECTION_MESSAGE.to_string()
|
||||
} else {
|
||||
"rejected by user".to_string()
|
||||
};
|
||||
return Err(ToolError::Rejected(reason));
|
||||
return Err(approval_denial_tool_error(tool_ctx, turn_ctx).await);
|
||||
}
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
|
||||
@@ -8,6 +8,7 @@ use crate::exec::is_likely_sandbox_denied;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::take_guardian_timeout_message;
|
||||
use crate::sandboxing::ExecOptions;
|
||||
use crate::sandboxing::ExecRequest;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
@@ -598,10 +599,26 @@ impl CoreShellActionProvider {
|
||||
}
|
||||
},
|
||||
ReviewDecision::Denied => {
|
||||
EscalationDecision::deny(Some("User denied execution".to_string()))
|
||||
if let Some(message) =
|
||||
take_guardian_timeout_message(self.session.as_ref(), &self.call_id)
|
||||
.await
|
||||
{
|
||||
EscalationDecision::deny(Some(message))
|
||||
} else {
|
||||
EscalationDecision::deny(Some("User denied execution".to_string()))
|
||||
}
|
||||
}
|
||||
ReviewDecision::Abort => {
|
||||
EscalationDecision::deny(Some("User cancelled execution".to_string()))
|
||||
if let Some(message) =
|
||||
take_guardian_timeout_message(self.session.as_ref(), &self.call_id)
|
||||
.await
|
||||
{
|
||||
EscalationDecision::deny(Some(message))
|
||||
} else {
|
||||
EscalationDecision::deny(Some(
|
||||
"User cancelled execution".to_string(),
|
||||
))
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -300,6 +300,7 @@ pub(crate) struct ToolCtx {
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(crate) enum ToolError {
|
||||
Message(String),
|
||||
Rejected(String),
|
||||
Codex(CodexErr),
|
||||
}
|
||||
|
||||
@@ -98,6 +98,7 @@ pub enum GuardianAssessmentStatus {
|
||||
InProgress,
|
||||
Approved,
|
||||
Denied,
|
||||
TimedOut,
|
||||
Aborted,
|
||||
}
|
||||
|
||||
|
||||
+18
@@ -0,0 +1,18 @@
|
||||
---
|
||||
source: tui/src/chatwidget/tests.rs
|
||||
expression: term.backend().vt100().screen().contents()
|
||||
---
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
⚠ Automatic approval review timed out before it could finish evaluating this
|
||||
action. The action was not intentionally rejected, and a retry may succeed.
|
||||
|
||||
• Working (0s • esc to interrupt)
|
||||
|
||||
|
||||
› Ask Codex to do anything
|
||||
|
||||
? for shortcuts 100% context left
|
||||
@@ -10453,6 +10453,71 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_timed_out_exec_renders_warning_without_denied_request() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
chat.show_welcome_banner = false;
|
||||
let action = serde_json::json!({
|
||||
"tool": "shell",
|
||||
"command": "git push origin main",
|
||||
});
|
||||
|
||||
chat.handle_codex_event(Event {
|
||||
id: "guardian-in-progress".into(),
|
||||
msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent {
|
||||
id: "guardian-timeout-1".into(),
|
||||
turn_id: "turn-1".into(),
|
||||
status: GuardianAssessmentStatus::InProgress,
|
||||
risk_score: None,
|
||||
risk_level: None,
|
||||
rationale: None,
|
||||
action: Some(action.clone()),
|
||||
}),
|
||||
});
|
||||
chat.handle_codex_event(Event {
|
||||
id: "guardian-warning".into(),
|
||||
msg: EventMsg::Warning(WarningEvent {
|
||||
message: "Automatic approval review timed out before it could finish evaluating this action. The action was not intentionally rejected, and a retry may succeed.".into(),
|
||||
}),
|
||||
});
|
||||
chat.handle_codex_event(Event {
|
||||
id: "guardian-assessment".into(),
|
||||
msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent {
|
||||
id: "guardian-timeout-1".into(),
|
||||
turn_id: "turn-1".into(),
|
||||
status: GuardianAssessmentStatus::TimedOut,
|
||||
risk_score: None,
|
||||
risk_level: None,
|
||||
rationale: Some("Automatic approval review timed out before it could finish evaluating this action. The action was not intentionally rejected, and a retry may succeed.".into()),
|
||||
action: Some(action),
|
||||
}),
|
||||
});
|
||||
|
||||
let width: u16 = 140;
|
||||
let ui_height: u16 = chat.desired_height(width);
|
||||
let vt_height: u16 = 14;
|
||||
let viewport = Rect::new(0, vt_height - ui_height - 1, width, ui_height);
|
||||
|
||||
let backend = VT100Backend::new(width, vt_height);
|
||||
let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal");
|
||||
term.set_viewport_area(viewport);
|
||||
|
||||
for lines in drain_insert_history(&mut rx) {
|
||||
crate::insert_history::insert_history_lines(&mut term, lines)
|
||||
.expect("Failed to insert history lines in test");
|
||||
}
|
||||
|
||||
term.draw(|f| {
|
||||
chat.render(f.area(), f.buffer_mut());
|
||||
})
|
||||
.expect("draw guardian timeout history");
|
||||
|
||||
assert_snapshot!(
|
||||
"guardian_timed_out_exec_renders_warning_without_denied_request",
|
||||
term.backend().vt100().screen().contents()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_approved_exec_renders_approved_request() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
|
||||
@@ -6377,6 +6377,9 @@ impl ChatWidget {
|
||||
codex_app_server_protocol::GuardianApprovalReviewStatus::Denied => {
|
||||
GuardianAssessmentStatus::Denied
|
||||
}
|
||||
codex_app_server_protocol::GuardianApprovalReviewStatus::TimedOut => {
|
||||
GuardianAssessmentStatus::TimedOut
|
||||
}
|
||||
codex_app_server_protocol::GuardianApprovalReviewStatus::Aborted => {
|
||||
GuardianAssessmentStatus::Aborted
|
||||
}
|
||||
|
||||
+18
@@ -0,0 +1,18 @@
|
||||
---
|
||||
source: tui_app_server/src/chatwidget/tests.rs
|
||||
expression: term.backend().vt100().screen().contents()
|
||||
---
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
⚠ Automatic approval review timed out before it could finish evaluating this
|
||||
action. The action was not intentionally rejected, and a retry may succeed.
|
||||
|
||||
• Working (0s • esc to interrupt)
|
||||
|
||||
|
||||
› Ask Codex to do anything
|
||||
|
||||
? for shortcuts 100% context left
|
||||
@@ -11010,6 +11010,71 @@ async fn guardian_denied_exec_renders_warning_and_denied_request() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_timed_out_exec_renders_warning_without_denied_request() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
chat.show_welcome_banner = false;
|
||||
let action = serde_json::json!({
|
||||
"tool": "shell",
|
||||
"command": "git push origin main",
|
||||
});
|
||||
|
||||
chat.handle_codex_event(Event {
|
||||
id: "guardian-in-progress".into(),
|
||||
msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent {
|
||||
id: "guardian-timeout-1".into(),
|
||||
turn_id: "turn-1".into(),
|
||||
status: GuardianAssessmentStatus::InProgress,
|
||||
risk_score: None,
|
||||
risk_level: None,
|
||||
rationale: None,
|
||||
action: Some(action.clone()),
|
||||
}),
|
||||
});
|
||||
chat.handle_codex_event(Event {
|
||||
id: "guardian-warning".into(),
|
||||
msg: EventMsg::Warning(WarningEvent {
|
||||
message: "Automatic approval review timed out before it could finish evaluating this action. The action was not intentionally rejected, and a retry may succeed.".into(),
|
||||
}),
|
||||
});
|
||||
chat.handle_codex_event(Event {
|
||||
id: "guardian-assessment".into(),
|
||||
msg: EventMsg::GuardianAssessment(GuardianAssessmentEvent {
|
||||
id: "guardian-timeout-1".into(),
|
||||
turn_id: "turn-1".into(),
|
||||
status: GuardianAssessmentStatus::TimedOut,
|
||||
risk_score: None,
|
||||
risk_level: None,
|
||||
rationale: Some("Automatic approval review timed out before it could finish evaluating this action. The action was not intentionally rejected, and a retry may succeed.".into()),
|
||||
action: Some(action),
|
||||
}),
|
||||
});
|
||||
|
||||
let width: u16 = 140;
|
||||
let ui_height: u16 = chat.desired_height(width);
|
||||
let vt_height: u16 = 14;
|
||||
let viewport = Rect::new(0, vt_height - ui_height - 1, width, ui_height);
|
||||
|
||||
let backend = VT100Backend::new(width, vt_height);
|
||||
let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal");
|
||||
term.set_viewport_area(viewport);
|
||||
|
||||
for lines in drain_insert_history(&mut rx) {
|
||||
crate::insert_history::insert_history_lines(&mut term, lines)
|
||||
.expect("Failed to insert history lines in test");
|
||||
}
|
||||
|
||||
term.draw(|f| {
|
||||
chat.render(f.area(), f.buffer_mut());
|
||||
})
|
||||
.expect("draw guardian timeout history");
|
||||
|
||||
assert_snapshot!(
|
||||
"guardian_timed_out_exec_renders_warning_without_denied_request",
|
||||
term.backend().vt100().screen().contents()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_approved_exec_renders_approved_request() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;
|
||||
|
||||
Reference in New Issue
Block a user