chore: rework unified exec events #7775
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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 open_call_id = "uexec-delayed-open"; | ||
| let open_args = json!({ | ||
| "cmd": "sleep 5 && echo MARKER1 && sleep 5 && echo MARKER2", |
There was a problem hiding this comment.
anything we can do to avoid a 15 second test?
There was a problem hiding this comment.
like start a tail -f and write to a file that's being tailed?
There was a problem hiding this comment.
If we don't want it to be flaky, not easily. Would be cool to have a custom shell command that waits for a signal from us
| /// events) can reference the exact PTY bytes corresponding to this tool call, | ||
| /// while the `output` field remains the human‑oriented, possibly truncated | ||
| /// representation used in tool responses. | ||
| pub raw_output: Vec<u8>, |
There was a problem hiding this comment.
Similar question to above, do we need separate output on TerminalInteraction?
There was a problem hiding this comment.
Same answer. I would argue that yes because this can contain truncation or so
| let yield_time_ms = clamp_yield_time(request.yield_time_ms); | ||
|
|
||
| let start = Instant::now(); | ||
| // For the initial exec_command call, we both stream output to events |
There was a problem hiding this comment.
can we read from CommandTranscript ? So there is only one way for output to flow. Similarly with exit code, is it better to have a single thing watching for exit?
There was a problem hiding this comment.
We would beed a notify in there then...
pakrym-oai
left a comment
pakrym-oai
left a comment
There was a problem hiding this comment.
The flow of output/error codes feels a bit complicated with two paralel systems used to serve events and write_stdin.
…ts-2 # Conflicts: # codex-rs/core/src/tools/handlers/unified_exec.rs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| tokio::spawn(async move { | ||
| exit_token.cancelled().await; | ||
|
|
||
| let exit_code = session.exit_code().unwrap_or(-1); | ||
| let duration = Instant::now().saturating_duration_since(started_at); |
There was a problem hiding this comment.
Long‑running unified exec sessions are inserted into the session store (and their process IDs reserved) but when the background exit watcher fires it only emits ExecCommandEnd and returns, never removing the session entry or releasing the process ID. If a command finishes without any subsequent write_stdin poll, the stale entry stays counted toward the session limits, causing unnecessary "Waiting for …" warnings and eventual pruning of newer sessions even though the completed PTY has already exited. The watcher here needs to clean up the store/reserved IDs once it sends the end event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is on purpose
No description provided.