Skip to content

feat: emit events for unified_exec#5448

Merged
jif-oai merged 7 commits intomainfrom
jif/event_uexec
Oct 21, 2025
Merged

feat: emit events for unified_exec#5448
jif-oai merged 7 commits intomainfrom
jif/event_uexec

Conversation

@jif-oai
Copy link
Copy Markdown
Collaborator

@jif-oai jif-oai commented Oct 21, 2025

No description provided.

Copy link
Copy Markdown
Contributor

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +251 to +260
let event_command = command_tokens_for_event(request.input_chunks);
if !event_command.is_empty() {
emit_exec_command_begin(
context.session,
context.sub_id,
context.call_id,
&event_command,
context.turn.cwd.as_path(),
)
.await;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Emit matching ExecCommandEnd for unified_exec

The new begin event is fired for every unified exec request, but no matching ExecCommandEnd is emitted anywhere in this code path. Clients such as the TUI keep a running command map and clear it only when an end event arrives (handle_exec_begin_now inserts into running_commands, while handle_exec_end_now removes it), so unified exec commands will now remain stuck in a “running” state forever if the request succeeds or fails after this point. Consider emitting an end event (including errors) after the command completes or when acquisition fails to avoid UI spinners that never clear.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an open question: do we want to handle the end event emission? This seems not relevant by definition of the unified exec tool

Copy link
Copy Markdown
Collaborator

@pakrym-oai pakrym-oai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test?

@jif-oai jif-oai requested a review from pakrym-oai October 21, 2025 16:01
.await?;

let mut begin_events = Vec::new();
loop {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why loop? two separate wait_for_event_with_timeout should do it

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we want to make sure there is only one event

@jif-oai jif-oai merged commit 4bd68e4 into main Oct 21, 2025
20 checks passed
@jif-oai jif-oai deleted the jif/event_uexec branch October 21, 2025 16:32
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants