From 741f36d2be1e3060dcd34781ebc6fe9ee1cc721e Mon Sep 17 00:00:00 2001 From: LinQ Date: Sun, 10 May 2026 19:11:52 +0100 Subject: [PATCH] fix(ui): drain active thinking entry on MessageComplete (#861 RC3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the order-of-events race in #861 root cause 3: when the engine bursts events, the dispatch loop can pull `MessageComplete` off the channel ahead of `ThinkingComplete`. Today's `MessageComplete` reads `app.last_reasoning.take()` to attach the reasoning block to the assistant message in `api_messages`. If `ThinkingComplete` has not fired yet, `last_reasoning` is `None` and the thinking content is dropped — DeepSeek V4 then returns HTTP 400 on the next turn because it requires `reasoning_content` replay for assistant messages that carry tool calls. Adds a defensive head-of-handler drain in `MessageComplete`: when `streaming_thinking_active_entry.is_some()`, finalize the active thinking entry and stash the reasoning buffer into `last_reasoning` before the existing body runs. The drain is a no-op in the normal case where `ThinkingComplete` arrived first (the entry has already been cleared), so this branch is order-independent. Adds `message_complete_drain_preserves_thinking_when_thinking_complete_lost` which exercises the head-of-handler invariant directly: with a thinking entry still active and `last_reasoning` empty, the drain must move the buffer into `last_reasoning` before downstream reads. Refs #861 (RC3). RC1 and RC2 are already addressed by the existing `finalize_current_streaming_thinking` plumbing in `apply_engine_error_to_app` and `start_streaming_thinking_block`; RC4 (streaming-time truncation affordance) is left out of this PR to keep the scope on the data-loss path. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/tui/src/tui/ui.rs | 18 ++++++++++++ crates/tui/src/tui/ui/tests.rs | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index 00b40c56..7a50558a 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -698,6 +698,24 @@ async fn run_event_loop( } } EngineEvent::MessageComplete { .. } => { + // #861 RC3: defensive drain of a still-active thinking + // entry. Normally `ThinkingComplete` arrives first and + // populates `last_reasoning` before we get here, but + // when the engine bursts events the channel can + // deliver `MessageComplete` first, in which case + // `last_reasoning.take()` below would be `None` and + // the thinking block would be dropped from + // `api_messages` — causing a DeepSeek HTTP 400 on the + // next turn (V4 thinking-mode requires + // `reasoning_content` replay). Inline-finalize the + // thinking entry here so this branch is order- + // independent. + if app.streaming_thinking_active_entry.is_some() { + if finalize_current_streaming_thinking(app) { + transcript_batch_updated = true; + } + stash_reasoning_buffer_into_last_reasoning(app); + } if let Some(index) = app.streaming_message_index.take() { let remaining = app.streaming_state.finalize_block_text(0); if !remaining.is_empty() { diff --git a/crates/tui/src/tui/ui/tests.rs b/crates/tui/src/tui/ui/tests.rs index bb4c628a..d9ba428b 100644 --- a/crates/tui/src/tui/ui/tests.rs +++ b/crates/tui/src/tui/ui/tests.rs @@ -3860,6 +3860,57 @@ fn engine_error_finalizes_active_thinking_block() { assert!(app.streaming_thinking_active_entry.is_none()); } +#[test] +fn message_complete_drain_preserves_thinking_when_thinking_complete_lost() { + // #861 RC3: when the engine bursts events, `MessageComplete` can be + // dispatched ahead of `ThinkingComplete`. Without the defensive drain, + // `app.last_reasoning` would be `None` at `last_reasoning.take()` time + // and the thinking block would be dropped from `api_messages`, + // causing a DeepSeek HTTP 400 on the next turn (V4 thinking-mode + // requires `reasoning_content` replay). + // + // This test exercises the head-of-handler drain in isolation: with a + // thinking entry still active and `last_reasoning` empty, the drain + // must transfer `reasoning_buffer` into `last_reasoning` before the + // remainder of `MessageComplete` reads it. + let mut app = create_test_app(); + + let _ = ensure_streaming_thinking_active_entry(&mut app); + app.thinking_started_at = Some(Instant::now()); + app.streaming_state.start_thinking(0, None); + app.streaming_state.push_content(0, "deep reasoning text"); + let _ = app.streaming_state.commit_text(0); + app.reasoning_buffer.push_str("deep reasoning text"); + + assert!( + app.last_reasoning.is_none(), + "precondition: ThinkingComplete has NOT fired" + ); + assert!( + app.streaming_thinking_active_entry.is_some(), + "precondition: thinking entry is still active" + ); + + // Mirror the head of `EngineEvent::MessageComplete` — the new defensive + // drain installed by the #861 RC3 fix. + if app.streaming_thinking_active_entry.is_some() { + let _ = finalize_current_streaming_thinking(&mut app); + stash_reasoning_buffer_into_last_reasoning(&mut app); + } + + assert!( + app.last_reasoning + .as_deref() + .is_some_and(|s| s.contains("deep reasoning text")), + "defensive drain must move reasoning into last_reasoning so the\ + downstream `last_reasoning.take()` produces a Thinking block" + ); + assert!( + app.streaming_thinking_active_entry.is_none(), + "thinking entry must be cleared after the drain" + ); +} + #[test] fn second_thinking_block_appends_new_entry_in_same_active_cell() { // Real V4 turns can emit Thinking → Tool → Thinking → Tool before any