From 956f2f439ed2fc6a82e571adefe8dafdd7aaf7ed Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 23 Feb 2026 00:31:29 -0800 Subject: [PATCH] refactor: decouple MCP policy construction from escalate server (#12555) ## Why The current escalate path in `codex-rs/exec-server` still had policy creation coupled to MCP details, which makes it hard to reuse the shell execution flow outside the MCP server. This change is part of a broader goal to split MCP-specific behavior from shared escalation execution so other handlers (for example a future `ShellCommandHandler`) can reuse it without depending on MCP request context types. ## What changed - Added a new `EscalationPolicyFactory` abstraction in `mcp.rs`: - `crate`-relative path: `codex-rs/exec-server/src/posix/mcp.rs` - https://github.com/openai/codex/blob/main/codex-rs/exec-server/src/posix/mcp.rs#L87-L107 - Made `run_escalate_server` in `mcp.rs` accept a policy factory instead of constructing `McpEscalationPolicy` directly. - https://github.com/openai/codex/blob/main/codex-rs/exec-server/src/posix/mcp.rs#L178-L201 - Introduced `McpEscalationPolicyFactory` that stores MCP-only state (`RequestContext`, `preserve_program_paths`) and implements the new trait. - https://github.com/openai/codex/blob/main/codex-rs/exec-server/src/posix/mcp.rs#L100-L117 - Updated `shell()` to pass a `McpEscalationPolicyFactory` instance into `run_escalate_server`, so the server remains the MCP-specific wiring layer. - https://github.com/openai/codex/blob/main/codex-rs/exec-server/src/posix/mcp.rs#L163-L170 ## Verification - Build and test execution was not re-run in this pass; changes are limited to `mcp.rs` and preserve the existing escalation flow semantics by only extracting policy construction behind a factory. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/12555). * #12556 * __->__ #12555 --- codex-rs/exec-server/src/posix/mcp.rs | 97 ++++++++++++++++++++++----- 1 file changed, 81 insertions(+), 16 deletions(-) diff --git a/codex-rs/exec-server/src/posix/mcp.rs b/codex-rs/exec-server/src/posix/mcp.rs index 5319a1f91..f413fed21 100644 --- a/codex-rs/exec-server/src/posix/mcp.rs +++ b/codex-rs/exec-server/src/posix/mcp.rs @@ -1,3 +1,4 @@ +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; @@ -30,6 +31,7 @@ use tokio::sync::RwLock; use crate::posix::escalate_server::EscalateServer; use crate::posix::escalate_server::{self}; +use crate::posix::escalation_policy::EscalationPolicy; use crate::posix::mcp_escalation_policy::McpEscalationPolicy; use crate::posix::stopwatch::Stopwatch; @@ -85,6 +87,30 @@ pub struct ExecTool { sandbox_state: Arc>>, } +trait EscalationPolicyFactory { + type Policy: EscalationPolicy + Send + Sync + 'static; + + fn create_policy(&self, policy: Arc>, stopwatch: Stopwatch) -> Self::Policy; +} + +struct McpEscalationPolicyFactory { + context: RequestContext, + preserve_program_paths: bool, +} + +impl EscalationPolicyFactory for McpEscalationPolicyFactory { + type Policy = McpEscalationPolicy; + + fn create_policy(&self, policy: Arc>, stopwatch: Stopwatch) -> Self::Policy { + McpEscalationPolicy::new( + policy, + self.context.clone(), + stopwatch, + self.preserve_program_paths, + ) + } +} + #[tool_router] impl ExecTool { pub fn new( @@ -115,8 +141,6 @@ impl ExecTool { .timeout_ms .unwrap_or(codex_core::exec::DEFAULT_EXEC_COMMAND_TIMEOUT_MS), ); - let stopwatch = Stopwatch::new(effective_timeout); - let cancel_token = stopwatch.cancellation_token(); let sandbox_state = self.sandbox_state .read() @@ -128,27 +152,68 @@ impl ExecTool { sandbox_cwd: PathBuf::from(¶ms.workdir), use_linux_sandbox_bwrap: false, }); - let escalate_server = EscalateServer::new( - self.bash_path.clone(), - self.execve_wrapper.clone(), - McpEscalationPolicy::new( - self.policy.clone(), + let result = run_escalate_server( + params, + sandbox_state, + &self.bash_path, + &self.execve_wrapper, + self.policy.clone(), + McpEscalationPolicyFactory { context, - stopwatch.clone(), - self.preserve_program_paths, - ), - ); - - let result = escalate_server - .exec(params, cancel_token, &sandbox_state) - .await - .map_err(|e| McpError::internal_error(e.to_string(), None))?; + preserve_program_paths: self.preserve_program_paths, + }, + effective_timeout, + ) + .await + .map_err(|e| McpError::internal_error(e.to_string(), None))?; Ok(CallToolResult::success(vec![Content::json( ExecResult::from(result), )?])) } } +/// Runs the escalate server to execute a shell command with potential +/// escalation of execve calls. +/// +/// - `exec_params` defines the shell command to run +/// - `sandbox_state` is the sandbox to use to run the shell program +/// - `shell_program` is the path to the shell program to run (e.g. /bin/bash) +/// - `execve_wrapper` is the path to the execve wrapper binary to use for +/// handling execve calls from the shell program. This is likely a symlink to +/// Codex using a special name. +/// - `policy` is the exec policy to use for deciding whether to allow or deny +/// execve calls from the shell program. +/// - `escalation_policy_factory` is a factory for creating an +/// `EscalationPolicy` to use for deciding whether to allow, deny, or prompt +/// the user for execve calls from the shell program. We use a factory here +/// because the `EscalationPolicy` may need to capture request-specific +/// context (e.g. the MCP request context) that is not available at the time +/// we create the `ExecTool`. +/// - `effective_timeout` is the timeout to use for running the shell command. +/// Implementations are encouraged to excludeany time spent prompting the +/// user. +async fn run_escalate_server( + exec_params: ExecParams, + sandbox_state: SandboxState, + shell_program: impl AsRef, + execve_wrapper: impl AsRef, + policy: Arc>, + escalation_policy_factory: impl EscalationPolicyFactory, + effective_timeout: Duration, +) -> anyhow::Result { + let stopwatch = Stopwatch::new(effective_timeout); + let cancel_token = stopwatch.cancellation_token(); + let escalate_server = EscalateServer::new( + shell_program.as_ref().to_path_buf(), + execve_wrapper.as_ref().to_path_buf(), + escalation_policy_factory.create_policy(policy, stopwatch), + ); + + escalate_server + .exec(exec_params, cancel_token, &sandbox_state) + .await +} + #[derive(Default)] pub struct CodexSandboxStateUpdateMethod;