-
Notifications
You must be signed in to change notification settings - Fork 7.9k
fix(tui): conditionally restore status indicator using message phase #10947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
use new message phase field emitted by preamble-supported models to determine whether an AgentMessage is mid-turn commentary. if so, restore the status indicator afterwards to indicate the turn has not completed.
There was a problem hiding this 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.
Reviewed commit: 105274b834
ℹ️ 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 |
There was a problem hiding this 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.
Reviewed commit: ce3db63ba9
ℹ️ 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".
Document how AgentMessage phase metadata drives status-indicator restoration in the TUI so preamble-capable models are easier to review and debug. Clarify optional phase compatibility behavior in protocol types and note that legacy event conversion drops phase metadata. Add a documentation-pass writeup for PR 10947 in docs/.
fc8a774 to
4a5fb0a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At what point do we version bump the schema - is it left alone when the change is backwards compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure actually, maybe @owenlin0 knows?
TLDR: use new message phase field emitted by preamble-supported models to determine whether an AgentMessage is mid-turn commentary. if so, restore the status indicator afterwards to indicate the turn has not completed.
Problem
commit_tickhides the status indicator while streaming assistant text.For preamble-capable models, that text can be commentary mid-turn, so hiding was correct during streaming but restore timing mattered:
Fix
phasetoAgentMessageItemand propagate it fromResponseItem::Messagephase=commentary, andphase=Noneas final-answer behavior (no restore) to keep existing behavior for non-preamble modelsTests
Add/update tests for: