fix: prevent thinking content from leaking into visible answer in shared chat (Go mode)#17225
Open
euvre wants to merge 1 commit into
Open
fix: prevent thinking content from leaking into visible answer in shared chat (Go mode)#17225euvre wants to merge 1 commit into
euvre wants to merge 1 commit into
Conversation
…nswer When using the shared chat embed (chat-share), reasoning/thinking content was appearing in the main answer body instead of being isolated in the collapsible 'Thinking...' section. Root cause (two layers): 1. think_tag.go: EnterReasoning()/ExitReasoning() did not write <think>/</think> markers into fullText, so ExtractVisibleAnswer() could not strip reasoning from the final answer when the model exposes reasoning through a separate channel (delta.reasoning_content). 2. bot_completion.go: SSE frames sent accumulated fullAnswer on every frame instead of per-frame deltas. The frontend accumulates answer text and appends <think>/</think> around start_to_think/end_to_think markers, so sending accumulated text caused reasoning to be duplicated in the visible answer. Fix: - EnterReasoning()/ExitReasoning() now append <think>/</think> to fullText - Marker frames (StartToThink/EndToThink) send empty Data - Regular answer frames send res.Anwardelta, not fullAnswer This matches the Python async_chat behavior which yields deltas.
dosubot
Bot
added
the
size:S
Contributor
📝 WalkthroughWalkthroughChatbot completion now streams incremental answer deltas, emits empty data for reasoning markers, and retains the accumulated final answer. Reasoning state transitions append ChangesReasoning-aware answer streaming
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wangq8
reviewed
Jul 22, 2026
wangq8
left a comment
wangq8
left a comment
Collaborator
There was a problem hiding this comment.
Nice fix, LGTM. The root cause analysis in the PR description is thorough.
What I reviewed:
think_tag.go: Adding<think>/</think>markers inEnterReasoning()/ExitReasoning()is the correct fix for the first layer — ensuresExtractVisibleAnswer()can strip reasoning that comes through a separate channel.bot_completion.go: Switching marker frames to emptyDataand answer frames tores.Answer(delta) instead of accumulatedfullAnswerfixes the duplication issue. This aligns with the Python path's delta semantics.think_tag_test.go: The new testTestReasoningChannelWrapsThinkTagscovers the exact scenario described — reasoning channel → visible answer, verifyingExtractVisibleAnswerreturns only the visible part.
No regressions spotted. The test coverage on the think-tag state machine is solid. ✅
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/service/think_tag.go (1)
56-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing think-tag constants.
thinkOpenandthinkClosealready define these exact markers at Lines 25-26, but both transitions duplicate the string literals. Using the constants keeps marker emission aligned withExtractVisibleAnswerif the tag values change.Proposed change
- s.fullText += "<think>" + s.fullText += thinkOpen ... - s.fullText += "</think>" + s.fullText += thinkCloseAlso applies to: 69-78
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/think_tag.go` around lines 56 - 67, Update ThinkStreamState.EnterReasoning and the corresponding exit transition to append the existing thinkOpen and thinkClose constants instead of duplicating "<think>" and "</think>" literals, preserving the current marker-emission behavior.internal/service/bot_completion.go (1)
431-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the final-frame exception in the documentation.
The comment says each result’s own delta is forwarded, but the
res.Finalbranch at Lines 465-470 intentionally sends accumulatedfullAnswer. State explicitly that only non-final frames carry deltas and the final frame carries the decorated accumulated answer.Proposed wording
- // We accumulate the full answer locally for persistence and for the - // final frame, but forward each result's own delta in the SSE frame - // so the front-end can build the <think>-wrapped answer correctly. + // We accumulate the full answer locally for persistence. Non-final + // frames forward each result's delta so the front-end can build the + // <think>-wrapped answer correctly; the final frame intentionally + // sends the decorated accumulated answer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/bot_completion.go` around lines 431 - 439, Update the streaming comment near the pipeline-to-chatbot frame translation to state that non-final frames forward each result’s delta, while the res.Final branch intentionally sends the decorated accumulated fullAnswer. Keep the explanation of front-end think-marker handling intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/service/think_tag_test.go`:
- Around line 304-339: Strengthen TestReasoningChannelWrapsThinkTags by
asserting the exact state.fullText after reasoning and visible-answer chunks are
processed, including both the opening <think> and closing </think> markers
around the reasoning text followed by the visible answer. Keep the existing
ExtractVisibleAnswer assertion unchanged.
---
Nitpick comments:
In `@internal/service/bot_completion.go`:
- Around line 431-439: Update the streaming comment near the pipeline-to-chatbot
frame translation to state that non-final frames forward each result’s delta,
while the res.Final branch intentionally sends the decorated accumulated
fullAnswer. Keep the explanation of front-end think-marker handling intact.
In `@internal/service/think_tag.go`:
- Around line 56-67: Update ThinkStreamState.EnterReasoning and the
corresponding exit transition to append the existing thinkOpen and thinkClose
constants instead of duplicating "<think>" and "</think>" literals, preserving
the current marker-emission behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92c84a5d-4219-4fa6-aba1-d9fc85b7f707
📒 Files selected for processing (3)
internal/service/bot_completion.gointernal/service/think_tag.gointernal/service/think_tag_test.go
Comment on lines
+304
to
+339
|
|
||
| // TestReasoningChannelWrapsThinkTags verifies that model-level reasoning | ||
| // (arriving through a separate reason channel rather than <think> tags in | ||
| // the text) is bracketed by <think></think> in fullText. This prevents the | ||
| // reasoning content from leaking into the final visible answer. | ||
| func TestReasoningChannelWrapsThinkTags(t *testing.T) { | ||
| state := &ThinkStreamState{} | ||
|
|
||
| if !state.EnterReasoning() { | ||
| t.Fatal("EnterReasoning should return true on first call") | ||
| } | ||
| // Simulate the chat pipeline feeding reasoning chunks through NextThinkDelta. | ||
| deltas := NextThinkDelta(state, "reasoning step 1", 0) | ||
| if len(deltas) != 1 || deltas[0].Value != "reasoning step 1" { | ||
| t.Fatalf("unexpected reasoning deltas: %+v", deltas) | ||
| } | ||
| deltas = NextThinkDelta(state, " reasoning step 2", 0) | ||
| if len(deltas) != 1 || deltas[0].Value != " reasoning step 2" { | ||
| t.Fatalf("unexpected reasoning deltas: %+v", deltas) | ||
| } | ||
|
|
||
| if state.ExitReasoning() != true { | ||
| t.Fatal("ExitReasoning should return true when reasoning was active") | ||
| } | ||
|
|
||
| // Visible answer arrives after reasoning ends. | ||
| deltas = BufferAnswerDelta(state, "visible answer", 0) | ||
| if len(deltas) != 1 || deltas[0].Value != "visible answer" { | ||
| t.Fatalf("unexpected answer deltas: %+v", deltas) | ||
| } | ||
|
|
||
| wantVisible := "visible answer" | ||
| if got := ExtractVisibleAnswer(state.fullText); got != wantVisible { | ||
| t.Errorf("ExtractVisibleAnswer(%q) = %q, want %q", state.fullText, got, wantVisible) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that both reasoning markers are persisted.
The current test can pass even if EnterReasoning() stops appending <think>, because ExtractVisibleAnswer only needs </think> to discard the preceding text. Assert the exact state.fullText value so the new opening-marker behavior is covered.
Proposed test assertion
wantVisible := "visible answer"
if got := ExtractVisibleAnswer(state.fullText); got != wantVisible {
t.Errorf("ExtractVisibleAnswer(%q) = %q, want %q", state.fullText, got, wantVisible)
}
+ wantFullText := "<think>reasoning step 1 reasoning step 2</think>visible answer"
+ if state.fullText != wantFullText {
+ t.Errorf("fullText = %q, want %q", state.fullText, wantFullText)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TestReasoningChannelWrapsThinkTags verifies that model-level reasoning | |
| // (arriving through a separate reason channel rather than <think> tags in | |
| // the text) is bracketed by <think></think> in fullText. This prevents the | |
| // reasoning content from leaking into the final visible answer. | |
| func TestReasoningChannelWrapsThinkTags(t *testing.T) { | |
| state := &ThinkStreamState{} | |
| if !state.EnterReasoning() { | |
| t.Fatal("EnterReasoning should return true on first call") | |
| } | |
| // Simulate the chat pipeline feeding reasoning chunks through NextThinkDelta. | |
| deltas := NextThinkDelta(state, "reasoning step 1", 0) | |
| if len(deltas) != 1 || deltas[0].Value != "reasoning step 1" { | |
| t.Fatalf("unexpected reasoning deltas: %+v", deltas) | |
| } | |
| deltas = NextThinkDelta(state, " reasoning step 2", 0) | |
| if len(deltas) != 1 || deltas[0].Value != " reasoning step 2" { | |
| t.Fatalf("unexpected reasoning deltas: %+v", deltas) | |
| } | |
| if state.ExitReasoning() != true { | |
| t.Fatal("ExitReasoning should return true when reasoning was active") | |
| } | |
| // Visible answer arrives after reasoning ends. | |
| deltas = BufferAnswerDelta(state, "visible answer", 0) | |
| if len(deltas) != 1 || deltas[0].Value != "visible answer" { | |
| t.Fatalf("unexpected answer deltas: %+v", deltas) | |
| } | |
| wantVisible := "visible answer" | |
| if got := ExtractVisibleAnswer(state.fullText); got != wantVisible { | |
| t.Errorf("ExtractVisibleAnswer(%q) = %q, want %q", state.fullText, got, wantVisible) | |
| } | |
| } | |
| // TestReasoningChannelWrapsThinkTags verifies that model-level reasoning | |
| // (arriving through a separate reason channel rather than <think> tags in | |
| // the text) is bracketed by <think></think> in fullText. This prevents the | |
| // reasoning content from leaking into the final visible answer. | |
| func TestReasoningChannelWrapsThinkTags(t *testing.T) { | |
| state := &ThinkStreamState{} | |
| if !state.EnterReasoning() { | |
| t.Fatal("EnterReasoning should return true on first call") | |
| } | |
| // Simulate the chat pipeline feeding reasoning chunks through NextThinkDelta. | |
| deltas := NextThinkDelta(state, "reasoning step 1", 0) | |
| if len(deltas) != 1 || deltas[0].Value != "reasoning step 1" { | |
| t.Fatalf("unexpected reasoning deltas: %+v", deltas) | |
| } | |
| deltas = NextThinkDelta(state, " reasoning step 2", 0) | |
| if len(deltas) != 1 || deltas[0].Value != " reasoning step 2" { | |
| t.Fatalf("unexpected reasoning deltas: %+v", deltas) | |
| } | |
| if state.ExitReasoning() != true { | |
| t.Fatal("ExitReasoning should return true when reasoning was active") | |
| } | |
| // Visible answer arrives after reasoning ends. | |
| deltas = BufferAnswerDelta(state, "visible answer", 0) | |
| if len(deltas) != 1 || deltas[0].Value != "visible answer" { | |
| t.Fatalf("unexpected answer deltas: %+v", deltas) | |
| } | |
| wantVisible := "visible answer" | |
| if got := ExtractVisibleAnswer(state.fullText); got != wantVisible { | |
| t.Errorf("ExtractVisibleAnswer(%q) = %q, want %q", state.fullText, got, wantVisible) | |
| } | |
| wantFullText := "<think>reasoning step 1 reasoning step 2</think>visible answer" | |
| if state.fullText != wantFullText { | |
| t.Errorf("fullText = %q, want %q", state.fullText, wantFullText) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/think_tag_test.go` around lines 304 - 339, Strengthen
TestReasoningChannelWrapsThinkTags by asserting the exact state.fullText after
reasoning and visible-answer chunks are processed, including both the opening
<think> and closing </think> markers around the reasoning text followed by the
visible answer. Keep the existing ExtractVisibleAnswer assertion unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a bug in Go mode where the shared chat embed (chat-share) incorrectly displayed reasoning/thinking content in the main answer body instead of isolating it in the collapsible "Thinking..." section.
Root cause (two layers):
EnterReasoning()/ExitReasoning()inthink_tag.godid not write<think>/</think>markers intofullText, soExtractVisibleAnswer()could not strip reasoning from the final answer when the model exposes reasoning through a separate channel (e.g.delta.reasoning_content).bot_completion.gosent accumulated full answer text (fullAnswer) on every SSE frame instead of per-frame deltas. The frontend accumulates the answer and appends<think>/</think>aroundstart_to_think/end_to_thinkmarkers, so sending accumulated text caused reasoning to be duplicated in the visible answer.Fix:
EnterReasoning()/ExitReasoning()now append<think>/</think>markers tofullText, ensuringExtractVisibleAnswer()correctly strips reasoning from the final visible answer.StartToThink/EndToThink) now send emptyDatainstead of the accumulatedfullAnswer.res.Answer(the per-frame delta) instead of the accumulatedfullAnswer.This aligns the Go SSE streaming path with the Python
async_chatbehavior, which yields deltas rather than accumulated full text.