-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: gpt-oss thinking block #6071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to cae9053 in 1 minute and 41 seconds. Click for details.
- Reviewed
94lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/ThinkingBlock.tsx:44
- Draft comment:
Consider consolidating and documenting the multiple regex replacements in extractThinkingContent for clarity and maintainability, as they handle similar marker patterns. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/containers/ThinkingBlock.tsx:48
- Draft comment:
Review the need for both replacements for '<|start|>assistant<|channel|>final<|message|>' and 'assistant<|channel|>final<|message|>' as they might be redundant. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Without knowing the exact format of the input text and all possible variations, it's hard to be certain if both replacements are truly redundant. The author might have included both patterns to handle different message formats. The comment is speculative and doesn't provide clear evidence that this is actually a problem. I might be missing some edge cases where both replacements are actually necessary. The author might have good reasons for handling both patterns separately. Even if there are edge cases, the comment is phrased as a suggestion to "review" rather than providing concrete evidence of an issue or a clear action item. The comment should be deleted as it's speculative and doesn't provide strong evidence that there's actually a problem to fix.
3. web-app/src/containers/ThreadContent.tsx:172
- Draft comment:
The logic for detecting and segmenting thinking markers is duplicated here. Consider refactoring shared extraction logic to maintain consistency with ThinkingBlock. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the code does handle thinking marker extraction, I cannot verify if this logic is actually duplicated in ThinkingBlock since I don't have access to that file. The comment makes an assumption about duplication without providing concrete evidence. Making architectural suggestions without full context could lead to incorrect recommendations. I could be wrong about dismissing this - maybe there is obvious duplication that an experienced developer would recognize. The thinking marker formats look like they could be standardized. Without seeing the ThinkingBlock implementation, I cannot verify the duplication claim. Making assumptions about code in other files violates the cross-file issues rule. The comment should be removed since it makes claims about code duplication without sufficient evidence and requires examining other files to verify.
4. web-app/src/containers/ThreadContent.tsx:337
- Draft comment:
Using .replace('', '') on textSegment may not fully clean all leftover markers. Consider applying a comprehensive cleanup similar to extractThinkingContent. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ZgUDcpKkit6dv9Jn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 33.15%Your code coverage diff: -0.04% ▾ Uncovered files and lines
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (refactor later)
|
Using 0.6.9-1535 (nightly). Thinking and final response from gpt-oss-120b still crammed inside the "Thought" block: |
Describe Your Changes
This pull request updates the handling and rendering of "thinking" or reasoning content in the app to support both legacy
<think>...</think>tags and the newer channel/message marker format. The changes ensure that both formats are detected, extracted, and displayed consistently, improving compatibility and maintainability.Improvements to "thinking" content detection and extraction:
ThinkingBlock.tsxto detect both<think>...</think>and<|channel|>analysis<|message|>...<|start|>assistant<|channel|>final<|message|>formats, and updated the loading state to reflect either in-progress format.extractThinkingContentfunction inThinkingBlock.tsxto consistently remove all "thinking" and channel/message markers from the text, ensuring only the relevant content is rendered.Consistent segmentation of message content:
ThreadContent.tsxto segment messages into reasoning and main text portions for both the<think>...</think>and channel/message marker formats, supporting both in-progress and completed states. This ensures that reasoning content is displayed separately and consistently, regardless of the format used.Fixes Issues
Self Checklist
Important
Enhances 'thinking' content handling in
ThinkingBlock.tsxandThreadContent.tsxto support both legacy and new formats, ensuring consistent detection, extraction, and rendering.ThinkingBlock.tsx: Detects both<think>...</think>and<|channel|>analysis<|message|>...<|start|>assistant<|channel|>final<|message|>formats for 'thinking' content.extractThinkingContentfunction added to remove 'thinking' and channel/message markers, ensuring only relevant content is rendered.ThreadContent.tsx: Segments messages into reasoning and main text for both formats, supporting in-progress and completed states.This description was created by
for cae9053. You can customize this summary. It will automatically update as commits are pushed.