mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Fall back to manual approval when Guardian times out
This commit is contained in:
@@ -91,6 +91,7 @@ pub(crate) async fn run_codex_thread_interactive(
|
||||
extensions: Arc::clone(&parent_session.services.extensions),
|
||||
conversation_history,
|
||||
session_source: SessionSource::SubAgent(subagent_source.clone()),
|
||||
client_session_source: parent_ctx.session_source.clone(),
|
||||
forked_from_thread_id,
|
||||
parent_thread_id: Some(parent_session.thread_id),
|
||||
thread_source: Some(ThreadSource::Subagent),
|
||||
|
||||
@@ -21,6 +21,7 @@ use std::time::Duration;
|
||||
|
||||
use codex_protocol::protocol::GuardianAssessmentDecisionSource;
|
||||
use codex_protocol::protocol::GuardianAssessmentOutcome;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
|
||||
@@ -59,6 +60,12 @@ const GUARDIAN_MAX_ACTION_STRING_TOKENS: usize = 16_000;
|
||||
const GUARDIAN_RECENT_ENTRY_LIMIT: usize = 40;
|
||||
const TRUNCATION_TAG: &str = "truncated";
|
||||
|
||||
pub(crate) fn can_fallback_to_manual_after_guardian_timeout(
|
||||
session_source: &SessionSource,
|
||||
) -> bool {
|
||||
matches!(session_source, SessionSource::Cli | SessionSource::VSCode)
|
||||
}
|
||||
|
||||
/// Structured output contract that the guardian reviewer must satisfy.
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
|
||||
pub(crate) struct GuardianAssessment {
|
||||
|
||||
@@ -57,9 +57,8 @@ const GUARDIAN_REJECTION_INSTRUCTIONS: &str = concat!(
|
||||
);
|
||||
|
||||
const GUARDIAN_TIMEOUT_INSTRUCTIONS: &str = concat!(
|
||||
"The automatic permission approval review did not finish before its deadline. ",
|
||||
"Do not assume the action is unsafe based on the timeout alone. ",
|
||||
"You may retry once, or ask the user for guidance or explicit approval.",
|
||||
"Automatic approval review timed out. ",
|
||||
"Please review this request manually.",
|
||||
);
|
||||
|
||||
const GUARDIAN_REVIEW_MAX_ATTEMPTS: i64 = 3;
|
||||
|
||||
@@ -1125,8 +1125,8 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() {
|
||||
#[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("timed out"));
|
||||
assert!(message.contains("review this request manually"));
|
||||
assert!(!message.contains("unacceptable risk"));
|
||||
}
|
||||
|
||||
|
||||
@@ -402,6 +402,7 @@ pub(crate) struct CodexSpawnArgs {
|
||||
pub(crate) extensions: Arc<codex_extension_api::ExtensionRegistry<crate::config::Config>>,
|
||||
pub(crate) conversation_history: InitialHistory,
|
||||
pub(crate) session_source: SessionSource,
|
||||
pub(crate) client_session_source: SessionSource,
|
||||
pub(crate) forked_from_thread_id: Option<ThreadId>,
|
||||
pub(crate) parent_thread_id: Option<ThreadId>,
|
||||
pub(crate) thread_source: Option<ThreadSource>,
|
||||
@@ -487,6 +488,7 @@ impl Codex {
|
||||
extensions,
|
||||
conversation_history,
|
||||
session_source,
|
||||
client_session_source,
|
||||
forked_from_thread_id,
|
||||
parent_thread_id,
|
||||
thread_source,
|
||||
@@ -616,6 +618,10 @@ impl Codex {
|
||||
metrics_service_name,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled:
|
||||
crate::guardian::can_fallback_to_manual_after_guardian_timeout(
|
||||
&client_session_source,
|
||||
),
|
||||
session_source,
|
||||
forked_from_thread_id,
|
||||
parent_thread_id,
|
||||
|
||||
@@ -120,6 +120,7 @@ pub(super) async fn spawn_review_thread(
|
||||
reasoning_effort,
|
||||
reasoning_summary,
|
||||
session_source,
|
||||
manual_approval_fallback_enabled: parent_turn_context.manual_approval_fallback_enabled,
|
||||
parent_thread_id: parent_turn_context.parent_thread_id,
|
||||
thread_source: parent_turn_context.thread_source.clone(),
|
||||
environments: parent_turn_context.environments.clone(),
|
||||
|
||||
@@ -96,6 +96,8 @@ pub(crate) struct SessionConfiguration {
|
||||
pub(super) app_server_client_version: Option<String>,
|
||||
/// Source of the session (cli, vscode, exec, mcp, ...)
|
||||
pub(super) session_source: SessionSource,
|
||||
/// Whether the currently attached client can answer manual approval prompts.
|
||||
pub(super) manual_approval_fallback_enabled: bool,
|
||||
/// Immediate history source copied into this thread, when this thread was forked.
|
||||
pub(super) forked_from_thread_id: Option<ThreadId>,
|
||||
/// Immediate control/spawn parent for this thread, when it has one.
|
||||
|
||||
@@ -3190,6 +3190,7 @@ async fn set_rate_limits_retains_previous_credits() {
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled: false,
|
||||
session_source: SessionSource::Exec,
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
@@ -3297,6 +3298,7 @@ async fn set_rate_limits_updates_plan_type_when_present() {
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled: false,
|
||||
session_source: SessionSource::Exec,
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
@@ -3829,6 +3831,7 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled: false,
|
||||
session_source: SessionSource::Exec,
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
@@ -4681,6 +4684,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() {
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled: false,
|
||||
session_source: SessionSource::Exec,
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
@@ -4789,6 +4793,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled: false,
|
||||
session_source: SessionSource::Exec,
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
@@ -5021,6 +5026,7 @@ async fn make_session_with_config_and_rx(
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled: false,
|
||||
session_source: SessionSource::Exec,
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
@@ -5123,6 +5129,7 @@ async fn make_session_with_history_source_and_agent_control_and_rx(
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled: false,
|
||||
session_source: session_source.clone(),
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
@@ -6867,6 +6874,7 @@ where
|
||||
metrics_service_name: None,
|
||||
app_server_client_name: None,
|
||||
app_server_client_version: None,
|
||||
manual_approval_fallback_enabled: false,
|
||||
session_source: SessionSource::Exec,
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
|
||||
@@ -717,6 +717,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() {
|
||||
session_source: SessionSource::SubAgent(SubAgentSource::Other(
|
||||
GUARDIAN_REVIEWER_NAME.to_string(),
|
||||
)),
|
||||
client_session_source: SessionSource::Exec,
|
||||
forked_from_thread_id: None,
|
||||
parent_thread_id: None,
|
||||
thread_source: None,
|
||||
|
||||
@@ -67,6 +67,7 @@ pub struct TurnContext {
|
||||
pub(crate) reasoning_effort: Option<ReasoningEffortConfig>,
|
||||
pub(crate) reasoning_summary: ReasoningSummaryConfig,
|
||||
pub(crate) session_source: SessionSource,
|
||||
pub(crate) manual_approval_fallback_enabled: bool,
|
||||
pub(crate) parent_thread_id: Option<ThreadId>,
|
||||
pub(crate) thread_source: Option<ThreadSource>,
|
||||
pub(crate) environments: ResolvedTurnEnvironments,
|
||||
@@ -238,6 +239,7 @@ impl TurnContext {
|
||||
reasoning_effort,
|
||||
reasoning_summary: self.reasoning_summary,
|
||||
session_source: self.session_source.clone(),
|
||||
manual_approval_fallback_enabled: self.manual_approval_fallback_enabled,
|
||||
parent_thread_id: self.parent_thread_id,
|
||||
thread_source: self.thread_source.clone(),
|
||||
environments: self.environments.clone(),
|
||||
@@ -537,6 +539,8 @@ impl Session {
|
||||
reasoning_effort,
|
||||
reasoning_summary,
|
||||
session_source,
|
||||
manual_approval_fallback_enabled: session_configuration
|
||||
.manual_approval_fallback_enabled,
|
||||
parent_thread_id: session_configuration.parent_thread_id,
|
||||
thread_source: session_configuration.thread_source.clone(),
|
||||
environments,
|
||||
|
||||
@@ -1331,6 +1331,7 @@ impl ThreadManagerState {
|
||||
mcp_manager: Arc::clone(&self.mcp_manager),
|
||||
extensions: Arc::clone(&self.extensions),
|
||||
conversation_history: initial_history,
|
||||
client_session_source: self.session_source.clone(),
|
||||
session_source,
|
||||
forked_from_thread_id,
|
||||
parent_thread_id,
|
||||
|
||||
@@ -50,6 +50,16 @@ pub(crate) struct OrchestratorRunResult<Out> {
|
||||
pub deferred_network_approval: Option<DeferredNetworkApproval>,
|
||||
}
|
||||
|
||||
struct ApprovalDecision {
|
||||
decision: ReviewDecision,
|
||||
guardian_review_id: Option<String>,
|
||||
}
|
||||
|
||||
struct ApprovalRequestOptions {
|
||||
evaluate_permission_request_hooks: bool,
|
||||
manual_fallback_for_guardian_timeout: bool,
|
||||
}
|
||||
|
||||
impl ToolOrchestrator {
|
||||
pub fn new() -> Self {
|
||||
Self {
|
||||
@@ -144,6 +154,7 @@ impl ToolOrchestrator {
|
||||
let otel_ci = &tool_ctx.call_id;
|
||||
let strict_auto_review = tool_ctx.session.strict_auto_review_enabled_for_turn().await;
|
||||
let use_guardian = routes_approval_to_guardian(turn_ctx) || strict_auto_review;
|
||||
let manual_fallback_for_guardian_timeout = turn_ctx.manual_approval_fallback_enabled;
|
||||
|
||||
// 1) Approval
|
||||
let mut already_approved = false;
|
||||
@@ -165,18 +176,25 @@ impl ToolOrchestrator {
|
||||
retry_reason: None,
|
||||
network_approval_context: None,
|
||||
};
|
||||
let decision = Self::request_approval(
|
||||
let approval_decision = Self::request_approval_with_manual_fallback(
|
||||
tool,
|
||||
req,
|
||||
tool_ctx.call_id.as_str(),
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ false,
|
||||
ApprovalRequestOptions {
|
||||
evaluate_permission_request_hooks: false,
|
||||
manual_fallback_for_guardian_timeout,
|
||||
},
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
Self::reject_if_not_approved(
|
||||
tool_ctx,
|
||||
approval_decision.guardian_review_id.as_deref(),
|
||||
approval_decision.decision,
|
||||
)
|
||||
.await?;
|
||||
already_approved = true;
|
||||
} else {
|
||||
otel.tool_decision(
|
||||
@@ -200,19 +218,26 @@ impl ToolOrchestrator {
|
||||
retry_reason: reason.clone(),
|
||||
network_approval_context: None,
|
||||
};
|
||||
let decision = Self::request_approval(
|
||||
let approval_decision = Self::request_approval_with_manual_fallback(
|
||||
tool,
|
||||
req,
|
||||
tool_ctx.call_id.as_str(),
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ !strict_auto_review,
|
||||
ApprovalRequestOptions {
|
||||
evaluate_permission_request_hooks: !strict_auto_review,
|
||||
manual_fallback_for_guardian_timeout,
|
||||
},
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
Self::reject_if_not_approved(
|
||||
tool_ctx,
|
||||
approval_decision.guardian_review_id.as_deref(),
|
||||
approval_decision.decision,
|
||||
)
|
||||
.await?;
|
||||
already_approved = true;
|
||||
}
|
||||
}
|
||||
@@ -382,19 +407,26 @@ impl ToolOrchestrator {
|
||||
};
|
||||
|
||||
let permission_request_run_id = format!("{}:retry", tool_ctx.call_id);
|
||||
let decision = Self::request_approval(
|
||||
let approval_decision = Self::request_approval_with_manual_fallback(
|
||||
tool,
|
||||
req,
|
||||
&permission_request_run_id,
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ !strict_auto_review,
|
||||
ApprovalRequestOptions {
|
||||
evaluate_permission_request_hooks: !strict_auto_review,
|
||||
manual_fallback_for_guardian_timeout,
|
||||
},
|
||||
&otel,
|
||||
)
|
||||
.await?;
|
||||
|
||||
Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision)
|
||||
.await?;
|
||||
Self::reject_if_not_approved(
|
||||
tool_ctx,
|
||||
approval_decision.guardian_review_id.as_deref(),
|
||||
approval_decision.decision,
|
||||
)
|
||||
.await?;
|
||||
}
|
||||
|
||||
let retry_sandbox = if unsandboxed_allowed {
|
||||
@@ -546,6 +578,68 @@ impl ToolOrchestrator {
|
||||
Ok(decision)
|
||||
}
|
||||
|
||||
async fn request_approval_with_manual_fallback<Rq, Out, T>(
|
||||
tool: &mut T,
|
||||
req: &Rq,
|
||||
permission_request_run_id: &str,
|
||||
approval_ctx: ApprovalCtx<'_>,
|
||||
tool_ctx: &ToolCtx,
|
||||
options: ApprovalRequestOptions,
|
||||
otel: &codex_otel::SessionTelemetry,
|
||||
) -> Result<ApprovalDecision, ToolError>
|
||||
where
|
||||
T: ToolRuntime<Rq, Out>,
|
||||
{
|
||||
let guardian_review_id = approval_ctx.guardian_review_id.clone();
|
||||
let session = approval_ctx.session;
|
||||
let turn = approval_ctx.turn;
|
||||
let call_id = approval_ctx.call_id;
|
||||
let network_approval_context = approval_ctx.network_approval_context.clone();
|
||||
let decision = Self::request_approval(
|
||||
tool,
|
||||
req,
|
||||
permission_request_run_id,
|
||||
approval_ctx,
|
||||
tool_ctx,
|
||||
options.evaluate_permission_request_hooks,
|
||||
otel,
|
||||
)
|
||||
.await?;
|
||||
|
||||
if !options.manual_fallback_for_guardian_timeout
|
||||
|| !matches!(decision, ReviewDecision::TimedOut)
|
||||
|| guardian_review_id.is_none()
|
||||
{
|
||||
return Ok(ApprovalDecision {
|
||||
decision,
|
||||
guardian_review_id,
|
||||
});
|
||||
}
|
||||
|
||||
let fallback_ctx = ApprovalCtx {
|
||||
session,
|
||||
turn,
|
||||
call_id,
|
||||
guardian_review_id: None,
|
||||
retry_reason: Some(guardian_timeout_message()),
|
||||
network_approval_context,
|
||||
};
|
||||
let decision = Self::request_approval(
|
||||
tool,
|
||||
req,
|
||||
permission_request_run_id,
|
||||
fallback_ctx,
|
||||
tool_ctx,
|
||||
/*evaluate_permission_request_hooks*/ false,
|
||||
otel,
|
||||
)
|
||||
.await?;
|
||||
Ok(ApprovalDecision {
|
||||
decision,
|
||||
guardian_review_id: None,
|
||||
})
|
||||
}
|
||||
|
||||
async fn reject_if_not_approved(
|
||||
tool_ctx: &ToolCtx,
|
||||
guardian_review_id: Option<&str>,
|
||||
@@ -590,3 +684,7 @@ fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String {
|
||||
// output so we can evolve heuristics later without touching call sites.
|
||||
"command failed; retry without sandbox?".to_string()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "orchestrator_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -0,0 +1,175 @@
|
||||
use super::*;
|
||||
|
||||
use crate::tools::sandboxing::Approvable;
|
||||
use crate::tools::sandboxing::Sandboxable;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_sandboxing::SandboxablePreference;
|
||||
use futures::future::BoxFuture;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::sync::Arc;
|
||||
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
struct ApprovalCall {
|
||||
used_guardian: bool,
|
||||
retry_reason: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
struct TimeoutThenManualRuntime {
|
||||
calls: Vec<ApprovalCall>,
|
||||
}
|
||||
|
||||
impl Approvable<()> for TimeoutThenManualRuntime {
|
||||
type ApprovalKey = ();
|
||||
|
||||
fn approval_keys(&self, _req: &()) -> Vec<Self::ApprovalKey> {
|
||||
Vec::new()
|
||||
}
|
||||
|
||||
fn start_approval_async<'a>(
|
||||
&'a mut self,
|
||||
_req: &'a (),
|
||||
ctx: ApprovalCtx<'a>,
|
||||
) -> BoxFuture<'a, ReviewDecision> {
|
||||
Box::pin(async move {
|
||||
let used_guardian = ctx.guardian_review_id.is_some();
|
||||
self.calls.push(ApprovalCall {
|
||||
used_guardian,
|
||||
retry_reason: ctx.retry_reason.clone(),
|
||||
});
|
||||
if used_guardian {
|
||||
ReviewDecision::TimedOut
|
||||
} else {
|
||||
ReviewDecision::Approved
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
impl Sandboxable for TimeoutThenManualRuntime {
|
||||
fn sandbox_preference(&self) -> SandboxablePreference {
|
||||
SandboxablePreference::Auto
|
||||
}
|
||||
}
|
||||
|
||||
impl ToolRuntime<(), ()> for TimeoutThenManualRuntime {
|
||||
async fn run(
|
||||
&mut self,
|
||||
_req: &(),
|
||||
_attempt: &SandboxAttempt<'_>,
|
||||
_ctx: &ToolCtx,
|
||||
) -> Result<(), ToolError> {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_timeout_falls_back_to_manual_approval() {
|
||||
let (session, mut turn) = crate::session::tests::make_session_and_context().await;
|
||||
turn.session_source = SessionSource::Cli;
|
||||
let session = Arc::new(session);
|
||||
let turn = Arc::new(turn);
|
||||
let tool_ctx = ToolCtx {
|
||||
session: Arc::clone(&session),
|
||||
turn: Arc::clone(&turn),
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: codex_tools::ToolName::plain("exec_command"),
|
||||
};
|
||||
let approval_ctx = ApprovalCtx {
|
||||
session: &session,
|
||||
turn: &turn,
|
||||
call_id: "call-1",
|
||||
guardian_review_id: Some("guardian-review-1".to_string()),
|
||||
retry_reason: None,
|
||||
network_approval_context: None,
|
||||
};
|
||||
let mut runtime = TimeoutThenManualRuntime::default();
|
||||
|
||||
let approval_decision = ToolOrchestrator::request_approval_with_manual_fallback(
|
||||
&mut runtime,
|
||||
&(),
|
||||
"permission-request-1",
|
||||
approval_ctx,
|
||||
&tool_ctx,
|
||||
super::ApprovalRequestOptions {
|
||||
evaluate_permission_request_hooks: false,
|
||||
manual_fallback_for_guardian_timeout: true,
|
||||
},
|
||||
&turn.session_telemetry,
|
||||
)
|
||||
.await
|
||||
.expect("approval request should succeed");
|
||||
|
||||
assert_eq!(
|
||||
approval_decision.decision,
|
||||
ReviewDecision::Approved,
|
||||
"manual fallback approval should become the terminal decision"
|
||||
);
|
||||
assert_eq!(approval_decision.guardian_review_id, None);
|
||||
assert_eq!(
|
||||
runtime.calls,
|
||||
vec![
|
||||
ApprovalCall {
|
||||
used_guardian: true,
|
||||
retry_reason: None,
|
||||
},
|
||||
ApprovalCall {
|
||||
used_guardian: false,
|
||||
retry_reason: Some(guardian_timeout_message()),
|
||||
},
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_timeout_stays_terminal_when_manual_fallback_is_disabled() {
|
||||
let (session, mut turn) = crate::session::tests::make_session_and_context().await;
|
||||
turn.session_source = SessionSource::Cli;
|
||||
turn.manual_approval_fallback_enabled = false;
|
||||
let session = Arc::new(session);
|
||||
let turn = Arc::new(turn);
|
||||
let tool_ctx = ToolCtx {
|
||||
session: Arc::clone(&session),
|
||||
turn: Arc::clone(&turn),
|
||||
call_id: "call-1".to_string(),
|
||||
tool_name: codex_tools::ToolName::plain("exec_command"),
|
||||
};
|
||||
let approval_ctx = ApprovalCtx {
|
||||
session: &session,
|
||||
turn: &turn,
|
||||
call_id: "call-1",
|
||||
guardian_review_id: Some("guardian-review-1".to_string()),
|
||||
retry_reason: None,
|
||||
network_approval_context: None,
|
||||
};
|
||||
let mut runtime = TimeoutThenManualRuntime::default();
|
||||
|
||||
let approval_decision = ToolOrchestrator::request_approval_with_manual_fallback(
|
||||
&mut runtime,
|
||||
&(),
|
||||
"permission-request-1",
|
||||
approval_ctx,
|
||||
&tool_ctx,
|
||||
super::ApprovalRequestOptions {
|
||||
evaluate_permission_request_hooks: false,
|
||||
manual_fallback_for_guardian_timeout: false,
|
||||
},
|
||||
&turn.session_telemetry,
|
||||
)
|
||||
.await
|
||||
.expect("approval request should succeed");
|
||||
|
||||
assert_eq!(approval_decision.decision, ReviewDecision::TimedOut);
|
||||
assert_eq!(
|
||||
approval_decision.guardian_review_id,
|
||||
Some("guardian-review-1".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
runtime.calls,
|
||||
vec![ApprovalCall {
|
||||
used_guardian: true,
|
||||
retry_reason: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
@@ -457,7 +457,7 @@ impl CoreShellActionProvider {
|
||||
}
|
||||
|
||||
// 2) Route to Guardian if configured
|
||||
if let Some(review_id) = guardian_review_id.clone() {
|
||||
let manual_prompt_reason = if let Some(review_id) = guardian_review_id.clone() {
|
||||
let decision = review_approval_request(
|
||||
&session,
|
||||
&turn,
|
||||
@@ -468,17 +468,24 @@ impl CoreShellActionProvider {
|
||||
program: program.to_string_lossy().into_owned(),
|
||||
argv: argv.to_vec(),
|
||||
cwd: workdir.clone(),
|
||||
additional_permissions,
|
||||
additional_permissions: additional_permissions.clone(),
|
||||
},
|
||||
/*retry_reason*/ None,
|
||||
)
|
||||
.await;
|
||||
return PromptDecision {
|
||||
decision,
|
||||
guardian_review_id,
|
||||
rejection_message: None,
|
||||
};
|
||||
}
|
||||
if !matches!(decision, ReviewDecision::TimedOut)
|
||||
|| !turn.manual_approval_fallback_enabled
|
||||
{
|
||||
return PromptDecision {
|
||||
decision,
|
||||
guardian_review_id,
|
||||
rejection_message: None,
|
||||
};
|
||||
}
|
||||
Some(guardian_timeout_message())
|
||||
} else {
|
||||
None
|
||||
};
|
||||
|
||||
// 3) Fall back to regular user prompt
|
||||
let decision = session
|
||||
@@ -488,7 +495,7 @@ impl CoreShellActionProvider {
|
||||
approval_id,
|
||||
command,
|
||||
workdir.clone(),
|
||||
/*reason*/ None,
|
||||
manual_prompt_reason,
|
||||
/*network_approval_context*/ None,
|
||||
/*proposed_execpolicy_amendment*/ None,
|
||||
additional_permissions,
|
||||
|
||||
@@ -8,6 +8,7 @@ use super::extract_shell_script;
|
||||
use super::join_program_and_argv;
|
||||
use super::map_exec_result;
|
||||
use crate::config::Constrained;
|
||||
use crate::guardian::can_fallback_to_manual_after_guardian_timeout;
|
||||
use crate::sandboxing::SandboxPermissions;
|
||||
use crate::session::tests::make_session_and_context;
|
||||
use anyhow::Context;
|
||||
@@ -32,6 +33,7 @@ use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::GranularApprovalConfig;
|
||||
use codex_protocol::protocol::GuardianCommandSource;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_sandboxing::SandboxType;
|
||||
use codex_sandboxing::policy_transforms::effective_permission_profile;
|
||||
use codex_shell_escalation::EscalationExecution;
|
||||
@@ -128,6 +130,19 @@ fn execve_prompt_rejection_keeps_unmatched_commands_on_sandbox_flag() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn execve_guardian_timeout_manual_fallback_is_limited_to_interactive_sources() {
|
||||
assert!(can_fallback_to_manual_after_guardian_timeout(
|
||||
&SessionSource::Cli
|
||||
));
|
||||
assert!(can_fallback_to_manual_after_guardian_timeout(
|
||||
&SessionSource::VSCode
|
||||
));
|
||||
assert!(!can_fallback_to_manual_after_guardian_timeout(
|
||||
&SessionSource::Exec
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_sandbox_permissions_only_downgrades_preapproved_additional_permissions() {
|
||||
assert_eq!(
|
||||
|
||||
@@ -231,6 +231,7 @@ pub struct TestCodexBuilder {
|
||||
user_shell_override: Option<Shell>,
|
||||
exec_server_url: Option<String>,
|
||||
extensions: Arc<ExtensionRegistry<Config>>,
|
||||
session_source: SessionSource,
|
||||
}
|
||||
|
||||
impl TestCodexBuilder {
|
||||
@@ -320,6 +321,11 @@ impl TestCodexBuilder {
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_session_source(mut self, session_source: SessionSource) -> Self {
|
||||
self.session_source = session_source;
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_windows_cmd_shell(self) -> Self {
|
||||
if cfg!(windows) {
|
||||
self.with_user_shell(get_shell_by_model_provided_path(&PathBuf::from("cmd.exe")))
|
||||
@@ -511,7 +517,7 @@ impl TestCodexBuilder {
|
||||
let thread_manager = ThreadManager::new(
|
||||
&config,
|
||||
codex_core::test_support::auth_manager_from_auth(auth.clone()),
|
||||
SessionSource::Exec,
|
||||
self.session_source.clone(),
|
||||
Arc::clone(&environment_manager),
|
||||
Arc::clone(&self.extensions),
|
||||
/*analytics_events_client*/ None,
|
||||
@@ -1084,6 +1090,7 @@ pub fn test_codex() -> TestCodexBuilder {
|
||||
user_shell_override: None,
|
||||
exec_server_url: None,
|
||||
extensions: empty_extension_registry(),
|
||||
session_source: SessionSource::Exec,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -7,7 +7,9 @@ use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::fs_wait;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
@@ -19,6 +21,8 @@ use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::skip_if_sandbox;
|
||||
use core_test_support::streaming_sse::StreamingSseChunk;
|
||||
use core_test_support::streaming_sse::start_streaming_sse_server;
|
||||
use core_test_support::test_codex::local_selections;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::wait_for_event;
|
||||
@@ -29,6 +33,143 @@ use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use std::time::Duration;
|
||||
use tempfile::TempDir;
|
||||
use tokio::sync::oneshot;
|
||||
|
||||
fn parent_tool_response(call_id: &str, tool_args: &Value) -> String {
|
||||
let serialized_tool_args = serde_json::to_string(tool_args).unwrap_or_else(|err| {
|
||||
panic!("serialize tool args failed: {err}");
|
||||
});
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-tool"),
|
||||
ev_function_call(call_id, "exec_command", &serialized_tool_args),
|
||||
ev_completed("resp-parent-tool"),
|
||||
])
|
||||
}
|
||||
|
||||
fn parent_done_response() -> String {
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-done"),
|
||||
ev_assistant_message("msg-parent-done", "done"),
|
||||
ev_completed("resp-parent-done"),
|
||||
])
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn guardian_timeout_falls_back_to_manual_approval_end_to_end() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let sandbox_policy = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
};
|
||||
let sandbox_policy_for_config = sandbox_policy.clone();
|
||||
let mut builder = test_codex()
|
||||
.with_session_source(SessionSource::Cli)
|
||||
.with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
|
||||
config
|
||||
.set_legacy_sandbox_policy(sandbox_policy_for_config)
|
||||
.expect("set sandbox policy");
|
||||
});
|
||||
|
||||
let output_dir = TempDir::new()?;
|
||||
let output_file = output_dir.path().join("guardian-timeout-fallback.txt");
|
||||
let command = format!("printf guardian-manual > {}", output_file.display());
|
||||
let tool_args = json!({
|
||||
"cmd": command,
|
||||
"yield_time_ms": 1_000_u64,
|
||||
"sandbox_permissions": SandboxPermissions::RequireEscalated,
|
||||
"justification": "Exercise Guardian timeout fallback.",
|
||||
});
|
||||
let (_guardian_gate_tx, guardian_gate_rx) = oneshot::channel();
|
||||
let (server, _completions) = start_streaming_sse_server(vec![
|
||||
vec![StreamingSseChunk {
|
||||
gate: None,
|
||||
body: parent_tool_response("exec-call", &tool_args),
|
||||
}],
|
||||
vec![
|
||||
StreamingSseChunk {
|
||||
gate: None,
|
||||
body: sse(vec![ev_response_created("resp-guardian-timeout")]),
|
||||
},
|
||||
StreamingSseChunk {
|
||||
gate: Some(guardian_gate_rx),
|
||||
body: sse(vec![ev_completed("resp-guardian-timeout")]),
|
||||
},
|
||||
],
|
||||
vec![StreamingSseChunk {
|
||||
gate: None,
|
||||
body: parent_done_response(),
|
||||
}],
|
||||
])
|
||||
.await;
|
||||
let test = builder.build_with_streaming_server(&server).await?;
|
||||
|
||||
test.codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: "run a command that requires Guardian review".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: codex_protocol::protocol::ThreadSettingsOverrides {
|
||||
environments: Some(local_selections(test.config.cwd.clone())),
|
||||
approval_policy: Some(approval_policy),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
sandbox_policy: Some(sandbox_policy),
|
||||
..Default::default()
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
server.wait_for_request_count(/*count*/ 2).await;
|
||||
tokio::time::pause();
|
||||
tokio::time::advance(Duration::from_secs(91)).await;
|
||||
tokio::time::resume();
|
||||
|
||||
let approval = loop {
|
||||
match wait_for_event(&test.codex, |_| true).await {
|
||||
EventMsg::ExecApprovalRequest(approval) => break approval,
|
||||
EventMsg::TurnComplete(_) => {
|
||||
panic!("expected manual approval request before completion")
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
};
|
||||
assert_eq!(approval.effective_approval_id(), "exec-call");
|
||||
assert!(
|
||||
approval
|
||||
.reason
|
||||
.as_deref()
|
||||
.is_some_and(|reason| reason.contains("did not finish before its deadline"))
|
||||
);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
.await?;
|
||||
|
||||
loop {
|
||||
if matches!(
|
||||
wait_for_event(&test.codex, |_| true).await,
|
||||
EventMsg::TurnComplete(_)
|
||||
) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert_eq!(fs::read_to_string(&output_file)?, "guardian-manual");
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn guardian_review_session_does_not_inherit_legacy_notify() -> Result<()> {
|
||||
|
||||
Reference in New Issue
Block a user