[5/6] Wire executor-backed MCP stdio#18088
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e61df763
ℹ️ 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".
| Err(broadcast::error::RecvError::Lagged(skipped)) => { | ||
| warn!( | ||
| "Remote MCP server output stream lagged ({}): skipped {skipped} events", | ||
| self.program_name | ||
| ); | ||
| self.closed = true; | ||
| } |
There was a problem hiding this comment.
Recover lagged event streams instead of terminating transport
RecvError::Lagged means this receiver skipped broadcast entries, not that the process is dead. The code sets closed = true, which makes receive() return EOF and triggers transport shutdown, aborting otherwise healthy MCP sessions during bursty output. Since ExecProcess also exposes retained read() data, this path should resync (or fail explicitly) rather than force-close.
Useful? React with 👍 / 👎.
b440257 to
c603449
Compare
f6e61df to
f11cc84
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f11cc84bdc
ℹ️ 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".
| Err(error) => { | ||
| debug!( | ||
| "Failed to parse remote MCP server message ({}): {error}", | ||
| self.program_name | ||
| ); | ||
| None | ||
| } |
There was a problem hiding this comment.
Drain buffered frames after parse errors
take_stdout_message drops an invalid line and returns None, then receive_message immediately waits on events.recv(). If a single output chunk contains bad\n{valid-json}\n, the valid JSON frame remains in self.stdout but is never processed until another event arrives. A quiet server can then stall MCP initialization until timeout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f11cc84bdc
ℹ️ 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".
|
|
||
| impl ExecutorProcessTransport { | ||
| pub(super) fn new(process: Arc<dyn ExecProcess>, program_name: String) -> Self { | ||
| let events = process.subscribe_events(); |
There was a problem hiding this comment.
Replay missed process events before awaiting broadcast
ExecutorProcessTransport::new subscribes to process.subscribe_events() only after start() has already returned. If the child exits immediately, Closed/Failed can be emitted before this subscription and are not replayed by broadcast. receive_message() then waits on events.recv() with closed == false, which can stall MCP handshake/reads indefinitely for dead-on-start servers.
Useful? React with 👍 / 👎.
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0af64c7008
ℹ️ 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".
| LaunchedStdioServerTransport::Executor { transport } => ( | ||
| service::serve_client(client_service, transport).boxed(), | ||
| None, | ||
| None, |
There was a problem hiding this comment.
Terminate executor process when handshake setup fails
Add cleanup parity with local stdio: this branch keeps no guard/owner for the spawned executor process. If serve_client errors or time::timeout elapses during initialization, the future is dropped and the process can remain running without a later terminate(), leaking MCP subprocesses across retries.
Useful? React with 👍 / 👎.
c603449 to
db32dd3
Compare
aad254a to
ad686d6
Compare
db32dd3 to
2ccbd13
Compare
## Summary - Add an MCP server environment setting with local as the default. - Thread the default through config serialization, schema generation, and existing config fixtures. ## Stack ```text o #18027 [8/8] Fail exec client operations after disconnect │ o #18025 [7/8] Cover MCP stdio tests with executor placement │ o #18089 [6/8] Wire remote MCP stdio through executor │ o #18088 [5/8] Add executor process transport for MCP stdio │ o #18087 [4/8] Abstract MCP stdio server launching │ o #18020 [3/8] Add pushed exec process events │ o #18086 [2/8] Support piped stdin in exec process API │ @ #18085 [1/8] Add MCP server environment config │ o main ``` Co-authored-by: Codex <noreply@openai.com>
ad686d6 to
ceffe05
Compare
0030462 to
afdd04f
Compare
ceffe05 to
2913392
Compare
afdd04f to
6075a2b
Compare
2913392 to
0281bcc
Compare
6075a2b to
b75ff9b
Compare
1f31311 to
19766ff
Compare
b75ff9b to
1823e57
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19766ff88a
ℹ️ 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".
| /// The executor process API still supports retained-output reads, but MCP | ||
| /// stdio is naturally streaming. This receiver lets rmcp wait for stdout | ||
| /// chunks without issuing `process/read` after each output notification. | ||
| events: broadcast::Receiver<ExecProcessEvent>, |
There was a problem hiding this comment.
Store the correct event receiver type
Change events to ExecProcessEventReceiver (or adapt subscribe_events() output) before constructing ExecutorProcessTransport. ExecProcess::subscribe_events returns ExecProcessEventReceiver, but the struct field is declared as tokio::sync::broadcast::Receiver<ExecProcessEvent>, causing a hard compile failure (E0308) when assigning events in new().
Useful? React with 👍 / 👎.
1823e57 to
df8be89
Compare
19766ff to
62f2369
Compare
df8be89 to
321268f
Compare
62f2369 to
bc359fc
Compare
321268f to
d94930b
Compare
bc359fc to
4c9f73c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c9f73c75f
ℹ️ 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 resolved_program = | ||
| program_resolver::resolve(program, &envs).map_err(io::Error::other)?; |
There was a problem hiding this comment.
Resolve executable path on the executor host
Avoid resolving program locally before process/start. program_resolver::resolve(...) runs in the orchestrator context, so on remote executor setups (especially Windows path resolution) it can produce a path that does not exist on the executor, causing MCP server launch failures. Pass the original program token and let the executor-side spawn environment resolve it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c9f73c75f
ℹ️ 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 resolved_program = | ||
| program_resolver::resolve(program, &envs).map_err(io::Error::other)?; |
There was a problem hiding this comment.
Resolve MCP command on executor host
ExecutorStdioServerLauncher::launch_server resolves program on the orchestrator before process/start. program_resolver::resolve is client-OS specific (notably Windows extension/path resolution), but executor processes may run on a different OS/FS. This can pass host-only paths (e.g., .cmd/C:\...) to the executor and cause startup failures. Send the original command and resolve at execution side.
Useful? React with 👍 / 👎.
## Summary - Add an explicit stdin mode to process/start. - Keep normal non-interactive exec stdin closed while allowing pipe-backed processes. ## Stack ```text o #18027 [8/8] Fail exec client operations after disconnect │ o #18025 [7/8] Cover MCP stdio tests with executor placement │ o #18089 [6/8] Wire remote MCP stdio through executor │ o #18088 [5/8] Add executor process transport for MCP stdio │ o #18087 [4/8] Abstract MCP stdio server launching │ o #18020 [3/8] Add pushed exec process events │ @ #18086 [2/8] Support piped stdin in exec process API │ o #18085 [1/8] Add MCP server environment config │ o main ``` Co-authored-by: Codex <noreply@openai.com>
1b53598 to
b04c034
Compare
b3a55be to
0666bc7
Compare
Add an rmcp transport and runtime that bridge stdio bytes through the executor process API. Co-authored-by: Codex <noreply@openai.com>
b04c034 to
97fd5d4
Compare
Use the MCP server experimental_environment string to choose local stdio or executor-backed stdio at client startup time. Co-authored-by: Codex <noreply@openai.com>
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: 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".
| submit_id, | ||
| tx_event, | ||
| SandboxPolicy::new_read_only_policy(), | ||
| /*environment*/ None, |
There was a problem hiding this comment.
Thread executor environment into MCP snapshot manager
collect_mcp_snapshot_with_detail constructs McpConnectionManager with /*environment*/ None. In make_rmcp_client, any stdio server with experimental_environment = "remote" then fails with requires an executor environment, so MCP snapshot/status APIs will always mark remote servers failed instead of actually connecting.
Useful? React with 👍 / 👎.
| submit_id, | ||
| tx_event, | ||
| SandboxPolicy::new_read_only_policy(), | ||
| /*environment*/ None, |
There was a problem hiding this comment.
Pass executor environment into MCP status snapshot startup
collect_mcp_server_status_snapshot_with_detail builds McpConnectionManager with environment = None. After this change, any stdio server configured with experimental_environment = "remote" now fails in make_rmcp_client (requires an executor environment), so status/list APIs report startup failures even when executor-backed MCP should work.
Useful? React with 👍 / 👎.
| INITIAL_SUBMIT_ID.to_owned(), | ||
| tx_event, | ||
| SandboxPolicy::new_read_only_policy(), | ||
| /*environment*/ None, |
There was a problem hiding this comment.
Supply executor environment when listing accessible connectors
list_accessible_connectors_from_mcp_tools_with_options_and_status also calls McpConnectionManager::new with environment = None. With the new remote stdio path, connector discovery now hard-fails for MCP servers using experimental_environment = "remote", regressing connector availability in app/tui flows.
Useful? React with 👍 / 👎.
## Summary - Add an MCP server environment setting with local as the default. - Thread the default through config serialization, schema generation, and existing config fixtures. ## Stack ```text o openai#18027 [8/8] Fail exec client operations after disconnect │ o openai#18025 [7/8] Cover MCP stdio tests with executor placement │ o openai#18089 [6/8] Wire remote MCP stdio through executor │ o openai#18088 [5/8] Add executor process transport for MCP stdio │ o openai#18087 [4/8] Abstract MCP stdio server launching │ o openai#18020 [3/8] Add pushed exec process events │ o openai#18086 [2/8] Support piped stdin in exec process API │ @ openai#18085 [1/8] Add MCP server environment config │ o main ``` Co-authored-by: Codex <noreply@openai.com>
## Summary - Add an explicit stdin mode to process/start. - Keep normal non-interactive exec stdin closed while allowing pipe-backed processes. ## Stack ```text o openai#18027 [8/8] Fail exec client operations after disconnect │ o openai#18025 [7/8] Cover MCP stdio tests with executor placement │ o openai#18089 [6/8] Wire remote MCP stdio through executor │ o openai#18088 [5/8] Add executor process transport for MCP stdio │ o openai#18087 [4/8] Abstract MCP stdio server launching │ o openai#18020 [3/8] Add pushed exec process events │ @ openai#18086 [2/8] Support piped stdin in exec process API │ o openai#18085 [1/8] Add MCP server environment config │ o main ``` Co-authored-by: Codex <noreply@openai.com>
## Summary - Add an MCP server environment setting with local as the default. - Thread the default through config serialization, schema generation, and existing config fixtures. ## Stack ```text o openai#18027 [8/8] Fail exec client operations after disconnect │ o openai#18025 [7/8] Cover MCP stdio tests with executor placement │ o openai#18089 [6/8] Wire remote MCP stdio through executor │ o openai#18088 [5/8] Add executor process transport for MCP stdio │ o openai#18087 [4/8] Abstract MCP stdio server launching │ o openai#18020 [3/8] Add pushed exec process events │ o openai#18086 [2/8] Support piped stdin in exec process API │ @ openai#18085 [1/8] Add MCP server environment config │ o main ``` Co-authored-by: Codex <noreply@openai.com>
## Summary - Add an explicit stdin mode to process/start. - Keep normal non-interactive exec stdin closed while allowing pipe-backed processes. ## Stack ```text o openai#18027 [8/8] Fail exec client operations after disconnect │ o openai#18025 [7/8] Cover MCP stdio tests with executor placement │ o openai#18089 [6/8] Wire remote MCP stdio through executor │ o openai#18088 [5/8] Add executor process transport for MCP stdio │ o openai#18087 [4/8] Abstract MCP stdio server launching │ o openai#18020 [3/8] Add pushed exec process events │ @ openai#18086 [2/8] Support piped stdin in exec process API │ o openai#18085 [1/8] Add MCP server environment config │ o main ``` Co-authored-by: Codex <noreply@openai.com>
## Summary - Add an MCP server environment setting with local as the default. - Thread the default through config serialization, schema generation, and existing config fixtures. ## Stack ```text o openai#18027 [8/8] Fail exec client operations after disconnect │ o openai#18025 [7/8] Cover MCP stdio tests with executor placement │ o openai#18089 [6/8] Wire remote MCP stdio through executor │ o openai#18088 [5/8] Add executor process transport for MCP stdio │ o openai#18087 [4/8] Abstract MCP stdio server launching │ o openai#18020 [3/8] Add pushed exec process events │ o openai#18086 [2/8] Support piped stdin in exec process API │ @ openai#18085 [1/8] Add MCP server environment config │ o main ``` Co-authored-by: Codex <noreply@openai.com>
## Summary - Add an explicit stdin mode to process/start. - Keep normal non-interactive exec stdin closed while allowing pipe-backed processes. ## Stack ```text o openai#18027 [8/8] Fail exec client operations after disconnect │ o openai#18025 [7/8] Cover MCP stdio tests with executor placement │ o openai#18089 [6/8] Wire remote MCP stdio through executor │ o openai#18088 [5/8] Add executor process transport for MCP stdio │ o openai#18087 [4/8] Abstract MCP stdio server launching │ o openai#18020 [3/8] Add pushed exec process events │ @ openai#18086 [2/8] Support piped stdin in exec process API │ o openai#18085 [1/8] Add MCP server environment config │ o main ``` Co-authored-by: Codex <noreply@openai.com>
Summary
Stack