Fix Apply failure with <think> tags (#283) #287
Conversation
This file contains 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
- Add utility to strip <think> tags from responses - Strip tags before computing diffs in Apply operation - Handle nested tags properly - Keep original response intact for UI display Fixes voideditor#283 Co-Authored-By: Jack Hacksman <[email protected]>
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 7849b0a in 2 minutes and 43 seconds
More details
- Looked at
69lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. src/vs/workbench/contrib/chat/browser/actions/codeBlockOperations.ts:389
- Draft comment:
Ensure that stripping tags here strictly affects the diff computation and does not alter the UI display. The transformation looks correct, but verify that any legitimate appearing after the initial block is preserved if intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the author to ensure that a specific change does not affect the UI display, which is against the rules. It also asks to verify the preservation of certain tags, which is similar to asking for confirmation of intention. This comment should be removed.
2. src/vs/workbench/contrib/chat/common/chatModel.ts:358
- Draft comment:
The stripThinkTags function removes all blocks in the text. According to requirements, only the initial block (if present at the very beginning) should be stripped. Consider adding a check to only remove the first block and leave subsequent tags intact if they are part of the intended answer. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment makes a claim about requirements, but I don't see any evidence in the code or comments that suggests this requirement exists. The function's documentation clearly states it removes ALL think tags. The stack-based approach seems intentional for handling nested tags. Without seeing clear evidence of the "initial block only" requirement, I should be skeptical of this comment.
I could be wrong if there are requirements documented elsewhere that specify only the first block should be removed. The commenter may have access to information I don't have.
However, the function's own documentation and implementation clearly indicate the intended behavior is to remove all tags. If there was such a requirement, it would likely be mentioned in the function's documentation.
The comment should be deleted because it makes claims about requirements without evidence, and contradicts the clear intent shown in the function's documentation and implementation.
3. src/vs/workbench/contrib/chat/browser/actions/codeBlockOperations.ts:389
- Draft comment:
Good change: stripping tags before diff computation resolves the diff desync without altering the displayed response. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. src/vs/workbench/contrib/chat/common/chatModel.ts:369
- Draft comment:
Consider adding a safeguard in stripThinkTags for unmatched closing tags (when depth goes negative) to ensure extra '' tags don't suppress subsequent text. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_70uHRjke5fjYhxZB
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Nice work. Can deepseek really do nested thinking? How does their chat interface deal with this? Would be ideal to add support for streaming, so if it types Also preferably we would only apply this to models that actually produce tags when thinking, will revisit this later. |
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-Authored-By: Jack Hacksman <[email protected]>
Co-Authored-By: Jack Hacksman <[email protected]>
Co-Authored-By: Jack Hacksman <[email protected]>
Co-Authored-By: Jack Hacksman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #283
Important
Add
stripThinkTags()to remove<think>tags from chat responses, ensuring diffs are computed correctly while keeping original responses intact for UI.stripThinkTags()inchatModel.tsto remove<think>tags from text, handling nested tags.stripThinkTags()ingetChatConversation()incodeBlockOperations.tsto strip tags before computing diffs.stripThinkTags()added to handle nested<think>tags.getChatConversation()updated to usestripThinkTags()for response processing.This description was created by
for 7849b0a. It will automatically update as commits are pushed.