[3/6] Add pushed exec process events#18020
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5c217e55a
ℹ️ 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".
cf20755 to
315ecdf
Compare
a5c217e to
69f6bd3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69f6bd31a5
ℹ️ 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: 69f6bd31a5
ℹ️ 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".
69f6bd3 to
0681926
Compare
4942819 to
cd30150
Compare
0681926 to
4909acf
Compare
cd30150 to
8978634
Compare
4909acf to
e89f9e2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e89f9e2eee
ℹ️ 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".
## 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>
8978634 to
1c5a436
Compare
5d0fa36 to
fe13f51
Compare
1c5a436 to
f99255d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe13f51991
ℹ️ 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".
| session.note_change(params.seq); | ||
| session.publish_event(ExecProcessEvent::Output(ProcessOutputChunk { | ||
| seq: params.seq, | ||
| stream: params.stream, | ||
| chunk: params.chunk, | ||
| })); |
There was a problem hiding this comment.
Reorder remote exec events by seq before publishing
handle_server_notification publishes remote Output/Exited events immediately in arrival order. Server notifications are emitted from concurrent stdout/stderr/exit tasks, so lower-seq events can arrive after higher-seq ones. Event-only consumers can then reconstruct output incorrectly or see lifecycle transitions out of order. Buffer/reorder by seq before appending to the event log.
Useful? React with 👍 / 👎.
7d383a5 to
60d1e5b
Compare
f99255d to
d29ecfc
Compare
725940a to
e9b4e30
Compare
893c39b to
ed6b28c
Compare
Add pipe_stdin to process/start so non-tty processes can opt into a writable stdin pipe for process/write. Co-authored-by: Codex <noreply@openai.com>
Add a retained event stream for exec processes so subscribers can consume output and lifecycle events either live or after the process has already closed. Co-authored-by: Codex <noreply@openai.com>
e9b4e30 to
53ea2a8
Compare
ed6b28c to
086ae0a
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/exec-server/src/local_process.rs
Lines 121 to 127 in 53ea2a8
LocalProcess::shutdown drains inner.processes before terminating child sessions. Once removed, stream_output/watch_exit can no longer find the process and skip publishing Exited/Closed (or Failed) events. Event-only callers waiting on subscribe_events() can block indefinitely after shutdown because LocalExecProcess still holds an event sender.
ℹ️ 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".
## 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>
Add an empty exec process event receiver for mocks and update the unified exec test mock to implement the new event subscription hook. Co-authored-by: Codex <noreply@openai.com>
jif-oai
left a comment
There was a problem hiding this comment.
The fact notifications can arrive out of orders was expected (which is why I love having seq numbers) but we will have to be very careful on the client side
Approving to unblock but please process my comments
| .history | ||
| .lock() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| history.push_back(event.clone()); |
There was a problem hiding this comment.
This event history is capped by event count, not retained bytes. So with large chunks we could potentially be unbounded (and above the existing 1MiNB cap)
| } | ||
| } | ||
|
|
||
| pub async fn recv(&mut self) -> Result<ExecProcessEvent, broadcast::error::RecvError> { |
There was a problem hiding this comment.
OOC what's the plan to support Lagged? I think this function should be documented as it will be pretty important
| Ok(()) | ||
| } | ||
|
|
||
| async fn assert_exec_process_pushes_events(use_remote: bool) -> Result<()> { |
There was a problem hiding this comment.
We should also test the hard case where output/exited/closed/.. notifications arrive out of seq order
| Output(ProcessOutputChunk), | ||
| Exited { seq: u64, exit_code: i32 }, | ||
| Closed { seq: u64 }, | ||
| Failed(String), |
There was a problem hiding this comment.
Should we add a seq number? Would make things cleaner
Buffer pushed process events by sequence number so out-of-order closed notifications do not cause later output or exited notifications to be dropped. Bound replay history by retained bytes as well as event count. Co-authored-by: Codex <noreply@openai.com>
3fbc20c to
869142f
Compare
|
@codex review |
Summary
ExecProcessEventstream alongside retainedprocess/readoutput.Testing
cargo check -p codex-exec-servercargo check -p codex-rmcp-clientcargo testper repo instruction; CI will cover.Stack