From eed0e07825a5def3ff73ea6c365eb46f8c241910 Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Wed, 22 Apr 2026 17:14:22 -0700 Subject: [PATCH 1/8] hooks: emit Bash PostToolUse when exec_command completes via write_stdin (#18888) Fixes #16246. ## Why `exec_command` already emits `PreToolUse`, but long-running unified exec commands that finish on a later `write_stdin` poll could miss the matching `PostToolUse`. That left the Bash hook lifecycle inconsistent, broke expectations around `tool_use_id` and `tool_input.command`, and meant `PostToolUse` block/replacement feedback could fail to replace the final session output before it reached model context. This keeps the fix scoped to the `exec_command` / `write_stdin` lifecycle. Broader non-Bash hook expansion is still out of scope here and remains tracked separately in #16732. ## What changed - Compute and store `PostToolUsePayload` while handlers still have access to their concrete output type, and carry `tool_use_id` through that payload. - Preserve the original hook-facing `exec_command` string through unified exec state (`ExecCommandRequest`, `ProcessEntry`, `PreparedProcessHandles`, and `ExecCommandToolOutput`) via `hook_command`, and remove the now-unused `session_command` output metadata. - Emit exactly one Bash `PostToolUse` for long-running `exec_command` sessions when a later `write_stdin` poll observes final completion, using the original `exec_command` call id and hook-facing command. - Keep one-shot `exec_command` behavior aligned with the same payload construction, including interactive completions that return a final result directly. - Apply `PostToolUse` block/replacement feedback before the final `write_stdin` completion output is sent back to the model. - Keep `write_stdin` itself out of `PreToolUse` matching so it continues to act as transport/polling for the original Bash tool call. - Restore plain matcher behavior for tool-name matchers such as `Bash` and `Edit|Write`, while still treating patterns with regex characters (for example `mcp__.*`) as regexes. - Add unit coverage for unified exec payload construction and parallel session separation, plus a core integration regression that verifies a blocked `PostToolUse` replaces the final `write_stdin` output in model context. ## Testing - `cargo test -p codex-hooks` - `cargo test -p codex-core post_tool_use_payload` - `cargo test -p codex-core post_tool_use_blocks_when_exec_session_completes_via_write_stdin` --- codex-rs/core/src/tools/context.rs | 4 +- codex-rs/core/src/tools/context_tests.rs | 2 +- .../core/src/tools/handlers/apply_patch.rs | 3 +- .../src/tools/handlers/apply_patch_tests.rs | 1 + codex-rs/core/src/tools/handlers/shell.rs | 6 +- .../core/src/tools/handlers/shell_tests.rs | 1 + .../core/src/tools/handlers/unified_exec.rs | 30 ++-- .../src/tools/handlers/unified_exec_tests.rs | 116 ++++++++++-- codex-rs/core/src/tools/parallel.rs | 1 + codex-rs/core/src/tools/registry.rs | 42 ++--- codex-rs/core/src/unified_exec/mod.rs | 2 +- codex-rs/core/src/unified_exec/mod_tests.rs | 4 +- .../core/src/unified_exec/process_manager.rs | 14 +- codex-rs/core/tests/suite/hooks.rs | 165 ++++++++++++++++++ codex-rs/hooks/src/events/common.rs | 32 +++- 15 files changed, 345 insertions(+), 78 deletions(-) diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index 15ada81bb9..feb4b02d71 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -370,7 +370,7 @@ pub struct ExecCommandToolOutput { pub process_id: Option, pub exit_code: Option, pub original_token_count: Option, - pub session_command: Option>, + pub hook_command: Option, } impl ToolOutput for ExecCommandToolOutput { @@ -394,7 +394,7 @@ impl ToolOutput for ExecCommandToolOutput { } fn post_tool_use_response(&self, _call_id: &str, _payload: &ToolPayload) -> Option { - if self.process_id.is_some() || self.session_command.is_none() { + if self.process_id.is_some() || self.hook_command.is_none() { return None; } diff --git a/codex-rs/core/src/tools/context_tests.rs b/codex-rs/core/src/tools/context_tests.rs index 4b903347ab..60ad456151 100644 --- a/codex-rs/core/src/tools/context_tests.rs +++ b/codex-rs/core/src/tools/context_tests.rs @@ -390,7 +390,7 @@ fn exec_command_tool_output_formats_truncated_response() { process_id: None, exit_code: Some(0), original_token_count: Some(10), - session_command: None, + hook_command: None, } .to_response_item("call-42", &payload); diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 8eca2caf3f..0ccc62d9e9 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -324,11 +324,12 @@ impl ToolHandler for ApplyPatchHandler { &self, call_id: &str, payload: &ToolPayload, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { let tool_response = result.post_tool_use_response(call_id, payload)?; Some(PostToolUsePayload { tool_name: HookToolName::apply_patch(), + tool_use_id: call_id.to_string(), command: apply_patch_payload_command(payload)?, tool_response, }) diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 979b4d4e96..39c3133103 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -90,6 +90,7 @@ fn post_tool_use_payload_uses_patch_input_and_tool_output() { handler.post_tool_use_payload("call-apply-patch", &payload, &output), Some(PostToolUsePayload { tool_name: HookToolName::apply_patch(), + tool_use_id: "call-apply-patch".to_string(), command: patch.to_string(), tool_response: json!("Success. Updated files."), }) diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 00343d3cea..b9718173b7 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -216,11 +216,12 @@ impl ToolHandler for ShellHandler { &self, call_id: &str, payload: &ToolPayload, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { let tool_response = result.post_tool_use_response(call_id, payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), + tool_use_id: call_id.to_string(), command: shell_payload_command(payload)?, tool_response, }) @@ -328,11 +329,12 @@ impl ToolHandler for ShellCommandHandler { &self, call_id: &str, payload: &ToolPayload, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { let tool_response = result.post_tool_use_response(call_id, payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), + tool_use_id: call_id.to_string(), command: shell_command_payload_command(payload)?, tool_response, }) diff --git a/codex-rs/core/src/tools/handlers/shell_tests.rs b/codex-rs/core/src/tools/handlers/shell_tests.rs index b36dd0225c..c13d834c06 100644 --- a/codex-rs/core/src/tools/handlers/shell_tests.rs +++ b/codex-rs/core/src/tools/handlers/shell_tests.rs @@ -284,6 +284,7 @@ fn build_post_tool_use_payload_uses_tool_output_wire_value() { handler.post_tool_use_payload("call-42", &payload, &output), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), + tool_use_id: "call-42".to_string(), command: "printf shell command".to_string(), tool_response: json!("shell output"), }) diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index b25f5a69b7..813f4f8628 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -147,21 +147,23 @@ impl ToolHandler for UnifiedExecHandler { &self, call_id: &str, payload: &ToolPayload, - result: &dyn ToolOutput, + result: &Self::Output, ) -> Option { - let ToolPayload::Function { arguments } = payload else { + let ToolPayload::Function { .. } = payload else { return None; }; - let args = parse_arguments::(arguments).ok()?; - if args.tty { - return None; - } - - let tool_response = result.post_tool_use_response(call_id, payload)?; + let command = result.hook_command.clone()?; + let tool_use_id = if result.event_call_id.is_empty() { + call_id.to_string() + } else { + result.event_call_id.clone() + }; + let tool_response = result.post_tool_use_response(&tool_use_id, payload)?; Some(PostToolUsePayload { tool_name: HookToolName::bash(), - command: args.cmd, + tool_use_id, + command, tool_response, }) } @@ -200,11 +202,12 @@ impl ToolHandler for UnifiedExecHandler { "exec_command" => { let cwd = resolve_workdir_base_path(&arguments, &context.turn.cwd)?; let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?; + let hook_command = args.cmd.clone(); let workdir = context.turn.resolve_path(args.workdir.clone()); maybe_emit_implicit_skill_invocation( session.as_ref(), context.turn.as_ref(), - &args.cmd, + &hook_command, &workdir, ) .await; @@ -313,17 +316,16 @@ impl ToolHandler for UnifiedExecHandler { process_id: None, exit_code: None, original_token_count: None, - session_command: None, + hook_command: None, }); } emit_unified_exec_tty_metric(&turn.session_telemetry, tty); - let session_command = command.clone(); match manager .exec_command( ExecCommandRequest { command, - hook_command: args.cmd, + hook_command: hook_command.clone(), process_id, yield_time_ms, max_output_tokens, @@ -357,7 +359,7 @@ impl ToolHandler for UnifiedExecHandler { process_id: None, exit_code: Some(output.exit_code), original_token_count: Some(original_token_count), - session_command: Some(session_command), + hook_command: Some(hook_command), } } Err(err) => { diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 26f86bd66d..0a5abe51d7 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -252,7 +252,7 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co arguments: serde_json::json!({ "cmd": "echo three", "tty": false }).to_string(), }; let output = ExecCommandToolOutput { - event_call_id: "event-43".to_string(), + event_call_id: "call-43".to_string(), chunk_id: "chunk-1".to_string(), wall_time: std::time::Duration::from_millis(498), raw_output: b"three".to_vec(), @@ -260,17 +260,14 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co process_id: None, exit_code: Some(0), original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "echo three".to_string(), - ]), + hook_command: Some("echo three".to_string()), }; assert_eq!( UnifiedExecHandler.post_tool_use_payload("call-43", &payload, &output), Some(crate::tools::registry::PostToolUsePayload { tool_name: HookToolName::bash(), + tool_use_id: "call-43".to_string(), command: "echo three".to_string(), tool_response: serde_json::json!("three"), }) @@ -278,12 +275,12 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co } #[test] -fn exec_command_post_tool_use_payload_skips_interactive_exec() { +fn exec_command_post_tool_use_payload_uses_output_for_interactive_completion() { let payload = ToolPayload::Function { arguments: serde_json::json!({ "cmd": "echo three", "tty": true }).to_string(), }; let output = ExecCommandToolOutput { - event_call_id: "event-44".to_string(), + event_call_id: "call-44".to_string(), chunk_id: "chunk-1".to_string(), wall_time: std::time::Duration::from_millis(498), raw_output: b"three".to_vec(), @@ -291,16 +288,17 @@ fn exec_command_post_tool_use_payload_skips_interactive_exec() { process_id: None, exit_code: Some(0), original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "echo three".to_string(), - ]), + hook_command: Some("echo three".to_string()), }; assert_eq!( UnifiedExecHandler.post_tool_use_payload("call-44", &payload, &output), - None + Some(crate::tools::registry::PostToolUsePayload { + tool_name: HookToolName::bash(), + tool_use_id: "call-44".to_string(), + command: "echo three".to_string(), + tool_response: serde_json::json!("three"), + }) ); } @@ -318,11 +316,7 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() { process_id: Some(45), exit_code: None, original_token_count: None, - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "echo three".to_string(), - ]), + hook_command: Some("echo three".to_string()), }; assert_eq!( @@ -330,3 +324,87 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() { None ); } + +#[test] +fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_completion() { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ + "session_id": 45, + "chars": "", + }) + .to_string(), + }; + let output = ExecCommandToolOutput { + event_call_id: "exec-call-45".to_string(), + chunk_id: "chunk-2".to_string(), + wall_time: std::time::Duration::from_millis(498), + raw_output: b"finished\n".to_vec(), + max_output_tokens: None, + process_id: None, + exit_code: Some(0), + original_token_count: None, + hook_command: Some("sleep 1; echo finished".to_string()), + }; + + assert_eq!( + UnifiedExecHandler.post_tool_use_payload("write-stdin-call", &payload, &output), + Some(crate::tools::registry::PostToolUsePayload { + tool_name: HookToolName::bash(), + tool_use_id: "exec-call-45".to_string(), + command: "sleep 1; echo finished".to_string(), + tool_response: serde_json::json!("finished\n"), + }) + ); +} + +#[test] +fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() { + let payload = ToolPayload::Function { + arguments: serde_json::json!({ "session_id": 45, "chars": "" }).to_string(), + }; + let output_a = ExecCommandToolOutput { + event_call_id: "exec-call-a".to_string(), + chunk_id: "chunk-a".to_string(), + wall_time: std::time::Duration::from_millis(498), + raw_output: b"alpha\n".to_vec(), + max_output_tokens: None, + process_id: None, + exit_code: Some(0), + original_token_count: None, + hook_command: Some("sleep 2; echo alpha".to_string()), + }; + let output_b = ExecCommandToolOutput { + event_call_id: "exec-call-b".to_string(), + chunk_id: "chunk-b".to_string(), + wall_time: std::time::Duration::from_millis(498), + raw_output: b"beta\n".to_vec(), + max_output_tokens: None, + process_id: None, + exit_code: Some(0), + original_token_count: None, + hook_command: Some("sleep 1; echo beta".to_string()), + }; + + let payloads = [ + UnifiedExecHandler.post_tool_use_payload("write-call-b", &payload, &output_b), + UnifiedExecHandler.post_tool_use_payload("write-call-a", &payload, &output_a), + ]; + + assert_eq!( + payloads, + [ + Some(crate::tools::registry::PostToolUsePayload { + tool_name: HookToolName::bash(), + tool_use_id: "exec-call-b".to_string(), + command: "sleep 1; echo beta".to_string(), + tool_response: serde_json::json!("beta\n"), + }), + Some(crate::tools::registry::PostToolUsePayload { + tool_name: HookToolName::bash(), + tool_use_id: "exec-call-a".to_string(), + command: "sleep 2; echo alpha".to_string(), + tool_response: serde_json::json!("alpha\n"), + }), + ] + ); +} diff --git a/codex-rs/core/src/tools/parallel.rs b/codex-rs/core/src/tools/parallel.rs index 06cf7e5eed..384a800b4b 100644 --- a/codex-rs/core/src/tools/parallel.rs +++ b/codex-rs/core/src/tools/parallel.rs @@ -178,6 +178,7 @@ impl ToolCallRuntime { result: Box::new(AbortedToolOutput { message: Self::abort_message(call, secs), }), + post_tool_use_payload: None, } } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index d70550d0e5..678e620bd8 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -72,7 +72,7 @@ pub trait ToolHandler: Send + Sync { &self, _call_id: &str, _payload: &ToolPayload, - _result: &dyn ToolOutput, + _result: &Self::Output, ) -> Option { None } @@ -107,6 +107,7 @@ pub(crate) struct AnyToolResult { pub(crate) call_id: String, pub(crate) payload: ToolPayload, pub(crate) result: Box, + pub(crate) post_tool_use_payload: Option, } impl AnyToolResult { @@ -143,9 +144,11 @@ pub(crate) struct PreToolUsePayload { pub(crate) struct PostToolUsePayload { /// Hook-facing tool name model. /// - /// Keep this aligned with the corresponding pre-use payload so external - /// hook consumers can pair events by `tool_use_id`. + /// The canonical name is serialized to hook stdin, while aliases are used + /// only for matcher compatibility. pub(crate) tool_name: HookToolName, + /// The originating tool-use id exposed at `tool_use_id`. + pub(crate) tool_use_id: String, /// Command-shaped input exposed at `tool_input.command`. pub(crate) command: String, /// Tool result exposed at `tool_response`. @@ -159,15 +162,7 @@ trait AnyToolHandler: Send + Sync { fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option; - fn post_tool_use_payload( - &self, - call_id: &str, - payload: &ToolPayload, - result: &dyn ToolOutput, - ) -> Option; - fn create_diff_consumer(&self) -> Option>; - fn handle_any<'a>( &'a self, invocation: ToolInvocation, @@ -190,19 +185,9 @@ where ToolHandler::pre_tool_use_payload(self, invocation) } - fn post_tool_use_payload( - &self, - call_id: &str, - payload: &ToolPayload, - result: &dyn ToolOutput, - ) -> Option { - ToolHandler::post_tool_use_payload(self, call_id, payload, result) - } - fn create_diff_consumer(&self) -> Option> { ToolHandler::create_diff_consumer(self) } - fn handle_any<'a>( &'a self, invocation: ToolInvocation, @@ -211,10 +196,13 @@ where let call_id = invocation.call_id.clone(); let payload = invocation.payload.clone(); let output = self.handle(invocation).await?; + let post_tool_use_payload = + ToolHandler::post_tool_use_payload(self, &call_id, &payload, &output); Ok(AnyToolResult { call_id, payload, result: Box::new(output), + post_tool_use_payload, }) }) } @@ -400,13 +388,9 @@ impl ToolRegistry { emit_metric_for_tool_read(&invocation, success).await; let post_tool_use_payload = if success { let guard = response_cell.lock().await; - guard.as_ref().and_then(|result| { - handler.post_tool_use_payload( - &result.call_id, - &result.payload, - result.result.as_ref(), - ) - }) + guard + .as_ref() + .and_then(|result| result.post_tool_use_payload.clone()) } else { None }; @@ -415,7 +399,7 @@ impl ToolRegistry { run_post_tool_use_hooks( &invocation.session, &invocation.turn, - invocation.call_id.clone(), + post_tool_use_payload.tool_use_id, post_tool_use_payload.tool_name.name().to_string(), post_tool_use_payload.tool_name.matcher_aliases().to_vec(), post_tool_use_payload.command, diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 01c8914bc2..a5a6e69f89 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -148,7 +148,7 @@ struct ProcessEntry { process: Arc, call_id: String, process_id: i32, - command: Vec, + hook_command: String, tty: bool, network_approval_id: Option, session: Weak, diff --git a/codex-rs/core/src/unified_exec/mod_tests.rs b/codex-rs/core/src/unified_exec/mod_tests.rs index 1865188d3e..f96498b5a8 100644 --- a/codex-rs/core/src/unified_exec/mod_tests.rs +++ b/codex-rs/core/src/unified_exec/mod_tests.rs @@ -113,7 +113,7 @@ async fn exec_command_with_tty( process: Arc::clone(&process), call_id: context.call_id.clone(), process_id, - command: command.clone(), + hook_command: cmd.to_string(), tty, network_approval_id: None, session: Arc::downgrade(session), @@ -165,7 +165,7 @@ async fn exec_command_with_tty( process_id: response_process_id, exit_code, original_token_count: Some(approx_token_count(&text)), - session_command: Some(command), + hook_command: Some(cmd.to_string()), }) } diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index ef1f3bd81c..ec0b00cf59 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -169,7 +169,7 @@ struct PreparedProcessHandles { output_closed_notify: Arc, cancellation_token: CancellationToken, pause_state: Option>, - command: Vec, + hook_command: String, process_id: i32, tty: bool, } @@ -279,6 +279,7 @@ impl UnifiedExecProcessManager { Arc::clone(&process), context, &request.command, + request.hook_command.clone(), cwd.clone(), start, request.process_id, @@ -398,7 +399,7 @@ impl UnifiedExecProcessManager { process_id: response_process_id, exit_code, original_token_count: Some(original_token_count), - session_command: Some(request.command.clone()), + hook_command: Some(request.hook_command.clone()), }; Ok(response) @@ -418,7 +419,7 @@ impl UnifiedExecProcessManager { output_closed_notify, cancellation_token, pause_state, - command: session_command, + hook_command, process_id, tty, .. @@ -517,7 +518,7 @@ impl UnifiedExecProcessManager { process_id, exit_code, original_token_count: Some(original_token_count), - session_command: Some(session_command.clone()), + hook_command: Some(hook_command), }; Ok(response) @@ -585,7 +586,7 @@ impl UnifiedExecProcessManager { output_closed_notify, cancellation_token, pause_state, - command: entry.command.clone(), + hook_command: entry.hook_command.clone(), process_id: entry.process_id, tty: entry.tty, }) @@ -597,6 +598,7 @@ impl UnifiedExecProcessManager { process: Arc, context: &UnifiedExecContext, command: &[String], + hook_command: String, cwd: AbsolutePathBuf, started_at: Instant, process_id: i32, @@ -608,7 +610,7 @@ impl UnifiedExecProcessManager { process: Arc::clone(&process), call_id: context.call_id.clone(), process_id, - command: command.to_vec(), + hook_command, tty, network_approval_id, session: Arc::downgrade(&context.session), diff --git a/codex-rs/core/tests/suite/hooks.rs b/codex-rs/core/tests/suite/hooks.rs index 4c96de85fa..022c528753 100644 --- a/codex-rs/core/tests/suite/hooks.rs +++ b/codex-rs/core/tests/suite/hooks.rs @@ -33,6 +33,7 @@ use core_test_support::responses::mount_sse_sequence; 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_windows; use core_test_support::streaming_sse::StreamingSseChunk; use core_test_support::streaming_sse::start_streaming_sse_server; use core_test_support::test_codex::test_codex; @@ -407,6 +408,64 @@ elif mode == "exit_2": Ok(()) } +fn write_logging_pre_and_blocking_post_tool_use_hooks(home: &Path, feedback: &str) -> Result<()> { + let pre_script_path = home.join("pre_tool_use_hook.py"); + let pre_log_path = home.join("pre_tool_use_hook_log.jsonl"); + let post_script_path = home.join("post_tool_use_hook.py"); + let post_log_path = home.join("post_tool_use_hook_log.jsonl"); + let feedback_json = + serde_json::to_string(feedback).context("serialize post tool use feedback")?; + let pre_script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{pre_log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") +"#, + pre_log_path = pre_log_path.display(), + ); + let post_script = format!( + r#"import json +from pathlib import Path +import sys + +payload = json.load(sys.stdin) +with Path(r"{post_log_path}").open("a", encoding="utf-8") as handle: + handle.write(json.dumps(payload) + "\n") +sys.stderr.write({feedback_json} + "\n") +raise SystemExit(2) +"#, + post_log_path = post_log_path.display(), + ); + let hooks = serde_json::json!({ + "hooks": { + "PreToolUse": [{ + "matcher": "Bash", + "hooks": [{ + "type": "command", + "command": format!("python3 {}", pre_script_path.display()), + "statusMessage": "running pre tool use hook", + }] + }], + "PostToolUse": [{ + "matcher": "Bash", + "hooks": [{ + "type": "command", + "command": format!("python3 {}", post_script_path.display()), + "statusMessage": "running post tool use hook", + }] + }] + } + }); + + fs::write(&pre_script_path, pre_script).context("write pre tool use hook script")?; + fs::write(&post_script_path, post_script).context("write post tool use hook script")?; + fs::write(home.join("hooks.json"), hooks.to_string()).context("write hooks.json")?; + Ok(()) +} + fn write_session_start_hook_recording_transcript(home: &Path) -> Result<()> { let script_path = home.join("session_start_hook.py"); let log_path = home.join("session_start_hook_log.jsonl"); @@ -2514,6 +2573,112 @@ async fn post_tool_use_exit_two_replaces_one_shot_exec_command_output_with_feedb Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn post_tool_use_blocks_when_exec_session_completes_via_write_stdin() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_windows!(Ok(())); + + let server = start_mock_server().await; + let start_call_id = "posttooluse-exec-session-start"; + let poll_call_id = "posttooluse-exec-session-poll"; + let command = "sleep 1; printf session-post-hook-output".to_string(); + let start_args = serde_json::json!({ + "cmd": command, + "shell": "/bin/sh", + "login": false, + "tty": false, + "yield_time_ms": 250, + }); + let poll_args = serde_json::json!({ + "session_id": 1000, + "chars": "", + "yield_time_ms": 5_000, + }); + let feedback = "blocked by session post hook"; + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + core_test_support::responses::ev_function_call( + start_call_id, + "exec_command", + &serde_json::to_string(&start_args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + core_test_support::responses::ev_function_call( + poll_call_id, + "write_stdin", + &serde_json::to_string(&poll_args)?, + ), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_response_created("resp-3"), + ev_assistant_message("msg-1", "session post hook observed"), + ev_completed("resp-3"), + ]), + ], + ) + .await; + + let mut builder = test_codex() + .with_pre_build_hook(|home| { + if let Err(error) = write_logging_pre_and_blocking_post_tool_use_hooks(home, feedback) { + panic!("failed to write tool use hook test fixture: {error}"); + } + }) + .with_config(|config| { + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::CodexHooks) + .expect("test config should allow feature update"); + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + + test.submit_turn("run the exec command session with post hook") + .await?; + + let requests = responses.requests(); + assert_eq!(requests.len(), 3); + let output_item = requests[2].function_call_output(poll_call_id); + let output = output_item + .get("output") + .and_then(Value::as_str) + .expect("write_stdin output string"); + assert_eq!(output, feedback); + + let pre_hook_inputs = read_pre_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(pre_hook_inputs.len(), 1); + assert_eq!(pre_hook_inputs[0]["tool_name"], "Bash"); + assert_eq!(pre_hook_inputs[0]["tool_use_id"], start_call_id); + assert_eq!(pre_hook_inputs[0]["tool_input"]["command"], command); + + let post_hook_inputs = read_post_tool_use_hook_inputs(test.codex_home_path())?; + assert_eq!(post_hook_inputs.len(), 1); + assert_eq!(post_hook_inputs[0]["hook_event_name"], "PostToolUse"); + assert_eq!(post_hook_inputs[0]["tool_name"], "Bash"); + assert_eq!(post_hook_inputs[0]["tool_use_id"], start_call_id); + assert_eq!(post_hook_inputs[0]["tool_input"]["command"], command); + assert!( + post_hook_inputs[0]["tool_response"] + .as_str() + .is_some_and(|tool_response| tool_response.contains("session-post-hook-output")), + "PostToolUse should see the final session output, got {:?}", + post_hook_inputs[0]["tool_response"] + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn post_tool_use_records_additional_context_for_apply_patch() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/hooks/src/events/common.rs b/codex-rs/hooks/src/events/common.rs index e74c2912dd..80dd325378 100644 --- a/codex-rs/hooks/src/events/common.rs +++ b/codex-rs/hooks/src/events/common.rs @@ -109,7 +109,7 @@ pub(crate) fn matcher_pattern_for_event( } pub(crate) fn validate_matcher_pattern(matcher: &str) -> Result<(), regex::Error> { - if is_match_all_matcher(matcher) { + if is_match_all_matcher(matcher) || is_exact_matcher(matcher) { return Ok(()); } regex::Regex::new(matcher).map(|_| ()) @@ -119,6 +119,9 @@ pub(crate) fn matches_matcher(matcher: Option<&str>, input: Option<&str>) -> boo match matcher { None => true, Some(matcher) if is_match_all_matcher(matcher) => true, + Some(matcher) if is_exact_matcher(matcher) => input + .map(|input| matcher.split('|').any(|candidate| candidate == input)) + .unwrap_or(false), Some(matcher) => input .and_then(|input| { regex::Regex::new(matcher) @@ -144,6 +147,12 @@ fn is_match_all_matcher(matcher: &str) -> bool { matcher.is_empty() || matcher == "*" } +fn is_exact_matcher(matcher: &str) -> bool { + matcher + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || ch == '_' || ch == '|') +} + #[cfg(test)] mod tests { use codex_protocol::protocol::HookEventName; @@ -181,6 +190,27 @@ mod tests { assert_eq!(validate_matcher_pattern("Edit|Write"), Ok(())); } + #[test] + fn matcher_exact_string_does_not_substring_match() { + assert!(matches_matcher(Some("Bash"), Some("Bash"))); + assert!(!matches_matcher(Some("Bash"), Some("BashOutput"))); + assert_eq!(validate_matcher_pattern("Bash"), Ok(())); + } + + #[test] + fn matcher_uses_regex_when_it_contains_other_characters() { + assert!(matches_matcher(Some("^Bash"), Some("BashOutput"))); + assert!(matches_matcher( + Some("mcp__memory__.*"), + Some("mcp__memory__create_entities") + )); + assert!(!matches_matcher( + Some("mcp__memory"), + Some("mcp__memory__create_entities") + )); + assert_eq!(validate_matcher_pattern("mcp__memory__.*"), Ok(())); + } + #[test] fn matcher_supports_anchored_regexes() { assert!(matches_matcher(Some("^Bash$"), Some("Bash"))); From 83ec1eb5d69daf77c40e8bc3a39195f4cd679d68 Mon Sep 17 00:00:00 2001 From: Won Park Date: Wed, 22 Apr 2026 17:22:35 -0700 Subject: [PATCH 2/8] Rename approvals reviewer variant to auto-review (#19056) ## Why `approvals_reviewer` now uses `auto_review` as the canonical config/API value after #18504, but the Rust enum variant and nearby helper/test names still used `GuardianSubagent` / guardian approval wording. That made follow-up code and reviews confusing even though the external value had already moved to Auto-review. ## What changed - Renamed `ApprovalsReviewer::GuardianSubagent` to `ApprovalsReviewer::AutoReview`. - Updated protocol, app-server, config, core, TUI, exec, and analytics test callsites. - Renamed nearby helper/test names from guardian approval wording to Auto-review wording where they refer to the approvals reviewer mode. - Preserved wire compatibility: - `auto_review` remains the canonical serialized value. - `guardian_subagent` remains accepted as a legacy alias. This intentionally does not rename the `[features].guardian_approval` key, `Feature::GuardianApproval`, `core/src/guardian`, analytics event names, or app-server Guardian review event types. ## Verification - `cargo test -p codex-protocol approvals_reviewer_serializes_auto_review_and_accepts_legacy_guardian_subagent` - `cargo test -p codex-app-server-protocol approvals_reviewer_serializes_auto_review_and_accepts_legacy_guardian_subagent` - `cargo test -p codex-config approvals_reviewer` - `cargo test -p codex-tui update_feature_flags` - `cargo test -p codex-core permissions_instructions` - `cargo test -p codex-tui permissions_selection` --- .../analytics/src/analytics_client_tests.rs | 2 +- .../app-server-protocol/src/protocol/v2.rs | 15 ++--- codex-rs/app-server/src/config_api.rs | 4 +- codex-rs/config/src/config_requirements.rs | 10 +-- codex-rs/core/src/codex_delegate_tests.rs | 4 +- codex-rs/core/src/config/config_tests.rs | 33 +++------ codex-rs/core/src/config_loader/mod.rs | 9 +-- .../src/context/permissions_instructions.rs | 2 +- .../context/permissions_instructions_tests.rs | 8 +-- codex-rs/core/src/guardian/review.rs | 2 +- codex-rs/core/src/guardian/tests.rs | 4 +- codex-rs/core/src/mcp_tool_call_tests.rs | 6 +- codex-rs/core/src/session/tests.rs | 4 +- .../core/src/session/tests/guardian_tests.rs | 4 +- codex-rs/exec/src/lib_tests.rs | 11 ++- codex-rs/protocol/src/config_types.rs | 7 +- codex-rs/tui/src/app.rs | 10 +-- codex-rs/tui/src/app/config_persistence.rs | 26 +++---- codex-rs/tui/src/app/tests.rs | 67 +++++++++---------- codex-rs/tui/src/chatwidget.rs | 4 +- .../tui/src/chatwidget/tests/permissions.rs | 19 +++--- codex-rs/tui/src/debug_config.rs | 8 +-- 22 files changed, 114 insertions(+), 145 deletions(-) diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index cd96bac628..9b45a1a82d 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -324,7 +324,7 @@ fn sample_turn_resolved_config(turn_id: &str) -> TurnResolvedConfigFact { reasoning_summary: None, service_tier: None, approval_policy: AskForApproval::OnRequest, - approvals_reviewer: ApprovalsReviewer::GuardianSubagent, + approvals_reviewer: ApprovalsReviewer::AutoReview, sandbox_network_access: true, collaboration_mode: ModeKind::Plan, personality: None, diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 2760313dc7..934fbc9516 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -323,7 +323,7 @@ pub enum ApprovalsReviewer { #[serde(rename = "user")] User, #[serde(rename = "auto_review", alias = "guardian_subagent")] - GuardianSubagent, + AutoReview, } impl JsonSchema for ApprovalsReviewer { @@ -361,7 +361,7 @@ impl ApprovalsReviewer { pub fn to_core(self) -> CoreApprovalsReviewer { match self { ApprovalsReviewer::User => CoreApprovalsReviewer::User, - ApprovalsReviewer::GuardianSubagent => CoreApprovalsReviewer::GuardianSubagent, + ApprovalsReviewer::AutoReview => CoreApprovalsReviewer::AutoReview, } } } @@ -370,7 +370,7 @@ impl From for ApprovalsReviewer { fn from(value: CoreApprovalsReviewer) -> Self { match value { CoreApprovalsReviewer::User => ApprovalsReviewer::User, - CoreApprovalsReviewer::GuardianSubagent => ApprovalsReviewer::GuardianSubagent, + CoreApprovalsReviewer::AutoReview => ApprovalsReviewer::AutoReview, } } } @@ -7393,8 +7393,7 @@ mod tests { "\"user\"" ); assert_eq!( - serde_json::to_string(&ApprovalsReviewer::GuardianSubagent) - .expect("serialize reviewer"), + serde_json::to_string(&ApprovalsReviewer::AutoReview).expect("serialize reviewer"), "\"auto_review\"" ); @@ -7405,7 +7404,7 @@ mod tests { let expected = if value == "user" { ApprovalsReviewer::User } else { - ApprovalsReviewer::GuardianSubagent + ApprovalsReviewer::AutoReview }; assert_eq!(expected, reviewer); } @@ -8679,7 +8678,7 @@ mod tests { model_auto_compact_token_limit: None, model_provider: None, approval_policy: None, - approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), sandbox_mode: None, sandbox_workspace_write: None, forced_chatgpt_workspace_id: None, @@ -8781,7 +8780,7 @@ mod tests { model: None, model_provider: None, approval_policy: None, - approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), service_tier: None, model_reasoning_effort: None, model_reasoning_summary: None, diff --git a/codex-rs/app-server/src/config_api.rs b/codex-rs/app-server/src/config_api.rs index df4ad24d63..a1df875c28 100644 --- a/codex-rs/app-server/src/config_api.rs +++ b/codex-rs/app-server/src/config_api.rs @@ -459,7 +459,7 @@ mod tests { ]), allowed_approvals_reviewers: Some(vec![ CoreApprovalsReviewer::User, - CoreApprovalsReviewer::GuardianSubagent, + CoreApprovalsReviewer::AutoReview, ]), allowed_sandbox_modes: Some(vec![ CoreSandboxModeRequirement::ReadOnly, @@ -524,7 +524,7 @@ mod tests { mapped.allowed_approvals_reviewers, Some(vec![ codex_app_server_protocol::ApprovalsReviewer::User, - codex_app_server_protocol::ApprovalsReviewer::GuardianSubagent, + codex_app_server_protocol::ApprovalsReviewer::AutoReview, ]) ); assert_eq!( diff --git a/codex-rs/config/src/config_requirements.rs b/codex-rs/config/src/config_requirements.rs index ac954a1f4a..1d3ea49cf3 100644 --- a/codex-rs/config/src/config_requirements.rs +++ b/codex-rs/config/src/config_requirements.rs @@ -1185,7 +1185,7 @@ mod tests { let allowed_approval_policies = vec![AskForApproval::UnlessTrusted, AskForApproval::Never]; let allowed_approvals_reviewers = - vec![ApprovalsReviewer::GuardianSubagent, ApprovalsReviewer::User]; + vec![ApprovalsReviewer::AutoReview, ApprovalsReviewer::User]; let allowed_sandbox_modes = vec![ SandboxModeRequirement::WorkspaceWrite, SandboxModeRequirement::DangerFullAccess, @@ -1690,7 +1690,7 @@ allowed_approvals_reviewers = ["user"] Err(ConstraintError::InvalidValue { field_name: "approvals_reviewer", candidate: "User".into(), - allowed: "[GuardianSubagent]".into(), + allowed: "[AutoReview]".into(), requirement_source: source_location, }) ); @@ -1837,13 +1837,13 @@ allowed_approvals_reviewers = ["user"] assert_eq!( requirements.approvals_reviewer.value(), - ApprovalsReviewer::GuardianSubagent, + ApprovalsReviewer::AutoReview, "currently, there is no way to specify the default value for approvals reviewer in the toml, so it picks the first allowed value" ); assert!( requirements .approvals_reviewer - .can_set(&ApprovalsReviewer::GuardianSubagent) + .can_set(&ApprovalsReviewer::AutoReview) .is_ok() ); assert!( @@ -1866,7 +1866,7 @@ allowed_approvals_reviewers = ["user"] assert_eq!( requirements.approvals_reviewer.value(), - ApprovalsReviewer::GuardianSubagent + ApprovalsReviewer::AutoReview ); Ok(()) diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 3e685d92e9..3ce70c513b 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -251,7 +251,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f crate::session::tests::make_session_and_context_with_rx().await; let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref"); let mut config = (*parent_ctx.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; parent_ctx.config = Arc::new(config); parent_ctx .approval_policy @@ -363,7 +363,7 @@ async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() { crate::session::tests::make_session_and_context_with_rx().await; let mut parent_ctx = Arc::try_unwrap(parent_ctx).expect("single turn context ref"); let mut config = (*parent_ctx.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; parent_ctx.config = Arc::new(config); parent_ctx .approval_policy diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index a0d0583cb8..044c6c8289 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -6741,10 +6741,7 @@ approvals_reviewer = "guardian_subagent" .build() .await?; - assert_eq!( - config.approvals_reviewer, - ApprovalsReviewer::GuardianSubagent - ); + assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview); Ok(()) } @@ -6757,17 +6754,14 @@ async fn requirements_disallowing_default_approvals_reviewer_falls_back_to_requi .codex_home(codex_home.path().to_path_buf()) .cloud_requirements(CloudRequirementsLoader::new(async { Ok(Some(crate::config_loader::ConfigRequirementsToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]), + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), ..Default::default() })) })) .build() .await?; - assert_eq!( - config.approvals_reviewer, - ApprovalsReviewer::GuardianSubagent - ); + assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview); Ok(()) } @@ -6786,17 +6780,14 @@ async fn root_approvals_reviewer_falls_back_when_disallowed_by_requirements() -> .fallback_cwd(Some(codex_home.path().to_path_buf())) .cloud_requirements(CloudRequirementsLoader::new(async { Ok(Some(crate::config_loader::ConfigRequirementsToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]), + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), ..Default::default() })) })) .build() .await?; - assert_eq!( - config.approvals_reviewer, - ApprovalsReviewer::GuardianSubagent - ); + assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview); assert!( config.startup_warnings.iter().any(|warning| { warning @@ -6826,17 +6817,14 @@ approvals_reviewer = "user" .fallback_cwd(Some(codex_home.path().to_path_buf())) .cloud_requirements(CloudRequirementsLoader::new(async { Ok(Some(crate::config_loader::ConfigRequirementsToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]), + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), ..Default::default() })) })) .build() .await?; - assert_eq!( - config.approvals_reviewer, - ApprovalsReviewer::GuardianSubagent - ); + assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview); Ok(()) } @@ -6857,7 +6845,7 @@ async fn approvals_reviewer_preserves_valid_user_choice_when_allowed_by_requirem Ok(Some(crate::config_loader::ConfigRequirementsToml { allowed_approvals_reviewers: Some(vec![ ApprovalsReviewer::User, - ApprovalsReviewer::GuardianSubagent, + ApprovalsReviewer::AutoReview, ]), ..Default::default() })) @@ -6865,10 +6853,7 @@ async fn approvals_reviewer_preserves_valid_user_choice_when_allowed_by_requirem .build() .await?; - assert_eq!( - config.approvals_reviewer, - ApprovalsReviewer::GuardianSubagent - ); + assert_eq!(config.approvals_reviewer, ApprovalsReviewer::AutoReview); assert!( config .startup_warnings diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index d7a215c17d..95dd76f622 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -1048,7 +1048,7 @@ impl From for ConfigRequirementsToml { } if let Some(approvals_reviewer) = approvals_reviewer { let mut allowed_reviewers = vec![approvals_reviewer]; - if approvals_reviewer == ApprovalsReviewer::GuardianSubagent { + if approvals_reviewer == ApprovalsReviewer::AutoReview { allowed_reviewers.push(ApprovalsReviewer::User); } config_requirements_toml.allowed_approvals_reviewers = Some(allowed_reviewers); @@ -1135,7 +1135,7 @@ foo = "xyzzy" fn legacy_managed_config_backfill_allows_user_when_guardian_is_required() { let legacy = LegacyManagedConfigToml { approval_policy: None, - approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), sandbox_mode: None, }; @@ -1143,10 +1143,7 @@ foo = "xyzzy" assert_eq!( requirements.allowed_approvals_reviewers, - Some(vec![ - ApprovalsReviewer::GuardianSubagent, - ApprovalsReviewer::User, - ]) + Some(vec![ApprovalsReviewer::AutoReview, ApprovalsReviewer::User,]) ); } diff --git a/codex-rs/core/src/context/permissions_instructions.rs b/codex-rs/core/src/context/permissions_instructions.rs index db420f7464..6ba4e7c15d 100644 --- a/codex-rs/core/src/context/permissions_instructions.rs +++ b/codex-rs/core/src/context/permissions_instructions.rs @@ -188,7 +188,7 @@ fn approval_text( ), }; - if approvals_reviewer == ApprovalsReviewer::GuardianSubagent + if approvals_reviewer == ApprovalsReviewer::AutoReview && approval_policy != AskForApproval::Never { format!("{text}\n\n{AUTO_REVIEW_APPROVAL_SUFFIX}") diff --git a/codex-rs/core/src/context/permissions_instructions_tests.rs b/codex-rs/core/src/context/permissions_instructions_tests.rs index 2f3e8c6400..866c68b4a7 100644 --- a/codex-rs/core/src/context/permissions_instructions_tests.rs +++ b/codex-rs/core/src/context/permissions_instructions_tests.rs @@ -197,10 +197,10 @@ fn on_request_includes_tool_guidance_alongside_inline_permission_guidance_when_b } #[test] -fn guardian_subagent_approvals_append_guardian_specific_guidance() { +fn auto_review_approvals_append_auto_review_specific_guidance() { let text = approval_text( AskForApproval::OnRequest, - ApprovalsReviewer::GuardianSubagent, + ApprovalsReviewer::AutoReview, &Policy::empty(), /*exec_permission_approvals_enabled*/ false, /*request_permissions_tool_enabled*/ false, @@ -212,10 +212,10 @@ fn guardian_subagent_approvals_append_guardian_specific_guidance() { } #[test] -fn guardian_subagent_approvals_omit_guardian_specific_guidance_when_approval_is_never() { +fn auto_review_approvals_omit_auto_review_specific_guidance_when_approval_is_never() { let text = approval_text( AskForApproval::Never, - ApprovalsReviewer::GuardianSubagent, + ApprovalsReviewer::AutoReview, &Policy::empty(), /*exec_permission_approvals_enabled*/ false, /*request_permissions_tool_enabled*/ false, diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 3b5c63b4dc..2635d641d5 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -146,7 +146,7 @@ pub(crate) fn routes_approval_to_guardian(turn: &TurnContext) -> bool { matches!( turn.approval_policy.value(), AskForApproval::OnRequest | AskForApproval::Granular(_) - ) && turn.config.approvals_reviewer == ApprovalsReviewer::GuardianSubagent + ) && turn.config.approvals_reviewer == ApprovalsReviewer::AutoReview } pub(crate) fn is_guardian_reviewer_source( diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 41e33accf4..470a46498f 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -909,7 +909,7 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() { assert!(!routes_approval_to_guardian(&turn)); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; turn.config = Arc::new(config); assert!(routes_approval_to_guardian(&turn)); @@ -919,7 +919,7 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() { async fn routes_approval_to_guardian_allows_granular_review_policy() { let (_session, mut turn) = crate::session::tests::make_session_and_context().await; let mut config = (*turn.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; turn.config = Arc::new(config); turn.approval_policy .set(AskForApproval::Granular(GranularApprovalConfig { diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index d20f233951..ed355c47b4 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1411,7 +1411,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() { .expect("test setup should allow updating approval policy"); let mut config = (*turn_context.config).clone(); config.model_provider.base_url = Some(format!("{}/v1", server.uri())); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; let config = Arc::new(config); let models_manager = Arc::new(crate::test_support::models_manager_with_provider( config.codex_home.to_path_buf(), @@ -1490,7 +1490,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { .expect("test setup should allow updating approval policy"); let mut config = (*turn_context.config).clone(); config.model_provider.base_url = Some(format!("{}/v1", server.uri())); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; let config = Arc::new(config); let models_manager = Arc::new(crate::test_support::models_manager_with_provider( config.codex_home.to_path_buf(), @@ -1947,7 +1947,7 @@ async fn approve_mode_routes_arc_ask_user_to_guardian_when_guardian_reviewer_is_ let mut config = (*turn_context.config).clone(); config.chatgpt_base_url = server.uri(); config.model_provider.base_url = Some(format!("{}/v1", server.uri())); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; let config = Arc::new(config); let models_manager = Arc::new(crate::test_support::models_manager_with_provider( config.codex_home.to_path_buf(), diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 4c5526869d..4d88fa9233 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -3827,7 +3827,7 @@ async fn user_turn_updates_approvals_reviewer() { }], cwd: config.cwd.to_path_buf(), approval_policy: config.permissions.approval_policy.value(), - approvals_reviewer: Some(codex_config::types::ApprovalsReviewer::GuardianSubagent), + approvals_reviewer: Some(codex_config::types::ApprovalsReviewer::AutoReview), sandbox_policy: config.permissions.sandbox_policy.get().clone(), model: turn_context.model_info.slug.clone(), effort: config.model_reasoning_effort, @@ -3843,7 +3843,7 @@ async fn user_turn_updates_approvals_reviewer() { let state = session.state.lock().await; assert_eq!( state.session_configuration.approvals_reviewer, - codex_config::types::ApprovalsReviewer::GuardianSubagent + codex_config::types::ApprovalsReviewer::AutoReview ); } diff --git a/codex-rs/core/src/session/tests/guardian_tests.rs b/codex-rs/core/src/session/tests/guardian_tests.rs index c7496c0482..5057c43cee 100644 --- a/codex-rs/core/src/session/tests/guardian_tests.rs +++ b/codex-rs/core/src/session/tests/guardian_tests.rs @@ -88,7 +88,7 @@ async fn request_permissions_routes_to_guardian_when_reviewer_is_enabled() { .enable(Feature::GuardianApproval) .expect("test setup should allow enabling guardian approvals"); let mut config = (*turn_context_raw.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; 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( @@ -166,7 +166,7 @@ async fn request_permissions_guardian_review_stops_when_cancelled() { .enable(Feature::GuardianApproval) .expect("test setup should allow enabling guardian approvals"); let mut config = (*turn_context_raw.config).clone(); - config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + config.approvals_reviewer = ApprovalsReviewer::AutoReview; 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( diff --git a/codex-rs/exec/src/lib_tests.rs b/codex-rs/exec/src/lib_tests.rs index 50fe8552ba..f24c3fd578 100644 --- a/codex-rs/exec/src/lib_tests.rs +++ b/codex-rs/exec/src/lib_tests.rs @@ -375,7 +375,7 @@ async fn thread_start_params_include_review_policy_when_auto_review_is_enabled() let config = ConfigBuilder::default() .codex_home(codex_home.path().to_path_buf()) .harness_overrides(ConfigOverrides { - approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), ..Default::default() }) .fallback_cwd(Some(cwd.path().to_path_buf())) @@ -387,7 +387,7 @@ async fn thread_start_params_include_review_policy_when_auto_review_is_enabled() assert_eq!( params.approvals_reviewer, - Some(codex_app_server_protocol::ApprovalsReviewer::GuardianSubagent) + Some(codex_app_server_protocol::ApprovalsReviewer::AutoReview) ); } @@ -419,7 +419,7 @@ fn session_configured_from_thread_response_uses_review_policy_from_response() { cwd: test_path_buf("/tmp").abs(), instruction_sources: Vec::new(), approval_policy: codex_app_server_protocol::AskForApproval::OnRequest, - approvals_reviewer: codex_app_server_protocol::ApprovalsReviewer::GuardianSubagent, + approvals_reviewer: codex_app_server_protocol::ApprovalsReviewer::AutoReview, sandbox: codex_app_server_protocol::SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: codex_app_server_protocol::ReadOnlyAccess::FullAccess, @@ -440,8 +440,5 @@ fn session_configured_from_thread_response_uses_review_policy_from_response() { let event = session_configured_from_thread_start_response(&response) .expect("build bootstrap session configured event"); - assert_eq!( - event.approvals_reviewer, - ApprovalsReviewer::GuardianSubagent - ); + assert_eq!(event.approvals_reviewer, ApprovalsReviewer::AutoReview); } diff --git a/codex-rs/protocol/src/config_types.rs b/codex-rs/protocol/src/config_types.rs index 5c407d9960..06484c6441 100644 --- a/codex-rs/protocol/src/config_types.rs +++ b/codex-rs/protocol/src/config_types.rs @@ -89,7 +89,7 @@ pub enum ApprovalsReviewer { User, #[serde(rename = "auto_review", alias = "guardian_subagent")] #[strum(serialize = "auto_review")] - GuardianSubagent, + AutoReview, } impl JsonSchema for ApprovalsReviewer { @@ -608,8 +608,7 @@ mod tests { "\"user\"" ); assert_eq!( - serde_json::to_string(&ApprovalsReviewer::GuardianSubagent) - .expect("serialize reviewer"), + serde_json::to_string(&ApprovalsReviewer::AutoReview).expect("serialize reviewer"), "\"auto_review\"" ); @@ -620,7 +619,7 @@ mod tests { let expected = if value == "user" { ApprovalsReviewer::User } else { - ApprovalsReviewer::GuardianSubagent + ApprovalsReviewer::AutoReview }; assert_eq!(expected, reviewer); } diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index f2dfd5fa2b..8a90924412 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -300,7 +300,7 @@ fn default_exec_approval_decisions( } #[derive(Clone, Debug, PartialEq, Eq)] -struct GuardianApprovalsMode { +struct AutoReviewMode { approval_policy: AskForApproval, approvals_reviewer: ApprovalsReviewer, sandbox_policy: SandboxPolicy, @@ -309,11 +309,11 @@ struct GuardianApprovalsMode { /// Enabling the Auto-review experiment in the TUI should also switch the /// current `/approvals` settings to the matching Auto-review mode. Users /// can still change `/approvals` afterward; this just assumes that opting into -/// the experiment means they want guardian review enabled immediately. -fn guardian_approvals_mode() -> GuardianApprovalsMode { - GuardianApprovalsMode { +/// the experiment means they want Auto-review enabled immediately. +fn auto_review_mode() -> AutoReviewMode { + AutoReviewMode { approval_policy: AskForApproval::OnRequest, - approvals_reviewer: ApprovalsReviewer::GuardianSubagent, + approvals_reviewer: ApprovalsReviewer::AutoReview, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), } } diff --git a/codex-rs/tui/src/app/config_persistence.rs b/codex-rs/tui/src/app/config_persistence.rs index edfde7ceeb..20056e8d5d 100644 --- a/codex-rs/tui/src/app/config_persistence.rs +++ b/codex-rs/tui/src/app/config_persistence.rs @@ -126,7 +126,7 @@ impl App { return; } - let guardian_approvals_preset = guardian_approvals_mode(); + let auto_review_preset = auto_review_mode(); let mut next_config = self.config.clone(); let active_profile = self.active_profile.clone(); let scoped_segments = |key: &str| { @@ -202,16 +202,12 @@ impl App { // Persist the reviewer setting so future sessions keep the // experiment's matching `/approvals` mode until the user // changes it explicitly. - feature_config.approvals_reviewer = - guardian_approvals_preset.approvals_reviewer; + feature_config.approvals_reviewer = auto_review_preset.approvals_reviewer; feature_edits.push(ConfigEdit::SetPath { segments: scoped_segments("approvals_reviewer"), - value: guardian_approvals_preset - .approvals_reviewer - .to_string() - .into(), + value: auto_review_preset.approvals_reviewer.to_string().into(), }); - if previous_approvals_reviewer != guardian_approvals_preset.approvals_reviewer { + if previous_approvals_reviewer != auto_review_preset.approvals_reviewer { permissions_history_label = Some("Auto-review"); } } else if !effective_enabled { @@ -234,17 +230,17 @@ impl App { // makes guardian review observable in the current thread. if !self.try_set_approval_policy_on_config( &mut feature_config, - guardian_approvals_preset.approval_policy, + auto_review_preset.approval_policy, "Failed to enable Auto-review", - "failed to set guardian approvals approval policy on staged config", + "failed to set auto-review approval policy on staged config", ) { continue; } if !self.try_set_sandbox_policy_on_config( &mut feature_config, - guardian_approvals_preset.sandbox_policy.clone(), + auto_review_preset.sandbox_policy.clone(), "Failed to enable Auto-review", - "failed to set guardian approvals sandbox policy on staged config", + "failed to set auto-review sandbox policy on staged config", ) { continue; } @@ -258,8 +254,8 @@ impl App { value: "workspace-write".into(), }, ]); - approval_policy_override = Some(guardian_approvals_preset.approval_policy); - sandbox_policy_override = Some(guardian_approvals_preset.sandbox_policy.clone()); + approval_policy_override = Some(auto_review_preset.approval_policy); + sandbox_policy_override = Some(auto_review_preset.sandbox_policy.clone()); } next_config = feature_config; feature_updates_to_apply.push((feature, effective_enabled)); @@ -305,7 +301,7 @@ impl App { { tracing::error!( error = %err, - "failed to set guardian approvals sandbox policy on chat config" + "failed to set auto-review sandbox policy on chat config" ); self.chat_widget .add_error_message(format!("Failed to enable Auto-review: {err}")); diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 891c9649da..3cf6ff1a1b 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -1598,11 +1598,11 @@ async fn reset_memories_clears_local_memory_directories() -> Result<()> { } #[tokio::test] -async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() -> Result<()> { +async fn update_feature_flags_enabling_guardian_selects_auto_review() -> Result<()> { let (mut app, mut app_event_rx, mut op_rx) = make_test_app_with_channels().await; let codex_home = tempdir()?; app.config.codex_home = codex_home.path().to_path_buf().abs(); - let guardian_approvals = guardian_approvals_mode(); + let auto_review = auto_review_mode(); app.update_feature_flags(vec![(Feature::GuardianApproval, true)]) .await; @@ -1616,11 +1616,11 @@ async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() -> ); assert_eq!( app.config.approvals_reviewer, - guardian_approvals.approvals_reviewer + auto_review.approvals_reviewer ); assert_eq!( app.config.permissions.approval_policy.value(), - guardian_approvals.approval_policy + auto_review.approval_policy ); assert_eq!( app.chat_widget @@ -1628,7 +1628,7 @@ async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() -> .permissions .approval_policy .value(), - guardian_approvals.approval_policy + auto_review.approval_policy ); assert_eq!( app.chat_widget @@ -1636,11 +1636,11 @@ async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() -> .permissions .sandbox_policy .get(), - &guardian_approvals.sandbox_policy + &auto_review.sandbox_policy ); assert_eq!( app.chat_widget.config_ref().approvals_reviewer, - guardian_approvals.approvals_reviewer + auto_review.approvals_reviewer ); assert_eq!(app.runtime_approval_policy_override, None); assert_eq!(app.runtime_sandbox_policy_override, None); @@ -1648,9 +1648,9 @@ async fn update_feature_flags_enabling_guardian_selects_guardian_approvals() -> op_rx.try_recv(), Ok(Op::OverrideTurnContext { cwd: None, - approval_policy: Some(guardian_approvals.approval_policy), - approvals_reviewer: Some(guardian_approvals.approvals_reviewer), - sandbox_policy: Some(guardian_approvals.sandbox_policy.clone()), + approval_policy: Some(auto_review.approval_policy), + approvals_reviewer: Some(auto_review.approvals_reviewer), + sandbox_policy: Some(auto_review.sandbox_policy.clone()), permission_profile: None, windows_sandbox_level: None, model: None, @@ -1700,9 +1700,9 @@ async fn update_feature_flags_disabling_guardian_clears_review_policy_and_restor .set_enabled(Feature::GuardianApproval, /*enabled*/ true)?; app.chat_widget .set_feature_enabled(Feature::GuardianApproval, /*enabled*/ true); - app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + app.config.approvals_reviewer = ApprovalsReviewer::AutoReview; app.chat_widget - .set_approvals_reviewer(ApprovalsReviewer::GuardianSubagent); + .set_approvals_reviewer(ApprovalsReviewer::AutoReview); app.config .permissions .approval_policy @@ -1779,7 +1779,7 @@ async fn update_feature_flags_enabling_guardian_overrides_explicit_manual_review let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await; let codex_home = tempdir()?; app.config.codex_home = codex_home.path().to_path_buf().abs(); - let guardian_approvals = guardian_approvals_mode(); + let auto_review = auto_review_mode(); let config_toml_path = codex_home.path().join("config.toml").abs(); let config_toml = "approvals_reviewer = \"user\"\n"; std::fs::write(config_toml_path.as_path(), config_toml)?; @@ -1798,15 +1798,15 @@ async fn update_feature_flags_enabling_guardian_overrides_explicit_manual_review assert!(app.config.features.enabled(Feature::GuardianApproval)); assert_eq!( app.config.approvals_reviewer, - guardian_approvals.approvals_reviewer + auto_review.approvals_reviewer ); assert_eq!( app.chat_widget.config_ref().approvals_reviewer, - guardian_approvals.approvals_reviewer + auto_review.approvals_reviewer ); assert_eq!( app.config.permissions.approval_policy.value(), - guardian_approvals.approval_policy + auto_review.approval_policy ); assert_eq!( app.chat_widget @@ -1814,15 +1814,15 @@ async fn update_feature_flags_enabling_guardian_overrides_explicit_manual_review .permissions .sandbox_policy .get(), - &guardian_approvals.sandbox_policy + &auto_review.sandbox_policy ); assert_eq!( op_rx.try_recv(), Ok(Op::OverrideTurnContext { cwd: None, - approval_policy: Some(guardian_approvals.approval_policy), - approvals_reviewer: Some(guardian_approvals.approvals_reviewer), - sandbox_policy: Some(guardian_approvals.sandbox_policy.clone()), + approval_policy: Some(auto_review.approval_policy), + approvals_reviewer: Some(auto_review.approvals_reviewer), + sandbox_policy: Some(auto_review.sandbox_policy.clone()), permission_profile: None, windows_sandbox_level: None, model: None, @@ -1908,7 +1908,7 @@ async fn update_feature_flags_enabling_guardian_in_profile_sets_profile_auto_rev let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await; let codex_home = tempdir()?; app.config.codex_home = codex_home.path().to_path_buf().abs(); - let guardian_approvals = guardian_approvals_mode(); + let auto_review = auto_review_mode(); app.active_profile = Some("guardian".to_string()); let config_toml_path = codex_home.path().join("config.toml").abs(); let config_toml = "profile = \"guardian\"\napprovals_reviewer = \"user\"\n"; @@ -1928,19 +1928,19 @@ async fn update_feature_flags_enabling_guardian_in_profile_sets_profile_auto_rev assert!(app.config.features.enabled(Feature::GuardianApproval)); assert_eq!( app.config.approvals_reviewer, - guardian_approvals.approvals_reviewer + auto_review.approvals_reviewer ); assert_eq!( app.chat_widget.config_ref().approvals_reviewer, - guardian_approvals.approvals_reviewer + auto_review.approvals_reviewer ); assert_eq!( op_rx.try_recv(), Ok(Op::OverrideTurnContext { cwd: None, - approval_policy: Some(guardian_approvals.approval_policy), - approvals_reviewer: Some(guardian_approvals.approvals_reviewer), - sandbox_policy: Some(guardian_approvals.sandbox_policy.clone()), + approval_policy: Some(auto_review.approval_policy), + approvals_reviewer: Some(auto_review.approvals_reviewer), + sandbox_policy: Some(auto_review.sandbox_policy.clone()), permission_profile: None, windows_sandbox_level: None, model: None, @@ -2003,9 +2003,9 @@ guardian_approval = true .set_enabled(Feature::GuardianApproval, /*enabled*/ true)?; app.chat_widget .set_feature_enabled(Feature::GuardianApproval, /*enabled*/ true); - app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + app.config.approvals_reviewer = ApprovalsReviewer::AutoReview; app.chat_widget - .set_approvals_reviewer(ApprovalsReviewer::GuardianSubagent); + .set_approvals_reviewer(ApprovalsReviewer::AutoReview); app.update_feature_flags(vec![(Feature::GuardianApproval, false)]) .await; @@ -2083,9 +2083,9 @@ async fn update_feature_flags_disabling_guardian_in_profile_keeps_inherited_non_ .set_enabled(Feature::GuardianApproval, /*enabled*/ true)?; app.chat_widget .set_feature_enabled(Feature::GuardianApproval, /*enabled*/ true); - app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + app.config.approvals_reviewer = ApprovalsReviewer::AutoReview; app.chat_widget - .set_approvals_reviewer(ApprovalsReviewer::GuardianSubagent); + .set_approvals_reviewer(ApprovalsReviewer::AutoReview); app.update_feature_flags(vec![(Feature::GuardianApproval, false)]) .await; @@ -2097,13 +2097,10 @@ async fn update_feature_flags_disabling_guardian_in_profile_keeps_inherited_non_ .features .enabled(Feature::GuardianApproval) ); - assert_eq!( - app.config.approvals_reviewer, - ApprovalsReviewer::GuardianSubagent - ); + assert_eq!(app.config.approvals_reviewer, ApprovalsReviewer::AutoReview); assert_eq!( app.chat_widget.config_ref().approvals_reviewer, - ApprovalsReviewer::GuardianSubagent + ApprovalsReviewer::AutoReview ); assert!( op_rx.try_recv().is_err(), diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 093ff91aae..e154880fec 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -9042,7 +9042,7 @@ impl ChatWidget { "Same workspace-write permissions as Default, but eligible `on-request` approvals are routed through the auto-reviewer subagent." .to_string(), ), - is_current: current_review_policy == ApprovalsReviewer::GuardianSubagent + is_current: current_review_policy == ApprovalsReviewer::AutoReview && Self::preset_matches_current( current_approval, current_sandbox, @@ -9052,7 +9052,7 @@ impl ChatWidget { preset.approval, preset.sandbox.clone(), "Auto-review".to_string(), - ApprovalsReviewer::GuardianSubagent, + ApprovalsReviewer::AutoReview, ), dismiss_on_select: true, disabled_reason: approval_disabled_reason diff --git a/codex-rs/tui/src/chatwidget/tests/permissions.rs b/codex-rs/tui/src/chatwidget/tests/permissions.rs index 92ca170605..7ea6047700 100644 --- a/codex-rs/tui/src/chatwidget/tests/permissions.rs +++ b/codex-rs/tui/src/chatwidget/tests/permissions.rs @@ -386,7 +386,7 @@ async fn permissions_selection_emits_history_cell_when_current_is_selected() { } #[tokio::test] -async fn permissions_selection_hides_guardian_approvals_when_feature_disabled() { +async fn permissions_selection_hides_auto_review_when_feature_disabled() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; #[cfg(target_os = "windows")] { @@ -406,7 +406,7 @@ async fn permissions_selection_hides_guardian_approvals_when_feature_disabled() } #[tokio::test] -async fn permissions_selection_hides_guardian_approvals_when_feature_disabled_even_if_auto_review_is_active() +async fn permissions_selection_hides_auto_review_when_feature_disabled_even_if_auto_review_is_active() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; #[cfg(target_os = "windows")] @@ -416,7 +416,7 @@ async fn permissions_selection_hides_guardian_approvals_when_feature_disabled_ev } chat.set_feature_enabled(Feature::GuardianApproval, /*enabled*/ false); chat.config.notices.hide_full_access_warning = Some(true); - chat.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent; + chat.config.approvals_reviewer = ApprovalsReviewer::AutoReview; chat.config .permissions .approval_policy @@ -438,7 +438,7 @@ async fn permissions_selection_hides_guardian_approvals_when_feature_disabled_ev } #[tokio::test] -async fn permissions_selection_marks_guardian_approvals_current_after_session_configured() { +async fn permissions_selection_marks_auto_review_current_after_session_configured() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; #[cfg(target_os = "windows")] { @@ -461,7 +461,7 @@ async fn permissions_selection_marks_guardian_approvals_current_after_session_co model_provider_id: "test-provider".to_string(), service_tier: None, approval_policy: AskForApproval::OnRequest, - approvals_reviewer: ApprovalsReviewer::GuardianSubagent, + approvals_reviewer: ApprovalsReviewer::AutoReview, sandbox_policy: SandboxPolicy::new_workspace_write_policy(), cwd: test_project_path().abs(), reasoning_effort: None, @@ -483,8 +483,7 @@ async fn permissions_selection_marks_guardian_approvals_current_after_session_co } #[tokio::test] -async fn permissions_selection_marks_guardian_approvals_current_with_custom_workspace_write_details() - { +async fn permissions_selection_marks_auto_review_current_with_custom_workspace_write_details() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; #[cfg(target_os = "windows")] { @@ -509,7 +508,7 @@ async fn permissions_selection_marks_guardian_approvals_current_with_custom_work model_provider_id: "test-provider".to_string(), service_tier: None, approval_policy: AskForApproval::OnRequest, - approvals_reviewer: ApprovalsReviewer::GuardianSubagent, + approvals_reviewer: ApprovalsReviewer::AutoReview, sandbox_policy: SandboxPolicy::WorkspaceWrite { writable_roots: vec![extra_root], read_only_access: ReadOnlyAccess::FullAccess, @@ -537,7 +536,7 @@ async fn permissions_selection_marks_guardian_approvals_current_with_custom_work } #[tokio::test] -async fn permissions_selection_can_disable_guardian_approvals() { +async fn permissions_selection_can_disable_auto_review() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; #[cfg(target_os = "windows")] { @@ -630,7 +629,7 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context Op::OverrideTurnContext { cwd: None, approval_policy: Some(AskForApproval::OnRequest), - approvals_reviewer: Some(ApprovalsReviewer::GuardianSubagent), + approvals_reviewer: Some(ApprovalsReviewer::AutoReview), sandbox_policy: Some(SandboxPolicy::new_workspace_write_policy()), permission_profile: None, windows_sandbox_level: None, diff --git a/codex-rs/tui/src/debug_config.rs b/codex-rs/tui/src/debug_config.rs index 057611af24..c3383497fb 100644 --- a/codex-rs/tui/src/debug_config.rs +++ b/codex-rs/tui/src/debug_config.rs @@ -586,7 +586,7 @@ mod tests { Some(RequirementSource::CloudRequirements), ), approvals_reviewer: ConstrainedWithSource::new( - Constrained::allow_any(ApprovalsReviewer::GuardianSubagent), + Constrained::allow_any(ApprovalsReviewer::AutoReview), Some(RequirementSource::LegacyManagedConfigTomlFromMdm), ), sandbox_policy: ConstrainedWithSource::new( @@ -647,7 +647,7 @@ mod tests { let requirements_toml = ConfigRequirementsToml { allowed_approval_policies: Some(vec![AskForApproval::OnRequest]), - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]), + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), allowed_sandbox_modes: Some(vec![SandboxModeRequirement::ReadOnly]), remote_sandbox_config: None, allowed_web_search_modes: Some(vec![WebSearchModeRequirement::Cached]), @@ -731,13 +731,13 @@ mod tests { fn debug_config_output_lists_approvals_reviewer_as_requirement() { let requirements = ConfigRequirements { approvals_reviewer: ConstrainedWithSource::new( - Constrained::allow_any(ApprovalsReviewer::GuardianSubagent), + Constrained::allow_any(ApprovalsReviewer::AutoReview), Some(RequirementSource::LegacyManagedConfigTomlFromMdm), ), ..ConfigRequirements::default() }; let requirements_toml = ConfigRequirementsToml { - allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::GuardianSubagent]), + allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]), ..ConfigRequirementsToml::default() }; let stack = ConfigLayerStack::new(Vec::new(), requirements, requirements_toml) From 0e78ce80eebbb08856e4a212ecd045ab42d72948 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 22 Apr 2026 17:38:04 -0700 Subject: [PATCH 3/8] [3/4] Add executor-backed RMCP HTTP client (#18583) ### Why The RMCP layer needs a Streamable HTTP client that can talk either directly over `reqwest` or through the executor HTTP runner without duplicating MCP session logic higher in the stack. This PR adds that client-side transport boundary so remote Streamable HTTP MCP can reuse the same RMCP flow as the local path. ### What - Add a shared `rmcp-client/src/streamable_http/` module with: - `transport_client.rs` for the local-or-remote transport enum - `local_client.rs` for the direct `reqwest` implementation - `remote_client.rs` for the executor-backed implementation - `common.rs` for the small shared Streamable HTTP helpers - Teach `RmcpClient` to build Streamable HTTP transports in either local or remote mode while keeping the existing OAuth ownership in RMCP. - Translate remote POST, GET, and DELETE session operations into executor `http/request` calls. - Preserve RMCP session expiry handling and reconnect behavior for the remote transport. - Add remote transport coverage in `rmcp-client/tests/streamable_http_remote.rs` and keep the shared test support in `rmcp-client/tests/streamable_http_test_support.rs`. ### Verification - `cargo check -p codex-rmcp-client` - online CI ### Stack 1. #18581 protocol 2. #18582 runner 3. #18583 RMCP client 4. #18584 manager wiring and local/remote coverage --------- Co-authored-by: Codex --- codex-rs/Cargo.lock | 3 + .../codex-mcp/src/mcp_connection_manager.rs | 17 +- codex-rs/exec-server/Cargo.toml | 2 + codex-rs/exec-server/src/client.rs | 24 + .../exec-server/src/client/http_client.rs | 544 +----------------- .../src/client/http_response_body_stream.rs | 355 ++++++++++++ .../src/client/reqwest_http_client.rs | 267 +++++++++ .../exec-server/src/client/rpc_http_client.rs | 88 +++ codex-rs/exec-server/src/client_api.rs | 26 + codex-rs/exec-server/src/environment.rs | 13 +- codex-rs/exec-server/src/lib.rs | 3 + codex-rs/exec-server/src/server/handler.rs | 10 +- codex-rs/rmcp-client/BUILD.bazel | 3 + codex-rs/rmcp-client/Cargo.toml | 1 + .../rmcp-client/src/http_client_adapter.rs | 391 +++++++++++++ codex-rs/rmcp-client/src/lib.rs | 1 + codex-rs/rmcp-client/src/rmcp_client.rs | 278 +-------- .../tests/streamable_http_recovery.rs | 190 +----- .../tests/streamable_http_remote.rs | 37 ++ .../tests/streamable_http_test_support.rs | 314 ++++++++++ 20 files changed, 1595 insertions(+), 972 deletions(-) create mode 100644 codex-rs/exec-server/src/client/http_response_body_stream.rs create mode 100644 codex-rs/exec-server/src/client/reqwest_http_client.rs create mode 100644 codex-rs/exec-server/src/client/rpc_http_client.rs create mode 100644 codex-rs/rmcp-client/src/http_client_adapter.rs create mode 100644 codex-rs/rmcp-client/tests/streamable_http_remote.rs create mode 100644 codex-rs/rmcp-client/tests/streamable_http_test_support.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 6bc53a49fb..74473e174d 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2579,7 +2579,9 @@ dependencies = [ "arc-swap", "async-trait", "base64 0.22.1", + "bytes", "codex-app-server-protocol", + "codex-client", "codex-config", "codex-protocol", "codex-sandboxing", @@ -3141,6 +3143,7 @@ version = "0.0.0" dependencies = [ "anyhow", "axum", + "bytes", "codex-client", "codex-config", "codex-exec-server", diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager.rs b/codex-rs/codex-mcp/src/mcp_connection_manager.rs index 10c48e040e..fe8bc523ea 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager.rs @@ -1589,23 +1589,11 @@ async fn make_rmcp_client( env_http_headers, bearer_token_env_var, } => { - if remote_environment { - if !runtime_environment.environment().is_remote() { - return Err(StartupOutcomeError::from(anyhow!( - "remote MCP server `{server_name}` requires a remote executor environment" - ))); - } + if remote_environment && !runtime_environment.environment().is_remote() { return Err(StartupOutcomeError::from(anyhow!( - // Remote HTTP needs the future low-level executor - // `network/request` API so reqwest runs on the executor side. - // Do not fall back to local HTTP here; the config explicitly - // asked for remote placement. - "remote streamable HTTP MCP server `{server_name}` is not implemented yet" + "remote MCP server `{server_name}` requires a remote environment" ))); } - - // Local streamable HTTP remains the existing reqwest path from - // the orchestrator process. let resolved_bearer_token = match resolve_bearer_token(server_name, bearer_token_env_var.as_deref()) { Ok(token) => token, @@ -1618,6 +1606,7 @@ async fn make_rmcp_client( http_headers, env_http_headers, store_mode, + runtime_environment.environment().get_http_client(), ) .await .map_err(StartupOutcomeError::from) diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index 9688063278..a1a25e6e91 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -14,7 +14,9 @@ workspace = true arc-swap = { workspace = true } async-trait = { workspace = true } base64 = { workspace = true } +bytes = { workspace = true } codex-app-server-protocol = { workspace = true } +codex-client = { workspace = true } codex-config = { workspace = true } codex-protocol = { workspace = true } codex-sandboxing = { workspace = true } diff --git a/codex-rs/exec-server/src/client.rs b/codex-rs/exec-server/src/client.rs index 375571b779..f26069ac7a 100644 --- a/codex-rs/exec-server/src/client.rs +++ b/codex-rs/exec-server/src/client.rs @@ -8,6 +8,8 @@ use std::time::Duration; use arc_swap::ArcSwap; use codex_app_server_protocol::JSONRPCNotification; +use futures::FutureExt; +use futures::future::BoxFuture; use serde_json::Value; use tokio::sync::Mutex; use tokio::sync::OnceCell; @@ -20,6 +22,7 @@ use tracing::debug; use crate::ProcessId; use crate::client_api::ExecServerClientConnectOptions; +use crate::client_api::HttpClient; use crate::client_api::RemoteExecServerConnectArgs; use crate::connection::JsonRpcConnection; use crate::process::ExecProcessEvent; @@ -206,6 +209,25 @@ impl LazyRemoteExecServerClient { } } +impl HttpClient for LazyRemoteExecServerClient { + fn http_request( + &self, + params: crate::HttpRequestParams, + ) -> BoxFuture<'_, Result> { + async move { self.get().await?.http_request(params).await }.boxed() + } + + fn http_request_stream( + &self, + params: crate::HttpRequestParams, + ) -> BoxFuture< + '_, + Result<(crate::HttpRequestResponse, crate::HttpResponseBodyStream), ExecServerError>, + > { + async move { self.get().await?.http_request_stream(params).await }.boxed() + } +} + #[derive(Debug, thiserror::Error)] pub enum ExecServerError { #[error("failed to spawn exec-server: {0}")] @@ -226,6 +248,8 @@ pub enum ExecServerError { Disconnected(String), #[error("failed to serialize or deserialize exec-server JSON: {0}")] Json(#[from] serde_json::Error), + #[error("HTTP request failed: {0}")] + HttpRequest(String), #[error("exec-server protocol error: {0}")] Protocol(String), #[error("exec-server rejected request ({code}): {message}")] diff --git a/codex-rs/exec-server/src/client/http_client.rs b/codex-rs/exec-server/src/client/http_client.rs index 1e91fa4489..cfbb3a60bb 100644 --- a/codex-rs/exec-server/src/client/http_client.rs +++ b/codex-rs/exec-server/src/client/http_client.rs @@ -1,522 +1,26 @@ -use std::collections::HashMap; -use std::sync::Arc; -use std::sync::atomic::Ordering; -use std::time::Duration; +//! HTTP client capability implementations shared by local and remote environments. +//! +//! This module is the facade for the environment-owned [`crate::HttpClient`] +//! capability: +//! - [`ReqwestHttpClient`] executes requests directly with `reqwest` +//! - [`ExecServerClient`] forwards requests over the JSON-RPC transport +//! - [`HttpResponseBodyStream`] presents buffered local bodies and streamed +//! remote `http/request/bodyDelta` notifications through one byte-stream API +//! +//! Runtime split: +//! - orchestrator process: holds an `Arc` and chooses local or +//! remote execution +//! - remote runtime: serves the `http/request` RPC and runs the concrete local +//! HTTP request there when the orchestrator uses [`ExecServerClient`] -use codex_app_server_protocol::JSONRPCErrorError; -use futures::StreamExt; -use reqwest::Method; -use reqwest::Url; -use reqwest::header::HeaderMap; -use reqwest::header::HeaderName; -use reqwest::header::HeaderValue; -use serde_json::Value; -use serde_json::from_value; -use tokio::runtime::Handle; -use tokio::sync::mpsc; -use tokio::sync::mpsc::error::TrySendError; -use tracing::debug; +#[path = "reqwest_http_client.rs"] +mod reqwest_http_client; +#[path = "http_response_body_stream.rs"] +pub(crate) mod response_body_stream; +#[path = "rpc_http_client.rs"] +mod rpc_http_client; -use super::ExecServerClient; -use super::ExecServerError; -use super::Inner; -use crate::protocol::HTTP_REQUEST_BODY_DELTA_METHOD; -use crate::protocol::HTTP_REQUEST_METHOD; -use crate::protocol::HttpHeader; -use crate::protocol::HttpRequestBodyDeltaNotification; -use crate::protocol::HttpRequestParams; -use crate::protocol::HttpRequestResponse; -use crate::rpc::RpcNotificationSender; -use crate::rpc::internal_error; -use crate::rpc::invalid_params; - -/// Maximum queued body frames per streamed executor HTTP response. -const HTTP_BODY_DELTA_CHANNEL_CAPACITY: usize = 256; - -pub(crate) struct ExecutorPendingHttpBodyStream { - pub(crate) request_id: String, - response: reqwest::Response, -} - -pub(crate) struct ExecutorHttpRequestRunner { - client: reqwest::Client, -} - -/// Request-scoped stream of body chunks for an executor HTTP response. -/// -/// The initial `http/request` call returns status and headers. This stream then -/// receives the ordered `http/request/bodyDelta` notifications for that request -/// id until EOF or a terminal error. -pub struct HttpResponseBodyStream { - inner: Arc, - request_id: String, - next_seq: u64, - rx: mpsc::Receiver, - // Terminal frames can carry a final chunk; return that once, then EOF. - pending_eof: bool, - closed: bool, -} - -impl ExecServerClient { - /// Performs an executor-side HTTP request and buffers the response body. - pub async fn http_request( - &self, - mut params: HttpRequestParams, - ) -> Result { - params.stream_response = false; - self.call(HTTP_REQUEST_METHOD, ¶ms).await - } - - /// Performs an executor-side HTTP request and returns a body stream. - /// - /// The method sets `stream_response` and replaces any caller-supplied - /// `request_id` with a connection-local id, so late deltas from abandoned - /// streams cannot be confused with later requests. - pub async fn http_request_stream( - &self, - mut params: HttpRequestParams, - ) -> Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError> { - params.stream_response = true; - let request_id = self.inner.next_http_body_stream_request_id(); - params.request_id = request_id.clone(); - let (tx, rx) = mpsc::channel(HTTP_BODY_DELTA_CHANNEL_CAPACITY); - self.inner - .insert_http_body_stream(request_id.clone(), tx) - .await?; - let mut registration = HttpBodyStreamRegistration { - inner: Arc::clone(&self.inner), - request_id: request_id.clone(), - active: true, - }; - let response = match self.call(HTTP_REQUEST_METHOD, ¶ms).await { - Ok(response) => response, - Err(error) => { - self.inner.remove_http_body_stream(&request_id).await; - registration.active = false; - return Err(error); - } - }; - registration.active = false; - Ok(( - response, - HttpResponseBodyStream { - inner: Arc::clone(&self.inner), - request_id, - next_seq: 1, - rx, - pending_eof: false, - closed: false, - }, - )) - } -} - -impl HttpResponseBodyStream { - /// Receives the next response-body chunk. - /// - /// Returns `Ok(None)` at EOF and converts sequence gaps or executor-side - /// stream errors into protocol errors. - pub async fn recv(&mut self) -> Result>, ExecServerError> { - if self.pending_eof { - self.pending_eof = false; - self.finish().await; - return Ok(None); - } - - let Some(delta) = self.rx.recv().await else { - self.finish().await; - if let Some(error) = self - .inner - .take_http_body_stream_failure(&self.request_id) - .await - { - return Err(ExecServerError::Protocol(format!( - "http response stream `{}` failed: {error}", - self.request_id - ))); - } - return Ok(None); - }; - if delta.seq != self.next_seq { - self.finish().await; - return Err(ExecServerError::Protocol(format!( - "http response stream `{}` received seq {}, expected {}", - self.request_id, delta.seq, self.next_seq - ))); - } - self.next_seq += 1; - let chunk = delta.delta.into_inner(); - - if let Some(error) = delta.error { - self.finish().await; - return Err(ExecServerError::Protocol(format!( - "http response stream `{}` failed: {error}", - self.request_id - ))); - } - if delta.done { - self.finish().await; - if chunk.is_empty() { - return Ok(None); - } - self.pending_eof = true; - } - Ok(Some(chunk)) - } - - /// Removes this stream from the connection routing table once it reaches EOF. - async fn finish(&mut self) { - if self.closed { - return; - } - self.closed = true; - self.inner.remove_http_body_stream(&self.request_id).await; - } -} - -impl Drop for HttpResponseBodyStream { - /// Schedules stream-route removal if the consumer drops before EOF. - fn drop(&mut self) { - if self.closed { - return; - } - self.closed = true; - spawn_remove_http_body_stream(Arc::clone(&self.inner), self.request_id.clone()); - } -} - -impl ExecutorHttpRequestRunner { - pub(crate) fn new(timeout_ms: Option) -> Result { - let client = match timeout_ms { - None => reqwest::Client::builder(), - Some(timeout_ms) => { - reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) - } - } - .build() - .map_err(|err| internal_error(format!("failed to build http/request client: {err}")))?; - Ok(Self { client }) - } - - pub(crate) async fn run( - &self, - params: HttpRequestParams, - ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> - { - let method = Method::from_bytes(params.method.as_bytes()) - .map_err(|err| invalid_params(format!("http/request method is invalid: {err}")))?; - let url = Url::parse(¶ms.url) - .map_err(|err| invalid_params(format!("http/request url is invalid: {err}")))?; - match url.scheme() { - "http" | "https" => {} - scheme => { - return Err(invalid_params(format!( - "http/request only supports http and https URLs, got {scheme}" - ))); - } - } - - let headers = Self::build_headers(params.headers)?; - let mut request = self.client.request(method, url).headers(headers); - if let Some(body) = params.body { - request = request.body(body.into_inner()); - } - - let response = request - .send() - .await - .map_err(|err| internal_error(format!("http/request failed: {err}")))?; - let status = response.status().as_u16(); - let headers = Self::response_headers(response.headers()); - - if params.stream_response { - return Ok(( - HttpRequestResponse { - status, - headers, - body: Vec::new().into(), - }, - Some(ExecutorPendingHttpBodyStream { - request_id: params.request_id, - response, - }), - )); - } - - let body = response.bytes().await.map_err(|err| { - internal_error(format!("failed to read http/request response body: {err}")) - })?; - - Ok(( - HttpRequestResponse { - status, - headers, - body: body.to_vec().into(), - }, - None, - )) - } - - fn build_headers(headers: Vec) -> Result { - let mut header_map = HeaderMap::new(); - for header in headers { - let name = HeaderName::from_bytes(header.name.as_bytes()).map_err(|err| { - invalid_params(format!("http/request header name is invalid: {err}")) - })?; - let value = HeaderValue::from_str(&header.value).map_err(|err| { - invalid_params(format!( - "http/request header value is invalid for {}: {err}", - header.name - )) - })?; - header_map.append(name, value); - } - Ok(header_map) - } - - fn response_headers(headers: &HeaderMap) -> Vec { - headers - .iter() - .filter_map(|(name, value)| { - Some(HttpHeader { - name: name.as_str().to_string(), - value: value.to_str().ok()?.to_string(), - }) - }) - .collect() - } - - pub(crate) async fn stream_body( - pending_stream: ExecutorPendingHttpBodyStream, - notifications: RpcNotificationSender, - ) { - let ExecutorPendingHttpBodyStream { - request_id, - response, - } = pending_stream; - let mut seq = 1; - let mut body = response.bytes_stream(); - while let Some(chunk) = body.next().await { - match chunk { - Ok(bytes) => { - if !send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id: request_id.clone(), - seq, - delta: bytes.to_vec().into(), - done: false, - error: None, - }, - ) - .await - { - return; - } - seq += 1; - } - Err(err) => { - let _ = send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: Some(err.to_string()), - }, - ) - .await; - return; - } - } - } - - let _ = send_executor_body_delta( - ¬ifications, - HttpRequestBodyDeltaNotification { - request_id, - seq, - delta: Vec::new().into(), - done: true, - error: None, - }, - ) - .await; - } -} - -impl Inner { - /// Routes one streamed HTTP body notification into its request-local receiver. - pub(super) async fn handle_http_body_delta_notification( - &self, - params: Option, - ) -> Result<(), ExecServerError> { - let params: HttpRequestBodyDeltaNotification = from_value(params.unwrap_or(Value::Null))?; - // Unknown request ids are ignored intentionally: a stream may have already - // reached EOF and released its route. - if let Some(tx) = self - .http_body_streams - .load() - .get(¶ms.request_id) - .cloned() - { - let request_id = params.request_id.clone(); - let terminal_delta = params.done || params.error.is_some(); - match tx.try_send(params) { - Ok(()) => { - if terminal_delta { - self.remove_http_body_stream(&request_id).await; - } - } - Err(TrySendError::Closed(_)) => { - self.remove_http_body_stream(&request_id).await; - debug!("http response stream receiver dropped before body delta delivery"); - } - Err(TrySendError::Full(_)) => { - self.record_http_body_stream_failure( - &request_id, - "body delta channel filled before delivery".to_string(), - ) - .await; - self.remove_http_body_stream(&request_id).await; - debug!( - "closing http response stream `{request_id}` after body delta backpressure" - ); - } - } - } - Ok(()) - } - - /// Fails active streamed HTTP bodies so callers do not wait forever after a - /// transport disconnect or notification handling failure. - pub(super) async fn fail_all_http_body_streams(&self, message: String) { - let _streams_write_guard = self.http_body_streams_write_lock.lock().await; - let streams = self.http_body_streams.load(); - let streams = streams.as_ref().clone(); - self.http_body_streams.store(Arc::new(HashMap::new())); - for (request_id, tx) in streams { - if tx - .try_send(HttpRequestBodyDeltaNotification { - request_id: request_id.clone(), - seq: 1, - delta: Vec::new().into(), - done: true, - error: Some(message.clone()), - }) - .is_err() - { - let mut next_failures = self.http_body_stream_failures.load().as_ref().clone(); - next_failures.insert(request_id, message.clone()); - self.http_body_stream_failures - .store(Arc::new(next_failures)); - } - } - } - - /// Allocates a connection-local streamed HTTP response id. - fn next_http_body_stream_request_id(&self) -> String { - let id = self - .http_body_stream_next_id - .fetch_add(1, Ordering::Relaxed); - format!("http-{id}") - } - - /// Registers a request id before issuing an executor streaming HTTP call. - async fn insert_http_body_stream( - &self, - request_id: String, - tx: mpsc::Sender, - ) -> Result<(), ExecServerError> { - let _streams_write_guard = self.http_body_streams_write_lock.lock().await; - let streams = self.http_body_streams.load(); - if streams.contains_key(&request_id) { - return Err(ExecServerError::Protocol(format!( - "http response stream already registered for request {request_id}" - ))); - } - let mut next_streams = streams.as_ref().clone(); - next_streams.insert(request_id.clone(), tx); - self.http_body_streams.store(Arc::new(next_streams)); - let failures = self.http_body_stream_failures.load(); - if failures.contains_key(&request_id) { - let mut next_failures = failures.as_ref().clone(); - next_failures.remove(&request_id); - self.http_body_stream_failures - .store(Arc::new(next_failures)); - } - Ok(()) - } - - /// Removes a request id after EOF, terminal error, or request failure. - async fn remove_http_body_stream( - &self, - request_id: &str, - ) -> Option> { - let _streams_write_guard = self.http_body_streams_write_lock.lock().await; - let streams = self.http_body_streams.load(); - let stream = streams.get(request_id).cloned(); - stream.as_ref()?; - let mut next_streams = streams.as_ref().clone(); - next_streams.remove(request_id); - self.http_body_streams.store(Arc::new(next_streams)); - stream - } - - async fn record_http_body_stream_failure(&self, request_id: &str, message: String) { - let _streams_write_guard = self.http_body_streams_write_lock.lock().await; - let failures = self.http_body_stream_failures.load(); - let mut next_failures = failures.as_ref().clone(); - next_failures.insert(request_id.to_string(), message); - self.http_body_stream_failures - .store(Arc::new(next_failures)); - } - - async fn take_http_body_stream_failure(&self, request_id: &str) -> Option { - let _streams_write_guard = self.http_body_streams_write_lock.lock().await; - let failures = self.http_body_stream_failures.load(); - let error = failures.get(request_id).cloned(); - error.as_ref()?; - let mut next_failures = failures.as_ref().clone(); - next_failures.remove(request_id); - self.http_body_stream_failures - .store(Arc::new(next_failures)); - error - } -} - -/// Active route registration owned while `http_request_stream` awaits headers. -struct HttpBodyStreamRegistration { - inner: Arc, - request_id: String, - active: bool, -} - -impl Drop for HttpBodyStreamRegistration { - /// Removes the route if the stream request future is cancelled before headers return. - fn drop(&mut self) { - if self.active { - spawn_remove_http_body_stream(Arc::clone(&self.inner), self.request_id.clone()); - } - } -} - -/// Schedules HTTP body route removal from synchronous drop paths. -fn spawn_remove_http_body_stream(inner: Arc, request_id: String) { - if let Ok(handle) = Handle::try_current() { - handle.spawn(async move { - inner.remove_http_body_stream(&request_id).await; - }); - } -} - -async fn send_executor_body_delta( - notifications: &RpcNotificationSender, - delta: HttpRequestBodyDeltaNotification, -) -> bool { - notifications - .notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta) - .await - .is_ok() -} +pub(crate) use reqwest_http_client::PendingReqwestHttpBodyStream; +pub use reqwest_http_client::ReqwestHttpClient; +pub(crate) use reqwest_http_client::ReqwestHttpRequestRunner; +pub use response_body_stream::HttpResponseBodyStream; diff --git a/codex-rs/exec-server/src/client/http_response_body_stream.rs b/codex-rs/exec-server/src/client/http_response_body_stream.rs new file mode 100644 index 0000000000..3435af7d59 --- /dev/null +++ b/codex-rs/exec-server/src/client/http_response_body_stream.rs @@ -0,0 +1,355 @@ +//! Shared HTTP response-body stream plumbing for local and remote execution. +//! +//! This module owns the byte-stream type exposed by the `HttpClient` +//! capability plus the remote-side routing table used to turn +//! `http/request/bodyDelta` notifications back into per-request streams. + +use std::collections::HashMap; +use std::pin::Pin; +use std::sync::Arc; +use std::sync::atomic::Ordering; + +use bytes::Bytes; +use futures::StreamExt; +use reqwest::Response; +use serde_json::Value; +use serde_json::from_value; +use tokio::runtime::Handle; +use tokio::sync::mpsc; +use tokio::sync::mpsc::error::TrySendError; +use tracing::debug; + +use crate::client::ExecServerError; +use crate::client::Inner; +use crate::protocol::HTTP_REQUEST_BODY_DELTA_METHOD; +use crate::protocol::HttpRequestBodyDeltaNotification; +use crate::rpc::RpcNotificationSender; + +pub(super) struct HttpBodyStreamRegistration { + inner: Arc, + request_id: String, + active: bool, +} + +enum HttpResponseBodyStreamInner { + Local { + body: Pin> + Send>>, + }, + Remote { + inner: Arc, + request_id: String, + next_seq: u64, + rx: mpsc::Receiver, + pending_eof: bool, + closed: bool, + }, +} + +/// Request-scoped stream of body chunks for an HTTP response. +/// +/// The initial `http/request` call returns status and headers. This stream then +/// receives the ordered `http/request/bodyDelta` notifications for that request +/// id until EOF or a terminal error. +pub struct HttpResponseBodyStream { + inner: HttpResponseBodyStreamInner, +} + +impl HttpResponseBodyStream { + pub(super) fn local(response: Response) -> Self { + Self { + inner: HttpResponseBodyStreamInner::Local { + body: Box::pin(response.bytes_stream()), + }, + } + } + + pub(super) fn remote( + inner: Arc, + request_id: String, + rx: mpsc::Receiver, + ) -> Self { + Self { + inner: HttpResponseBodyStreamInner::Remote { + inner, + request_id, + next_seq: 1, + rx, + pending_eof: false, + closed: false, + }, + } + } + + /// Receives the next response-body chunk. + /// + /// Returns `Ok(None)` at EOF and converts sequence gaps or stream-side + /// stream errors into protocol errors. + pub async fn recv(&mut self) -> Result>, ExecServerError> { + match &mut self.inner { + HttpResponseBodyStreamInner::Local { body } => match body.next().await { + Some(chunk) => match chunk { + Ok(bytes) => Ok(Some(bytes.to_vec())), + Err(error) => Err(ExecServerError::HttpRequest(error.to_string())), + }, + None => Ok(None), + }, + HttpResponseBodyStreamInner::Remote { + inner, + request_id, + next_seq, + rx, + pending_eof, + closed, + } => { + if *pending_eof { + *pending_eof = false; + finish_remote_stream(inner, request_id, closed).await; + return Ok(None); + } + + let Some(delta) = rx.recv().await else { + finish_remote_stream(inner, request_id, closed).await; + if let Some(error) = inner.take_http_body_stream_failure(request_id).await { + return Err(ExecServerError::Protocol(format!( + "http response stream `{request_id}` failed: {error}", + ))); + } + return Ok(None); + }; + if delta.seq != *next_seq { + finish_remote_stream(inner, request_id, closed).await; + return Err(ExecServerError::Protocol(format!( + "http response stream `{request_id}` received seq {}, expected {}", + delta.seq, *next_seq + ))); + } + *next_seq += 1; + let chunk = delta.delta.into_inner(); + + if let Some(error) = delta.error { + finish_remote_stream(inner, request_id, closed).await; + return Err(ExecServerError::Protocol(format!( + "http response stream `{request_id}` failed: {error}", + ))); + } + if delta.done { + finish_remote_stream(inner, request_id, closed).await; + if chunk.is_empty() { + return Ok(None); + } + *pending_eof = true; + } + Ok(Some(chunk)) + } + } + } +} + +impl Drop for HttpResponseBodyStream { + /// Schedules stream-route removal if the consumer drops before EOF. + fn drop(&mut self) { + if let HttpResponseBodyStreamInner::Remote { + inner, + request_id, + closed, + .. + } = &mut self.inner + { + if *closed { + return; + } + *closed = true; + spawn_remove_http_body_stream(Arc::clone(inner), request_id.clone()); + } + } +} + +impl HttpBodyStreamRegistration { + pub(super) fn new(inner: Arc, request_id: String) -> Self { + Self { + inner, + request_id, + active: true, + } + } + + pub(super) fn disarm(&mut self) { + self.active = false; + } +} + +impl Drop for HttpBodyStreamRegistration { + /// Removes the route if the stream request future is cancelled before headers return. + fn drop(&mut self) { + if self.active { + spawn_remove_http_body_stream(Arc::clone(&self.inner), self.request_id.clone()); + } + } +} + +async fn finish_remote_stream(inner: &Arc, request_id: &str, closed: &mut bool) { + if *closed { + return; + } + *closed = true; + inner.remove_http_body_stream(request_id).await; +} + +/// Schedules HTTP body route removal from synchronous drop paths. +fn spawn_remove_http_body_stream(inner: Arc, request_id: String) { + if let Ok(handle) = Handle::try_current() { + handle.spawn(async move { + inner.remove_http_body_stream(&request_id).await; + }); + } +} + +pub(super) async fn send_body_delta( + notifications: &RpcNotificationSender, + delta: HttpRequestBodyDeltaNotification, +) -> bool { + notifications + .notify(HTTP_REQUEST_BODY_DELTA_METHOD, &delta) + .await + .is_ok() +} + +impl Inner { + /// Routes one streamed HTTP body notification into its request-local receiver. + pub(crate) async fn handle_http_body_delta_notification( + &self, + params: Option, + ) -> Result<(), ExecServerError> { + let params: HttpRequestBodyDeltaNotification = from_value(params.unwrap_or(Value::Null))?; + // Unknown request ids are ignored intentionally: a stream may have already + // reached EOF and released its route. + if let Some(tx) = self + .http_body_streams + .load() + .get(¶ms.request_id) + .cloned() + { + let request_id = params.request_id.clone(); + let terminal_delta = params.done || params.error.is_some(); + match tx.try_send(params) { + Ok(()) => { + if terminal_delta { + self.remove_http_body_stream(&request_id).await; + } + } + Err(TrySendError::Closed(_)) => { + self.remove_http_body_stream(&request_id).await; + debug!("http response stream receiver dropped before body delta delivery"); + } + Err(TrySendError::Full(_)) => { + self.record_http_body_stream_failure( + &request_id, + "body delta channel filled before delivery".to_string(), + ) + .await; + self.remove_http_body_stream(&request_id).await; + debug!( + "closing http response stream `{request_id}` after body delta backpressure" + ); + } + } + } + Ok(()) + } + + /// Fails active streamed HTTP bodies so callers do not wait forever after a + /// transport disconnect or notification handling failure. + pub(crate) async fn fail_all_http_body_streams(&self, message: String) { + let _streams_write_guard = self.http_body_streams_write_lock.lock().await; + let streams = self.http_body_streams.load(); + let streams = streams.as_ref().clone(); + self.http_body_streams.store(Arc::new(HashMap::new())); + for (request_id, tx) in streams { + if tx + .try_send(HttpRequestBodyDeltaNotification { + request_id: request_id.clone(), + seq: 1, + delta: Vec::new().into(), + done: true, + error: Some(message.clone()), + }) + .is_err() + { + let mut next_failures = self.http_body_stream_failures.load().as_ref().clone(); + next_failures.insert(request_id, message.clone()); + self.http_body_stream_failures + .store(Arc::new(next_failures)); + } + } + } + + /// Allocates a connection-local streamed HTTP response id. + pub(super) fn next_http_body_stream_request_id(&self) -> String { + let id = self + .http_body_stream_next_id + .fetch_add(1, Ordering::Relaxed); + format!("http-{id}") + } + + /// Registers a request id before issuing a streaming HTTP call. + pub(super) async fn insert_http_body_stream( + &self, + request_id: String, + tx: mpsc::Sender, + ) -> Result<(), ExecServerError> { + let _streams_write_guard = self.http_body_streams_write_lock.lock().await; + let streams = self.http_body_streams.load(); + if streams.contains_key(&request_id) { + return Err(ExecServerError::Protocol(format!( + "http response stream already registered for request {request_id}" + ))); + } + let mut next_streams = streams.as_ref().clone(); + next_streams.insert(request_id.clone(), tx); + self.http_body_streams.store(Arc::new(next_streams)); + let failures = self.http_body_stream_failures.load(); + if failures.contains_key(&request_id) { + let mut next_failures = failures.as_ref().clone(); + next_failures.remove(&request_id); + self.http_body_stream_failures + .store(Arc::new(next_failures)); + } + Ok(()) + } + + /// Removes a request id after EOF, terminal error, or request failure. + pub(super) async fn remove_http_body_stream( + &self, + request_id: &str, + ) -> Option> { + let _streams_write_guard = self.http_body_streams_write_lock.lock().await; + let streams = self.http_body_streams.load(); + let stream = streams.get(request_id).cloned(); + stream.as_ref()?; + let mut next_streams = streams.as_ref().clone(); + next_streams.remove(request_id); + self.http_body_streams.store(Arc::new(next_streams)); + stream + } + + async fn record_http_body_stream_failure(&self, request_id: &str, message: String) { + let _streams_write_guard = self.http_body_streams_write_lock.lock().await; + let failures = self.http_body_stream_failures.load(); + let mut next_failures = failures.as_ref().clone(); + next_failures.insert(request_id.to_string(), message); + self.http_body_stream_failures + .store(Arc::new(next_failures)); + } + + async fn take_http_body_stream_failure(&self, request_id: &str) -> Option { + let _streams_write_guard = self.http_body_streams_write_lock.lock().await; + let failures = self.http_body_stream_failures.load(); + let error = failures.get(request_id).cloned(); + error.as_ref()?; + let mut next_failures = failures.as_ref().clone(); + next_failures.remove(request_id); + self.http_body_stream_failures + .store(Arc::new(next_failures)); + error + } +} diff --git a/codex-rs/exec-server/src/client/reqwest_http_client.rs b/codex-rs/exec-server/src/client/reqwest_http_client.rs new file mode 100644 index 0000000000..44305e466a --- /dev/null +++ b/codex-rs/exec-server/src/client/reqwest_http_client.rs @@ -0,0 +1,267 @@ +//! `reqwest`-backed `HttpClient` implementation. +//! +//! This code runs wherever the real network request should originate: +//! - in a local environment, that means the orchestrator process +//! - in a remote environment, that means the remote runtime after the +//! orchestrator has forwarded `http/request` over JSON-RPC + +use std::time::Duration; + +use codex_app_server_protocol::JSONRPCErrorError; +use codex_client::build_reqwest_client_with_custom_ca; +use futures::FutureExt; +use futures::StreamExt; +use futures::future::BoxFuture; +use reqwest::Method; +use reqwest::Url; +use reqwest::header::HeaderMap; +use reqwest::header::HeaderName; +use reqwest::header::HeaderValue; + +use super::HttpResponseBodyStream; +use super::response_body_stream::send_body_delta; +use crate::HttpClient; +use crate::client::ExecServerError; +use crate::protocol::HttpHeader; +use crate::protocol::HttpRequestBodyDeltaNotification; +use crate::protocol::HttpRequestParams; +use crate::protocol::HttpRequestResponse; +use crate::rpc::RpcNotificationSender; +use crate::rpc::internal_error; +use crate::rpc::invalid_params; + +/// `HttpClient` implementation that performs the actual HTTP request with +/// `reqwest`. +#[derive(Clone, Default)] +pub struct ReqwestHttpClient; + +/// Streaming response state held between the initial HTTP response and +/// downstream body-delta forwarding. +pub(crate) struct PendingReqwestHttpBodyStream { + pub(crate) request_id: String, + pub(crate) response: reqwest::Response, +} + +/// Validates `http/request` parameters and runs the actual `reqwest` call used +/// by the exec-server route and the local [`HttpClient`] backend. +pub(crate) struct ReqwestHttpRequestRunner { + client: reqwest::Client, +} + +impl ReqwestHttpClient { + fn build_client(timeout_ms: Option) -> Result { + let builder = match timeout_ms { + None => reqwest::Client::builder(), + Some(timeout_ms) => { + reqwest::Client::builder().timeout(Duration::from_millis(timeout_ms)) + } + }; + build_reqwest_client_with_custom_ca(builder) + .map_err(|error| ExecServerError::HttpRequest(error.to_string())) + } +} + +impl HttpClient for ReqwestHttpClient { + fn http_request( + &self, + params: HttpRequestParams, + ) -> BoxFuture<'_, Result> { + async move { + let runner = ReqwestHttpRequestRunner::new(params.timeout_ms) + .map_err(|error| ExecServerError::HttpRequest(error.message))?; + let (response, _) = runner + .run(HttpRequestParams { + stream_response: false, + ..params + }) + .await + .map_err(|error| ExecServerError::HttpRequest(error.message))?; + Ok(response) + } + .boxed() + } + + fn http_request_stream( + &self, + params: HttpRequestParams, + ) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>> { + async move { + let runner = ReqwestHttpRequestRunner::new(params.timeout_ms) + .map_err(|error| ExecServerError::HttpRequest(error.message))?; + let (response, pending_stream) = runner + .run(HttpRequestParams { + stream_response: true, + ..params + }) + .await + .map_err(|error| ExecServerError::HttpRequest(error.message))?; + let pending_stream = pending_stream.ok_or_else(|| { + ExecServerError::Protocol( + "http request stream did not return a response body stream".to_string(), + ) + })?; + Ok(( + response, + HttpResponseBodyStream::local(pending_stream.response), + )) + } + .boxed() + } +} + +impl ReqwestHttpRequestRunner { + pub(crate) fn new(timeout_ms: Option) -> Result { + let client = ReqwestHttpClient::build_client(timeout_ms) + .map_err(|error| internal_error(error.to_string()))?; + Ok(Self { client }) + } + + pub(crate) async fn run( + &self, + params: HttpRequestParams, + ) -> Result<(HttpRequestResponse, Option), JSONRPCErrorError> + { + let method = Method::from_bytes(params.method.as_bytes()) + .map_err(|error| invalid_params(format!("http/request method is invalid: {error}")))?; + let url = Url::parse(¶ms.url) + .map_err(|error| invalid_params(format!("http/request url is invalid: {error}")))?; + match url.scheme() { + "http" | "https" => {} + scheme => { + return Err(invalid_params(format!( + "http/request only supports http and https URLs, got {scheme}" + ))); + } + } + + let headers = Self::build_headers(params.headers)?; + let mut request = self.client.request(method, url).headers(headers); + if let Some(body) = params.body { + request = request.body(body.into_inner()); + } + + let response = request + .send() + .await + .map_err(|error| internal_error(format!("http/request failed: {error}")))?; + let status = response.status().as_u16(); + let headers = Self::response_headers(response.headers()); + + if params.stream_response { + return Ok(( + HttpRequestResponse { + status, + headers, + body: Vec::new().into(), + }, + Some(PendingReqwestHttpBodyStream { + request_id: params.request_id, + response, + }), + )); + } + + let body = response.bytes().await.map_err(|error| { + internal_error(format!( + "failed to read http/request response body: {error}" + )) + })?; + + Ok(( + HttpRequestResponse { + status, + headers, + body: body.to_vec().into(), + }, + None, + )) + } + + pub(crate) async fn stream_body( + pending_stream: PendingReqwestHttpBodyStream, + notifications: RpcNotificationSender, + ) { + let PendingReqwestHttpBodyStream { + request_id, + response, + } = pending_stream; + let mut seq = 1; + let mut body = response.bytes_stream(); + while let Some(chunk) = body.next().await { + match chunk { + Ok(bytes) => { + if !send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id: request_id.clone(), + seq, + delta: bytes.to_vec().into(), + done: false, + error: None, + }, + ) + .await + { + return; + } + seq += 1; + } + Err(error) => { + let _ = send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: Some(error.to_string()), + }, + ) + .await; + return; + } + } + } + + let _ = send_body_delta( + ¬ifications, + HttpRequestBodyDeltaNotification { + request_id, + seq, + delta: Vec::new().into(), + done: true, + error: None, + }, + ) + .await; + } + + fn build_headers(headers: Vec) -> Result { + let mut header_map = HeaderMap::new(); + for header in headers { + let name = HeaderName::from_bytes(header.name.as_bytes()).map_err(|error| { + invalid_params(format!("http/request header name is invalid: {error}")) + })?; + let value = HeaderValue::from_str(&header.value).map_err(|error| { + invalid_params(format!( + "http/request header value is invalid for {}: {error}", + header.name + )) + })?; + header_map.append(name, value); + } + Ok(header_map) + } + + fn response_headers(headers: &HeaderMap) -> Vec { + headers + .iter() + .filter_map(|(name, value)| { + Some(HttpHeader { + name: name.as_str().to_string(), + value: value.to_str().ok()?.to_string(), + }) + }) + .collect() + } +} diff --git a/codex-rs/exec-server/src/client/rpc_http_client.rs b/codex-rs/exec-server/src/client/rpc_http_client.rs new file mode 100644 index 0000000000..d2ce842ca9 --- /dev/null +++ b/codex-rs/exec-server/src/client/rpc_http_client.rs @@ -0,0 +1,88 @@ +//! JSON-RPC-backed `HttpClient` implementation. +//! +//! This code runs in the orchestrator process. It does not issue network +//! requests directly; instead it forwards `http/request` to the remote runtime +//! and then reconstructs streamed bodies from `http/request/bodyDelta` +//! notifications on the shared connection. + +use std::sync::Arc; + +use futures::FutureExt; +use futures::future::BoxFuture; +use tokio::sync::mpsc; + +use super::HttpResponseBodyStream; +use super::response_body_stream::HttpBodyStreamRegistration; +use crate::HttpClient; +use crate::client::ExecServerClient; +use crate::client::ExecServerError; +use crate::protocol::HTTP_REQUEST_METHOD; +use crate::protocol::HttpRequestParams; +use crate::protocol::HttpRequestResponse; + +/// Maximum queued body frames per streamed HTTP response. +const HTTP_BODY_DELTA_CHANNEL_CAPACITY: usize = 256; + +impl ExecServerClient { + /// Performs an HTTP request and buffers the response body. + pub async fn http_request( + &self, + mut params: HttpRequestParams, + ) -> Result { + params.stream_response = false; + self.call(HTTP_REQUEST_METHOD, ¶ms).await + } + + /// Performs an HTTP request and returns a body stream. + /// + /// The method sets `stream_response` and replaces any caller-supplied + /// `request_id` with a connection-local id, so late deltas from abandoned + /// streams cannot be confused with later requests. + pub async fn http_request_stream( + &self, + mut params: HttpRequestParams, + ) -> Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError> { + params.stream_response = true; + let request_id = self.inner.next_http_body_stream_request_id(); + params.request_id = request_id.clone(); + let (tx, rx) = mpsc::channel(HTTP_BODY_DELTA_CHANNEL_CAPACITY); + self.inner + .insert_http_body_stream(request_id.clone(), tx) + .await?; + let mut registration = + HttpBodyStreamRegistration::new(Arc::clone(&self.inner), request_id.clone()); + let response = match self.call(HTTP_REQUEST_METHOD, ¶ms).await { + Ok(response) => response, + Err(error) => { + self.inner.remove_http_body_stream(&request_id).await; + registration.disarm(); + return Err(error); + } + }; + registration.disarm(); + Ok(( + response, + HttpResponseBodyStream::remote(Arc::clone(&self.inner), request_id, rx), + )) + } +} + +impl HttpClient for ExecServerClient { + /// Orchestrator-side adapter that forwards buffered HTTP requests to the + /// remote runtime over the shared JSON-RPC connection. + fn http_request( + &self, + params: HttpRequestParams, + ) -> BoxFuture<'_, Result> { + async move { ExecServerClient::http_request(self, params).await }.boxed() + } + + /// Orchestrator-side adapter that forwards streamed HTTP requests to the + /// remote runtime and exposes body deltas as a byte stream. + fn http_request_stream( + &self, + params: HttpRequestParams, + ) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>> { + async move { ExecServerClient::http_request_stream(self, params).await }.boxed() + } +} diff --git a/codex-rs/exec-server/src/client_api.rs b/codex-rs/exec-server/src/client_api.rs index ac4371e2ea..b1761b69f1 100644 --- a/codex-rs/exec-server/src/client_api.rs +++ b/codex-rs/exec-server/src/client_api.rs @@ -1,5 +1,12 @@ use std::time::Duration; +use futures::future::BoxFuture; + +use crate::ExecServerError; +use crate::HttpRequestParams; +use crate::HttpRequestResponse; +use crate::HttpResponseBodyStream; + /// Connection options for any exec-server client transport. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ExecServerClientConnectOptions { @@ -17,3 +24,22 @@ pub struct RemoteExecServerConnectArgs { pub initialize_timeout: Duration, pub resume_session_id: Option, } + +/// Sends HTTP requests through a runtime-selected transport. +/// +/// This is the HTTP capability counterpart to [`crate::ExecBackend`]. Callers +/// use it when they need environment-owned network requests but should not +/// depend on the concrete connection type or how that connection is established. +pub trait HttpClient: Send + Sync { + /// Perform an HTTP request and buffer the response body. + fn http_request( + &self, + params: HttpRequestParams, + ) -> BoxFuture<'_, Result>; + + /// Perform an HTTP request and return a streamed body handle. + fn http_request_stream( + &self, + params: HttpRequestParams, + ) -> BoxFuture<'_, Result<(HttpRequestResponse, HttpResponseBodyStream), ExecServerError>>; +} diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 02fd493b10..9e4c69c41d 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -3,7 +3,9 @@ use std::sync::Arc; use crate::ExecServerError; use crate::ExecServerRuntimePaths; +use crate::HttpClient; use crate::client::LazyRemoteExecServerClient; +use crate::client::http_client::ReqwestHttpClient; use crate::file_system::ExecutorFileSystem; use crate::local_file_system::LocalFileSystem; use crate::local_process::LocalProcess; @@ -136,6 +138,7 @@ pub struct Environment { exec_server_url: Option, exec_backend: Arc, filesystem: Arc, + http_client: Arc, local_runtime_paths: Option, } @@ -146,6 +149,7 @@ impl Environment { exec_server_url: None, exec_backend: Arc::new(LocalProcess::default()), filesystem: Arc::new(LocalFileSystem::unsandboxed()), + http_client: Arc::new(ReqwestHttpClient), local_runtime_paths: None, } } @@ -202,6 +206,7 @@ impl Environment { filesystem: Arc::new(LocalFileSystem::with_runtime_paths( local_runtime_paths.clone(), )), + http_client: Arc::new(ReqwestHttpClient), local_runtime_paths: Some(local_runtime_paths), } } @@ -216,12 +221,14 @@ impl Environment { ) -> Self { let client = LazyRemoteExecServerClient::new(exec_server_url.clone()); let exec_backend: Arc = Arc::new(RemoteProcess::new(client.clone())); - let filesystem: Arc = Arc::new(RemoteFileSystem::new(client)); + let filesystem: Arc = + Arc::new(RemoteFileSystem::new(client.clone())); Self { exec_server_url: Some(exec_server_url), exec_backend, filesystem, + http_client: Arc::new(client), local_runtime_paths, } } @@ -243,6 +250,10 @@ impl Environment { Arc::clone(&self.exec_backend) } + pub fn get_http_client(&self) -> Arc { + Arc::clone(&self.http_client) + } + pub fn get_filesystem(&self) -> Arc { Arc::clone(&self.filesystem) } diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index fc6a86f508..3da7aa73fd 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -20,7 +20,10 @@ mod server; pub use client::ExecServerClient; pub use client::ExecServerError; +pub use client::http_client::HttpResponseBodyStream; +pub use client::http_client::ReqwestHttpClient; pub use client_api::ExecServerClientConnectOptions; +pub use client_api::HttpClient; pub use client_api::RemoteExecServerConnectArgs; pub use environment::CODEX_EXEC_SERVER_URL_ENV_VAR; pub use environment::Environment; diff --git a/codex-rs/exec-server/src/server/handler.rs b/codex-rs/exec-server/src/server/handler.rs index 15035335ea..d0645724c4 100644 --- a/codex-rs/exec-server/src/server/handler.rs +++ b/codex-rs/exec-server/src/server/handler.rs @@ -12,8 +12,8 @@ use tokio_util::sync::CancellationToken; use tokio_util::task::TaskTracker; use crate::ExecServerRuntimePaths; -use crate::client::http_client::ExecutorHttpRequestRunner; -use crate::client::http_client::ExecutorPendingHttpBodyStream; +use crate::client::http_client::PendingReqwestHttpBodyStream; +use crate::client::http_client::ReqwestHttpRequestRunner; use crate::protocol::ExecParams; use crate::protocol::ExecResponse; use crate::protocol::FsCopyParams; @@ -178,7 +178,7 @@ impl ExecServerHandler { if stream_response { self.reserve_http_body_stream(&http_request_id).await?; } - let response = ExecutorHttpRequestRunner::new(params.timeout_ms)? + let response = ReqwestHttpRequestRunner::new(params.timeout_ms)? .run(params) .await; if response.is_err() && stream_response { @@ -306,7 +306,7 @@ impl ExecServerHandler { async fn start_http_body_stream( self: &Arc, - pending_stream: ExecutorPendingHttpBodyStream, + pending_stream: PendingReqwestHttpBodyStream, ) { let request_id = pending_stream.request_id.clone(); if self.background_task_shutdown.is_cancelled() { @@ -320,7 +320,7 @@ impl ExecServerHandler { self.background_tasks.spawn(async move { tokio::select! { _ = shutdown.cancelled() => {} - _ = ExecutorHttpRequestRunner::stream_body(pending_stream, notifications) => {} + _ = ReqwestHttpRequestRunner::stream_body(pending_stream, notifications) => {} } handler.release_http_body_stream(&finished_request_id).await; }); diff --git a/codex-rs/rmcp-client/BUILD.bazel b/codex-rs/rmcp-client/BUILD.bazel index ad5b626038..89a1963ecc 100644 --- a/codex-rs/rmcp-client/BUILD.bazel +++ b/codex-rs/rmcp-client/BUILD.bazel @@ -3,4 +3,7 @@ load("//:defs.bzl", "codex_rust_crate") codex_rust_crate( name = "rmcp-client", crate_name = "codex_rmcp_client", + extra_binaries = [ + "//codex-rs/cli:codex", + ], ) diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index b81c224a44..40e461314a 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -20,6 +20,7 @@ codex-keyring-store = { workspace = true } codex-protocol = { workspace = true } codex-utils-pty = { workspace = true } codex-utils-home-dir = { workspace = true } +bytes = { workspace = true } futures = { workspace = true, default-features = false, features = ["std"] } keyring = { workspace = true, features = ["crypto-rust"] } oauth2 = "5" diff --git a/codex-rs/rmcp-client/src/http_client_adapter.rs b/codex-rs/rmcp-client/src/http_client_adapter.rs new file mode 100644 index 0000000000..0656b8ce3c --- /dev/null +++ b/codex-rs/rmcp-client/src/http_client_adapter.rs @@ -0,0 +1,391 @@ +//! RMCP Streamable HTTP adapter built on top of the shared `HttpClient` +//! capability. +//! +//! This module runs in the orchestrator process. It turns high-level RMCP +//! operations like `post_message` and `get_stream` into calls on +//! `Arc`, which may be: +//! - a local HTTP client that issues requests from the orchestrator, or +//! - a remote HTTP client that forwards requests to the remote runtime + +use std::io; +use std::sync::Arc; + +use bytes::Bytes; +use codex_exec_server::ExecServerError; +use codex_exec_server::HttpClient; +use codex_exec_server::HttpHeader; +use codex_exec_server::HttpRequestParams; +use codex_exec_server::HttpResponseBodyStream; +use futures::StreamExt; +use futures::stream; +use futures::stream::BoxStream; +use reqwest::StatusCode; +use reqwest::header::ACCEPT; +use reqwest::header::AUTHORIZATION; +use reqwest::header::CONTENT_TYPE; +use reqwest::header::HeaderMap; +use reqwest::header::HeaderName; +use rmcp::model::ClientJsonRpcMessage; +use rmcp::model::ServerJsonRpcMessage; +use rmcp::transport::streamable_http_client::AuthRequiredError; +use rmcp::transport::streamable_http_client::StreamableHttpClient; +use rmcp::transport::streamable_http_client::StreamableHttpError; +use rmcp::transport::streamable_http_client::StreamableHttpPostResponse; +use sse_stream::Sse; +use sse_stream::SseStream; + +const EVENT_STREAM_MIME_TYPE: &str = "text/event-stream"; +const JSON_MIME_TYPE: &str = "application/json"; +const HEADER_SESSION_ID: &str = "Mcp-Session-Id"; +const NON_JSON_RESPONSE_BODY_PREVIEW_BYTES: usize = 8_192; + +#[derive(Clone)] +pub(crate) struct StreamableHttpClientAdapter { + http_client: Arc, + default_headers: HeaderMap, +} + +#[derive(Debug, thiserror::Error)] +pub(crate) enum StreamableHttpClientAdapterError { + #[error("streamable HTTP session expired with 404 Not Found")] + SessionExpired404, + #[error(transparent)] + HttpRequest(#[from] ExecServerError), + #[error("invalid HTTP header: {0}")] + Header(String), +} + +impl StreamableHttpClientAdapter { + pub(crate) fn new(http_client: Arc, default_headers: HeaderMap) -> Self { + Self { + http_client, + default_headers, + } + } +} + +impl StreamableHttpClient for StreamableHttpClientAdapter { + type Error = StreamableHttpClientAdapterError; + + async fn post_message( + &self, + uri: Arc, + message: ClientJsonRpcMessage, + session_id: Option>, + auth_token: Option, + ) -> std::result::Result> { + let mut headers = self.default_headers.clone(); + insert_header( + &mut headers, + ACCEPT, + [EVENT_STREAM_MIME_TYPE, JSON_MIME_TYPE].join(", "), + StreamableHttpClientAdapterError::Header, + )?; + insert_header( + &mut headers, + CONTENT_TYPE, + JSON_MIME_TYPE.to_string(), + StreamableHttpClientAdapterError::Header, + )?; + if let Some(auth_token) = auth_token { + insert_header( + &mut headers, + AUTHORIZATION, + format!("Bearer {auth_token}"), + StreamableHttpClientAdapterError::Header, + )?; + } + if let Some(session_id_value) = session_id.as_ref() { + insert_header( + &mut headers, + HeaderName::from_static("mcp-session-id"), + session_id_value.to_string(), + StreamableHttpClientAdapterError::Header, + )?; + } + + let body = serde_json::to_vec(&message).map_err(StreamableHttpError::Deserialize)?; + let (response, mut body_stream) = self + .http_client + .http_request_stream(HttpRequestParams { + method: "POST".to_string(), + url: uri.to_string(), + headers: protocol_headers(&headers), + body: Some(body.into()), + timeout_ms: None, + request_id: "buffered-request".to_string(), + stream_response: true, + }) + .await + .map_err(StreamableHttpClientAdapterError::from) + .map_err(StreamableHttpError::Client)?; + + if response.status == StatusCode::NOT_FOUND.as_u16() && session_id.is_some() { + return Err(StreamableHttpError::Client( + StreamableHttpClientAdapterError::SessionExpired404, + )); + } + if response.status == StatusCode::UNAUTHORIZED.as_u16() + && let Some(header) = + response_header(&response.headers, reqwest::header::WWW_AUTHENTICATE) + { + return Err(StreamableHttpError::AuthRequired(AuthRequiredError { + www_authenticate_header: header, + })); + } + if matches!( + StatusCode::from_u16(response.status).ok(), + Some(StatusCode::ACCEPTED | StatusCode::NO_CONTENT) + ) { + return Ok(StreamableHttpPostResponse::Accepted); + } + + let content_type = response_header(&response.headers, CONTENT_TYPE); + let session_id = response_header(&response.headers, HEADER_SESSION_ID); + match content_type.as_deref() { + Some(content_type) if content_type.starts_with(EVENT_STREAM_MIME_TYPE) => { + let event_stream = sse_stream_from_body(body_stream); + Ok(StreamableHttpPostResponse::Sse(event_stream, session_id)) + } + Some(content_type) if content_type.starts_with(JSON_MIME_TYPE) => { + let body = collect_body(&mut body_stream).await?; + let message: ServerJsonRpcMessage = + serde_json::from_slice(&body).map_err(StreamableHttpError::Deserialize)?; + Ok(StreamableHttpPostResponse::Json(message, session_id)) + } + _ => { + let body = collect_body(&mut body_stream).await?; + let content_type = content_type.unwrap_or_else(|| "missing-content-type".into()); + Err(StreamableHttpError::UnexpectedContentType(Some(format!( + "{content_type}; body: {}", + body_preview(String::from_utf8_lossy(&body).to_string()) + )))) + } + } + } + + async fn delete_session( + &self, + uri: Arc, + session: Arc, + auth_token: Option, + ) -> std::result::Result<(), StreamableHttpError> { + let mut headers = self.default_headers.clone(); + if let Some(auth_token) = auth_token { + insert_header( + &mut headers, + AUTHORIZATION, + format!("Bearer {auth_token}"), + StreamableHttpClientAdapterError::Header, + )?; + } + insert_header( + &mut headers, + HeaderName::from_static("mcp-session-id"), + session.to_string(), + StreamableHttpClientAdapterError::Header, + )?; + + let response = self + .http_client + .http_request(HttpRequestParams { + method: "DELETE".to_string(), + url: uri.to_string(), + headers: protocol_headers(&headers), + body: None, + timeout_ms: None, + request_id: "buffered-request".to_string(), + stream_response: false, + }) + .await + .map_err(StreamableHttpClientAdapterError::from) + .map_err(StreamableHttpError::Client)?; + + if response.status == StatusCode::METHOD_NOT_ALLOWED.as_u16() { + return Ok(()); + } + if !status_is_success(response.status) { + return Err(StreamableHttpError::UnexpectedServerResponse( + format!("DELETE returned HTTP {}", response.status).into(), + )); + } + Ok(()) + } + + async fn get_stream( + &self, + uri: Arc, + session_id: Arc, + last_event_id: Option, + auth_token: Option, + ) -> std::result::Result< + BoxStream<'static, std::result::Result>, + StreamableHttpError, + > { + let mut headers = self.default_headers.clone(); + insert_header( + &mut headers, + ACCEPT, + [EVENT_STREAM_MIME_TYPE, JSON_MIME_TYPE].join(", "), + StreamableHttpClientAdapterError::Header, + )?; + insert_header( + &mut headers, + HeaderName::from_static("mcp-session-id"), + session_id.to_string(), + StreamableHttpClientAdapterError::Header, + )?; + if let Some(last_event_id) = last_event_id { + insert_header( + &mut headers, + HeaderName::from_static("last-event-id"), + last_event_id, + StreamableHttpClientAdapterError::Header, + )?; + } + if let Some(auth_token) = auth_token { + insert_header( + &mut headers, + AUTHORIZATION, + format!("Bearer {auth_token}"), + StreamableHttpClientAdapterError::Header, + )?; + } + + let (response, body_stream) = self + .http_client + .http_request_stream(HttpRequestParams { + method: "GET".to_string(), + url: uri.to_string(), + headers: protocol_headers(&headers), + body: None, + timeout_ms: None, + request_id: "buffered-request".to_string(), + stream_response: true, + }) + .await + .map_err(StreamableHttpClientAdapterError::from) + .map_err(StreamableHttpError::Client)?; + + if response.status == StatusCode::METHOD_NOT_ALLOWED.as_u16() { + return Err(StreamableHttpError::ServerDoesNotSupportSse); + } + if response.status == StatusCode::NOT_FOUND.as_u16() { + return Err(StreamableHttpError::Client( + StreamableHttpClientAdapterError::SessionExpired404, + )); + } + if !status_is_success(response.status) { + return Err(StreamableHttpError::UnexpectedServerResponse( + format!("GET returned HTTP {}", response.status).into(), + )); + } + + match response_header(&response.headers, CONTENT_TYPE).as_deref() { + Some(content_type) if is_streamable_http_content_type(content_type) => {} + Some(content_type) => { + return Err(StreamableHttpError::UnexpectedContentType(Some( + content_type.to_string(), + ))); + } + None => { + return Err(StreamableHttpError::UnexpectedContentType(None)); + } + } + + Ok(sse_stream_from_body(body_stream)) + } +} + +fn body_preview(body: impl Into) -> String { + let mut body_preview = body.into(); + let body_len = body_preview.len(); + if body_len > NON_JSON_RESPONSE_BODY_PREVIEW_BYTES { + let mut boundary = NON_JSON_RESPONSE_BODY_PREVIEW_BYTES; + while !body_preview.is_char_boundary(boundary) { + boundary = boundary.saturating_sub(1); + } + body_preview.truncate(boundary); + body_preview.push_str(&format!( + "... (truncated {} bytes)", + body_len.saturating_sub(boundary) + )); + } + body_preview +} + +fn insert_header( + headers: &mut HeaderMap, + name: HeaderName, + value: String, + map_error: impl FnOnce(String) -> Error, +) -> std::result::Result<(), StreamableHttpError> +where + Error: std::error::Error + Send + Sync + 'static, +{ + let value = reqwest::header::HeaderValue::from_str(&value) + .map_err(|error| StreamableHttpError::Client(map_error(error.to_string())))?; + headers.insert(name, value); + Ok(()) +} + +fn is_streamable_http_content_type(content_type: &str) -> bool { + content_type + .as_bytes() + .starts_with(EVENT_STREAM_MIME_TYPE.as_bytes()) + || content_type + .as_bytes() + .starts_with(JSON_MIME_TYPE.as_bytes()) +} + +fn protocol_headers(headers: &HeaderMap) -> Vec { + headers + .iter() + .filter_map(|(name, value)| { + Some(HttpHeader { + name: name.as_str().to_string(), + value: value.to_str().ok()?.to_string(), + }) + }) + .collect() +} + +fn response_header(headers: &[HttpHeader], name: impl AsRef) -> Option { + let name = name.as_ref(); + headers + .iter() + .find(|header| header.name.eq_ignore_ascii_case(name)) + .map(|header| header.value.clone()) +} + +fn status_is_success(status: u16) -> bool { + StatusCode::from_u16(status).is_ok_and(|status| status.is_success()) +} + +async fn collect_body( + body_stream: &mut HttpResponseBodyStream, +) -> std::result::Result, StreamableHttpError> { + let mut body = Vec::new(); + while let Some(chunk) = body_stream + .recv() + .await + .map_err(StreamableHttpClientAdapterError::from) + .map_err(StreamableHttpError::Client)? + { + body.extend_from_slice(&chunk); + } + Ok(body) +} + +fn sse_stream_from_body( + body_stream: HttpResponseBodyStream, +) -> BoxStream<'static, std::result::Result> { + SseStream::from_byte_stream(stream::unfold(body_stream, |mut body_stream| async move { + match body_stream.recv().await { + Ok(Some(bytes)) => Some((Ok(Bytes::from(bytes)), body_stream)), + Ok(None) => None, + Err(error) => Some((Err(io::Error::other(error)), body_stream)), + } + })) + .boxed() +} diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index f3870b9319..57e9f0e800 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -1,6 +1,7 @@ mod auth_status; mod elicitation_client_service; mod executor_process_transport; +mod http_client_adapter; mod logging_client_handler; mod oauth; mod perform_oauth_login; diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index 50891ec8c0..0608e00d78 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -1,4 +1,3 @@ -use std::borrow::Cow; use std::collections::HashMap; use std::ffi::OsString; use std::future::Future; @@ -14,16 +13,12 @@ use anyhow::Result; use anyhow::anyhow; use codex_client::build_reqwest_client_with_custom_ca; use codex_config::types::McpServerEnvVar; +use codex_exec_server::HttpClient; use futures::FutureExt; -use futures::StreamExt; use futures::future::BoxFuture; -use futures::stream::BoxStream; use oauth2::TokenResponse; -use reqwest::header::ACCEPT; use reqwest::header::AUTHORIZATION; -use reqwest::header::CONTENT_TYPE; use reqwest::header::HeaderMap; -use reqwest::header::WWW_AUTHENTICATE; use rmcp::model::CallToolRequestParams; use rmcp::model::CallToolResult; use rmcp::model::ClientNotification; @@ -52,16 +47,11 @@ use rmcp::transport::StreamableHttpClientTransport; use rmcp::transport::auth::AuthClient; use rmcp::transport::auth::AuthError; use rmcp::transport::auth::OAuthState; -use rmcp::transport::streamable_http_client::AuthRequiredError; -use rmcp::transport::streamable_http_client::StreamableHttpClient; use rmcp::transport::streamable_http_client::StreamableHttpClientTransportConfig; use rmcp::transport::streamable_http_client::StreamableHttpError; -use rmcp::transport::streamable_http_client::StreamableHttpPostResponse; use serde::Deserialize; use serde::Serialize; use serde_json::Value; -use sse_stream::Sse; -use sse_stream::SseStream; use tokio::sync::Mutex; use tokio::sync::Semaphore; use tokio::sync::watch; @@ -69,6 +59,8 @@ use tokio::time; use tracing::warn; use crate::elicitation_client_service::ElicitationClientService; +use crate::http_client_adapter::StreamableHttpClientAdapter; +use crate::http_client_adapter::StreamableHttpClientAdapterError; use crate::load_oauth_tokens; use crate::oauth::OAuthPersistor; use crate::oauth::StoredOAuthTokens; @@ -79,239 +71,15 @@ use crate::utils::apply_default_headers; use crate::utils::build_default_headers; use codex_config::types::OAuthCredentialsStoreMode; -const EVENT_STREAM_MIME_TYPE: &str = "text/event-stream"; -const JSON_MIME_TYPE: &str = "application/json"; -const HEADER_LAST_EVENT_ID: &str = "Last-Event-Id"; -const HEADER_SESSION_ID: &str = "Mcp-Session-Id"; -const NON_JSON_RESPONSE_BODY_PREVIEW_BYTES: usize = 8_192; - -#[derive(Clone)] -struct StreamableHttpResponseClient { - inner: reqwest::Client, -} - -impl StreamableHttpResponseClient { - fn new(inner: reqwest::Client) -> Self { - Self { inner } - } - - fn reqwest_error( - error: reqwest::Error, - ) -> StreamableHttpError { - StreamableHttpError::Client(StreamableHttpResponseClientError::from(error)) - } -} - -fn build_http_client(default_headers: &HeaderMap) -> Result { - let builder = apply_default_headers(reqwest::Client::builder(), default_headers); - Ok(build_reqwest_client_with_custom_ca(builder)?) -} - -#[derive(Debug, thiserror::Error)] -enum StreamableHttpResponseClientError { - #[error("streamable HTTP session expired with 404 Not Found")] - SessionExpired404, - #[error(transparent)] - Reqwest(#[from] reqwest::Error), -} - -impl StreamableHttpClient for StreamableHttpResponseClient { - type Error = StreamableHttpResponseClientError; - - async fn post_message( - &self, - uri: Arc, - message: rmcp::model::ClientJsonRpcMessage, - session_id: Option>, - auth_token: Option, - ) -> std::result::Result> { - let mut request = self - .inner - .post(uri.as_ref()) - .header(ACCEPT, [EVENT_STREAM_MIME_TYPE, JSON_MIME_TYPE].join(", ")); - if let Some(auth_header) = auth_token { - request = request.bearer_auth(auth_header); - } - if let Some(session_id_value) = session_id.as_ref() { - request = request.header(HEADER_SESSION_ID, session_id_value.as_ref()); - } - - let response = request - .json(&message) - .send() - .await - .map_err(StreamableHttpResponseClient::reqwest_error)?; - if response.status() == reqwest::StatusCode::NOT_FOUND && session_id.is_some() { - return Err(StreamableHttpError::Client( - StreamableHttpResponseClientError::SessionExpired404, - )); - } - if response.status() == reqwest::StatusCode::UNAUTHORIZED - && let Some(header) = response.headers().get(WWW_AUTHENTICATE) - { - let header = header - .to_str() - .map_err(|_| { - StreamableHttpError::UnexpectedServerResponse(Cow::Borrowed( - "invalid www-authenticate header value", - )) - })? - .to_string(); - return Err(StreamableHttpError::AuthRequired(AuthRequiredError { - www_authenticate_header: header, - })); - } - - let status = response.status(); - if matches!( - status, - reqwest::StatusCode::ACCEPTED | reqwest::StatusCode::NO_CONTENT - ) { - return Ok(StreamableHttpPostResponse::Accepted); - } - - let content_type = response - .headers() - .get(CONTENT_TYPE) - .and_then(|value| value.to_str().ok()) - .map(str::to_string); - let session_id = response - .headers() - .get(HEADER_SESSION_ID) - .and_then(|value| value.to_str().ok()) - .map(str::to_string); - - match content_type.as_deref() { - Some(ct) if ct.as_bytes().starts_with(EVENT_STREAM_MIME_TYPE.as_bytes()) => { - let event_stream = SseStream::from_byte_stream(response.bytes_stream()).boxed(); - Ok(StreamableHttpPostResponse::Sse(event_stream, session_id)) - } - Some(ct) if ct.as_bytes().starts_with(JSON_MIME_TYPE.as_bytes()) => { - let message = response - .json() - .await - .map_err(StreamableHttpResponseClient::reqwest_error)?; - Ok(StreamableHttpPostResponse::Json(message, session_id)) - } - _ => { - let body = response - .text() - .await - .map_err(StreamableHttpResponseClient::reqwest_error)?; - let mut body_preview = body; - let body_len = body_preview.len(); - if body_len > NON_JSON_RESPONSE_BODY_PREVIEW_BYTES { - let mut boundary = NON_JSON_RESPONSE_BODY_PREVIEW_BYTES; - while !body_preview.is_char_boundary(boundary) { - boundary = boundary.saturating_sub(1); - } - body_preview.truncate(boundary); - body_preview.push_str(&format!( - "... (truncated {} bytes)", - body_len.saturating_sub(boundary) - )); - } - - let content_type = content_type.unwrap_or_else(|| "missing-content-type".into()); - Err(StreamableHttpError::UnexpectedContentType(Some(format!( - "{content_type}; body: {body_preview}" - )))) - } - } - } - - async fn delete_session( - &self, - uri: Arc, - session: Arc, - auth_token: Option, - ) -> std::result::Result<(), StreamableHttpError> { - let mut request_builder = self.inner.delete(uri.as_ref()); - if let Some(auth_header) = auth_token { - request_builder = request_builder.bearer_auth(auth_header); - } - let response = request_builder - .header(HEADER_SESSION_ID, session.as_ref()) - .send() - .await - .map_err(StreamableHttpResponseClient::reqwest_error)?; - - if response.status() == reqwest::StatusCode::METHOD_NOT_ALLOWED { - return Ok(()); - } - - response - .error_for_status() - .map_err(StreamableHttpResponseClient::reqwest_error)?; - Ok(()) - } - - async fn get_stream( - &self, - uri: Arc, - session_id: Arc, - last_event_id: Option, - auth_token: Option, - ) -> std::result::Result< - BoxStream<'static, std::result::Result>, - StreamableHttpError, - > { - let mut request_builder = self - .inner - .get(uri.as_ref()) - .header(ACCEPT, [EVENT_STREAM_MIME_TYPE, JSON_MIME_TYPE].join(", ")) - .header(HEADER_SESSION_ID, session_id.as_ref()); - if let Some(last_event_id) = last_event_id { - request_builder = request_builder.header(HEADER_LAST_EVENT_ID, last_event_id); - } - if let Some(auth_header) = auth_token { - request_builder = request_builder.bearer_auth(auth_header); - } - - let response = request_builder - .send() - .await - .map_err(StreamableHttpResponseClient::reqwest_error)?; - if response.status() == reqwest::StatusCode::METHOD_NOT_ALLOWED { - return Err(StreamableHttpError::ServerDoesNotSupportSse); - } - if response.status() == reqwest::StatusCode::NOT_FOUND { - return Err(StreamableHttpError::Client( - StreamableHttpResponseClientError::SessionExpired404, - )); - } - - let response = response - .error_for_status() - .map_err(StreamableHttpResponseClient::reqwest_error)?; - match response.headers().get(CONTENT_TYPE) { - Some(ct) - if ct.as_bytes().starts_with(EVENT_STREAM_MIME_TYPE.as_bytes()) - || ct.as_bytes().starts_with(JSON_MIME_TYPE.as_bytes()) => {} - Some(ct) => { - return Err(StreamableHttpError::UnexpectedContentType(Some( - String::from_utf8_lossy(ct.as_bytes()).to_string(), - ))); - } - None => { - return Err(StreamableHttpError::UnexpectedContentType(None)); - } - } - - let event_stream = SseStream::from_byte_stream(response.bytes_stream()).boxed(); - Ok(event_stream) - } -} - enum PendingTransport { Stdio { transport: StdioServerTransport, }, StreamableHttp { - transport: StreamableHttpClientTransport, + transport: StreamableHttpClientTransport, }, StreamableHttpWithOAuth { - transport: StreamableHttpClientTransport>, + transport: StreamableHttpClientTransport>, oauth_persistor: OAuthPersistor, }, } @@ -339,6 +107,7 @@ enum TransportRecipe { http_headers: Option>, env_http_headers: Option>, store_mode: OAuthCredentialsStoreMode, + http_client: Arc, }, } @@ -536,6 +305,7 @@ impl RmcpClient { http_headers: Option>, env_http_headers: Option>, store_mode: OAuthCredentialsStoreMode, + http_client: Arc, ) -> Result { let transport_recipe = TransportRecipe::StreamableHttp { server_name: server_name.to_string(), @@ -544,6 +314,7 @@ impl RmcpClient { http_headers, env_http_headers, store_mode, + http_client, }; let transport = Self::create_pending_transport(&transport_recipe).await?; Ok(Self { @@ -895,6 +666,7 @@ impl RmcpClient { http_headers, env_http_headers, store_mode, + http_client, } => { let default_headers = build_default_headers(http_headers.clone(), env_http_headers.clone())?; @@ -919,6 +691,7 @@ impl RmcpClient { initial_tokens.clone(), *store_mode, default_headers.clone(), + Arc::clone(http_client), ) .await { @@ -945,9 +718,11 @@ impl RmcpClient { let http_config = StreamableHttpClientTransportConfig::with_uri(url.clone()) .auth_header(access_token); - let http_client = build_http_client(&default_headers)?; let transport = StreamableHttpClientTransport::with_client( - StreamableHttpResponseClient::new(http_client), + StreamableHttpClientAdapter::new( + Arc::clone(http_client), + default_headers, + ), http_config, ); Ok(PendingTransport::StreamableHttp { transport }) @@ -961,10 +736,8 @@ impl RmcpClient { http_config = http_config.auth_header(bearer_token); } - let http_client = build_http_client(&default_headers)?; - let transport = StreamableHttpClientTransport::with_client( - StreamableHttpResponseClient::new(http_client), + StreamableHttpClientAdapter::new(Arc::clone(http_client), default_headers), http_config, ); Ok(PendingTransport::StreamableHttp { transport }) @@ -1084,12 +857,12 @@ impl RmcpClient { error .error - .downcast_ref::>() + .downcast_ref::>() .is_some_and(|error| { matches!( error, StreamableHttpError::Client( - StreamableHttpResponseClientError::SessionExpired404 + StreamableHttpClientAdapterError::SessionExpired404 ) ) }) @@ -1156,12 +929,18 @@ async fn create_oauth_transport_and_runtime( initial_tokens: StoredOAuthTokens, credentials_store: OAuthCredentialsStoreMode, default_headers: HeaderMap, + http_client: Arc, ) -> Result<( - StreamableHttpClientTransport>, + StreamableHttpClientTransport>, OAuthPersistor, )> { - let http_client = build_http_client(&default_headers)?; - let mut oauth_state = OAuthState::new(url.to_string(), Some(http_client.clone())).await?; + let builder = apply_default_headers(reqwest::Client::builder(), &default_headers); + let oauth_metadata_client = build_reqwest_client_with_custom_ca(builder)?; + // TODO(aibrahim): teach OAuth bootstrap and refresh to use the same + // shared HTTP client abstraction instead of always creating the local + // reqwest metadata client here. + let mut oauth_state = + OAuthState::new(url.to_string(), Some(oauth_metadata_client.clone())).await?; oauth_state .set_credentials( @@ -1178,7 +957,10 @@ async fn create_oauth_transport_and_runtime( } }; - let auth_client = AuthClient::new(StreamableHttpResponseClient::new(http_client), manager); + let auth_client = AuthClient::new( + StreamableHttpClientAdapter::new(http_client, default_headers), + manager, + ); let auth_manager = auth_client.auth_manager.clone(); let transport = StreamableHttpClientTransport::with_client( diff --git a/codex-rs/rmcp-client/tests/streamable_http_recovery.rs b/codex-rs/rmcp-client/tests/streamable_http_recovery.rs index c0525aafaf..4be21f6cfc 100644 --- a/codex-rs/rmcp-client/tests/streamable_http_recovery.rs +++ b/codex-rs/rmcp-client/tests/streamable_http_recovery.rs @@ -1,190 +1,12 @@ -use std::net::TcpListener; -use std::path::PathBuf; -use std::time::Duration; -use std::time::Instant; +mod streamable_http_test_support; -use codex_config::types::OAuthCredentialsStoreMode; -use codex_rmcp_client::ElicitationAction; -use codex_rmcp_client::ElicitationResponse; -use codex_rmcp_client::RmcpClient; -use codex_utils_cargo_bin::CargoBinError; -use futures::FutureExt as _; use pretty_assertions::assert_eq; -use rmcp::model::CallToolResult; -use rmcp::model::ClientCapabilities; -use rmcp::model::ElicitationCapability; -use rmcp::model::FormElicitationCapability; -use rmcp::model::Implementation; -use rmcp::model::InitializeRequestParams; -use rmcp::model::ProtocolVersion; -use serde_json::json; -use tokio::net::TcpStream; -use tokio::process::Child; -use tokio::process::Command; -use tokio::time::sleep; -const SESSION_POST_FAILURE_CONTROL_PATH: &str = "/test/control/session-post-failure"; - -fn streamable_http_server_bin() -> Result { - codex_utils_cargo_bin::cargo_bin("test_streamable_http_server") -} - -fn init_params() -> InitializeRequestParams { - InitializeRequestParams { - meta: None, - capabilities: ClientCapabilities { - experimental: None, - extensions: None, - roots: None, - sampling: None, - elicitation: Some(ElicitationCapability { - form: Some(FormElicitationCapability { - schema_validation: None, - }), - url: None, - }), - tasks: None, - }, - client_info: Implementation { - name: "codex-test".into(), - version: "0.0.0-test".into(), - title: Some("Codex rmcp recovery test".into()), - description: None, - icons: None, - website_url: None, - }, - protocol_version: ProtocolVersion::V_2025_06_18, - } -} - -fn expected_echo_result(message: &str) -> CallToolResult { - CallToolResult { - content: Vec::new(), - structured_content: Some(json!({ - "echo": format!("ECHOING: {message}"), - "env": null, - })), - is_error: Some(false), - meta: None, - } -} - -async fn create_client(base_url: &str) -> anyhow::Result { - let client = RmcpClient::new_streamable_http_client( - "test-streamable-http", - &format!("{base_url}/mcp"), - Some("test-bearer".to_string()), - /*http_headers*/ None, - /*env_http_headers*/ None, - OAuthCredentialsStoreMode::File, - ) - .await?; - - client - .initialize( - init_params(), - Some(Duration::from_secs(5)), - Box::new(|_, _| { - async { - Ok(ElicitationResponse { - action: ElicitationAction::Accept, - content: Some(json!({})), - meta: None, - }) - } - .boxed() - }), - ) - .await?; - - Ok(client) -} - -async fn call_echo_tool(client: &RmcpClient, message: &str) -> anyhow::Result { - client - .call_tool( - "echo".to_string(), - Some(json!({ "message": message })), - /*meta*/ None, - Some(Duration::from_secs(5)), - ) - .await -} - -async fn arm_session_post_failure( - base_url: &str, - status: u16, - remaining: usize, -) -> anyhow::Result<()> { - let response = reqwest::Client::new() - .post(format!("{base_url}{SESSION_POST_FAILURE_CONTROL_PATH}")) - .json(&json!({ - "status": status, - "remaining": remaining, - })) - .send() - .await?; - - assert_eq!(response.status(), reqwest::StatusCode::NO_CONTENT); - Ok(()) -} - -async fn spawn_streamable_http_server() -> anyhow::Result<(Child, String)> { - let listener = TcpListener::bind("127.0.0.1:0")?; - let port = listener.local_addr()?.port(); - drop(listener); - - let bind_addr = format!("127.0.0.1:{port}"); - let base_url = format!("http://{bind_addr}"); - let mut child = Command::new(streamable_http_server_bin()?) - .kill_on_drop(true) - .env("MCP_STREAMABLE_HTTP_BIND_ADDR", &bind_addr) - .spawn()?; - - wait_for_streamable_http_server(&mut child, &bind_addr, Duration::from_secs(5)).await?; - Ok((child, base_url)) -} - -async fn wait_for_streamable_http_server( - server_child: &mut Child, - address: &str, - timeout: Duration, -) -> anyhow::Result<()> { - let deadline = Instant::now() + timeout; - - loop { - if let Some(status) = server_child.try_wait()? { - return Err(anyhow::anyhow!( - "streamable HTTP server exited early with status {status}" - )); - } - - let remaining = deadline.saturating_duration_since(Instant::now()); - if remaining.is_zero() { - return Err(anyhow::anyhow!( - "timed out waiting for streamable HTTP server at {address}: deadline reached" - )); - } - - match tokio::time::timeout(remaining, TcpStream::connect(address)).await { - Ok(Ok(_)) => return Ok(()), - Ok(Err(error)) => { - if Instant::now() >= deadline { - return Err(anyhow::anyhow!( - "timed out waiting for streamable HTTP server at {address}: {error}" - )); - } - } - Err(_) => { - return Err(anyhow::anyhow!( - "timed out waiting for streamable HTTP server at {address}: connect call timed out" - )); - } - } - - sleep(Duration::from_millis(50)).await; - } -} +use streamable_http_test_support::arm_session_post_failure; +use streamable_http_test_support::call_echo_tool; +use streamable_http_test_support::create_client; +use streamable_http_test_support::expected_echo_result; +use streamable_http_test_support::spawn_streamable_http_server; #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn streamable_http_404_session_expiry_recovers_and_retries_once() -> anyhow::Result<()> { diff --git a/codex-rs/rmcp-client/tests/streamable_http_remote.rs b/codex-rs/rmcp-client/tests/streamable_http_remote.rs new file mode 100644 index 0000000000..0d4690a825 --- /dev/null +++ b/codex-rs/rmcp-client/tests/streamable_http_remote.rs @@ -0,0 +1,37 @@ +//! Integration coverage for the remote Streamable HTTP RMCP path. +//! +//! These tests exercise the orchestrator-side RMCP adapter against a real +//! `exec-server` process so HTTP requests go through the remote runtime path +//! instead of direct local `reqwest` calls. + +mod streamable_http_test_support; + +use pretty_assertions::assert_eq; + +use streamable_http_test_support::call_echo_tool; +use streamable_http_test_support::create_remote_client; +use streamable_http_test_support::expected_echo_result; +use streamable_http_test_support::spawn_exec_server; +use streamable_http_test_support::spawn_streamable_http_server; + +/// What this tests: the RMCP remote Streamable HTTP adapter can initialize +/// a server and call a tool while every MCP HTTP request goes through a real +/// exec-server process instead of a direct reqwest transport. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn streamable_http_remote_client_round_trips_through_exec_server() -> anyhow::Result<()> { + // Phase 1: start the MCP Streamable HTTP test server and a local + // exec-server process that will own the HTTP network calls. + let (_server, base_url) = spawn_streamable_http_server().await?; + let exec_server = spawn_exec_server().await?; + + // Phase 2: create and initialize the RMCP client using the executor-backed + // Streamable HTTP transport. + let client = create_remote_client(&base_url, exec_server.client.clone()).await?; + + // Phase 3: prove the initialized client can complete a tool call and + // preserve the normal RMCP response shape. + let result = call_echo_tool(&client, "remote").await?; + assert_eq!(result, expected_echo_result("remote")); + + Ok(()) +} diff --git a/codex-rs/rmcp-client/tests/streamable_http_test_support.rs b/codex-rs/rmcp-client/tests/streamable_http_test_support.rs new file mode 100644 index 0000000000..ec7f7dc6f1 --- /dev/null +++ b/codex-rs/rmcp-client/tests/streamable_http_test_support.rs @@ -0,0 +1,314 @@ +//! Shared helpers for Streamable HTTP RMCP integration tests. +//! +//! This support module starts the test HTTP server, launches a real +//! `exec-server` when remote coverage is needed, and provides small helpers for +//! creating RMCP clients and asserting round-trip behavior. + +// This support module is included by multiple integration-test crates. Each +// crate uses a different subset of the helpers, so dead-code warnings would +// otherwise depend on which test file compiled the module. +#![allow(dead_code)] + +use std::net::TcpListener; +use std::path::PathBuf; +use std::process::Stdio; +use std::sync::Arc; +use std::time::Duration; +use std::time::Instant; + +use anyhow::Context as _; +use codex_config::types::OAuthCredentialsStoreMode; +use codex_exec_server::Environment; +use codex_exec_server::ExecServerClient; +use codex_exec_server::RemoteExecServerConnectArgs; +use codex_rmcp_client::ElicitationAction; +use codex_rmcp_client::ElicitationResponse; +use codex_rmcp_client::RmcpClient; +use codex_utils_cargo_bin::CargoBinError; +use futures::FutureExt as _; +use pretty_assertions::assert_eq; +use rmcp::model::CallToolResult; +use rmcp::model::ClientCapabilities; +use rmcp::model::ElicitationCapability; +use rmcp::model::FormElicitationCapability; +use rmcp::model::Implementation; +use rmcp::model::InitializeRequestParams; +use rmcp::model::ProtocolVersion; +use serde_json::json; +use tempfile::TempDir; +use tokio::io::AsyncBufReadExt; +use tokio::io::BufReader; +use tokio::net::TcpStream; +use tokio::process::Child; +use tokio::process::Command; +use tokio::time::sleep; + +const SESSION_POST_FAILURE_CONTROL_PATH: &str = "/test/control/session-post-failure"; + +fn streamable_http_server_bin() -> Result { + codex_utils_cargo_bin::cargo_bin("test_streamable_http_server") +} + +fn init_params() -> InitializeRequestParams { + InitializeRequestParams { + meta: None, + capabilities: ClientCapabilities { + experimental: None, + extensions: None, + roots: None, + sampling: None, + elicitation: Some(ElicitationCapability { + form: Some(FormElicitationCapability { + schema_validation: None, + }), + url: None, + }), + tasks: None, + }, + client_info: Implementation { + name: "codex-test".into(), + version: "0.0.0-test".into(), + title: Some("Codex rmcp recovery test".into()), + description: None, + icons: None, + website_url: None, + }, + protocol_version: ProtocolVersion::V_2025_06_18, + } +} + +pub(crate) fn expected_echo_result(message: &str) -> CallToolResult { + CallToolResult { + content: Vec::new(), + structured_content: Some(json!({ + "echo": format!("ECHOING: {message}"), + "env": null, + })), + is_error: Some(false), + meta: None, + } +} + +pub(crate) async fn create_client(base_url: &str) -> anyhow::Result { + let client = RmcpClient::new_streamable_http_client( + "test-streamable-http", + &format!("{base_url}/mcp"), + Some("test-bearer".to_string()), + /*http_headers*/ None, + /*env_http_headers*/ None, + OAuthCredentialsStoreMode::File, + Environment::default_for_tests().get_http_client(), + ) + .await?; + + client + .initialize( + init_params(), + Some(Duration::from_secs(5)), + Box::new(|_, _| { + async { + Ok(ElicitationResponse { + action: ElicitationAction::Accept, + content: Some(json!({})), + meta: None, + }) + } + .boxed() + }), + ) + .await?; + + Ok(client) +} + +/// Creates a Streamable HTTP RMCP client that sends traffic through the remote +/// runtime HTTP API. +pub(crate) async fn create_remote_client( + base_url: &str, + http_client: ExecServerClient, +) -> anyhow::Result { + let client = RmcpClient::new_streamable_http_client( + "test-streamable-http-remote", + &format!("{base_url}/mcp"), + Some("test-bearer".to_string()), + /*http_headers*/ None, + /*env_http_headers*/ None, + OAuthCredentialsStoreMode::File, + Arc::new(http_client), + ) + .await?; + + client + .initialize( + init_params(), + Some(Duration::from_secs(5)), + Box::new(|_, _| { + async { + Ok(ElicitationResponse { + action: ElicitationAction::Accept, + content: Some(json!({})), + meta: None, + }) + } + .boxed() + }), + ) + .await?; + + Ok(client) +} + +pub(crate) async fn call_echo_tool( + client: &RmcpClient, + message: &str, +) -> anyhow::Result { + client + .call_tool( + "echo".to_string(), + Some(json!({ "message": message })), + /*meta*/ None, + Some(Duration::from_secs(5)), + ) + .await +} + +pub(crate) async fn arm_session_post_failure( + base_url: &str, + status: u16, + remaining: usize, +) -> anyhow::Result<()> { + let response = reqwest::Client::new() + .post(format!("{base_url}{SESSION_POST_FAILURE_CONTROL_PATH}")) + .json(&json!({ + "status": status, + "remaining": remaining, + })) + .send() + .await?; + + assert_eq!(response.status(), reqwest::StatusCode::NO_CONTENT); + Ok(()) +} + +pub(crate) async fn spawn_streamable_http_server() -> anyhow::Result<(Child, String)> { + let listener = TcpListener::bind("127.0.0.1:0")?; + let port = listener.local_addr()?.port(); + drop(listener); + + let bind_addr = format!("127.0.0.1:{port}"); + let base_url = format!("http://{bind_addr}"); + let mut child = Command::new(streamable_http_server_bin()?) + .kill_on_drop(true) + .env("MCP_STREAMABLE_HTTP_BIND_ADDR", &bind_addr) + .spawn()?; + + wait_for_streamable_http_server(&mut child, &bind_addr, Duration::from_secs(5)).await?; + Ok((child, base_url)) +} + +/// Owns the exec-server process used by the remote-client integration test. +pub(crate) struct ExecServerProcess { + _codex_home: TempDir, + child: Child, + pub(crate) client: ExecServerClient, +} + +impl Drop for ExecServerProcess { + /// Stops the local exec-server process best-effort when the test exits. + fn drop(&mut self) { + let _ = self.child.start_kill(); + } +} + +/// Starts a local exec-server and connects an initialized `ExecServerClient`. +pub(crate) async fn spawn_exec_server() -> anyhow::Result { + let codex_home = TempDir::new()?; + let mut child = Command::new(codex_utils_cargo_bin::cargo_bin("codex")?) + .args(["exec-server", "--listen", "ws://127.0.0.1:0"]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::inherit()) + .kill_on_drop(true) + .env("CODEX_HOME", codex_home.path()) + .spawn()?; + + let websocket_url = read_exec_server_listen_url(&mut child).await?; + let client = ExecServerClient::connect_websocket(RemoteExecServerConnectArgs::new( + websocket_url, + "rmcp-client-remote-http-test".to_string(), + )) + .await?; + + Ok(ExecServerProcess { + _codex_home: codex_home, + child, + client, + }) +} + +/// Reads the websocket URL printed by `codex exec-server --listen`. +async fn read_exec_server_listen_url(child: &mut Child) -> anyhow::Result { + let stdout = child + .stdout + .take() + .context("failed to capture exec-server stdout")?; + let mut lines = BufReader::new(stdout).lines(); + let deadline = Instant::now() + Duration::from_secs(10); + + loop { + let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + anyhow::bail!("timed out waiting for exec-server listen URL"); + } + + let line = tokio::time::timeout(remaining, lines.next_line()) + .await + .context("timed out waiting for exec-server stdout")?? + .context("exec-server stdout closed before emitting listen URL")?; + let listen_url = line.trim(); + if listen_url.starts_with("ws://") { + return Ok(listen_url.to_string()); + } + } +} + +async fn wait_for_streamable_http_server( + server_child: &mut Child, + address: &str, + timeout: Duration, +) -> anyhow::Result<()> { + let deadline = Instant::now() + timeout; + + loop { + if let Some(status) = server_child.try_wait()? { + return Err(anyhow::anyhow!( + "streamable HTTP server exited early with status {status}" + )); + } + + let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + return Err(anyhow::anyhow!( + "timed out waiting for streamable HTTP server at {address}: deadline reached" + )); + } + + match tokio::time::timeout(remaining, TcpStream::connect(address)).await { + Ok(Ok(_)) => return Ok(()), + Ok(Err(error)) => { + if Instant::now() >= deadline { + return Err(anyhow::anyhow!( + "timed out waiting for streamable HTTP server at {address}: {error}" + )); + } + } + Err(_) => { + return Err(anyhow::anyhow!( + "timed out waiting for streamable HTTP server at {address}: connect call timed out" + )); + } + } + + sleep(Duration::from_millis(50)).await; + } +} From 3cc3763e6c0473d6f6666524709528fdcdb7d9c1 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 22 Apr 2026 17:48:13 -0700 Subject: [PATCH 4/8] 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 5/8] 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 6/8] 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 7/8] 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 From c52b5b5ce2555e401ca820eb27595d4150c7333c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 22 Apr 2026 19:20:06 -0700 Subject: [PATCH 8/8] increase stack size --- .github/workflows/rust-ci-full.yml | 3 +++ .github/workflows/rust-ci.yml | 2 ++ defs.bzl | 15 +++++++++++---- justfile | 4 +++- 4 files changed, 19 insertions(+), 5 deletions(-) diff --git a/.github/workflows/rust-ci-full.yml b/.github/workflows/rust-ci-full.yml index 7cda682952..eb690a9bad 100644 --- a/.github/workflows/rust-ci-full.yml +++ b/.github/workflows/rust-ci-full.yml @@ -76,6 +76,8 @@ jobs: - name: Test argument comment lint package working-directory: tools/argument-comment-lint run: cargo test + env: + RUST_MIN_STACK: "8388608" # 8 MiB argument_comment_lint_prebuilt: name: Argument comment lint - ${{ matrix.name }} @@ -671,6 +673,7 @@ jobs: run: cargo nextest run --no-fail-fast --target ${{ matrix.target }} --cargo-profile ci-test --timings env: RUST_BACKTRACE: 1 + RUST_MIN_STACK: "8388608" # 8 MiB NEXTEST_STATUS_LEVEL: leak - name: Upload Cargo timings (nextest) diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index f62e5458a3..42a3ca8764 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -131,6 +131,8 @@ jobs: - name: Test argument comment lint package working-directory: tools/argument-comment-lint run: cargo test + env: + RUST_MIN_STACK: "8388608" # 8 MiB argument_comment_lint_prebuilt: name: Argument comment lint - ${{ matrix.name }} diff --git a/defs.bzl b/defs.bzl index d1e38ea91b..c5cc402658 100644 --- a/defs.bzl +++ b/defs.bzl @@ -18,11 +18,11 @@ PLATFORMS = [ WINDOWS_RUSTC_LINK_FLAGS = select({ "@rules_rs//rs/experimental/platforms/constraints:windows_gnullvm": [ "-C", - "link-arg=-Wl,--stack,8388608", + "link-arg=-Wl,--stack,8388608", # 8 MiB ], "@rules_rs//rs/experimental/platforms/constraints:windows_msvc": [ "-C", - "link-arg=/STACK:8388608", + "link-arg=/STACK:8388608", # 8 MiB "-C", "link-arg=/NODEFAULTLIB:libucrt.lib", "-C", @@ -43,6 +43,13 @@ MACOS_WEBRTC_RUSTC_LINK_FLAGS = select({ "//conditions:default": [], }) +RUST_LIBTEST_ENV = { + # Rust's libtest harness runs test bodies on std-spawned threads, whose + # default 2 MiB stack can be too small for large async test futures on + # Windows. Keep this aligned with the Windows test binary stack reserve. + "RUST_MIN_STACK": "8388608", # 8 MiB +} + def multiplatform_binaries(name, platforms = PLATFORMS): for platform in platforms: platform_data( @@ -187,7 +194,7 @@ def codex_rust_crate( extra_binaries: Additional binary labels to surface as test data and `CARGO_BIN_EXE_*` environment variables. These are only needed for binaries from a different crate. """ - test_env = { + test_env = RUST_LIBTEST_ENV | { # The launcher resolves an absolute workspace root at runtime so # manifest-only platforms like macOS still point Insta at the real # `codex-rs` checkout. @@ -359,7 +366,7 @@ def codex_rust_crate( # Important: do not merge `test_env` here. Its unit-test-only # `INSTA_WORKSPACE_ROOT="codex-rs"` is tuned for unit tests that # execute from the repo root and can misplace integration snapshots. - env = cargo_env, + env = RUST_LIBTEST_ENV | cargo_env, tags = test_tags, **test_kwargs ) diff --git a/justfile b/justfile index d44bbda21d..ca74c2e5c6 100644 --- a/justfile +++ b/justfile @@ -1,6 +1,8 @@ set working-directory := "codex-rs" set positional-arguments +rust_min_stack := "8388608" # 8 MiB + # Display help help: just -l @@ -49,7 +51,7 @@ install: # Prefer this for routine local runs. Workspace crate features are banned, so # there should be no need to add `--all-features`. test: - cargo nextest run --no-fail-fast + RUST_MIN_STACK={{ rust_min_stack }} cargo nextest run --no-fail-fast # Build and run Codex from source using Bazel. # Note we have to use the combination of `[no-cd]` and `--run_under="cd $PWD &&"`