Skip to content

Commit 13d378f

Browse files
authored
chore: refactor exec-server to prepare it for standalone MCP use (#6944)
This PR reorganizes things slightly so that: - Instead of a single multitool executable, `codex-exec-server`, we now have two executables: - `codex-exec-mcp-server` to launch the MCP server - `codex-execve-wrapper` is the `execve(2)` wrapper to use with the `BASH_EXEC_WRAPPER` environment variable - `BASH_EXEC_WRAPPER` must be a single executable: it cannot be a command string composed of an executable with args (i.e., it no longer adds the `escalate` subcommand, as before) - `codex-exec-mcp-server` takes `--bash` and `--execve` as options. Though if `--execve` is not specified, the MCP server will check the directory containing `std::env::current_exe()` and attempt to use the file named `codex-execve-wrapper` within it. In development, this works out since these executables are side-by-side in the `target/debug` folder. With respect to testing, this also fixes an important bug in `dummy_exec_policy()`, as I was using `ends_with()` as if it applied to a `String`, but in this case, it is used with a `&Path`, so the semantics are slightly different. Putting this all together, I was able to test this by running the following: ``` ~/code/codex/codex-rs$ npx @modelcontextprotocol/inspector \ ./target/debug/codex-exec-mcp-server --bash ~/code/bash/bash ``` If I try to run `git status` in `/Users/mbolin/code/codex` via the `shell` tool from the MCP server: <img width="1589" height="1335" alt="image" src="https://github.com/user-attachments/assets/9db6aea8-7fbc-4675-8b1f-ec446685d6c4" /> then I get prompted with the following elicitation, as expected: <img width="1589" height="1335" alt="image" src="https://github.com/user-attachments/assets/21b68fe0-494d-4562-9bad-0ddc55fc846d" /> Though a current limitation is that the `shell` tool defaults to a timeout of 10s, which means I only have 10s to respond to the elicitation. Ideally, the time spent waiting for a response from a human should not count against the timeout for the command execution. I will address this in a subsequent PR. --- Note `~/code/bash/bash` was created by doing: ``` cd ~/code git clone https://github.com/bminor/bash cd bash git checkout a8a1c2fac029404d3f42cd39f5a20f24b6e4fe4b <apply the patch below> ./configure make ``` The patch: ``` diff --git a/execute_cmd.c b/execute_cmd.c index 070f5119..d20ad2b9 100644 --- a/execute_cmd.c +++ b/execute_cmd.c @@ -6129,6 +6129,19 @@ shell_execve (char *command, char **args, char **env) char sample[HASH_BANG_BUFSIZ]; size_t larray; + char* exec_wrapper = getenv("BASH_EXEC_WRAPPER"); + if (exec_wrapper && *exec_wrapper && !whitespace (*exec_wrapper)) + { + char *orig_command = command; + + larray = strvec_len (args); + + memmove (args + 2, args, (++larray) * sizeof (char *)); + args[0] = exec_wrapper; + args[1] = orig_command; + command = exec_wrapper; + } + ```
1 parent a6597a9 commit 13d378f

File tree

8 files changed

+108
-118
lines changed

8 files changed

+108
-118
lines changed

codex-rs/exec-server/Cargo.toml

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,16 @@ name = "codex-exec-server"
44
version = { workspace = true }
55

66
[[bin]]
7-
name = "codex-exec-server"
8-
path = "src/main.rs"
7+
name = "codex-execve-wrapper"
8+
path = "src/bin/main_execve_wrapper.rs"
9+
10+
[[bin]]
11+
name = "codex-exec-mcp-server"
12+
path = "src/bin/main_mcp_server.rs"
13+
14+
[lib]
15+
name = "codex_exec_server"
16+
path = "src/lib.rs"
917

1018
[lints]
1119
workspace = true
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#[cfg(not(unix))]
2+
fn main() {
3+
eprintln!("codex-execve-wrapper is only implemented for UNIX");
4+
std::process::exit(1);
5+
}
6+
7+
#[cfg(unix)]
8+
pub use codex_exec_server::main_execve_wrapper as main;
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#[cfg(not(unix))]
2+
fn main() {
3+
eprintln!("codex-exec-mcp-server is only implemented for UNIX");
4+
std::process::exit(1);
5+
}
6+
7+
#[cfg(unix)]
8+
pub use codex_exec_server::main_mcp_server as main;

codex-rs/exec-server/src/lib.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
#[cfg(unix)]
2+
mod posix;
3+
4+
#[cfg(unix)]
5+
pub use posix::main_execve_wrapper;
6+
7+
#[cfg(unix)]
8+
pub use posix::main_mcp_server;

codex-rs/exec-server/src/main.rs

Lines changed: 0 additions & 11 deletions
This file was deleted.

codex-rs/exec-server/src/posix.rs

Lines changed: 55 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,12 @@
5656
//! o<-----x
5757
//!
5858
use std::path::Path;
59+
use std::path::PathBuf;
5960

6061
use clap::Parser;
61-
use clap::Subcommand;
6262
use tracing_subscriber::EnvFilter;
6363
use tracing_subscriber::{self};
6464

65-
use crate::posix::escalate_protocol::EscalateAction;
66-
use crate::posix::escalate_server::EscalateServer;
67-
use crate::posix::escalation_policy::EscalationPolicy;
6865
use crate::posix::mcp_escalation_policy::ExecPolicyOutcome;
6966

7067
mod escalate_client;
@@ -75,124 +72,84 @@ mod mcp;
7572
mod mcp_escalation_policy;
7673
mod socket;
7774

75+
/// Default value of --execve option relative to the current executable.
76+
/// Note this must match the name of the binary as specified in Cargo.toml.
77+
const CODEX_EXECVE_WRAPPER_EXE_NAME: &str = "codex-execve-wrapper";
78+
7879
#[derive(Parser)]
79-
#[command(version)]
80-
pub struct Cli {
81-
#[command(subcommand)]
82-
subcommand: Option<Commands>,
80+
struct McpServerCli {
81+
/// Executable to delegate execve(2) calls to in Bash.
82+
#[arg(long = "execve")]
83+
execve_wrapper: Option<PathBuf>,
84+
85+
/// Path to Bash that has been patched to support execve() wrapping.
86+
#[arg(long = "bash")]
87+
bash_path: Option<PathBuf>,
8388
}
8489

85-
#[derive(Subcommand)]
86-
enum Commands {
87-
Escalate(EscalateArgs),
88-
ShellExec(ShellExecArgs),
90+
#[tokio::main]
91+
pub async fn main_mcp_server() -> anyhow::Result<()> {
92+
tracing_subscriber::fmt()
93+
.with_env_filter(EnvFilter::from_default_env())
94+
.with_writer(std::io::stderr)
95+
.with_ansi(false)
96+
.init();
97+
98+
let cli = McpServerCli::parse();
99+
let execve_wrapper = match cli.execve_wrapper {
100+
Some(path) => path,
101+
None => {
102+
let cwd = std::env::current_exe()?;
103+
cwd.parent()
104+
.map(|p| p.join(CODEX_EXECVE_WRAPPER_EXE_NAME))
105+
.ok_or_else(|| {
106+
anyhow::anyhow!("failed to determine execve wrapper path from current exe")
107+
})?
108+
}
109+
};
110+
let bash_path = match cli.bash_path {
111+
Some(path) => path,
112+
None => mcp::get_bash_path()?,
113+
};
114+
115+
tracing::info!("Starting MCP server");
116+
let service = mcp::serve(bash_path, execve_wrapper, dummy_exec_policy)
117+
.await
118+
.inspect_err(|e| {
119+
tracing::error!("serving error: {:?}", e);
120+
})?;
121+
122+
service.waiting().await?;
123+
Ok(())
89124
}
90125

91-
/// Invoked from within the sandbox to (potentially) escalate permissions.
92-
#[derive(Parser, Debug)]
93-
struct EscalateArgs {
126+
#[derive(Parser)]
127+
pub struct ExecveWrapperCli {
94128
file: String,
95129

96130
#[arg(trailing_var_arg = true)]
97131
argv: Vec<String>,
98132
}
99133

100-
impl EscalateArgs {
101-
/// This is the escalate client. It talks to the escalate server to determine whether to exec()
102-
/// the command directly or to proxy to the escalate server.
103-
async fn run(self) -> anyhow::Result<i32> {
104-
let EscalateArgs { file, argv } = self;
105-
escalate_client::run(file, argv).await
106-
}
107-
}
108-
109-
/// Debugging command to emulate an MCP "shell" tool call.
110-
#[derive(Parser, Debug)]
111-
struct ShellExecArgs {
112-
command: String,
113-
}
114-
115134
#[tokio::main]
116-
pub async fn main() -> anyhow::Result<()> {
117-
let cli = Cli::parse();
135+
pub async fn main_execve_wrapper() -> anyhow::Result<()> {
118136
tracing_subscriber::fmt()
119137
.with_env_filter(EnvFilter::from_default_env())
120138
.with_writer(std::io::stderr)
121139
.with_ansi(false)
122140
.init();
123141

124-
match cli.subcommand {
125-
Some(Commands::Escalate(args)) => {
126-
std::process::exit(args.run().await?);
127-
}
128-
Some(Commands::ShellExec(args)) => {
129-
let bash_path = mcp::get_bash_path()?;
130-
let escalate_server = EscalateServer::new(bash_path, DummyEscalationPolicy {});
131-
let result = escalate_server
132-
.exec(
133-
args.command.clone(),
134-
std::env::vars().collect(),
135-
std::env::current_dir()?,
136-
None,
137-
)
138-
.await?;
139-
println!("{result:?}");
140-
std::process::exit(result.exit_code);
141-
}
142-
None => {
143-
let bash_path = mcp::get_bash_path()?;
144-
145-
tracing::info!("Starting MCP server");
146-
let service = mcp::serve(bash_path, dummy_exec_policy)
147-
.await
148-
.inspect_err(|e| {
149-
tracing::error!("serving error: {:?}", e);
150-
})?;
151-
152-
service.waiting().await?;
153-
Ok(())
154-
}
155-
}
142+
let ExecveWrapperCli { file, argv } = ExecveWrapperCli::parse();
143+
let exit_code = escalate_client::run(file, argv).await?;
144+
std::process::exit(exit_code);
156145
}
157146

158147
// TODO: replace with execpolicy2
159148

160-
struct DummyEscalationPolicy;
161-
162-
#[async_trait::async_trait]
163-
impl EscalationPolicy for DummyEscalationPolicy {
164-
async fn determine_action(
165-
&self,
166-
file: &Path,
167-
argv: &[String],
168-
workdir: &Path,
169-
) -> Result<EscalateAction, rmcp::ErrorData> {
170-
let outcome = dummy_exec_policy(file, argv, workdir);
171-
let action = match outcome {
172-
ExecPolicyOutcome::Allow {
173-
run_with_escalated_permissions,
174-
} => {
175-
if run_with_escalated_permissions {
176-
EscalateAction::Escalate
177-
} else {
178-
EscalateAction::Run
179-
}
180-
}
181-
ExecPolicyOutcome::Forbidden => EscalateAction::Deny {
182-
reason: Some("Execution forbidden by policy".to_string()),
183-
},
184-
ExecPolicyOutcome::Prompt { .. } => EscalateAction::Deny {
185-
reason: Some("Could not prompt user for permission".to_string()),
186-
},
187-
};
188-
Ok(action)
189-
}
190-
}
191-
192149
fn dummy_exec_policy(file: &Path, argv: &[String], _workdir: &Path) -> ExecPolicyOutcome {
193-
if file.ends_with("/rm") {
150+
if file.ends_with("rm") {
194151
ExecPolicyOutcome::Forbidden
195-
} else if file.ends_with("/git") {
152+
} else if file.ends_with("git") {
196153
ExecPolicyOutcome::Prompt {
197154
run_with_escalated_permissions: false,
198155
}

codex-rs/exec-server/src/posix/escalate_server.rs

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,18 @@ use crate::posix::socket::AsyncSocket;
2727

2828
pub(crate) struct EscalateServer {
2929
bash_path: PathBuf,
30+
execve_wrapper: PathBuf,
3031
policy: Arc<dyn EscalationPolicy>,
3132
}
3233

3334
impl EscalateServer {
34-
pub fn new<P>(bash_path: PathBuf, policy: P) -> Self
35+
pub fn new<P>(bash_path: PathBuf, execve_wrapper: PathBuf, policy: P) -> Self
3536
where
3637
P: EscalationPolicy + Send + Sync + 'static,
3738
{
3839
Self {
3940
bash_path,
41+
execve_wrapper,
4042
policy: Arc::new(policy),
4143
}
4244
}
@@ -60,8 +62,15 @@ impl EscalateServer {
6062
);
6163
env.insert(
6264
BASH_EXEC_WRAPPER_ENV_VAR.to_string(),
63-
format!("{} escalate", std::env::current_exe()?.to_string_lossy()),
65+
self.execve_wrapper.to_string_lossy().to_string(),
6466
);
67+
68+
// TODO: use the sandbox policy and cwd from the calling client.
69+
// Note that sandbox_cwd is ignored for ReadOnly, but needs to be legit
70+
// for `SandboxPolicy::WorkspaceWrite`.
71+
let sandbox_policy = SandboxPolicy::ReadOnly;
72+
let sandbox_cwd = PathBuf::from("/__NONEXISTENT__");
73+
6574
let result = process_exec_tool_call(
6675
codex_core::exec::ExecParams {
6776
command: vec![
@@ -77,9 +86,8 @@ impl EscalateServer {
7786
arg0: None,
7887
},
7988
get_platform_sandbox().unwrap_or(SandboxType::None),
80-
// TODO: use the sandbox policy and cwd from the calling client
81-
&SandboxPolicy::ReadOnly,
82-
&PathBuf::from("/__NONEXISTENT__"), // This is ignored for ReadOnly
89+
&sandbox_policy,
90+
&sandbox_cwd,
8391
&None,
8492
None,
8593
)

codex-rs/exec-server/src/posix/mcp.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,15 +65,17 @@ impl From<escalate_server::ExecResult> for ExecResult {
6565
pub struct ExecTool {
6666
tool_router: ToolRouter<ExecTool>,
6767
bash_path: PathBuf,
68+
execve_wrapper: PathBuf,
6869
policy: ExecPolicy,
6970
}
7071

7172
#[tool_router]
7273
impl ExecTool {
73-
pub fn new(bash_path: PathBuf, policy: ExecPolicy) -> Self {
74+
pub fn new(bash_path: PathBuf, execve_wrapper: PathBuf, policy: ExecPolicy) -> Self {
7475
Self {
7576
tool_router: Self::tool_router(),
7677
bash_path,
78+
execve_wrapper,
7779
policy,
7880
}
7981
}
@@ -87,6 +89,7 @@ impl ExecTool {
8789
) -> Result<CallToolResult, McpError> {
8890
let escalate_server = EscalateServer::new(
8991
self.bash_path.clone(),
92+
self.execve_wrapper.clone(),
9093
McpEscalationPolicy::new(self.policy, context),
9194
);
9295
let result = escalate_server
@@ -130,8 +133,9 @@ impl ServerHandler for ExecTool {
130133

131134
pub(crate) async fn serve(
132135
bash_path: PathBuf,
136+
execve_wrapper: PathBuf,
133137
policy: ExecPolicy,
134138
) -> Result<RunningService<RoleServer, ExecTool>, rmcp::service::ServerInitializeError> {
135-
let tool = ExecTool::new(bash_path, policy);
139+
let tool = ExecTool::new(bash_path, execve_wrapper, policy);
136140
tool.serve(stdio()).await
137141
}

0 commit comments

Comments
 (0)