[5/6] Wire executor-backed MCP stdio#18212
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a087158ebe
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d4a2663d2
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4478580860
ℹ️ 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".
|
@codex review this |
Co-authored-by: Codex <noreply@openai.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7586c4629
ℹ️ 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".
|
|
||
| fn remote_env_policy() -> ExecEnvPolicy { | ||
| ExecEnvPolicy { | ||
| inherit: ShellEnvironmentPolicyInherit::Core, |
There was a problem hiding this comment.
Include HOME-like vars in remote MCP env inheritance
remote_env_policy() uses ShellEnvironmentPolicyInherit::Core, which only carries a minimal set of vars (e.g., PATH/SHELL/TMP*). Local stdio MCP launch includes DEFAULT_ENV_VARS like HOME/USER/LANG. Remote stdio servers that rely on HOME for config/auth paths can fail initialization when experimental_environment = "remote", creating behavior drift between local and remote placement.
Useful? React with 👍 / 👎.
|
|
||
| fn remote_env_policy() -> ExecEnvPolicy { | ||
| ExecEnvPolicy { | ||
| inherit: ShellEnvironmentPolicyInherit::Core, |
There was a problem hiding this comment.
Preserve HOME and user vars for remote MCP processes
remote_env_policy() inherits only Core, while local stdio includes DEFAULT_ENV_VARS (e.g., HOME, USER, LANG, TZ). Remote MCP servers that depend on home/profile paths for auth or config can fail only when experimental_environment = "remote", causing local/remote behavior drift.
Useful? React with 👍 / 👎.
| for chunk in response.chunks { | ||
| self.note_seq(chunk.seq); | ||
| self.push_process_output(chunk); |
There was a problem hiding this comment.
Deduplicate recovered chunks after broadcast lag
After RecvError::Lagged, recover_lagged_events() appends chunks from process.read(after_seq=last_seq), but the broadcast receiver will still yield retained events on later recv(). Since neither path filters by seq, the same output can be appended twice, yielding duplicated JSON-RPC frames and protocol-level failures.
Useful? React with 👍 / 👎.
| // accept arbitrary OsString values because it calls the OS directly; remote | ||
| // stdio must reject non-Unicode command, argument, or environment data | ||
| // before sending an executor request. | ||
| let argv = Self::process_api_argv(&program, &args).map_err(io::Error::other)?; |
There was a problem hiding this comment.
Resolve remote stdio command before building argv
Remote launch builds argv directly from raw program/args and skips the extension-aware resolver used by the local launcher. On Windows executors, script-based commands (npx, .cmd/.bat) may not resolve, so remote MCP stdio can fail even when the same config works locally.
Useful? React with 👍 / 👎.
|
|
||
| fn remote_env_policy() -> ExecEnvPolicy { | ||
| ExecEnvPolicy { | ||
| inherit: ShellEnvironmentPolicyInherit::Core, |
There was a problem hiding this comment.
Inherit HOME-like vars for remote MCP processes
remote_env_policy() uses ShellEnvironmentPolicyInherit::Core, which omits vars local stdio includes via DEFAULT_ENV_VARS (for example HOME, USER, LANG, TZ). Remote MCP servers that read config/auth data from home/profile paths can fail only when experimental_environment = "remote", creating local/remote behavior drift.
Useful? React with 👍 / 👎.
| for chunk in response.chunks { | ||
| self.note_seq(chunk.seq); | ||
| self.push_process_output(chunk); |
There was a problem hiding this comment.
Skip duplicate output after lag recovery
After RecvError::Lagged, recover_lagged_events() appends chunks from process.read(after_seq=last_seq), but the broadcast receiver can still deliver overlapping retained Output events. Because chunks are appended without seq dedupe, stdout lines can be duplicated and JSON-RPC frames may be replayed, breaking MCP message handling.
Useful? React with 👍 / 👎.
| // accept arbitrary OsString values because it calls the OS directly; remote | ||
| // stdio must reject non-Unicode command, argument, or environment data | ||
| // before sending an executor request. | ||
| let argv = Self::process_api_argv(&program, &args).map_err(io::Error::other)?; |
There was a problem hiding this comment.
Resolve remote stdio executable before process/start
Remote launch builds argv directly from raw program/args and skips resolver logic used by local launch. On Windows executors, script commands (.cmd/.bat, e.g. npx) may fail to execute remotely even when the same MCP config works locally.
Useful? React with 👍 / 👎.
| self.terminated = true; | ||
| self.process.terminate().await.map_err(io::Error::other) |
There was a problem hiding this comment.
Mark transport terminated only after terminate succeeds
close() sets self.terminated = true before awaiting process.terminate(). If terminate returns an error (e.g., transient remote RPC failure), Drop skips its termination fallback because terminated is already true, potentially leaking the remote MCP child process.
Useful? React with 👍 / 👎.
| Err(err) => { | ||
| let error = JSONRPCErrorError { | ||
| code: INTERNAL_ERROR_CODE, | ||
| message: format!("failed to create environment: {err}"), | ||
| data: None, | ||
| }; | ||
| self.outgoing.send_error(request, error).await; | ||
| return; |
There was a problem hiding this comment.
Degrade MCP status listing when env creation fails
list_mcp_server_status now returns a top-level internal error when environment_manager.current() fails. That aborts status listing for all servers, including local/HTTP servers that do not need executor-backed stdio, turning a recoverable environment issue into complete API failure.
Useful? React with 👍 / 👎.
|
@codex review this |
## Summary - Add a pushed `ExecProcessEvent` stream alongside retained `process/read` output. - Publish local and remote output, exit, close, and failure events. - Cover the event stream with shared local/remote exec process tests. ## Testing - `cargo check -p codex-exec-server` - `cargo check -p codex-rmcp-client` - Not run: `cargo test` per repo instruction; CI will cover. ## Stack ```text o #18027 [6/6] Fail exec client operations after disconnect │ o #18212 [5/6] Wire executor-backed MCP stdio │ o #18087 [4/6] Abstract MCP stdio server launching │ @ #18020 [3/6] Add pushed exec process events │ o #18086 [2/6] Support piped stdin in exec process API │ o #18085 [1/6] Add MCP server environment config │ o main ``` --------- Co-authored-by: Codex <noreply@openai.com>
0666bc7 to
39b8f9b
Compare
## Summary - Move local MCP stdio process startup behind a launcher trait. - Preserve existing local stdio behavior while making transport creation explicit. ## Stack ```text o #18027 [6/6] Fail exec client operations after disconnect │ o #18212 [5/6] Wire executor-backed MCP stdio │ @ #18087 [4/6] Abstract MCP stdio server launching │ o #18020 [3/6] Add pushed exec process events │ o #18086 [2/6] Support piped stdin in exec process API │ o #18085 [1/6] Add MCP server environment config │ o main ``` --------- Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
05c5a98 to
2b8b185
Compare
Co-authored-by: Codex <noreply@openai.com>
93f49bf to
be849e0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be849e0379
ℹ️ 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 process = Arc::clone(&self.process); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Guard drop-time terminate against missing Tokio runtime
ExecutorProcessTransport::drop unconditionally calls tokio::spawn. If this transport is dropped after runtime teardown (or on a non-Tokio thread), tokio::spawn panics (no reactor running) during cleanup. That turns a shutdown path into a crash and can still leave the MCP child alive. Use Handle::try_current() or a non-panicking fallback.
Useful? React with 👍 / 👎.
be849e0 to
0ac58e2
Compare
| let auth = self.auth_manager.auth().await; | ||
| let runtime_environment = match self.thread_manager.environment_manager().current().await { | ||
| Ok(Some(environment)) => { | ||
| McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf()) |
There was a problem hiding this comment.
Interesting, do we always start mcps in cwd?
0ac58e2 to
5615470
Compare
Add the executor-backed RMCP stdio transport, pass MCP runtime placement through manager construction, and cover stdio MCP tests with the remote-aware helpers. Co-authored-by: Codex <noreply@openai.com>
5615470 to
c219aeb
Compare
Allow stdio MCP env_vars entries to opt into local or remote sourcing while preserving the existing string form. Remote source names are inherited from the executor side through the existing process env policy, while local/default entries continue to be copied from Codex as explicit overlays. Add config, launcher, and remote-aware MCP tests that cover legacy env vars, local source vars, remote source filtering, and the effective executor env policy. Co-authored-by: Codex <noreply@openai.com>
Rename the runtime cwd carried into executor-backed MCP stdio to fallback_cwd so it is clear that per-server cwd still wins. Teach the stdio MCP test server to report its process cwd and add remote-aware coverage for configured cwd precedence plus remote fallback cwd behavior. Co-authored-by: Codex <noreply@openai.com>
Annotate empty env var slices in format_env_display tests so the generic env var display helper can infer the slice item type. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a7217771f
ℹ️ 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".
| .chain(remote_env_vars.iter().cloned()) | ||
| .collect() |
There was a problem hiding this comment.
Treat remote env var names as literals
remote_env_policy forwards remote_env_vars into include_only without escaping. include_only is matched as wildcard patterns, so a config like { name = "*", source = "remote" } will include all executor env vars (including secrets) instead of a single variable. Validate names or escape glob metacharacters before building include_only.
Useful? React with 👍 / 👎.
Use the MCP env var config type when mutating the stdio test fixture so CI builds after env var source support. Co-authored-by: Codex <noreply@openai.com>
Move cwd assertions to a dedicated stdio test tool so existing echo metadata and truncation expectations stay unchanged. Co-authored-by: Codex <noreply@openai.com>
Drop the helper left unused after moving cwd assertions to the dedicated cwd tool. Co-authored-by: Codex <noreply@openai.com>
Compare the MCP cwd tool output after path normalization so Windows short-path spellings do not fail the cwd precedence test. Co-authored-by: Codex <noreply@openai.com>
Use canonical path equality for local MCP cwd assertions while preserving raw remote cwd comparison. Co-authored-by: Codex <noreply@openai.com>
Summary
Stack