fix(tui): show commentary-phase text when no final_answer blocks present#69337
fix(tui): show commentary-phase text when no final_answer blocks present#69337CCcassiusdjs wants to merge 1 commit intoopenclaw:mainfrom
Conversation
TUI was invisible for messages with only commentary-phase text blocks because extractAssistantVisibleText returned undefined when hasExplicitPhasedTextBlocks=true and no final_answer phase existed. Add commentary-phase fallback before unphased extraction so streaming deltas and final messages are visible during the commentary phase and when models omit the final-answer tag entirely. Fixes openclaw#34513, openclaw#35523, openclaw#37647, openclaw#40824, openclaw#34537 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR adds a commentary-phase fallback to
Confidence Score: 4/5Safe to merge once the stale test is updated to reflect the new commentary-fallback behavior. The implementation change is correct and minimal, but an existing test asserts the old src/shared/chat-message-content.test.ts — the Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/shared/chat-message-content.ts
Line: 187-192
Comment:
**Existing test directly contradicts new behavior — test suite will fail**
The test at `src/shared/chat-message-content.test.ts:100` is named `"does not fall back to commentary-only text"` and asserts `extractAssistantVisibleText` returns `undefined` for a message with only commentary-phase blocks. The new fallback makes the same call return `"thinking like caveman"` instead, breaking that test.
The test needs to be updated to match the new intended behavior (expecting the commentary text, and likely renamed to `"falls back to commentary-only text when no final_answer blocks present"`).
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(tui): show commentary-phase text whe..." | Re-trigger Greptile |
| // Fall back to commentary-phase text when no final_answer blocks exist yet | ||
| // (streaming in progress, or model omits <final> tags entirely). | ||
| const commentaryText = extractAssistantTextForPhase(message, { phase: "commentary" }); | ||
| if (commentaryText) { | ||
| return commentaryText; | ||
| } |
There was a problem hiding this comment.
The test at src/shared/chat-message-content.test.ts:100 is named "does not fall back to commentary-only text" and asserts extractAssistantVisibleText returns undefined for a message with only commentary-phase blocks. The new fallback makes the same call return "thinking like caveman" instead, breaking that test.
The test needs to be updated to match the new intended behavior (expecting the commentary text, and likely renamed to "falls back to commentary-only text when no final_answer blocks present").
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/chat-message-content.ts
Line: 187-192
Comment:
**Existing test directly contradicts new behavior — test suite will fail**
The test at `src/shared/chat-message-content.test.ts:100` is named `"does not fall back to commentary-only text"` and asserts `extractAssistantVisibleText` returns `undefined` for a message with only commentary-phase blocks. The new fallback makes the same call return `"thinking like caveman"` instead, breaking that test.
The test needs to be updated to match the new intended behavior (expecting the commentary text, and likely renamed to `"falls back to commentary-only text when no final_answer blocks present"`).
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e40fef2b5d
ℹ️ 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".
| const commentaryText = extractAssistantTextForPhase(message, { phase: "commentary" }); | ||
| if (commentaryText) { | ||
| return commentaryText; |
There was a problem hiding this comment.
Limit commentary fallback to TUI-specific rendering
This helper is shared beyond TUI, including readPreview in src/gateway/session-utils.fs.ts, so the new unconditional commentary fallback makes commentary-only assistant messages appear in session previews instead of being hidden. That changes existing behavior validated by the gateway preview contract (see the "hides commentary-only assistant preview items" case in src/gateway/session-utils.fs.test.ts) and can expose reasoning text in non-TUI surfaces. If streaming needs commentary text, the fallback should be applied in the TUI call path (or behind a caller-specific option) rather than globally in extractAssistantVisibleText.
Useful? React with 👍 / 👎.
|
Codex review: found issues before merge. What this changes: The PR branch adds a commentary-phase fallback to the shared Maintainer follow-up before merge: A maintainer should choose the visibility policy for commentary text before rewriting this contributor branch; the current blocker is security/visibility-sensitive rather than a safe automated patch cleanup. Security review: Security review needs attention: The patch changes a shared visibility boundary and can expose commentary-only assistant text outside the intended TUI streaming context. Review findings:
Review detailsBest possible solution: Keep Do we have a high-confidence way to reproduce the issue? Yes. A source-level reproduction is a phased assistant message with only a commentary text block: current main returns no shared visible text, while this PR would return that commentary text and break the shared, Gateway preview, and Control UI suppression contracts. Is this the best way to solve the issue? No. The PR targets the observable TUI symptom, but changing the shared helper is broader than necessary and exposes commentary text in non-TUI surfaces; a caller-scoped TUI path or provider normalization is the safer direction. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 68c010906a94. |
Summary
extractAssistantVisibleTextreturnedundefinedwhen a message had phased text blocks but nofinal_answerblock yet, causing TUI to display nothing during streaming and for models that omit<final>tagsfinal_answercheck and the legacy unphased fallback so streaming deltas and fully-commentary messages are rendered correctlyRoot cause
Commit
22d8e47a50tightened phase handling: whenhasExplicitPhasedTextBlocksistrue, unphased extraction returnsundefined. This correctly prevents mixing phased and legacy content, but accidentally made all commentary-only windows invisible — the common state during streaming and the only state for models without<final>tags.Affected issues
Fixes #34513, #35523, #37647, #40824, #34537
Test plan
<final>tag arrives)<final>tags; reply should be visible in TUIpnpm test src/shared/green🤖 Generated with Claude Code