From 3cc3763e6c0473d6f6666524709528fdcdb7d9c1 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 22 Apr 2026 17:48:13 -0700 Subject: [PATCH 1/4] core: box multi-agent wrapper futures (#19059) ## Why While debugging the Windows stack overflows we saw in [#13429](https://github.com/openai/codex/pull/13429) and then again in [#18893](https://github.com/openai/codex/pull/18893), I hit another overflow in `tools::handlers::multi_agents::tests::tool_handlers_cascade_close_and_resume_and_keep_explicitly_closed_subtrees_closed`. That test drives the legacy multi-agent spawn / close / resume path. The behavior was fine, but several thin async wrappers were still inlining much larger `AgentControl` futures into their callers, which was enough to overflow the default Windows stack. ## What - Box the thin `AgentControl` wrappers around `spawn_agent_internal`, `resume_single_agent_from_rollout`, and `shutdown_agent_tree`. - Box the corresponding legacy `multi_agents` handler calls in `spawn`, `resume_agent`, and `close_agent`. - Keep behavior unchanged while reducing future size on this call path so the Windows test no longer overflows its stack. ## Testing - `cargo test -p codex-core --lib tools::handlers::multi_agents::tests::tool_handlers_cascade_close_and_resume_and_keep_explicitly_closed_subtrees_closed -- --exact --nocapture` - `cargo test -p codex-core` (this still hit unrelated local integration-test failures because `codex.exe` / `test_stdio_server.exe` were not present in this shell; the relevant unit tests passed) --- codex-rs/core/src/agent/control.rs | 30 +++++++------- .../handlers/multi_agents/close_agent.rs | 5 +-- .../handlers/multi_agents/resume_agent.rs | 40 ++++++++++--------- .../src/tools/handlers/multi_agents/spawn.rs | 37 ++++++++--------- 4 files changed, 56 insertions(+), 56 deletions(-) diff --git a/codex-rs/core/src/agent/control.rs b/codex-rs/core/src/agent/control.rs index 1c30d613e0..c54d0663ba 100644 --- a/codex-rs/core/src/agent/control.rs +++ b/codex-rs/core/src/agent/control.rs @@ -163,15 +163,14 @@ impl AgentControl { initial_operation: Op, session_source: Option, ) -> CodexResult { - Ok(self - .spawn_agent_internal( - config, - initial_operation, - session_source, - SpawnAgentOptions::default(), - ) - .await? - .thread_id) + let spawned_agent = Box::pin(self.spawn_agent_internal( + config, + initial_operation, + session_source, + SpawnAgentOptions::default(), + )) + .await?; + Ok(spawned_agent.thread_id) } /// Spawn an agent thread with some metadata. @@ -182,7 +181,7 @@ impl AgentControl { session_source: Option, options: SpawnAgentOptions, // TODO(jif) drop with new fork. ) -> CodexResult { - self.spawn_agent_internal(config, initial_operation, session_source, options) + Box::pin(self.spawn_agent_internal(config, initial_operation, session_source, options)) .await } @@ -418,9 +417,12 @@ impl AgentControl { session_source: SessionSource, ) -> CodexResult { let root_depth = thread_spawn_depth(&session_source).unwrap_or(0); - let resumed_thread_id = self - .resume_single_agent_from_rollout(config.clone(), thread_id, session_source) - .await?; + let resumed_thread_id = Box::pin(self.resume_single_agent_from_rollout( + config.clone(), + thread_id, + session_source, + )) + .await?; let state = self.upgrade()?; let Ok(resumed_thread) = state.get_thread(resumed_thread_id).await else { return Ok(resumed_thread_id); @@ -698,7 +700,7 @@ impl AgentControl { { warn!("failed to persist thread-spawn edge status for {agent_id}: {err}"); } - self.shutdown_agent_tree(agent_id).await + Box::pin(self.shutdown_agent_tree(agent_id)).await } /// Shut down `agent_id` and any live descendants reachable from the in-memory spawn tree. diff --git a/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs b/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs index 779a85e198..8c00b0a13c 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/close_agent.rs @@ -66,10 +66,7 @@ impl ToolHandler for Handler { return Err(collab_agent_error(agent_id, err)); } }; - let result = session - .services - .agent_control - .close_agent(agent_id) + let result = Box::pin(session.services.agent_control.close_agent(agent_id)) .await .map_err(|err| collab_agent_error(agent_id, err)) .map(|_| ()); diff --git a/codex-rs/core/src/tools/handlers/multi_agents/resume_agent.rs b/codex-rs/core/src/tools/handlers/multi_agents/resume_agent.rs index 9991a90671..2d4f2c3f47 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/resume_agent.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/resume_agent.rs @@ -61,7 +61,14 @@ impl ToolHandler for Handler { .get_status(receiver_thread_id) .await; let (receiver_agent, error) = if matches!(status, AgentStatus::NotFound) { - match try_resume_closed_agent(&session, &turn, receiver_thread_id, child_depth).await { + match Box::pin(try_resume_closed_agent( + &session, + &turn, + receiver_thread_id, + child_depth, + )) + .await + { Ok(()) => { status = session .services @@ -149,21 +156,18 @@ async fn try_resume_closed_agent( child_depth: i32, ) -> Result<(), FunctionCallError> { let config = build_agent_resume_config(turn.as_ref(), child_depth)?; - session - .services - .agent_control - .resume_agent_from_rollout( - config, - receiver_thread_id, - thread_spawn_source( - session.conversation_id, - &turn.session_source, - child_depth, - /*agent_role*/ None, - /*task_name*/ None, - )?, - ) - .await - .map(|_| ()) - .map_err(|err| collab_agent_error(receiver_thread_id, err)) + Box::pin(session.services.agent_control.resume_agent_from_rollout( + config, + receiver_thread_id, + thread_spawn_source( + session.conversation_id, + &turn.session_source, + child_depth, + /*agent_role*/ None, + /*task_name*/ None, + )?, + )) + .await + .map(|_| ()) + .map_err(|err| collab_agent_error(receiver_thread_id, err)) } diff --git a/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs b/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs index 523b1ed357..f1be8951eb 100644 --- a/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs +++ b/codex-rs/core/src/tools/handlers/multi_agents/spawn.rs @@ -82,26 +82,23 @@ impl ToolHandler for Handler { apply_spawn_agent_runtime_overrides(&mut config, turn.as_ref())?; apply_spawn_agent_overrides(&mut config, child_depth); - let result = session - .services - .agent_control - .spawn_agent_with_metadata( - config, - input_items, - Some(thread_spawn_source( - session.conversation_id, - &turn.session_source, - child_depth, - role_name, - /*task_name*/ None, - )?), - SpawnAgentOptions { - fork_parent_spawn_call_id: args.fork_context.then(|| call_id.clone()), - fork_mode: args.fork_context.then_some(SpawnAgentForkMode::FullHistory), - }, - ) - .await - .map_err(collab_spawn_error); + let result = Box::pin(session.services.agent_control.spawn_agent_with_metadata( + config, + input_items, + Some(thread_spawn_source( + session.conversation_id, + &turn.session_source, + child_depth, + role_name, + /*task_name*/ None, + )?), + SpawnAgentOptions { + fork_parent_spawn_call_id: args.fork_context.then(|| call_id.clone()), + fork_mode: args.fork_context.then_some(SpawnAgentForkMode::FullHistory), + }, + )) + .await + .map_err(collab_spawn_error); let (new_thread_id, new_agent_metadata, status) = match &result { Ok(spawned_agent) => ( Some(spawned_agent.thread_id), From 8f0a92c1e512aa469debc03ff353720ce7b84216 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Wed, 22 Apr 2026 17:52:17 -0700 Subject: [PATCH 2/4] Fix relative stdio MCP cwd fallback (#19031) --- .../app-server/src/codex_message_processor.rs | 4 +- .../codex-mcp/src/mcp_connection_manager.rs | 8 ++- codex-rs/core/tests/suite/rmcp_client.rs | 63 +++++++++++++++++++ codex-rs/rmcp-client/src/program_resolver.rs | 21 ++++--- .../rmcp-client/src/stdio_server_launcher.rs | 29 ++++++--- .../tests/process_group_cleanup.rs | 2 +- codex-rs/rmcp-client/tests/resources.rs | 2 +- 7 files changed, 107 insertions(+), 22 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 1ab367d078..525e75e650 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -5802,8 +5802,8 @@ impl CodexMessageProcessor { let environment_manager = self.thread_manager.environment_manager(); let runtime_environment = match environment_manager.default_environment() { Some(environment) => { - // Status listing has no turn cwd. This fallback is used only - // by executor-backed stdio MCPs whose config omits `cwd`. + // Status listing has no turn cwd. This fallback is used by + // stdio MCPs whose config omits `cwd`. McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf()) } None => McpRuntimeEnvironment::new( diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager.rs b/codex-rs/codex-mcp/src/mcp_connection_manager.rs index fe8bc523ea..18c15cff61 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager.rs @@ -666,8 +666,8 @@ pub struct McpConnectionManager { /// servers should run for the current caller. Keep it explicit at manager /// construction time so status/snapshot paths and real sessions make the same /// local-vs-remote decision. `fallback_cwd` is not a per-server override; it is -/// used only when an executor-backed stdio server omits `cwd` and the executor -/// API still needs a concrete process working directory. +/// used when a stdio server omits `cwd` and the launcher needs a concrete +/// process working directory. #[derive(Clone)] pub struct McpRuntimeEnvironment { environment: Arc, @@ -1573,7 +1573,9 @@ async fn make_rmcp_client( runtime_environment.fallback_cwd(), )) } else { - Arc::new(LocalStdioServerLauncher) as Arc + Arc::new(LocalStdioServerLauncher::new( + runtime_environment.fallback_cwd(), + )) as Arc }; // `RmcpClient` always sees a launched MCP stdio server. The diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index cb6553343f..1243849fba 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -618,6 +618,69 @@ async fn remote_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() - Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +#[serial(mcp_cwd)] +async fn local_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -> anyhow::Result<()> +{ + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let server_name = "rmcp_local_fallback_cwd"; + let expected_cwd = Arc::new(Mutex::new(None::)); + let expected_cwd_for_config = Arc::clone(&expected_cwd); + let rmcp_test_server_bin = cargo_bin("test_stdio_server")?; + let relative_server_path = PathBuf::from("mcp-bin").join( + rmcp_test_server_bin + .file_name() + .expect("test stdio server binary should have a file name"), + ); + let relative_command = relative_server_path.to_string_lossy().into_owned(); + + let fixture = test_codex() + .with_config(move |config| { + *expected_cwd_for_config + .lock() + .expect("expected cwd lock should not be poisoned") = + Some(config.cwd.to_path_buf()); + + let target_bin = config.cwd.join(&relative_server_path).into_path_buf(); + let target_dir = target_bin + .parent() + .expect("relative test server path should include a parent"); + fs::create_dir_all(target_dir).expect("create relative MCP bin directory"); + fs::copy(&rmcp_test_server_bin, &target_bin).expect("copy test stdio server"); + + insert_mcp_server( + config, + server_name, + stdio_transport( + relative_command, + Some(HashMap::from([( + "MCP_TEST_VALUE".to_string(), + "local-fallback-cwd".to_string(), + )])), + Vec::new(), + ), + TestMcpServerOptions::default(), + ); + }) + .build(&server) + .await?; + + let expected_cwd = expected_cwd + .lock() + .expect("expected cwd lock should not be poisoned") + .clone() + .expect("test config should record runtime fallback cwd"); + let structured = + call_cwd_tool(&server, &fixture, server_name, "call-local-fallback-cwd").await?; + + assert_cwd_tool_output(&structured, &expected_cwd); + server.verify().await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/rmcp-client/src/program_resolver.rs b/codex-rs/rmcp-client/src/program_resolver.rs index ce286bdcd3..cb32ae3114 100644 --- a/codex-rs/rmcp-client/src/program_resolver.rs +++ b/codex-rs/rmcp-client/src/program_resolver.rs @@ -12,6 +12,7 @@ use std::collections::HashMap; use std::ffi::OsString; +use std::path::Path; /// Resolves a program to its executable path on Unix systems. /// @@ -19,7 +20,11 @@ use std::ffi::OsString; /// the kernel's shebang (`#!`) mechanism, so this function simply returns /// the program name unchanged. #[cfg(unix)] -pub fn resolve(program: OsString, _env: &HashMap) -> std::io::Result { +pub fn resolve( + program: OsString, + _env: &HashMap, + _cwd: &Path, +) -> std::io::Result { Ok(program) } @@ -33,16 +38,16 @@ pub fn resolve(program: OsString, _env: &HashMap) -> std::io /// This enables tools like `npx`, `pnpm`, and `yarn` to work correctly on Windows /// without requiring users to specify full paths or extensions in their configuration. #[cfg(windows)] -pub fn resolve(program: OsString, env: &HashMap) -> std::io::Result { - // Get current directory for relative path resolution - let cwd = std::env::current_dir() - .map_err(|e| std::io::Error::other(format!("Failed to get current directory: {e}")))?; - +pub fn resolve( + program: OsString, + env: &HashMap, + cwd: &Path, +) -> std::io::Result { // Extract PATH from environment for search locations let search_path = env.get(std::ffi::OsStr::new("PATH")); // Attempt resolution via which crate - match which::which_in(&program, search_path, &cwd) { + match which::which_in(&program, search_path, cwd) { Ok(resolved) => { tracing::debug!("Resolved {program:?} to {resolved:?}"); Ok(resolved.into_os_string()) @@ -119,7 +124,7 @@ mod tests { let program = OsString::from(&env.program_name); // Apply platform-specific resolution - let resolved = resolve(program, &env.mcp_env)?; + let resolved = resolve(program, &env.mcp_env, std::env::current_dir()?.as_path())?; // Verify resolved path executes successfully let mut cmd = Command::new(resolved); diff --git a/codex-rs/rmcp-client/src/stdio_server_launcher.rs b/codex-rs/rmcp-client/src/stdio_server_launcher.rs index 4e9020e369..b3d3b849d0 100644 --- a/codex-rs/rmcp-client/src/stdio_server_launcher.rs +++ b/codex-rs/rmcp-client/src/stdio_server_launcher.rs @@ -162,14 +162,27 @@ impl StdioServerCommand { /// process spawns the configured command and rmcp talks to the child's local /// stdin/stdout pipes directly. #[derive(Clone)] -pub struct LocalStdioServerLauncher; +pub struct LocalStdioServerLauncher { + fallback_cwd: PathBuf, +} + +impl LocalStdioServerLauncher { + /// Creates a local stdio launcher. + /// + /// `fallback_cwd` is used when the MCP server config omits `cwd`, so + /// relative commands resolve from the caller's runtime working directory. + pub fn new(fallback_cwd: PathBuf) -> Self { + Self { fallback_cwd } + } +} impl StdioServerLauncher for LocalStdioServerLauncher { fn launch( &self, command: StdioServerCommand, ) -> BoxFuture<'static, io::Result> { - async move { Self::launch_server(command) }.boxed() + let fallback_cwd = self.fallback_cwd.clone(); + async move { Self::launch_server(command, fallback_cwd) }.boxed() } } @@ -193,7 +206,10 @@ mod private { impl private::Sealed for LocalStdioServerLauncher {} impl LocalStdioServerLauncher { - fn launch_server(command: StdioServerCommand) -> io::Result { + fn launch_server( + command: StdioServerCommand, + fallback_cwd: PathBuf, + ) -> io::Result { let StdioServerCommand { program, args, @@ -203,22 +219,21 @@ impl LocalStdioServerLauncher { } = command; let program_name = program.to_string_lossy().into_owned(); let envs = create_env_for_mcp_server(env, &env_vars).map_err(io::Error::other)?; + let cwd = cwd.unwrap_or(fallback_cwd); let resolved_program = - program_resolver::resolve(program, &envs).map_err(io::Error::other)?; + program_resolver::resolve(program, &envs, &cwd).map_err(io::Error::other)?; let mut command = Command::new(resolved_program); command .kill_on_drop(true) .stdin(Stdio::piped()) .stdout(Stdio::piped()) + .current_dir(cwd) .env_clear() .envs(envs) .args(args); #[cfg(unix)] command.process_group(0); - if let Some(cwd) = cwd { - command.current_dir(cwd); - } let (transport, stderr) = TokioChildProcess::builder(command) .stderr(Stdio::piped()) diff --git a/codex-rs/rmcp-client/tests/process_group_cleanup.rs b/codex-rs/rmcp-client/tests/process_group_cleanup.rs index aad28ac0ac..5f742f9816 100644 --- a/codex-rs/rmcp-client/tests/process_group_cleanup.rs +++ b/codex-rs/rmcp-client/tests/process_group_cleanup.rs @@ -80,7 +80,7 @@ async fn drop_kills_wrapper_process_group() -> Result<()> { )])), &[], /*cwd*/ None, - Arc::new(LocalStdioServerLauncher), + Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)), ) .await?; diff --git a/codex-rs/rmcp-client/tests/resources.rs b/codex-rs/rmcp-client/tests/resources.rs index b23c34df13..f2e4c49911 100644 --- a/codex-rs/rmcp-client/tests/resources.rs +++ b/codex-rs/rmcp-client/tests/resources.rs @@ -63,7 +63,7 @@ async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> { /*env*/ None, &[], /*cwd*/ None, - Arc::new(LocalStdioServerLauncher), + Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)), ) .await?; From c6ab601824bfe8295646b0eef788597504e5bde3 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Wed, 22 Apr 2026 18:51:39 -0700 Subject: [PATCH 3/4] chore(auto-review) feature => stable (#19063) ## Summary Turn on Auto Review ## Testing - [x] Update unit tests --- codex-rs/core/src/config/config_tests.rs | 7 +++---- codex-rs/features/src/lib.rs | 8 ++------ codex-rs/features/src/tests.rs | 15 +++----------- .../tui/src/chatwidget/tests/permissions.rs | 4 ++-- .../chatwidget/tests/popups_and_settings.rs | 20 ++++++------------- 5 files changed, 16 insertions(+), 38 deletions(-) diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 044c6c8289..500483f659 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -3628,8 +3628,7 @@ async fn set_feature_enabled_updates_profile() -> anyhow::Result<()> { } #[tokio::test] -async fn set_feature_enabled_persists_default_false_feature_disable_in_profile() --> anyhow::Result<()> { +async fn set_feature_enabled_persists_feature_disable_in_profile() -> anyhow::Result<()> { let codex_home = TempDir::new()?; ConfigEditsBuilder::new(codex_home.path()) @@ -6881,7 +6880,7 @@ smart_approvals = true .build() .await?; - assert!(!config.features.enabled(Feature::GuardianApproval)); + assert!(config.features.enabled(Feature::GuardianApproval)); assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User); let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?; @@ -6910,7 +6909,7 @@ smart_approvals = true .build() .await?; - assert!(!config.features.enabled(Feature::GuardianApproval)); + assert!(config.features.enabled(Feature::GuardianApproval)); assert_eq!(config.approvals_reviewer, ApprovalsReviewer::User); let serialized = tokio::fs::read_to_string(codex_home.path().join(CONFIG_TOML_FILE)).await?; diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 18f3eef9e2..9b9dddfc9b 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -917,12 +917,8 @@ pub const FEATURES: &[FeatureSpec] = &[ FeatureSpec { id: Feature::GuardianApproval, key: "guardian_approval", - stage: Stage::Experimental { - name: "Auto-review", - menu_description: "When Codex needs approval for higher-risk actions (e.g. sandbox escapes or blocked network access), route eligible approval requests to a carefully-prompted security reviewer subagent rather than blocking the agent on your input. This can consume significantly more tokens because it runs a subagent on every approval request.", - announcement: "", - }, - default_enabled: false, + stage: Stage::Stable, + default_enabled: true, }, FeatureSpec { id: Feature::CollaborationModes, diff --git a/codex-rs/features/src/tests.rs b/codex-rs/features/src/tests.rs index 276e573ab1..f5704dd713 100644 --- a/codex-rs/features/src/tests.rs +++ b/codex-rs/features/src/tests.rs @@ -87,20 +87,11 @@ fn code_mode_only_requires_code_mode() { } #[test] -fn guardian_approval_is_experimental_and_user_toggleable() { +fn guardian_approval_is_stable_and_enabled_by_default() { let spec = Feature::GuardianApproval.info(); - let stage = spec.stage; - assert!(matches!(stage, Stage::Experimental { .. })); - assert_eq!(stage.experimental_menu_name(), Some("Auto-review")); - assert_eq!( - stage.experimental_menu_description().map(str::to_owned), - Some( - "When Codex needs approval for higher-risk actions (e.g. sandbox escapes or blocked network access), route eligible approval requests to a carefully-prompted security reviewer subagent rather than blocking the agent on your input. This can consume significantly more tokens because it runs a subagent on every approval request.".to_string() - ) - ); - assert_eq!(stage.experimental_announcement(), None); - assert_eq!(Feature::GuardianApproval.default_enabled(), false); + assert_eq!(spec.stage, Stage::Stable); + assert_eq!(Feature::GuardianApproval.default_enabled(), true); } #[test] diff --git a/codex-rs/tui/src/chatwidget/tests/permissions.rs b/codex-rs/tui/src/chatwidget/tests/permissions.rs index 7ea6047700..f9fcd31797 100644 --- a/codex-rs/tui/src/chatwidget/tests/permissions.rs +++ b/codex-rs/tui/src/chatwidget/tests/permissions.rs @@ -401,7 +401,7 @@ async fn permissions_selection_hides_auto_review_when_feature_disabled() { assert!( !popup.contains("Auto-review"), - "expected Auto-review to stay hidden until the experimental feature is enabled: {popup}" + "expected Auto-review to stay hidden until the feature is enabled: {popup}" ); } @@ -433,7 +433,7 @@ async fn permissions_selection_hides_auto_review_when_feature_disabled_even_if_a assert!( !popup.contains("Auto-review"), - "expected Auto-review to stay hidden when the experimental feature is disabled: {popup}" + "expected Auto-review to stay hidden when the feature is disabled: {popup}" ); } diff --git a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs index e891e43912..776741d059 100644 --- a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs +++ b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs @@ -1,5 +1,6 @@ use super::*; use codex_app_server_protocol::AppInfo; +use codex_features::Stage; use pretty_assertions::assert_eq; #[tokio::test] @@ -1747,31 +1748,22 @@ async fn experimental_popup_shows_js_repl_node_requirement() { } #[tokio::test] -async fn experimental_popup_includes_guardian_approval() { +async fn experimental_popup_omits_stable_guardian_approval() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; let guardian_stage = FEATURES .iter() .find(|spec| spec.id == Feature::GuardianApproval) .map(|spec| spec.stage) .expect("expected guardian approval feature metadata"); - let guardian_name = guardian_stage - .experimental_menu_name() - .expect("expected guardian approval experimental menu name"); - let guardian_description = guardian_stage - .experimental_menu_description() - .expect("expected guardian approval experimental description"); + + assert_eq!(guardian_stage, Stage::Stable); chat.open_experimental_popup(); let popup = render_bottom_popup(&chat, /*width*/ 120); - let normalized_popup = popup.split_whitespace().collect::>().join(" "); assert!( - popup.contains(guardian_name), - "expected auto-review entry in experimental popup, got:\n{popup}" - ); - assert!( - normalized_popup.contains(guardian_description), - "expected auto-review description in experimental popup, got:\n{popup}" + !popup.contains("Auto-review"), + "expected stable auto-review feature to be omitted from experimental popup, got:\n{popup}" ); } From 5e71da14247f4d2dc2de1c0d71f9a1cac9de1a90 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Wed, 22 Apr 2026 18:56:32 -0700 Subject: [PATCH 4/4] feat(request-permissions) approve with strict review (#19050) ## Summary Allow the user to approve a request_permissions_tool request with the condition that all commands in the rest of the turn are reviewed by guardian, regardless of sandbox status. ## Testing - [x] Added unit tests - [x] Ran locally --- .../PermissionsRequestApprovalResponse.json | 7 + .../codex_app_server_protocol.schemas.json | 7 + .../v2/PermissionsRequestApprovalResponse.ts | 6 +- .../app-server-protocol/src/protocol/v2.rs | 16 ++ .../app-server/src/bespoke_event_handling.rs | 73 +++++++++ .../tests/suite/v2/request_permissions.rs | 1 + codex-rs/core/src/codex_delegate.rs | 2 + codex-rs/core/src/codex_delegate_tests.rs | 1 + codex-rs/core/src/session/mod.rs | 35 ++++- codex-rs/core/src/session/tests.rs | 67 ++++++++ .../core/src/session/tests/guardian_tests.rs | 114 ++++++++++++++ codex-rs/core/src/state/turn.rs | 9 ++ codex-rs/core/src/tools/orchestrator.rs | 143 +++++++++-------- .../core/src/tools/runtimes/apply_patch.rs | 6 +- .../core/tests/suite/request_permissions.rs | 8 + .../tests/suite/request_permissions_tool.rs | 148 ++++++++++++------ codex-rs/protocol/src/request_permissions.rs | 3 + codex-rs/tui/src/app/app_server_requests.rs | 3 + .../tui/src/bottom_pane/approval_overlay.rs | 87 ++++++++-- ...__approval_overlay_permissions_prompt.snap | 7 +- 20 files changed, 609 insertions(+), 134 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json index 11cce8c88c..e066108e6f 100644 --- a/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json +++ b/codex-rs/app-server-protocol/schema/json/PermissionsRequestApprovalResponse.json @@ -311,6 +311,13 @@ } ], "default": "turn" + }, + "strictAutoReview": { + "description": "Review every subsequent command in this turn before normal sandboxed execution.", + "type": [ + "boolean", + "null" + ] } }, "required": [ diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index c463df699f..e16d84cadd 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -3501,6 +3501,13 @@ } ], "default": "turn" + }, + "strictAutoReview": { + "description": "Review every subsequent command in this turn before normal sandboxed execution.", + "type": [ + "boolean", + "null" + ] } }, "required": [ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/PermissionsRequestApprovalResponse.ts b/codex-rs/app-server-protocol/schema/typescript/v2/PermissionsRequestApprovalResponse.ts index 6561417b40..f42b39560c 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/PermissionsRequestApprovalResponse.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/PermissionsRequestApprovalResponse.ts @@ -4,4 +4,8 @@ import type { GrantedPermissionProfile } from "./GrantedPermissionProfile"; import type { PermissionGrantScope } from "./PermissionGrantScope"; -export type PermissionsRequestApprovalResponse = { permissions: GrantedPermissionProfile, scope: PermissionGrantScope, }; +export type PermissionsRequestApprovalResponse = { permissions: GrantedPermissionProfile, scope: PermissionGrantScope, +/** + * Review every subsequent command in this turn before normal sandboxed execution. + */ +strictAutoReview?: boolean, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 934fbc9516..1f23c7b536 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -7052,6 +7052,10 @@ pub struct PermissionsRequestApprovalResponse { pub permissions: GrantedPermissionProfile, #[serde(default)] pub scope: PermissionGrantScope, + /// Review every subsequent command in this turn before normal sandboxed execution. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub strict_auto_review: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -7801,6 +7805,18 @@ mod tests { .expect("response should deserialize"); assert_eq!(response.scope, PermissionGrantScope::Turn); + assert_eq!(response.strict_auto_review, None); + } + + #[test] + fn permissions_request_approval_response_accepts_strict_auto_review() { + let response = serde_json::from_value::(json!({ + "permissions": {}, + "strictAutoReview": true, + })) + .expect("response should deserialize"); + + assert_eq!(response.strict_auto_review, Some(true)); } #[test] diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 4730d31679..240a070edc 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -961,6 +961,7 @@ pub(crate) async fn apply_bespoke_event_handling( let empty = CoreRequestPermissionsResponse { permissions: Default::default(), scope: CorePermissionGrantScope::Turn, + strict_auto_review: false, }; if let Err(err) = conversation .submit(Op::RequestPermissionsResponse { @@ -2669,6 +2670,7 @@ fn request_permissions_response_from_client_result( return Some(CoreRequestPermissionsResponse { permissions: Default::default(), scope: CorePermissionGrantScope::Turn, + strict_auto_review: false, }); } Err(err) => { @@ -2676,6 +2678,7 @@ fn request_permissions_response_from_client_result( return Some(CoreRequestPermissionsResponse { permissions: Default::default(), scope: CorePermissionGrantScope::Turn, + strict_auto_review: false, }); } }; @@ -2686,8 +2689,23 @@ fn request_permissions_response_from_client_result( PermissionsRequestApprovalResponse { permissions: V2GrantedPermissionProfile::default(), scope: codex_app_server_protocol::PermissionGrantScope::Turn, + strict_auto_review: None, } }); + let strict_auto_review = response.strict_auto_review.unwrap_or(false); + if strict_auto_review + && matches!( + response.scope, + codex_app_server_protocol::PermissionGrantScope::Session + ) + { + error!("strict auto review is only supported for turn-scoped permission grants"); + return Some(CoreRequestPermissionsResponse { + permissions: Default::default(), + scope: CorePermissionGrantScope::Turn, + strict_auto_review: false, + }); + } let granted_permissions: CorePermissionProfile = response.permissions.into(); let permissions = if granted_permissions.is_empty() { CoreRequestPermissionProfile::default() @@ -2697,6 +2715,7 @@ fn request_permissions_response_from_client_result( Some(CoreRequestPermissionsResponse { permissions, scope: response.scope.to_core(), + strict_auto_review, }) } @@ -3858,6 +3877,7 @@ mod tests { CoreRequestPermissionsResponse { permissions: expected_permissions, scope: CorePermissionGrantScope::Turn, + strict_auto_review: false, } ); } @@ -3880,10 +3900,63 @@ mod tests { CoreRequestPermissionsResponse { permissions: CoreRequestPermissionProfile::default(), scope: CorePermissionGrantScope::Session, + strict_auto_review: false, } ); } + #[test] + fn request_permissions_response_rejects_session_scoped_strict_auto_review() { + let response = request_permissions_response_from_client_result( + CoreRequestPermissionProfile::default(), + Ok(Ok(serde_json::json!({ + "scope": "session", + "strictAutoReview": true, + "permissions": { + "network": { + "enabled": true, + }, + }, + }))), + std::env::current_dir().expect("current dir").as_path(), + ) + .expect("response should be accepted"); + + assert_eq!( + response, + CoreRequestPermissionsResponse { + permissions: CoreRequestPermissionProfile::default(), + scope: CorePermissionGrantScope::Turn, + strict_auto_review: false, + } + ); + } + + #[test] + fn request_permissions_response_preserves_turn_scoped_strict_auto_review() { + let response = request_permissions_response_from_client_result( + CoreRequestPermissionProfile { + network: Some(codex_protocol::models::NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() + }, + Ok(Ok(serde_json::json!({ + "strictAutoReview": true, + "permissions": { + "network": { + "enabled": true, + }, + }, + }))), + std::env::current_dir().expect("current dir").as_path(), + ) + .expect("response should be accepted"); + + assert_eq!(response.scope, CorePermissionGrantScope::Turn); + assert!(response.strict_auto_review); + } + #[test] fn request_permissions_response_accepts_explicit_child_grant_for_requested_cwd_scope() { let temp_dir = TempDir::new().expect("temp dir"); diff --git a/codex-rs/app-server/tests/suite/v2/request_permissions.rs b/codex-rs/app-server/tests/suite/v2/request_permissions.rs index 4f7c398f98..4a4eb9340e 100644 --- a/codex-rs/app-server/tests/suite/v2/request_permissions.rs +++ b/codex-rs/app-server/tests/suite/v2/request_permissions.rs @@ -99,6 +99,7 @@ async fn request_permissions_round_trip() -> Result<()> { }), }, scope: PermissionGrantScope::Turn, + strict_auto_review: None, })?, ) .await?; diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 9fc64f2149..631c839743 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -804,6 +804,7 @@ where let empty = RequestPermissionsResponse { permissions: Default::default(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }; parent_session .notify_request_permissions_response(call_id, empty.clone()) @@ -813,6 +814,7 @@ where response = fut => response.unwrap_or_else(|| RequestPermissionsResponse { permissions: Default::default(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }), } } diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 3ce70c513b..5f34283562 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -179,6 +179,7 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() { ..RequestPermissionProfile::default() }, scope: PermissionGrantScope::Turn, + strict_auto_review: false, }; let delegated_cwd = parent_ctx.cwd.join("delegated-cwd"); let cancel_token = CancellationToken::new(); diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index b9c7116973..d740ee7421 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -1893,6 +1893,7 @@ impl Session { return Some(RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }); } AskForApproval::Granular(granular_config) @@ -1901,6 +1902,7 @@ impl Session { return Some(RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }); } AskForApproval::OnFailure @@ -1944,11 +1946,13 @@ impl Session { RequestPermissionsResponse { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, } } ReviewDecision::ApprovedForSession => RequestPermissionsResponse { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Session, + strict_auto_review: false, }, ReviewDecision::NetworkPolicyAmendment { network_policy_amendment, @@ -1956,16 +1960,19 @@ impl Session { NetworkPolicyRuleAction::Allow => RequestPermissionsResponse { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, NetworkPolicyRuleAction::Deny => RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, }, ReviewDecision::Abort | ReviewDecision::Denied | ReviewDecision::TimedOut => { RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, } } }; @@ -2137,6 +2144,14 @@ impl Session { response: RequestPermissionsResponse, cwd: &Path, ) -> RequestPermissionsResponse { + if response.strict_auto_review && matches!(response.scope, PermissionGrantScope::Session) { + return RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + }; + } + if response.permissions.is_empty() { return response; } @@ -2149,6 +2164,7 @@ impl Session { ) .into(), scope: response.scope, + strict_auto_review: response.strict_auto_review, } } @@ -2164,7 +2180,11 @@ impl Session { PermissionGrantScope::Turn => { if let Some(turn_state) = originating_turn_state { let mut ts = turn_state.lock().await; - ts.record_granted_permissions(response.permissions.clone().into()); + let permissions: PermissionProfile = response.permissions.clone().into(); + ts.record_granted_permissions(permissions); + if response.strict_auto_review { + ts.enable_strict_auto_review(); + } } } PermissionGrantScope::Session => { @@ -2185,6 +2205,19 @@ impl Session { ts.granted_permissions() } + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn reads must stay consistent with the matching turn state" + )] + pub(crate) async fn strict_auto_review_enabled_for_turn(&self) -> bool { + let active = self.active_turn.lock().await; + let Some(active) = active.as_ref() else { + return false; + }; + let ts = active.turn_state.lock().await; + ts.strict_auto_review_enabled() + } + pub(crate) async fn granted_session_permissions(&self) -> Option { let state = self.state.lock().await; state.granted_permissions() diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 4d88fa9233..015b144d0f 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -3352,6 +3352,7 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() { ..RequestPermissionProfile::default() }, scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, ) .await; @@ -3381,6 +3382,7 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() { &codex_protocol::request_permissions::RequestPermissionsResponse { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, Some(&originating_turn_state), ) @@ -3394,6 +3396,67 @@ async fn record_granted_request_permissions_for_turn_uses_originating_turn() { assert_eq!(session.granted_turn_permissions().await, None); } +#[tokio::test] +async fn enable_strict_auto_review_for_turn_uses_originating_turn() { + let (session, _turn_context) = make_session_and_context().await; + let originating_active_turn = ActiveTurn::default(); + let originating_turn_state = Arc::clone(&originating_active_turn.turn_state); + *session.active_turn.lock().await = Some(originating_active_turn); + + let requested_permissions = RequestPermissionProfile { + network: Some(codex_protocol::models::NetworkPermissions { + enabled: Some(true), + }), + ..RequestPermissionProfile::default() + }; + session + .record_granted_request_permissions_for_turn( + &codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: requested_permissions.clone(), + scope: PermissionGrantScope::Turn, + strict_auto_review: true, + }, + Some(&originating_turn_state), + ) + .await; + + assert!( + originating_turn_state + .lock() + .await + .strict_auto_review_enabled() + ); +} + +#[test] +fn strict_auto_review_session_scope_grants_no_permissions() { + let requested_permissions = RequestPermissionProfile { + network: Some(codex_protocol::models::NetworkPermissions { + enabled: Some(true), + }), + ..RequestPermissionProfile::default() + }; + + let response = Session::normalize_request_permissions_response( + requested_permissions.clone(), + codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: requested_permissions, + scope: PermissionGrantScope::Session, + strict_auto_review: true, + }, + std::path::Path::new("/tmp"), + ); + + assert_eq!( + response, + codex_protocol::request_permissions::RequestPermissionsResponse { + permissions: RequestPermissionProfile::default(), + scope: PermissionGrantScope::Turn, + strict_auto_review: false, + } + ); +} + #[tokio::test] async fn request_permissions_emits_event_when_granular_policy_allows_requests() { let (session, mut turn_context, rx) = make_session_and_context_with_rx().await; @@ -3421,6 +3484,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests() ..RequestPermissionProfile::default() }, scope: PermissionGrantScope::Turn, + strict_auto_review: false, }; let handle = tokio::spawn({ @@ -3536,6 +3600,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec codex_protocol::request_permissions::RequestPermissionsResponse { permissions: request.permissions, scope: PermissionGrantScope::Session, + strict_auto_review: false, }, ) .await; @@ -3550,6 +3615,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec let expected_response = codex_protocol::request_permissions::RequestPermissionsResponse { permissions: expected_permissions.clone(), scope: PermissionGrantScope::Session, + strict_auto_review: false, }; let response = tokio::time::timeout(StdDuration::from_secs(1), handle) @@ -3606,6 +3672,7 @@ async fn request_permissions_is_auto_denied_when_granular_policy_blocks_tool_req codex_protocol::request_permissions::RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, } ) ); diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index 5057c43cee..22db9f3937 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -131,6 +131,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { Some(RequestPermissionsResponse { permissions: requested_permissions.clone(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }) ); assert_eq!( @@ -379,6 +380,119 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid assert!(exec_output.output.contains("hi")); } +#[tokio::test] +async fn strict_auto_review_turn_grant_forces_guardian_for_shell_policy_skip() { + let server = start_mock_server().await; + let guardian_request_log = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-guardian"), + ev_assistant_message( + "msg-guardian", + &serde_json::json!({ + "risk_level": "low", + "user_authorization": "high", + "outcome": "allow", + "rationale": "The command stays within the strict turn permission grant.", + }) + .to_string(), + ), + ev_completed("resp-guardian"), + ]), + ) + .await; + + let (mut session, mut turn_context_raw) = make_session_and_context().await; + let active_turn = crate::state::ActiveTurn::default(); + let originating_turn_state = Arc::clone(&active_turn.turn_state); + *session.active_turn.lock().await = Some(active_turn); + session + .record_granted_request_permissions_for_turn( + &RequestPermissionsResponse { + permissions: RequestPermissionProfile { + network: Some(NetworkPermissions { + enabled: Some(true), + }), + ..Default::default() + }, + scope: PermissionGrantScope::Turn, + strict_auto_review: true, + }, + Some(&originating_turn_state), + ) + .await; + + turn_context_raw + .approval_policy + .set(AskForApproval::OnFailure) + .expect("test setup should allow updating approval policy"); + turn_context_raw + .sandbox_policy + .set(SandboxPolicy::DangerFullAccess) + .expect("test setup should allow updating sandbox policy"); + turn_context_raw.file_system_sandbox_policy = + FileSystemSandboxPolicy::from(turn_context_raw.sandbox_policy.get()); + turn_context_raw.network_sandbox_policy = + NetworkSandboxPolicy::from(turn_context_raw.sandbox_policy.get()); + let mut config = (*turn_context_raw.config).clone(); + config.approvals_reviewer = ApprovalsReviewer::User; + config.model_provider.base_url = Some(format!("{}/v1", server.uri())); + let config = Arc::new(config); + let models_manager = Arc::new(crate::test_support::models_manager_with_provider( + config.codex_home.to_path_buf(), + Arc::clone(&session.services.auth_manager), + config.model_provider.clone(), + )); + session.services.models_manager = models_manager; + turn_context_raw.config = Arc::clone(&config); + turn_context_raw.provider = create_model_provider( + config.model_provider.clone(), + turn_context_raw.auth_manager.clone(), + ); + let session = Arc::new(session); + let turn_context = Arc::new(turn_context_raw); + + let handler = ShellHandler; + let command = if cfg!(windows) { + vec![ + "cmd.exe".to_string(), + "/Q".to_string(), + "/D".to_string(), + "/C".to_string(), + "echo hi".to_string(), + ] + } else { + vec![ + "/bin/sh".to_string(), + "-c".to_string(), + "echo hi".to_string(), + ] + }; + let resp = handler + .handle(ToolInvocation { + session: Arc::clone(&session), + turn: Arc::clone(&turn_context), + cancellation_token: CancellationToken::new(), + tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())), + call_id: "strict-shell-call".to_string(), + tool_name: codex_tools::ToolName::plain("shell"), + payload: ToolPayload::Function { + arguments: serde_json::json!({ + "command": command, + "workdir": Some(turn_context.cwd.to_string_lossy().to_string()), + "timeout_ms": 1_000_u64, + }) + .to_string(), + }, + }) + .await; + + let output = expect_text_output(&resp.expect("expected Ok result")); + assert!(output.contains("hi")); + let guardian_request = guardian_request_log.single_request(); + assert!(guardian_request.body_contains_text("echo hi")); +} + #[tokio::test] async fn guardian_allows_unified_exec_additional_permissions_requests_past_policy_validation() { let (mut session, mut turn_context_raw) = make_session_and_context().await; diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index 903318c2a9..5e1526ad55 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -106,6 +106,7 @@ pub(crate) struct TurnState { pending_input: Vec, mailbox_delivery_phase: MailboxDeliveryPhase, granted_permissions: Option, + strict_auto_review_enabled: bool, pub(crate) tool_calls: u64, pub(crate) has_memory_citation: bool, pub(crate) token_usage_at_turn_start: TokenUsage, @@ -254,6 +255,14 @@ impl TurnState { pub(crate) fn granted_permissions(&self) -> Option { self.granted_permissions.clone() } + + pub(crate) fn enable_strict_auto_review(&mut self) { + self.strict_auto_review_enabled = true; + } + + pub(crate) fn strict_auto_review_enabled(&self) -> bool { + self.strict_auto_review_enabled + } } impl ActiveTurn { diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index c1b960bc8d..2e0f8072a9 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -116,7 +116,8 @@ impl ToolOrchestrator { let otel = turn_ctx.session_telemetry.clone(); let otel_tn = &tool_ctx.tool_name; let otel_ci = &tool_ctx.call_id; - let use_guardian = routes_approval_to_guardian(turn_ctx); + 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; // 1) Approval let mut already_approved = false; @@ -126,12 +127,37 @@ impl ToolOrchestrator { }); match requirement { ExecApprovalRequirement::Skip { .. } => { - otel.tool_decision( - otel_tn, - otel_ci, - &ReviewDecision::Approved, - ToolDecisionSource::Config, - ); + if strict_auto_review { + let guardian_review_id = Some(new_guardian_review_id()); + let approval_ctx = ApprovalCtx { + session: &tool_ctx.session, + turn: &tool_ctx.turn, + call_id: &tool_ctx.call_id, + guardian_review_id: guardian_review_id.clone(), + retry_reason: None, + network_approval_context: None, + }; + let decision = Self::request_approval( + tool, + req, + tool_ctx.call_id.as_str(), + approval_ctx, + tool_ctx, + /*evaluate_permission_request_hooks*/ false, + &otel, + ) + .await?; + Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) + .await?; + already_approved = true; + } else { + otel.tool_decision( + otel_tn, + otel_ci, + &ReviewDecision::Approved, + ToolDecisionSource::Config, + ); + } } ExecApprovalRequirement::Forbidden { reason } => { return Err(ToolError::Rejected(reason)); @@ -152,35 +178,13 @@ impl ToolOrchestrator { tool_ctx.call_id.as_str(), approval_ctx, tool_ctx, - use_guardian, + /*evaluate_permission_request_hooks*/ !strict_auto_review, &otel, ) .await?; - match decision { - ReviewDecision::Denied | ReviewDecision::Abort => { - let reason = if let Some(review_id) = guardian_review_id.as_deref() { - guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await - } else { - "rejected by user".to_string() - }; - return Err(ToolError::Rejected(reason)); - } - ReviewDecision::TimedOut => { - return Err(ToolError::Rejected(guardian_timeout_message())); - } - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession => {} - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - NetworkPolicyRuleAction::Allow => {} - NetworkPolicyRuleAction::Deny => { - return Err(ToolError::Rejected("rejected by user".to_string())); - } - }, - } + Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) + .await?; already_approved = true; } } @@ -287,9 +291,10 @@ impl ToolOrchestrator { build_denial_reason_from_output(output.as_ref()) }; - // Ask for approval before retrying with the escalated sandbox. - let bypass_retry_approval = tool - .should_bypass_approval(approval_policy, already_approved) + // Strict auto-review approval covers the sandboxed attempt only; + // retrying without the sandbox requires a fresh guardian review. + let bypass_retry_approval = !strict_auto_review + && tool.should_bypass_approval(approval_policy, already_approved) && network_approval_context.is_none(); if !bypass_retry_approval { let guardian_review_id = use_guardian.then(new_guardian_review_id); @@ -309,36 +314,13 @@ impl ToolOrchestrator { &permission_request_run_id, approval_ctx, tool_ctx, - use_guardian, + /*evaluate_permission_request_hooks*/ !strict_auto_review, &otel, ) .await?; - match decision { - ReviewDecision::Denied | ReviewDecision::Abort => { - let reason = if let Some(review_id) = guardian_review_id.as_deref() { - guardian_rejection_message(tool_ctx.session.as_ref(), review_id) - .await - } else { - "rejected by user".to_string() - }; - return Err(ToolError::Rejected(reason)); - } - ReviewDecision::TimedOut => { - return Err(ToolError::Rejected(guardian_timeout_message())); - } - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession => {} - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - } => match network_policy_amendment.action { - NetworkPolicyRuleAction::Allow => {} - NetworkPolicyRuleAction::Deny => { - return Err(ToolError::Rejected("rejected by user".to_string())); - } - }, - } + Self::reject_if_not_approved(tool_ctx, guardian_review_id.as_deref(), decision) + .await?; } let escalated_attempt = SandboxAttempt { @@ -385,13 +367,15 @@ impl ToolOrchestrator { permission_request_run_id: &str, approval_ctx: ApprovalCtx<'_>, tool_ctx: &ToolCtx, - use_guardian: bool, + evaluate_permission_request_hooks: bool, otel: &codex_otel::SessionTelemetry, ) -> Result where T: ToolRuntime, { - if let Some(permission_request) = tool.permission_request_payload(req) { + if evaluate_permission_request_hooks + && let Some(permission_request) = tool.permission_request_payload(req) + { match run_permission_request_hooks( approval_ctx.session, approval_ctx.turn, @@ -424,12 +408,12 @@ impl ToolOrchestrator { } } - let decision = tool.start_approval_async(req, approval_ctx).await; - let otel_source = if use_guardian { + let otel_source = if approval_ctx.guardian_review_id.is_some() { ToolDecisionSource::AutomatedReviewer } else { ToolDecisionSource::User }; + let decision = tool.start_approval_async(req, approval_ctx).await; otel.tool_decision( &tool_ctx.tool_name, &tool_ctx.call_id, @@ -438,6 +422,35 @@ impl ToolOrchestrator { ); Ok(decision) } + + async fn reject_if_not_approved( + tool_ctx: &ToolCtx, + guardian_review_id: Option<&str>, + decision: ReviewDecision, + ) -> Result<(), ToolError> { + match decision { + ReviewDecision::Denied | ReviewDecision::Abort => { + let reason = if let Some(review_id) = guardian_review_id { + guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await + } else { + "rejected by user".to_string() + }; + Err(ToolError::Rejected(reason)) + } + ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())), + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::ApprovedForSession => Ok(()), + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + } => match network_policy_amendment.action { + NetworkPolicyRuleAction::Allow => Ok(()), + NetworkPolicyRuleAction::Deny => { + Err(ToolError::Rejected("rejected by user".to_string())) + } + }, + } + } } fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String { diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index afc8751fa9..5de3235f83 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -139,14 +139,14 @@ impl Approvable for ApplyPatchRuntime { let changes = req.changes.clone(); let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { - if req.permissions_preapproved && retry_reason.is_none() { - return ReviewDecision::Approved; - } if let Some(review_id) = guardian_review_id { let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id); return review_approval_request(session, turn, review_id, action, retry_reason) .await; } + if req.permissions_preapproved && retry_reason.is_none() { + return ReviewDecision::Approved; + } if let Some(reason) = retry_reason { let rx_approve = session .request_patch_approval( diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index 866df13863..0b9dbf5844 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -483,6 +483,7 @@ async fn request_permissions_tool_is_auto_denied_when_granular_request_permissio RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, } ); @@ -1089,6 +1090,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul response: RequestPermissionsResponse { permissions: normalized_requested_permissions.clone(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, }) .await?; @@ -1203,6 +1205,7 @@ async fn request_permissions_preapprove_explicit_exec_permissions_outside_on_req response: RequestPermissionsResponse { permissions: normalized_requested_permissions, scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, }) .await?; @@ -1316,6 +1319,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls() -> Resu response: RequestPermissionsResponse { permissions: normalized_requested_permissions.clone(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, }) .await?; @@ -1425,6 +1429,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls_without_i response: RequestPermissionsResponse { permissions: normalized_requested_permissions.clone(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, }) .await?; @@ -1571,6 +1576,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() response: RequestPermissionsResponse { permissions: granted_permissions.clone(), scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, }) .await?; @@ -1692,6 +1698,7 @@ async fn request_permissions_grants_do_not_carry_across_turns() -> Result<()> { response: RequestPermissionsResponse { permissions: normalized_requested_permissions, scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, }) .await?; @@ -1809,6 +1816,7 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> { response: RequestPermissionsResponse { permissions: normalized_requested_permissions, scope: PermissionGrantScope::Session, + strict_auto_review: false, }, }) .await?; diff --git a/codex-rs/core/tests/suite/request_permissions_tool.rs b/codex-rs/core/tests/suite/request_permissions_tool.rs index d31b060c38..567af82944 100644 --- a/codex-rs/core/tests/suite/request_permissions_tool.rs +++ b/codex-rs/core/tests/suite/request_permissions_tool.rs @@ -4,6 +4,7 @@ use anyhow::Result; use codex_core::config::Constrained; use codex_features::Feature; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::FileSystemPermissions; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -134,6 +135,7 @@ async fn submit_turn( prompt: &str, approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, + approvals_reviewer: Option, ) -> Result<()> { let session_model = test.session_configured.model.clone(); test.codex @@ -146,7 +148,7 @@ async fn submit_turn( final_output_json_schema: None, cwd: test.cwd.path().to_path_buf(), approval_policy, - approvals_reviewer: None, + approvals_reviewer, sandbox_policy, model: session_model, effort: None, @@ -159,6 +161,13 @@ async fn submit_turn( Ok(()) } +async fn wait_for_completion(test: &TestCodex) { + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; +} + async fn expect_request_permissions_event( test: &TestCodex, expected_call_id: &str, @@ -248,6 +257,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s "write outside the workspace", approval_policy, sandbox_policy, + /*approvals_reviewer*/ None, ) .await?; @@ -262,6 +272,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s response: RequestPermissionsResponse { permissions: normalized_requested_permissions, scope: PermissionGrantScope::Turn, + strict_auto_review: false, }, }) .await?; @@ -305,11 +316,17 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s #[tokio::test(flavor = "current_thread")] #[cfg(target_os = "macos")] -async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_without_prompt() --> Result<()> { +async fn approved_folder_write_request_permissions_unblocks_later_apply_patch() -> Result<()> { skip_if_no_network!(Ok(())); skip_if_sandbox!(Ok(())); + apply_patch_after_request_permissions(/*strict_auto_review*/ false).await?; + apply_patch_after_request_permissions(/*strict_auto_review*/ true).await?; + + Ok(()) +} + +async fn apply_patch_after_request_permissions(strict_auto_review: bool) -> Result<()> { let server = start_mock_server().await; let approval_policy = AskForApproval::OnRequest; let sandbox_policy = workspace_write_excluding_tmp(); @@ -330,43 +347,72 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi let test = builder.build(&server).await?; let requested_dir = tempfile::tempdir()?; - let requested_file = requested_dir.path().join("allowed-patch.txt"); + let requested_file_name = if strict_auto_review { + "strict-allowed-patch.txt" + } else { + "allowed-patch.txt" + }; + let patch_content = if strict_auto_review { + "patched-after-strict-review" + } else { + "patched-via-request-permissions" + }; + let requested_file = requested_dir.path().join(requested_file_name); let requested_permissions = requested_directory_write_permissions(requested_dir.path()); let normalized_requested_permissions = normalized_directory_write_permissions(requested_dir.path())?; - let patch = build_add_file_patch(&requested_file, "patched-via-request-permissions"); + let patch = build_add_file_patch(&requested_file, patch_content); - let responses = mount_sse_sequence( - &server, - vec![ - sse(vec![ - ev_response_created("resp-request-permissions-patch-1"), - request_permissions_tool_event( - "permissions-call", - "Allow patching outside the workspace", - &requested_permissions, - )?, - ev_completed("resp-request-permissions-patch-1"), - ]), - sse(vec![ - ev_response_created("resp-request-permissions-patch-2"), - ev_apply_patch_function_call("apply-patch-call", &patch), - ev_completed("resp-request-permissions-patch-2"), - ]), - sse(vec![ - ev_response_created("resp-request-permissions-patch-3"), - ev_assistant_message("msg-request-permissions-patch-1", "done"), - ev_completed("resp-request-permissions-patch-3"), - ]), - ], - ) - .await; + let response_prefix = if strict_auto_review { + "resp-strict-request-permissions-patch" + } else { + "resp-request-permissions-patch" + }; + let mut sse_sequence = vec![ + sse(vec![ + ev_response_created(&format!("{response_prefix}-1")), + request_permissions_tool_event( + "permissions-call", + "Allow patching outside the workspace", + &requested_permissions, + )?, + ev_completed(&format!("{response_prefix}-1")), + ]), + sse(vec![ + ev_response_created(&format!("{response_prefix}-2")), + ev_apply_patch_function_call("apply-patch-call", &patch), + ev_completed(&format!("{response_prefix}-2")), + ]), + ]; + if strict_auto_review { + sse_sequence.push(sse(vec![ + ev_response_created(&format!("{response_prefix}-guardian")), + ev_assistant_message( + "msg-strict-request-permissions-patch-guardian", + &serde_json::json!({ + "risk_level": "low", + "user_authorization": "high", + "outcome": "allow", + "rationale": "The patch stays within the strict turn grant.", + }) + .to_string(), + ), + ev_completed(&format!("{response_prefix}-guardian")), + ])); + } + sse_sequence.push(sse(vec![ + ev_response_created(&format!("{response_prefix}-3")), + ev_assistant_message("msg-request-permissions-patch-1", "done"), + ev_completed(&format!("{response_prefix}-3")), + ])); + let responses = mount_sse_sequence(&server, sse_sequence).await; submit_turn( &test, "patch outside the workspace", approval_policy, sandbox_policy, + strict_auto_review.then_some(ApprovalsReviewer::User), ) .await?; @@ -381,26 +427,38 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi response: RequestPermissionsResponse { permissions: normalized_requested_permissions, scope: PermissionGrantScope::Turn, + strict_auto_review, }, }) .await?; - let event = wait_for_event(&test.codex, |event| { - matches!( - event, - EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TurnComplete(_) - ) - }) - .await; - match event { - EventMsg::TurnComplete(_) => {} - EventMsg::ApplyPatchApprovalRequest(approval) => { - panic!( - "unexpected apply_patch approval request after granted permissions: {:?}", - approval.call_id + if strict_auto_review { + wait_for_completion(&test).await; + let guardian_request = responses + .requests() + .into_iter() + .find(|request| request.body_contains_text(requested_file_name)) + .expect("expected guardian request for strict apply_patch"); + assert!(guardian_request.body_contains_text(requested_file_name)); + assert!(guardian_request.body_contains_text(patch_content)); + } else { + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ApplyPatchApprovalRequest(_) | EventMsg::TurnComplete(_) ) + }) + .await; + match event { + EventMsg::TurnComplete(_) => {} + EventMsg::ApplyPatchApprovalRequest(approval) => { + panic!( + "unexpected apply_patch approval request after granted permissions: {:?}", + approval.call_id + ) + } + other => panic!("unexpected event: {other:?}"), } - other => panic!("unexpected event: {other:?}"), } let patch_output = responses @@ -415,7 +473,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_apply_patch_wi ); assert_eq!( fs::read_to_string(&requested_file)?, - "patched-via-request-permissions\n" + format!("{patch_content}\n") ); Ok(()) diff --git a/codex-rs/protocol/src/request_permissions.rs b/codex-rs/protocol/src/request_permissions.rs index 473b8d973f..0649bf2886 100644 --- a/codex-rs/protocol/src/request_permissions.rs +++ b/codex-rs/protocol/src/request_permissions.rs @@ -58,6 +58,9 @@ pub struct RequestPermissionsResponse { pub permissions: RequestPermissionProfile, #[serde(default)] pub scope: PermissionGrantScope, + /// Review every subsequent command in this turn before normal sandboxed execution. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub strict_auto_review: bool, } #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 91701cd138..3149dd9f59 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -183,6 +183,7 @@ impl PendingAppServerRequests { response.permissions.clone(), ), scope: response.scope.into(), + strict_auto_review: response.strict_auto_review.then_some(true), }) .map_err(|err| { format!("failed to serialize permissions approval response: {err}") @@ -543,6 +544,7 @@ mod tests { )), }, scope: codex_protocol::request_permissions::PermissionGrantScope::Session, + strict_auto_review: false, }, }) .expect("permissions response should serialize") @@ -564,6 +566,7 @@ mod tests { }), }, scope: PermissionGrantScope::Session, + strict_auto_review: None, } ); diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 5212d81953..1f7e30c371 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -278,8 +278,8 @@ impl ApprovalOverlay { permissions, .. }, - ApprovalDecision::Review(decision), - ) => self.handle_permissions_decision(call_id, permissions, decision.clone()), + ApprovalDecision::Permissions(decision), + ) => self.handle_permissions_decision(call_id, permissions, *decision), (ApprovalRequest::ApplyPatch { id, .. }, ApprovalDecision::Review(decision)) => { self.handle_patch_decision(id, decision.clone()); } @@ -322,27 +322,31 @@ impl ApprovalOverlay { &self, call_id: &str, permissions: &RequestPermissionProfile, - decision: ReviewDecision, + decision: PermissionsDecision, ) { let Some(request) = self.current_request.as_ref() else { return; }; let granted_permissions = match decision { - ReviewDecision::Approved | ReviewDecision::ApprovedForSession => permissions.clone(), - ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => { - Default::default() - } - ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::NetworkPolicyAmendment { .. } => Default::default(), + PermissionsDecision::GrantForTurn + | PermissionsDecision::GrantForTurnWithStrictAutoReview + | PermissionsDecision::GrantForSession => permissions.clone(), + PermissionsDecision::Deny => Default::default(), }; - let scope = if matches!(decision, ReviewDecision::ApprovedForSession) { + let scope = if matches!(decision, PermissionsDecision::GrantForSession) { PermissionGrantScope::Session } else { PermissionGrantScope::Turn }; + let strict_auto_review = matches!( + decision, + PermissionsDecision::GrantForTurnWithStrictAutoReview + ); if request.thread_label().is_none() { let message = if granted_permissions.is_empty() { "You did not grant additional permissions" + } else if strict_auto_review { + "You granted additional permissions with strict auto review" } else if matches!(scope, PermissionGrantScope::Session) { "You granted additional permissions for this session" } else { @@ -359,6 +363,7 @@ impl ApprovalOverlay { codex_protocol::request_permissions::RequestPermissionsResponse { permissions: granted_permissions, scope, + strict_auto_review, }, ); } @@ -482,7 +487,11 @@ impl BottomPaneView for ApprovalOverlay { permissions, .. } => { - self.handle_permissions_decision(call_id, permissions, ReviewDecision::Abort); + self.handle_permissions_decision( + call_id, + permissions, + PermissionsDecision::Deny, + ); } ApprovalRequest::ApplyPatch { id, .. } => { self.handle_patch_decision(id, ReviewDecision::Abort); @@ -679,9 +688,18 @@ fn build_header(request: &ApprovalRequest) -> Box { #[derive(Clone)] enum ApprovalDecision { Review(ReviewDecision), + Permissions(PermissionsDecision), McpElicitation(ElicitationAction), } +#[derive(Clone, Copy)] +enum PermissionsDecision { + GrantForTurn, + GrantForTurnWithStrictAutoReview, + GrantForSession, + Deny, +} + #[derive(Clone)] struct ApprovalOption { label: String, @@ -901,20 +919,28 @@ fn patch_options() -> Vec { fn permissions_options() -> Vec { vec![ ApprovalOption { - label: "Yes, grant these permissions".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::Approved), + label: "Yes, grant these permissions for this turn".to_string(), + decision: ApprovalDecision::Permissions(PermissionsDecision::GrantForTurn), display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], }, + ApprovalOption { + label: "Yes, grant for this turn with strict auto review".to_string(), + decision: ApprovalDecision::Permissions( + PermissionsDecision::GrantForTurnWithStrictAutoReview, + ), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('r'))], + }, ApprovalOption { label: "Yes, grant these permissions for this session".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), + decision: ApprovalDecision::Permissions(PermissionsDecision::GrantForSession), display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], }, ApprovalOption { label: "No, continue without permissions".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::Denied), + decision: ApprovalDecision::Permissions(PermissionsDecision::Deny), display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))], }, @@ -1347,7 +1373,8 @@ mod tests { assert_eq!( labels, vec![ - "Yes, grant these permissions".to_string(), + "Yes, grant these permissions for this turn".to_string(), + "Yes, grant for this turn with strict auto review".to_string(), "Yes, grant these permissions for this session".to_string(), "No, continue without permissions".to_string(), ] @@ -1410,6 +1437,34 @@ mod tests { ); } + #[test] + fn permissions_strict_auto_review_shortcut_submits_turn_scope_with_strict_review() { + let (tx, mut rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + let mut view = + ApprovalOverlay::new(make_permissions_request(), tx, Features::with_defaults()); + + view.handle_key_event(KeyEvent::new(KeyCode::Char('r'), KeyModifiers::NONE)); + + let mut saw_op = false; + while let Ok(ev) = rx.try_recv() { + if let AppEvent::SubmitThreadOp { + op: Op::RequestPermissionsResponse { response, .. }, + .. + } = ev + { + assert_eq!(response.scope, PermissionGrantScope::Turn); + assert!(response.strict_auto_review); + saw_op = true; + break; + } + } + assert!( + saw_op, + "expected permission approval decision to emit a strict auto review response" + ); + } + #[test] fn additional_permissions_prompt_shows_permission_rule_line() { let (tx, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_permissions_prompt.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_permissions_prompt.snap index e7a21c42cc..391102e74f 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_permissions_prompt.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_permissions_prompt.snap @@ -9,8 +9,9 @@ expression: "normalize_snapshot_paths(render_overlay_lines(&view, 120))" Permission rule: network; read `/tmp/readme.txt`; write `/tmp/out.txt` -› 1. Yes, grant these permissions (y) - 2. Yes, grant these permissions for this session (a) - 3. No, continue without permissions (n) +› 1. Yes, grant these permissions for this turn (y) + 2. Yes, grant for this turn with strict auto review (r) + 3. Yes, grant these permissions for this session (a) + 4. No, continue without permissions (n) Press enter to confirm or esc to cancel