mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
representing guardian review timeouts in protocol types (#17381)
## Summary - Add `TimedOut` to Guardian/review carrier types: - `ReviewDecision::TimedOut` - `GuardianAssessmentStatus::TimedOut` - app-server v2 `GuardianApprovalReviewStatus::TimedOut` - Regenerate app-server JSON/TypeScript schemas for the new wire shape. - Wire the new status through core/app-server/TUI mappings with conservative fail-closed handling. - Keep `TimedOut` non-user-selectable in the approval UI. **Does not change runtime behavior yet; emitting `TimeOut` and parent-model timeout messaging will come in followup PRs**
This commit is contained in:
committed by
GitHub
Unverified
parent
824ec94eab
commit
37aac89a6d
@@ -94,6 +94,13 @@
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "Automatic approval review timed out before reaching a decision.",
|
||||
"enum": [
|
||||
"timed_out"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "User has denied this command and the agent should not do anything until the user's next command.",
|
||||
"enum": [
|
||||
|
||||
@@ -94,6 +94,13 @@
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "Automatic approval review timed out before reaching a decision.",
|
||||
"enum": [
|
||||
"timed_out"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "User has denied this command and the agent should not do anything until the user's next command.",
|
||||
"enum": [
|
||||
|
||||
@@ -1348,6 +1348,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
@@ -3398,6 +3398,13 @@
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "Automatic approval review timed out before reaching a decision.",
|
||||
"enum": [
|
||||
"timed_out"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
{
|
||||
"description": "User has denied this command and the agent should not do anything until the user's next command.",
|
||||
"enum": [
|
||||
@@ -8297,6 +8304,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
@@ -5056,6 +5056,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
+1
@@ -222,6 +222,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
+1
@@ -215,6 +215,7 @@
|
||||
"inProgress",
|
||||
"approved",
|
||||
"denied",
|
||||
"timedOut",
|
||||
"aborted"
|
||||
],
|
||||
"type": "string"
|
||||
|
||||
@@ -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" | "abort";
|
||||
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";
|
||||
|
||||
@@ -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";
|
||||
|
||||
@@ -221,6 +221,9 @@ pub 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
|
||||
}
|
||||
@@ -245,6 +248,7 @@ pub 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 {
|
||||
|
||||
@@ -408,6 +408,7 @@ impl ThreadHistoryBuilder {
|
||||
GuardianAssessmentStatus::Denied | GuardianAssessmentStatus::Aborted => {
|
||||
CommandExecutionStatus::Declined
|
||||
}
|
||||
GuardianAssessmentStatus::TimedOut => CommandExecutionStatus::Failed,
|
||||
GuardianAssessmentStatus::Approved => return,
|
||||
};
|
||||
let Some(item) = build_item_from_guardian_event(payload, status) else {
|
||||
|
||||
@@ -1069,6 +1069,7 @@ impl From<CoreReviewDecision> for CommandExecutionApprovalDecision {
|
||||
},
|
||||
CoreReviewDecision::Abort => Self::Cancel,
|
||||
CoreReviewDecision::Denied => Self::Decline,
|
||||
CoreReviewDecision::TimedOut => Self::Decline,
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -4532,6 +4533,7 @@ pub enum GuardianApprovalReviewStatus {
|
||||
InProgress,
|
||||
Approved,
|
||||
Denied,
|
||||
TimedOut,
|
||||
Aborted,
|
||||
}
|
||||
|
||||
|
||||
@@ -312,11 +312,19 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
&assessment,
|
||||
);
|
||||
outgoing.send_server_notification(notification).await;
|
||||
if matches!(
|
||||
assessment.status,
|
||||
let completion_status = match assessment.status {
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::Denied
|
||||
| codex_protocol::protocol::GuardianAssessmentStatus::Aborted
|
||||
) && let Some((target_item_id, completion_item)) = pending_command_execution
|
||||
| codex_protocol::protocol::GuardianAssessmentStatus::Aborted => {
|
||||
Some(CommandExecutionStatus::Declined)
|
||||
}
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::TimedOut => {
|
||||
Some(CommandExecutionStatus::Failed)
|
||||
}
|
||||
codex_protocol::protocol::GuardianAssessmentStatus::InProgress
|
||||
| codex_protocol::protocol::GuardianAssessmentStatus::Approved => None,
|
||||
};
|
||||
if let Some(completion_status) = completion_status
|
||||
&& let Some((target_item_id, completion_item)) = pending_command_execution
|
||||
{
|
||||
complete_command_execution_item(
|
||||
&conversation_id,
|
||||
@@ -327,7 +335,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
/*process_id*/ None,
|
||||
CommandExecutionSource::Agent,
|
||||
completion_item.command_actions,
|
||||
CommandExecutionStatus::Declined,
|
||||
completion_status,
|
||||
&outgoing,
|
||||
&thread_state,
|
||||
)
|
||||
@@ -3000,6 +3008,9 @@ mod tests {
|
||||
Some(codex_protocol::protocol::GuardianUserAuthorization::Low),
|
||||
Some("too risky".to_string()),
|
||||
),
|
||||
GuardianAssessmentStatus::TimedOut => {
|
||||
(None, None, Some("review timed out".to_string()))
|
||||
}
|
||||
GuardianAssessmentStatus::Aborted => (None, None, None),
|
||||
};
|
||||
GuardianAssessmentEvent {
|
||||
|
||||
@@ -716,7 +716,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::Abort => {
|
||||
ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => {
|
||||
MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string()
|
||||
}
|
||||
};
|
||||
|
||||
@@ -414,7 +414,7 @@ impl GuardianReviewSessionManager {
|
||||
let snapshot = state.last_committed_fork_snapshot.as_ref()?;
|
||||
match &snapshot.initial_history {
|
||||
InitialHistory::Forked(items) => Some(items.clone()),
|
||||
_ => None,
|
||||
InitialHistory::New | InitialHistory::Cleared | InitialHistory::Resumed(_) => None,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -980,7 +980,7 @@ async fn mcp_tool_approval_decision_from_guardian(
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::NetworkPolicyAmendment { .. } => McpToolApprovalDecision::Accept,
|
||||
ReviewDecision::ApprovedForSession => McpToolApprovalDecision::AcceptForSession,
|
||||
ReviewDecision::Denied => McpToolApprovalDecision::Decline {
|
||||
ReviewDecision::Denied | ReviewDecision::TimedOut => McpToolApprovalDecision::Decline {
|
||||
message: Some(guardian_rejection_message(sess, review_id).await),
|
||||
},
|
||||
ReviewDecision::Abort => McpToolApprovalDecision::Decline { message: None },
|
||||
|
||||
@@ -488,7 +488,7 @@ impl NetworkApprovalService {
|
||||
PendingApprovalDecision::Deny
|
||||
}
|
||||
},
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
ReviewDecision::Denied | ReviewDecision::TimedOut | 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;
|
||||
|
||||
@@ -151,7 +151,7 @@ impl ToolOrchestrator {
|
||||
otel.tool_decision(otel_tn, otel_ci, &decision, otel_source);
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
ReviewDecision::Denied | ReviewDecision::TimedOut | 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 {
|
||||
@@ -306,7 +306,9 @@ impl ToolOrchestrator {
|
||||
otel.tool_decision(otel_tn, otel_ci, &decision, otel_source);
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
ReviewDecision::Denied
|
||||
| ReviewDecision::TimedOut
|
||||
| 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
|
||||
|
||||
@@ -485,7 +485,7 @@ impl CoreShellActionProvider {
|
||||
EscalationDecision::deny(Some("User denied execution".to_string()))
|
||||
}
|
||||
},
|
||||
ReviewDecision::Denied => {
|
||||
ReviewDecision::Denied | ReviewDecision::TimedOut => {
|
||||
let message = if let Some(review_id) =
|
||||
prompt_decision.guardian_review_id.as_deref()
|
||||
{
|
||||
|
||||
@@ -105,6 +105,7 @@ pub enum GuardianAssessmentStatus {
|
||||
InProgress,
|
||||
Approved,
|
||||
Denied,
|
||||
TimedOut,
|
||||
Aborted,
|
||||
}
|
||||
|
||||
|
||||
@@ -3494,6 +3494,9 @@ pub enum ReviewDecision {
|
||||
#[default]
|
||||
Denied,
|
||||
|
||||
/// Automatic approval review timed out before reaching a decision.
|
||||
TimedOut,
|
||||
|
||||
/// User has denied this command and the agent should not do anything until
|
||||
/// the user's next command.
|
||||
Abort,
|
||||
@@ -3514,6 +3517,7 @@ impl ReviewDecision {
|
||||
NetworkPolicyRuleAction::Deny => "denied_with_network_policy_deny",
|
||||
},
|
||||
ReviewDecision::Denied => "denied",
|
||||
ReviewDecision::TimedOut => "timed_out",
|
||||
ReviewDecision::Abort => "abort",
|
||||
}
|
||||
}
|
||||
|
||||
@@ -258,6 +258,7 @@ fn file_change_decision(decision: &ReviewDecision) -> Result<FileChangeApprovalD
|
||||
ReviewDecision::Approved => Ok(FileChangeApprovalDecision::Accept),
|
||||
ReviewDecision::ApprovedForSession => Ok(FileChangeApprovalDecision::AcceptForSession),
|
||||
ReviewDecision::Denied => Ok(FileChangeApprovalDecision::Decline),
|
||||
ReviewDecision::TimedOut => Ok(FileChangeApprovalDecision::Decline),
|
||||
ReviewDecision::Abort => Ok(FileChangeApprovalDecision::Cancel),
|
||||
ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
|
||||
Err("execpolicy amendment is not a valid file change approval decision".to_string())
|
||||
|
||||
@@ -277,7 +277,9 @@ impl ApprovalOverlay {
|
||||
};
|
||||
let granted_permissions = match decision {
|
||||
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => permissions.clone(),
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => Default::default(),
|
||||
ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => {
|
||||
Default::default()
|
||||
}
|
||||
ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::NetworkPolicyAmendment { .. } => Default::default(),
|
||||
};
|
||||
@@ -720,6 +722,7 @@ fn exec_options(
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('d'))],
|
||||
}),
|
||||
ReviewDecision::TimedOut => None,
|
||||
ReviewDecision::Abort => Some(ApprovalOption {
|
||||
label: "No, and tell Codex what to do differently".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Abort),
|
||||
|
||||
@@ -6914,6 +6914,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
|
||||
}
|
||||
|
||||
@@ -898,6 +898,18 @@ pub fn new_approval_decision_cell(
|
||||
};
|
||||
("✗ ".red(), summary)
|
||||
}
|
||||
TimedOut => {
|
||||
let snippet = Span::from(exec_snippet(&command)).dim();
|
||||
(
|
||||
"✗ ".red(),
|
||||
vec![
|
||||
"Review ".into(),
|
||||
"timed out".bold(),
|
||||
" before codex could run ".into(),
|
||||
snippet,
|
||||
],
|
||||
)
|
||||
}
|
||||
Abort => {
|
||||
let snippet = Span::from(exec_snippet(&command)).dim();
|
||||
(
|
||||
|
||||
Reference in New Issue
Block a user