Skip to content

fix(tui): surface thinking-only turns instead of silently ending (#1727)#1742

Closed
LeoLin990405 wants to merge 2 commits into
Hmbown:mainfrom
LeoLin990405:fix/1727-thinking-only-turn-status
Closed

fix(tui): surface thinking-only turns instead of silently ending (#1727)#1742
LeoLin990405 wants to merge 2 commits into
Hmbown:mainfrom
LeoLin990405:fix/1727-thinking-only-turn-status

Conversation

@LeoLin990405

Copy link
Copy Markdown
Contributor

Summary / 概述

EN: Fixes #1727. When a model streams a turn containing only a reasoning/thinking block — empty content, no tool_calls (e.g. gpt-oss via ollama's harmony→OpenAI shim, which maps the answer into reasoning_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 a status notice 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 an if‑only branch gated on has_sendable_assistant_content (true only for Text/ToolUse). A thinking‑only stream produces neither, so the if is skipped — no message, no event — and if tool_uses.is_empty() then falls straight through to break, 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 if arm 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(...): "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 helper should_emit_thinking_only_status(...) so it is unit‑testable and matches the existing should_hold_turn_for_subagents style. 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 / 改动边界

  • The existing if has_sendable_assistant_content branch is untouched; normal text/tool turns persist and render exactly as before.
  • No double‑notice: guarded by 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_end pins the decision: notify only on clean end; suppressed when tool uses pending / an error already shown / cancelled.

cargo test -p deepseek-tui --bin deepseek-tui -- thinking_only_turn_emits_status_only_on_clean_end   # ok
cargo clippy -p deepseek-tui   # clean (no warnings/errors)

Honest test limitation: the test pins the extracted pure decision, not the full async handle_deepseek_turn loop — 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). The else if wiring is covered by inspection.

新增单测 thinking_only_turn_emits_status_only_on_clean_end,与本模块既有“纯函数式”测试风格一致;端到端回合测试需 mock 客户端/会话/通道,超出本次外科式修复范围,已如实说明。

Refs #1727, #1736.

…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.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

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.

Comment thread crates/tui/src/core/engine/turn_loop.rs Outdated
Comment on lines 877 to 896
} 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;
}

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.

medium

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.
@LeoLin990405

Copy link
Copy Markdown
Contributor Author

Addressed in the latest push. The premature-emission concern is fixed by capture-then-decide:

  • At the persist site we now only capture thinking_only_no_sendable = !has_sendable_assistant_content (no event emitted there).
  • The notice is emitted at the end of the tool_uses.is_empty() path, just before the terminal break — reachable only when there were no pending steers (continued above), no sub-agent completions to resume with, and we were not holding for running children.
  • should_emit_thinking_only_status gained steers_pending and holding_for_subagents params (returns false if either), recomputed live at the decision point as defense-in-depth. Unit test updated with the two new no-emit cases.

So the status can no longer fire right before the turn resumes for a steer or sub-agent completion.

@Hmbown Hmbown mentioned this pull request May 20, 2026
11 tasks
@Hmbown

Hmbown commented May 21, 2026

Copy link
Copy Markdown
Owner

Thanks for the thinking-only turn fix. This was harvested into v0.8.40 release PR #1823 as commits 4ab7c53 and 351898d, and #1823 is now CI-green. Closing as superseded by the release branch; thank you for helping make silent turn endings visible.

@Hmbown Hmbown closed this May 21, 2026
getong pushed a commit to getong/CodeWhale that referenced this pull request May 21, 2026
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants