From 351898d05f7c84fc0657b65699f56093816bed09 Mon Sep 17 00:00:00 2001 From: Zhongyue Lin <101193087+LeoLin990405@users.noreply.github.com> Date: Mon, 18 May 2026 01:50:07 +0800 Subject: [PATCH] fix(tui): emit thinking-only notice only at true turn end (review #1742) Address gemini-code-assist review on PR #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 #1727. --- crates/tui/src/core/engine/turn_loop.rs | 131 ++++++++++++++++++------ 1 file changed, 97 insertions(+), 34 deletions(-) diff --git a/crates/tui/src/core/engine/turn_loop.rs b/crates/tui/src/core/engine/turn_loop.rs index 74d142fe..f5ed2b26 100644 --- a/crates/tui/src/core/engine/turn_loop.rs +++ b/crates/tui/src/core/engine/turn_loop.rs @@ -867,6 +867,17 @@ impl Engine { ) }); + // Issue #1727: did this turn produce 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 do NOT + // surface anything here: after this point the same turn can still + // CONTINUE for pending steers (~below) or sub-agent completions, + // and emitting now would show a spurious "turn ended" notice right + // before the turn resumes. Capture the fact and decide later, at + // the point the turn is certain to be finishing with no sendable + // content (see the `tool_uses.is_empty()` tail). + let thinking_only_no_sendable = !has_sendable_assistant_content; + // Add assistant message to session if has_sendable_assistant_content { self.add_session_message(Message { @@ -874,25 +885,6 @@ impl Engine { content: content_blocks, }) .await; - } 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; } // If no tool uses, check for inline REPL blocks (paper §2) or @@ -1083,6 +1075,39 @@ impl Engine { continue; } + // Issue #1727: the turn is now genuinely finishing with no + // sendable content. Control only reaches here when there were + // no pending steers (`continue`d above), no sub-agent + // completions to resume with, and we were not holding for + // running children (the `should_hold_turn_for_subagents` + // branch above would have awaited / `continue`d / returned). + // If the assistant produced ONLY a reasoning block, the prior + // code fell straight through to this `break`, emitting nothing + // and leaving the UI spinner hung. Surface a status now — + // safe because the turn can no longer resume. + if thinking_only_no_sendable { + let holding_for_subagents = { + let running = { + let mgr = self.subagent_manager.read().await; + mgr.running_count() + }; + should_hold_turn_for_subagents(0, running) + }; + if should_emit_thinking_only_status( + tool_uses.is_empty(), + turn_error.is_none(), + self.cancel_token.is_cancelled(), + !pending_steers.is_empty(), + holding_for_subagents, + ) { + 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; + } + } + break; } @@ -1988,16 +2013,25 @@ fn should_hold_turn_for_subagents(queued_completions: usize, running_children: u /// /// Reached when the assistant turn had no sendable content (no Text, no /// ToolUse — only a reasoning/thinking block). We notify the user *only* when -/// the turn is otherwise ending cleanly: no tool uses to dispatch, no -/// `turn_error` already surfaced for this turn, and the request wasn't -/// cancelled. In those cases the prior code emitted nothing and the UI -/// spinner hung silently. +/// the turn is genuinely finishing: no tool uses to dispatch, no `turn_error` +/// already surfaced for this turn, the request wasn't cancelled, AND the turn +/// is not about to CONTINUE — there are no pending steers and we are not +/// holding the turn open for running sub-agents. The status must fire at the +/// point the turn truly ends; emitting it earlier (at the persist site) would +/// show a spurious "turn ended" notice immediately before the turn resumed +/// for a steer or a sub-agent completion. fn should_emit_thinking_only_status( tool_uses_empty: bool, turn_error_is_none: bool, cancelled: bool, + steers_pending: bool, + holding_for_subagents: bool, ) -> bool { - tool_uses_empty && turn_error_is_none && !cancelled + tool_uses_empty + && turn_error_is_none + && !cancelled + && !steers_pending + && !holding_for_subagents } /// Resolve an `"auto"` reasoning-effort tier to a concrete value. @@ -2092,30 +2126,59 @@ mod tests { /// error, looking hung. /// /// This pins the decision: a clean turn end (no tool uses to dispatch, no - /// `turn_error`, not cancelled) must surface a status. We must NOT spam the - /// status when the turn is ending for another reason (error already shown, - /// cancelled) or when there are tool uses still to dispatch. + /// `turn_error`, not cancelled, no pending steers, not holding for + /// sub-agents) must surface a status. We must NOT spam the status when the + /// turn is ending for another reason (error already shown, cancelled), + /// when there are tool uses still to dispatch, or — critically (the + /// MEDIUM review finding) — when the turn is about to CONTINUE because a + /// steer is pending or sub-agents are still running. Emitting at the old + /// persist site fired before those continuations were known. /// /// Limitation: this tests the extracted pure decision, not the full async /// `handle_deepseek_turn` loop (driving it would need a mock DeepSeek /// client + session + channels — far beyond a surgical fix and unlike any /// existing turn-loop test, which all pin pure helpers the same way). The - /// `else if` wiring at the persist site is reviewed by inspection. + /// wiring at the `tool_uses.is_empty()` tail (capture-then-decide, with the + /// live steer/sub-agent signals) is reviewed by inspection — consistent + /// with how the other turn-loop helpers in this module are tested. #[test] fn thinking_only_turn_emits_status_only_on_clean_end() { - // Thinking-only response, turn ending cleanly → surface a status so - // the user isn't left staring at a hung spinner. - assert!(should_emit_thinking_only_status(true, true, false)); + // Thinking-only response, turn genuinely ending (no tool uses, no + // error, not cancelled, no steers pending, not holding for + // sub-agents) → surface a status so the user isn't left staring at a + // hung spinner. + assert!(should_emit_thinking_only_status( + true, true, false, false, false + )); // Tool uses still pending → the normal dispatch path handles it; no // thinking-only status. - assert!(!should_emit_thinking_only_status(false, true, false)); + assert!(!should_emit_thinking_only_status( + false, true, false, false, false + )); // A turn_error was already surfaced → don't double-report. - assert!(!should_emit_thinking_only_status(true, false, false)); + assert!(!should_emit_thinking_only_status( + true, false, false, false, false + )); // Request was cancelled → cancellation status already covers it. - assert!(!should_emit_thinking_only_status(true, true, true)); + assert!(!should_emit_thinking_only_status( + true, true, true, false, false + )); + + // A steer is pending → the turn will resume with the steer; emitting + // "turn ended" now would be a spurious notice right before the turn + // continues (the MEDIUM correctness finding). + assert!(!should_emit_thinking_only_status( + true, true, false, true, false + )); + + // Sub-agents are still running / completions queued → the turn is + // held open and will resume; do not claim it ended. + assert!(!should_emit_thinking_only_status( + true, true, false, false, true + )); } /// Regression test for the OpenAI streaming batch tool_calls bug.