diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 5b68ecf917..936dc0e30f 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -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), diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index b06a2daad3..8cd31f331a 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -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 { diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 99f9c5d999..02b4b04138 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -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; diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 8f6e01298c..8a1382c7bf 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -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")); } diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index ddb31a9a52..e249af8e49 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -402,6 +402,7 @@ pub(crate) struct CodexSpawnArgs { pub(crate) extensions: Arc>, pub(crate) conversation_history: InitialHistory, pub(crate) session_source: SessionSource, + pub(crate) client_session_source: SessionSource, pub(crate) forked_from_thread_id: Option, pub(crate) parent_thread_id: Option, pub(crate) thread_source: Option, @@ -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, diff --git a/codex-rs/core/src/session/review.rs b/codex-rs/core/src/session/review.rs index 00a89b2b5c..49be5501d7 100644 --- a/codex-rs/core/src/session/review.rs +++ b/codex-rs/core/src/session/review.rs @@ -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(), diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index e42cf202e8..b63b1a89e5 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -96,6 +96,8 @@ pub(crate) struct SessionConfiguration { pub(super) app_server_client_version: Option, /// 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, /// Immediate control/spawn parent for this thread, when it has one. diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 29de7ca662..5ffb277ddb 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -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, diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index c73add2920..2219422ec0 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -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, diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index 10eff3f3b4..207e76c28b 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -67,6 +67,7 @@ pub struct TurnContext { pub(crate) reasoning_effort: Option, pub(crate) reasoning_summary: ReasoningSummaryConfig, pub(crate) session_source: SessionSource, + pub(crate) manual_approval_fallback_enabled: bool, pub(crate) parent_thread_id: Option, pub(crate) thread_source: Option, 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, diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index 5758c0cadf..078b738b15 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -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, diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index 77cbffde32..d736de0aba 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -50,6 +50,16 @@ pub(crate) struct OrchestratorRunResult { pub deferred_network_approval: Option, } +struct ApprovalDecision { + decision: ReviewDecision, + guardian_review_id: Option, +} + +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( + tool: &mut T, + req: &Rq, + permission_request_run_id: &str, + approval_ctx: ApprovalCtx<'_>, + tool_ctx: &ToolCtx, + options: ApprovalRequestOptions, + otel: &codex_otel::SessionTelemetry, + ) -> Result + where + T: ToolRuntime, + { + 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; diff --git a/codex-rs/core/src/tools/orchestrator_tests.rs b/codex-rs/core/src/tools/orchestrator_tests.rs new file mode 100644 index 0000000000..e03ff1aeed --- /dev/null +++ b/codex-rs/core/src/tools/orchestrator_tests.rs @@ -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, +} + +#[derive(Default)] +struct TimeoutThenManualRuntime { + calls: Vec, +} + +impl Approvable<()> for TimeoutThenManualRuntime { + type ApprovalKey = (); + + fn approval_keys(&self, _req: &()) -> Vec { + 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, + }] + ); +} diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 71fa89f980..4958b517e6 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -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, diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index ee5b9e7db3..9e2ff935cf 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -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!( diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index 62f422ba1c..85f60203f8 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -231,6 +231,7 @@ pub struct TestCodexBuilder { user_shell_override: Option, exec_server_url: Option, extensions: Arc>, + 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, } } diff --git a/codex-rs/core/tests/suite/guardian_review.rs b/codex-rs/core/tests/suite/guardian_review.rs index ae87350c39..8f5d7e1adb 100644 --- a/codex-rs/core/tests/suite/guardian_review.rs +++ b/codex-rs/core/tests/suite/guardian_review.rs @@ -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<()> {