fix(tui): surface thinking-only turns instead of silently ending (#1727)#1742
fix(tui): surface thinking-only turns instead of silently ending (#1727)#1742LeoLin990405 wants to merge 2 commits into
Conversation
…own#1727) When a model streams a turn with only a reasoning block (empty content, no tool_calls -- e.g. gpt-oss via ollama's harmony->OpenAI shim mapping to reasoning_content), has_sendable_assistant_content is false: the if-only persist branch was skipped, NO event was emitted, and the turn fell through to break. The UI spinner hung with no reply and no error. Add an else-if at the same persist site that, only on a clean end (tool_uses empty, turn_error.is_none(), not cancelled), warns and emits an Event::status notice telling the user the turn ended and they can retry. No assistant message is persisted (prior behavior preserved); no retry/re-prompt/placeholder policy is added (out of scope). Guard ordering extracted into a pure should_emit_thinking_only_status() helper with a unit test, matching the existing should_hold_turn_for_subagents style. Maintainer audit Hmbown#1736 confirms Hmbown#1727 is not shipped in v0.8.39 and release-blocking. Refs Hmbown#1727, Hmbown#1736.
There was a problem hiding this comment.
Code Review
This pull request addresses issue #1727, where the UI spinner would hang if a model returned only reasoning content without text or tool calls. It introduces a should_emit_thinking_only_status helper and logic in the turn loop to notify the user when a turn ends without output. A regression test was also included. Review feedback indicates that the status message may be emitted prematurely if the turn continues due to pending steers or subagent completions, and suggests moving the logic to the end of the tool-use check block to ensure the turn has truly finished.
| } else if should_emit_thinking_only_status( | ||
| tool_uses.is_empty(), | ||
| turn_error.is_none(), | ||
| self.cancel_token.is_cancelled(), | ||
| ) { | ||
| // Issue #1727: the model streamed ONLY a reasoning/thinking | ||
| // block — empty content, no tool calls (e.g. gpt-oss via | ||
| // ollama's harmony→OpenAI shim mapping to `reasoning_content`). | ||
| // We intentionally do NOT persist an assistant message | ||
| // (preserves prior behavior), but the previous code emitted | ||
| // nothing at all and fell straight through to finishing the | ||
| // turn, leaving the UI spinner hung with no error. Surface a | ||
| // status so the user knows the turn ended and can retry. | ||
| let message = | ||
| "Model returned reasoning but no answer or tool call; turn ended without \ | ||
| output. Send a follow-up to retry." | ||
| .to_string(); | ||
| crate::logging::warn(&message); | ||
| let _ = self.tx_event.send(Event::status(message)).await; | ||
| } |
There was a problem hiding this comment.
The status message is emitted prematurely here. If the turn is going to continue due to pending steers (line 910) or subagent completions (line 991), the user will see a confusing "turn ended without output" message followed by the turn resuming.
Consider moving this logic to the end of the if tool_uses.is_empty() block (around line 1086), where it is certain that the turn is actually finishing without any sendable content.
}…own#1742) Address gemini-code-assist review on PR Hmbown#1742 (MEDIUM): the status was emitted at the assistant-persist site, but the same turn can still CONTINUE for pending steers or sub-agent completions -- the user would see a spurious 'turn ended without output' notice immediately before the turn resumed. Capture thinking_only_no_sendable at the persist site (no emission there) and decide at the end of the tool_uses.is_empty() path, just before the terminal break -- reachable only when there were no pending steers, no sub-agent completions, and we were not holding for running children. Extend should_emit_thinking_only_status with steers_pending and holding_for_subagents (false if either), recomputed live at the decision point as defense-in-depth. Unit test updated with the two new no-emit cases. Refs Hmbown#1727.
|
Addressed in the latest push. The premature-emission concern is fixed by capture-then-decide:
So the status can no longer fire right before the turn resumes for a steer or sub-agent completion. |
…own#1742) Address gemini-code-assist review on PR Hmbown#1742 (MEDIUM): the status was emitted at the assistant-persist site, but the same turn can still CONTINUE for pending steers or sub-agent completions -- the user would see a spurious 'turn ended without output' notice immediately before the turn resumed. Capture thinking_only_no_sendable at the persist site (no emission there) and decide at the end of the tool_uses.is_empty() path, just before the terminal break -- reachable only when there were no pending steers, no sub-agent completions, and we were not holding for running children. Extend should_emit_thinking_only_status with steers_pending and holding_for_subagents (false if either), recomputed live at the decision point as defense-in-depth. Unit test updated with the two new no-emit cases. Refs Hmbown#1727.
Summary / 概述
EN: Fixes #1727. When a model streams a turn containing only a reasoning/thinking block — empty
content, notool_calls(e.g. gpt-oss via ollama's harmony→OpenAI shim, which maps the answer intoreasoning_content) — the engine persisted nothing, emitted no event, and finished the turn silently. The UI sat idle: spinner never cleared, no reply, no error. It looked hung. This PR surfaces astatusnotice instead of silence.中文: 修复 #1727。当模型本回合只返回了 reasoning/thinking 块——
content为空、无tool_calls(例如 ollama 的 harmony→OpenAI 适配把答案塞进reasoning_content的 gpt-oss)——引擎既不持久化任何消息、也不发出任何事件,回合静默结束。界面就此卡住:spinner 不消失、无回复、无报错,看起来像挂死。本 PR 用一条status提示替代这种静默。Root cause / 根因
EN: In
crates/tui/src/core/engine/turn_loop.rs, the assistant‑message persist site is anif‑only branch gated onhas_sendable_assistant_content(true only forText/ToolUse). A thinking‑only stream produces neither, so theifis skipped — no message, no event — andif tool_uses.is_empty()then falls straight through tobreak, ending the turn with zero events emitted. The maintainer's v0.8.40 audit (#1736) confirms #1727 was not shipped in v0.8.39 and is release‑blocking, and that this exact status branch is still missing.中文: 在
crates/tui/src/core/engine/turn_loop.rs中,assistant 消息的持久化处是一个仅有if的分支,条件为has_sendable_assistant_content(仅Text/ToolUse为真)。thinking‑only 流两者皆无,于是该if被跳过——既无消息也无事件——随后if tool_uses.is_empty()直接走到break,回合在零事件下结束。维护者的 v0.8.40 审计(#1736)确认 #1727 在 v0.8.39 并未随版本发布、属于发布阻断项,且这一 status 分支至今缺失。Fix / 修复
Minimum‑surface‑area (the issue author's "option 1"): add an
else ifarm at the same persist site that, only on a clean end (tool_usesempty,turn_error.is_none(), not cancelled),warns and emits anEvent::status(...): "Model returned reasoning but no answer or tool call; turn ended without output. Send a follow‑up to retry." No assistant message is persisted (prior behavior preserved). No retry/re‑prompt/placeholder policy is added (explicitly out of scope). The guard ordering is extracted into a tiny pure helpershould_emit_thinking_only_status(...)so it is unit‑testable and matches the existingshould_hold_turn_for_subagentsstyle. Reuses the exact surrounding patterns:Event::status,crate::logging::warn,self.cancel_token.is_cancelled(),turn_error.is_none().中文: 最小改动面(issue 作者的“方案一”):在同一持久化处新增
else if分支,仅在干净结束时(tool_uses为空、turn_error.is_none()、未取消)warn并发出Event::status(...):“模型返回了推理但没有答案或工具调用;本回合无输出结束,可发送追问重试。” 不持久化任何 assistant 消息(保持原行为);不加入重试/重提示/占位策略(明确超出范围)。守卫顺序被抽到一个纯函数小助手should_emit_thinking_only_status(...),便于单测,并与既有should_hold_turn_for_subagents风格一致。复用周边既有写法。Scope / 改动边界
if has_sendable_assistant_contentbranch is untouched; normal text/tool turns persist and render exactly as before.turn_error.is_none()(stream errors still surface their own error) and!cancelled.if分支不动;正常文本/工具回合行为不变。受turn_error.is_none()、!cancelled守卫,不会重复提示。Testing / 测试
New unit test
thinking_only_turn_emits_status_only_on_clean_endpins the decision: notify only on clean end; suppressed when tool uses pending / an error already shown / cancelled.Honest test limitation: the test pins the extracted pure decision, not the full async
handle_deepseek_turnloop — consistent with every existing turn‑loop test in this module (they all pin pure helpers; an end‑to‑end test would need a mock client/session/channel, outside a surgical fix). Theelse ifwiring is covered by inspection.新增单测
thinking_only_turn_emits_status_only_on_clean_end,与本模块既有“纯函数式”测试风格一致;端到端回合测试需 mock 客户端/会话/通道,超出本次外科式修复范围,已如实说明。Refs #1727, #1736.