[6/8] Wire remote MCP stdio through executor#17974
[6/8] Wire remote MCP stdio through executor#17974aibrahim-oai wants to merge 1 commit intoopenai:dev/remote-mcp-executor-stdiofrom
Conversation
3fdfd5a to
e356123
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fdfd5a3a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let argv = executor_stdio_argv(program, args)?; | ||
| let env = executor_stdio_env(envs)?; |
There was a problem hiding this comment.
Resolve remote stdio command before invoking exec backend
The remote stdio path builds argv directly from program/args and never runs program_resolver::resolve. On Windows, Command::new("npx")-style invocations can fail without extension/PATHEXT resolution (the same case local stdio already handles). This makes MCP servers that work locally fail when environment = "remote".
Useful? React with 👍 / 👎.
e356123 to
675e1ea
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/rmcp-client/src/rmcp_client.rs
Lines 1263 to 1266 in 675e1ea
The executor-backed stdio process is started before MCP initialization, but handshake is wrapped in time::timeout. On timeout/error, this path returns without calling ExecProcess::terminate, so the child can continue running orphaned on the executor for the rest of the session. The local stdio path has kill_on_drop semantics; this new remote path does not.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6778f3a to
7e8dc3c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e8dc3c3ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| StdioTransportInner::Executor { transport } => ( | ||
| service::serve_client(client_service, transport).boxed(), | ||
| None, | ||
| None, |
There was a problem hiding this comment.
Terminate executor stdio process when client init fails
RmcpClient::connect_pending_transport keeps no cleanup guard for StdioTransportInner::Executor (process_group_guard is None). If service::serve_client(...) fails or times out during MCP initialize, rmcp drops the transport without invoking close(), so the executor-started MCP child is left running. Repeated startup retries can leak remote processes until the whole exec session is torn down.
Useful? React with 👍 / 👎.
8044335 to
e703917
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e703917e5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| submit_id, | ||
| tx_event, | ||
| SandboxPolicy::new_read_only_policy(), | ||
| None, |
There was a problem hiding this comment.
Pass executor environment into MCP snapshot startup
collect_mcp_snapshot_with_detail/collect_mcp_server_status_snapshot_with_detail now hard-code environment to None when creating McpConnectionManager. Any MCP server configured with environment = "remote" will always fail with “requires an executor environment” in snapshot/status APIs (including app-server status calls), even when an executor environment exists in the caller.
Useful? React with 👍 / 👎.
| let exec_environment = exec_environment.ok_or_else(|| { | ||
| StartupOutcomeError::from(anyhow!( | ||
| "remote MCP server `{server_name}` requires an executor environment" | ||
| )) | ||
| })?; | ||
| Arc::new(ExecutorStdioTransportRuntime::new( | ||
| exec_environment.get_exec_backend(), | ||
| remote_stdio_cwd, |
There was a problem hiding this comment.
Enforce remote backend before using remote MCP runtime
The Remote branch only checks that exec_environment is Some, then uses get_exec_backend() directly. A local Environment (default case) still satisfies this and silently runs environment = "remote" MCP servers on the local backend, violating expected remote placement/isolation semantics. Validate Environment::is_remote() (or equivalent) before selecting the remote runtime.
Useful? React with 👍 / 👎.
c686550 to
0ffdf16
Compare
fdb0d74 to
377fd73
Compare
0ffdf16 to
a01f83a
Compare
377fd73 to
c468eef
Compare
Use the MCP server environment setting to choose local stdio or executor-backed stdio at client startup time. Co-authored-by: Codex <noreply@openai.com>
a01f83a to
d997af8
Compare
c468eef to
83e867a
Compare
Summary
Stack