fix(vscode-lm): sanitize surrogates, recover leaked tool calls, and window-safe tool_result truncation - #1188
fix(vscode-lm): sanitize surrogates, recover leaked tool calls, and window-safe tool_result truncation#1188simurg79 wants to merge 11 commits into
Conversation
…indow-safe tool_result truncation Hardens the VS Code Language Model provider (notably GitHub Copilot serving Anthropic Claude) against three failure modes: - Surrogate sanitization: a lone UTF-16 surrogate cannot be encoded as UTF-8, so the backend rejects the entire request with a 400. sanitizeSurrogates() replaces unpaired surrogates with U+FFFD while preserving valid pairs (emoji, CJK ext.), applied to string messages, tool results, and text parts. - Leaked tool-call recovery: some backends stream a tool call as raw <invoke> XML instead of a structured LanguageModelToolCallPart, leaving the turn with no tool_use block and stalling the task in a "no tools used" retry loop. extractLeakedToolCalls() and trailingPartialToolMarkerLength() detect the markup mid-stream (including markers split across chunk boundaries) and replay it as a real tool call, conservatively: only for <invoke> names matching a tool actually offered that turn, and only when tools were offered. - Window-safe tool_result truncation: Copilot's backend trims over-window requests without preserving tool_use/tool_result pairing, orphaning a tool_result and causing a 400 (unexpected tool_use_id). truncateToolResultsToFitWindow() and middleOutTruncate() shrink oversized tool_result payloads on our side (largest first, middle-out, pairing preserved) before sending. Ported from simurg79/Roo-Code#12.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe VS Code LM provider now recovers Anthropic-style tool calls from streamed text, enforces context budgets, and sanitizes invalid surrogates. A VS Code extension probes Claude models and records transcripts for false-positive analysis. ChangesVS Code LM robustness
VS Code LM behavior probe
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant VSCodeLM
participant createMessage
participant extractLeakedToolCalls
participant ToolResultTrimmer
VSCodeLM->>createMessage: provide streamed response and request context
createMessage->>ToolResultTrimmer: trim oversized tool results
ToolResultTrimmer-->>createMessage: context-fitting messages
VSCodeLM-->>createMessage: emit text and native tool-call chunks
createMessage->>extractLeakedToolCalls: parse buffered invoke markup
extractLeakedToolCalls-->>createMessage: recovered text and structured calls
createMessage-->>VSCodeLM: yield ordered events
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/transform/__tests__/vscode-lm-format.spec.ts (1)
333-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the conversion boundary.
These tests only exercise
sanitizeSurrogates. They do not prove thatconvertToVsCodeLmMessagessanitizes simple message strings, tool-result strings, tool-result text blocks, user text blocks, and assistant text blocks.Add converter unit tests that inspect the resulting VS Code text-part values for each changed path. As per coding guidelines, “Place tests in the narrowest layer that proves the behavior.”
🤖 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 `@src/api/transform/__tests__/vscode-lm-format.spec.ts` around lines 333 - 363, Add unit tests for convertToVsCodeLmMessages that verify surrogate sanitization in each affected conversion path: simple message strings, tool-result strings, tool-result text blocks, user text blocks, and assistant text blocks. Assert the resulting VS Code text-part values contain replacement characters for lone surrogates, while keeping sanitizeSurrogates tests focused on the helper’s direct behavior.Source: Coding guidelines
🤖 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 `@src/api/transform/vscode-lm-format.ts`:
- Around line 41-46: Update the systemPrompt handling in the VS Code provider
before constructing LanguageModelChatMessage.Assistant so it passes through
sanitizeSurrogates, while preserving existing behavior for valid prompts. Add a
provider regression test covering a systemPrompt containing a lone surrogate and
verify the constructed request uses the replacement character.
---
Nitpick comments:
In `@src/api/transform/__tests__/vscode-lm-format.spec.ts`:
- Around line 333-363: Add unit tests for convertToVsCodeLmMessages that verify
surrogate sanitization in each affected conversion path: simple message strings,
tool-result strings, tool-result text blocks, user text blocks, and assistant
text blocks. Assert the resulting VS Code text-part values contain replacement
characters for lone surrogates, while keeping sanitizeSurrogates tests focused
on the helper’s direct behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd5d6dfc-37c2-454f-abcf-c73712c01f83
📒 Files selected for processing (4)
src/api/providers/__tests__/vscode-lm.spec.tssrc/api/providers/vscode-lm.tssrc/api/transform/__tests__/vscode-lm-format.spec.tssrc/api/transform/vscode-lm-format.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
…ation paths Raises patch coverage on the new vscode-lm reliability code above the 80%% codecov/patch gate by exercising the streaming salvage state machine (marker split across chunks, multi-chunk buffering, unknown-tool passthrough, carried tail) and the tool_result truncation helpers (array-form content, surrogate-safe middle-out, guard clauses).
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for your contirbution
| if (!salvageBuffering && salvageCarry) { | ||
| yield { type: "text", text: salvageCarry } | ||
| } | ||
|
|
||
| if (salvageBuffering && salvageBuffer) { | ||
| const { calls, leftoverText } = extractLeakedToolCalls(salvageBuffer, providedToolNames) | ||
|
|
||
| // Emit surrounding prose first so recovered tool calls come last, matching the | ||
| // ordering of a normal native tool-calling turn. | ||
| if (leftoverText) { | ||
| yield { type: "text", text: leftoverText } |
There was a problem hiding this comment.
Can a native LanguageModelToolCallPart (yielded at :761) arrive while salvageBuffering is true? If so, this flush emits leftoverText after that native tool_use, and cleanConversationHistory serializes in order — leaving text content following a tool_use block, which Anthropic rejects. Worth flushing the buffer as text before yielding a native call, or dropping the leftover-text emission for a buffer that spans one.
There was a problem hiding this comment.
Apologies for the late reply — this is already resolved in the current branch.
flushSalvage() (src/api/providers/vscode-lm.ts, defined around line 708) is invoked before a native LanguageModelToolCallPart is yielded, so any buffered salvage text is emitted as a text chunk first rather than being dropped or reordered behind the tool call.
Covered by the interleaving test at src/api/providers/__tests__/vscode-lm.spec.ts:418.
| while ((match = LEAKED_INVOKE_BLOCK.exec(text)) !== null) { | ||
| leftover += text.slice(lastIndex, match.index) | ||
| const name = match[1] | ||
| if (validToolNames.has(name)) { | ||
| calls.push({ name, input: parseLeakedInvokeParams(match[2]) }) |
There was a problem hiding this comment.
Does this recover a call when the model merely quotes the <invoke> markup (e.g. echoing a file snippet, or a "Do NOT run <invoke>…" negative example)? The only gate here is the tool name; prose that reproduces the markup is replayed as a real call with whatever arguments accompany it. Should the block need to be self-delimited, or wrapped in <antml:function_calls>, to count as an invocation?
There was a problem hiding this comment.
You were right, and I want to correct the record before anything else.
My earlier claim that the real-world leak takes the form of a bare, unwrapped <invoke> was unsubstantiated. I could not produce a linked issue or a vscode-lm transcript for it, and the third-party reports I was leaning on (anthropics/claude-code#66153, #73808) are Anthropic-API, not vscode-lm. I withdraw that claim.
To answer your question with data instead, I ran a scratch extension against a real extension host: 210 live vscode.lm requests, 7 Copilot Claude models x 6 scenarios x 5 repeats, 0 errors. The probe, the replay harness, and representative transcripts are committed under .roo/skills/probe-vscode-lm-api/.
Your concern was legitimate. Models really do reproduce <invoke name="..."> in prose: 44 of 70 runs that asked for a quoted example (in prose or in a fenced block) contained the markup. A name-only gate would have been replaying those.
On the specific fix: the quoted/fenced guard caught 100% of them. Replaying extractLeakedToolCalls() over all 58 transcripts containing <invoke (with validToolNames = {read_file}) recovered 9 — all genuine invocations, arguments parsed correctly — and passed 49 through as text, including all 44 quoted cases. 0 false positives.
On the <function_calls> wrapper specifically, the sample inverts what I would have guessed. All 14 genuine emitted invocations were wrapped; 0 were bare. All 44 quoted cases were bare; 0 were wrapped. So in this data, bare correlates with quoting and wrapped with genuine invocation — requiring the wrapper would not have been the discriminator it appears to be, and the fence/quote guard is what actually does the work. I am happy to add the wrapper requirement on top if you still want the belt-and-braces, but I did not want to present it as the thing that fixes this.
One more limit worth stating plainly: the leak did not reproduce at all in the 105 tool-declared runs — every one emitted a proper LanguageModelToolCallPart and leaked nothing. That bounds the rate at something low, but 105 runs does not prove absence, and it does not tell us the shape of the failure when it does occur. The recovery path's target remains inferred rather than observed on this transport.
There was a problem hiding this comment.
I am not very familiar with the LLM behavior but I asked AI to run experiments. I think the result indicated that you are right and I am not sure if the code is updated as you suggested though but tests indicated the code is holding.
Sorry, I wish I know all these details.
| if (before) { | ||
| yield { type: "text", text: before } | ||
| } | ||
| salvageBuffering = true |
There was a problem hiding this comment.
Once this flips true it stays true until stream end — a <invoke/<function_calls match in ordinary prose latches buffering for the entire remaining response, so real-time streaming stops and the tail is emitted as one chunk. Should this reset if the buffer never progresses toward a complete block (or latch only after name="…" is seen)?
There was a problem hiding this comment.
Apologies for the late reply — this is already resolved in the current branch.
LEAKED_TOOL_CALL_START (src/api/providers/vscode-lm.ts, around line 81) no longer latches on a bare <invoke; the alternation requires either a <function_calls> wrapper or <invoke name=". Prose that merely mentions <invoke therefore does not stop real-time streaming for the rest of the response.
Covered by the tests at src/api/providers/__tests__/vscode-lm.spec.ts:430 and :377.
| describe("leaked tool-call recovery during streaming", () => { | ||
| const salvageTools = [ | ||
| { | ||
| type: "function" as const, | ||
| function: { | ||
| name: "calculator", | ||
| description: "A simple calculator", | ||
| parameters: { type: "object", properties: { operation: { type: "string" } } }, | ||
| }, | ||
| }, | ||
| ] | ||
|
|
||
| const streamTextParts = (parts: string[]) => { | ||
| mockLanguageModelChat.sendRequest.mockResolvedValueOnce({ | ||
| stream: (async function* () { | ||
| for (const part of parts) { | ||
| yield new vscode.LanguageModelTextPart(part) | ||
| } | ||
| return | ||
| })(), | ||
| text: (async function* () { | ||
| yield parts.join("") | ||
| return | ||
| })(), | ||
| }) | ||
| } | ||
|
|
||
| const collect = async (parts: string[]) => { | ||
| streamTextParts(parts) | ||
| const stream = handler.createMessage("system", [{ role: "user" as const, content: "hi" }], { | ||
| taskId: "test-task", | ||
| tools: salvageTools, | ||
| }) | ||
| const chunks = [] | ||
| for await (const chunk of stream) { | ||
| chunks.push(chunk) | ||
| } | ||
| return chunks | ||
| } | ||
|
|
||
| it("recovers a tool call the model streamed as raw invoke XML", async () => { |
There was a problem hiding this comment.
These recovery tests filter chunks by type and assert each independently, so the emission order (prose before the recovered tool_call) is never asserted — a swap would still pass. Also, no case mixes a native LanguageModelToolCallPart with leaked <invoke> text, which is the one interleaving that can yield an invalid tool_use-then-text message. Worth asserting the full chunk sequence and adding a native+leaked fixture?
There was a problem hiding this comment.
Apologies for the late reply — this is already covered in the current branch.
Chunk ordering is asserted explicitly at src/api/providers/__tests__/vscode-lm.spec.ts:409, which expects the exact sequence ["text", "tool_call", "usage"], so prose emitted before a recovered call cannot be reordered behind it or dropped. The interleaving case at :418 covers the mixed native-tool-call path.
github-actions
Bot
added
awaiting-author
Address review feedback on the leaked-tool-call salvage path: a tool name alone was not a sufficient gate, so prose or fenced examples reproducing the invoke markup could be replayed as real calls. Adds the quoted/fenced guard plus coverage. Also records the empirical vscode.lm probe as a project skill (probe-vscode-lm-api) with the scratch probe extension, the false-positive replay harness, representative transcripts, and the consent-gate gotcha.
Skill directories hold reference scripts and captured artifacts that are intentionally never imported by the build.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 @.roo/skills/probe-vscode-lm-api/scripts/extension.js:
- Around line 54-72: Update runOnce() to declare the CancellationTokenSource
outside the try block, then dispose that source in a finally block after request
processing or error handling completes. Preserve the existing streaming logic
and record.error assignment while ensuring every created source is released.
In @.roo/skills/probe-vscode-lm-api/SKILL.md:
- Around line 10-23: Update the Markdown links in the probe skill documentation,
including the links around extractLeakedToolCalls() and the vscode-lm tests, to
use ../../../src/... for repository source paths. Keep links to the sibling
scripts and transcripts directories rooted at scripts/ and transcripts/
respectively, and apply the same correction to the additional referenced
section.
In
@.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.8__E_quoted_markup_in_prose_false_positive_check__run1.txt:
- Around line 3-7: Extend the quoted-markup regression coverage by adding one
deterministic unfenced prose fixture with no backticks, where a known <invoke>
tool call is quoted as text. In
.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.8__E_quoted_markup_in_prose_false_positive_check__run1.txt:3-7
and
.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.8__E_quoted_markup_in_prose_false_positive_check__run1.json:61-67,
update the corresponding transcript input and expected result so
extractLeakedToolCalls() returns no recovered call and preserves the quoted
markup in leftoverText; apply the same fixture and expectation to
.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-5__E_quoted_markup_in_prose_false_positive_check__run1.txt:12-16
and
.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-5__E_quoted_markup_in_prose_false_positive_check__run1.json:49-55.
In `@src/api/providers/vscode-lm.ts`:
- Around line 147-149: Restrict global <function_calls> wrapper removal to
regions where calls were actually recovered and appended by the invoke parsing
flow. Preserve wrapper tags around unknown tools and quoted/fenced-code <invoke>
blocks that remain text, while retaining cleanup for recovered calls. Add
coverage for wrapped unknown-tool and wrapped fenced-code cases.
- Around line 93-101: Update trailingPartialToolMarkerLength so the partialTag
match is only carried when its length is at most MAX_PARTIAL_INVOKE_CARRY,
otherwise return 0. Add a regression test covering an overlong malformed generic
tag suffix and verify it is not retained across chunks.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 360d2a40-584a-4b2f-b537-9b4b534f5652
📒 Files selected for processing (23)
.roo/skills/probe-vscode-lm-api/SKILL.md.roo/skills/probe-vscode-lm-api/scripts/extension.js.roo/skills/probe-vscode-lm-api/scripts/package.json.roo/skills/probe-vscode-lm-api/scripts/probe-false-positives.spec.ts.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.6__A_tools_declared_compelling_prompt__run1.json.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.6__A_tools_declared_compelling_prompt__run1.txt.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.6__D_no_tools_asked_to_emit_markup__run2.json.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.6__D_no_tools_asked_to_emit_markup__run2.txt.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.8__E_quoted_markup_in_prose_false_positive_check__run1.json.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.8__E_quoted_markup_in_prose_false_positive_check__run1.txt.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.8__F_quoted_markup_in_fenced_code_block__run1.json.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-4.8__F_quoted_markup_in_fenced_code_block__run1.txt.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-5__E_quoted_markup_in_prose_false_positive_check__run1.json.roo/skills/probe-vscode-lm-api/transcripts/claude-opus-5__E_quoted_markup_in_prose_false_positive_check__run1.txt.roo/skills/probe-vscode-lm-api/transcripts/claude-sonnet-4.6__D_no_tools_asked_to_emit_markup__run1.json.roo/skills/probe-vscode-lm-api/transcripts/claude-sonnet-4.6__D_no_tools_asked_to_emit_markup__run1.txt.roo/skills/probe-vscode-lm-api/transcripts/claude-sonnet-4.6__F_quoted_markup_in_fenced_code_block__run1.json.roo/skills/probe-vscode-lm-api/transcripts/claude-sonnet-4.6__F_quoted_markup_in_fenced_code_block__run1.txt.roo/skills/probe-vscode-lm-api/transcripts/false-positive-report.txt.roo/skills/probe-vscode-lm-api/transcripts/summary.jsonsrc/api/providers/__tests__/vscode-lm.spec.tssrc/api/providers/vscode-lm.tssrc/api/transform/__tests__/vscode-lm-format.spec.ts
- dispose the probe CancellationTokenSource in a finally block
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@edelauna All 8 outstanding review items are addressed in 220ee89 and each thread has a threaded reply. I don't have permission to add a reviewer via the API ( |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/api/providers/vscode-lm.ts (1)
167-192: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve a wrapper that also contains an unrecovered block.
If one
<function_calls>wrapper contains an unknown<invoke>before a recovered known<invoke>, Line 168 marks the whole preceding segment asnearRecovery. Line 192 then removes the opening wrapper from the unknown block. Preserve wrapper tags unless all enclosed invoke blocks were recovered.Add a mixed known-tool and unknown-tool wrapper test.
🤖 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 `@src/api/providers/vscode-lm.ts` around lines 167 - 192, Update the recovery segmentation and wrapper cleanup around parseLeakedInvokeParams so a function_calls wrapper is stripped only when every enclosed invoke is recovered; preserve the wrapper verbatim when it contains any unrecovered or unknown invoke, including an unknown invoke before a recovered one. Add a test covering a mixed known-tool and unknown-tool wrapper.
🤖 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 `@src/api/providers/vscode-lm.ts`:
- Around line 105-123: Update isQuotedAsCode to reject invoke markers preceded
by non-tag prose, while recognizing variable-length backtick fences and tilde
fences instead of relying on fixed triple-backtick parity; preserve quoted
behavior for fenced, inline, and narrative text. In the candidate buffering flow
around the invocation parser at lines 824-832, flush the candidate as literal
text when it can no longer form a valid offered invocation or exceeds a bounded
recovery size. Apply these changes at src/api/providers/vscode-lm.ts:105-123 and
src/api/providers/vscode-lm.ts:824-832.
---
Duplicate comments:
In `@src/api/providers/vscode-lm.ts`:
- Around line 167-192: Update the recovery segmentation and wrapper cleanup
around parseLeakedInvokeParams so a function_calls wrapper is stripped only when
every enclosed invoke is recovered; preserve the wrapper verbatim when it
contains any unrecovered or unknown invoke, including an unknown invoke before a
recovered one. Add a test covering a mixed known-tool and unknown-tool wrapper.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 173d95d5-4bd7-401e-8bcc-3273c3c643ce
📒 Files selected for processing (4)
.roo/skills/probe-vscode-lm-api/SKILL.md.roo/skills/probe-vscode-lm-api/scripts/extension.jssrc/api/providers/__tests__/vscode-lm.spec.tssrc/api/providers/vscode-lm.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/api/providers/tests/vscode-lm.spec.ts
- .roo/skills/probe-vscode-lm-api/scripts/extension.js
github-actions
Bot
removed
the
awaiting-author
Remove the ~120KB raw probe transcript corpus from the vscode-lm probe skill; keep the measured findings and their stated limits in SKILL.md.
github-actions
Bot
added
the
awaiting-author
github-actions
Bot
added
awaiting-author
…buffer Loop tag stripping until stable so `<<script>>` cannot reconstruct a tag after a single pass (CodeQL incomplete multi-character sanitization). Track fence marker and width instead of counting ``` runs for parity, so tilde fences and 4+ backtick fences are recognized. Treat a quoted invoke that ends its line as quoted when an explicit quoting cue precedes it, rather than recovering it as a live tool call. Keying off leading prose alone was tried previously and regressed genuine recoveries, so the cue is deliberately narrow. Bound the salvage buffer so markup that never closes is flushed as plain text instead of withholding the response until the stream ends.
The first version of this test only checked the flushed text's content, which the end-of-stream drain produces even without the cap, so it passed against the unfixed code. Assert instead that text reaches the consumer before the stream is exhausted, which is what the bound actually changes.
github-actions
Bot
added
awaiting-review
Replace the vacuous four-backtick test with a nested inner-fence case and add a closed-fence recovery test, both of which fail under the old backtick-parity counting.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/api/providers/__tests__/vscode-lm.spec.ts (1)
323-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLet
collectreusedrain.
collectrepeats the body ofdrainexactly. One helper keeps the streaming setup in a single place.♻️ Proposed refactor
const collect = async (parts: string[]) => { streamTextParts(parts) - const stream = handler.createMessage("system", [{ role: "user" as const, content: "hi" }], { - taskId: "test-task", - tools: salvageTools, - }) - const chunks = [] - for await (const chunk of stream) { - chunks.push(chunk) - } - return chunks + return drain() }🤖 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 `@src/api/providers/__tests__/vscode-lm.spec.ts` around lines 323 - 346, Refactor the collect helper to call and return drain after invoking streamTextParts(parts), removing the duplicated createMessage and stream-consumption logic while preserving both helpers’ existing behavior.src/api/providers/vscode-lm.ts (2)
126-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the tag-stripping doc comment to
stripTagsCompletely.The block comment on Lines 126-129 describes repeated tag stripping. It sits above
hasCompleteInvokeBlock, which has its own doc comment on Line 130.stripTagsCompletelyon Line 138 has no doc comment.♻️ Proposed fix to reattach the doc comment
-/** - * Strips well-formed tags repeatedly until the result stops changing. A single pass is unsafe: - * `<<invoke>>` reassembles into a live-looking tag after one replacement. - */ /** True when `text` already contains a closed `<invoke>` block, so buffering is still productive. */ function hasCompleteInvokeBlock(text: string): boolean { LEAKED_INVOKE_BLOCK.lastIndex = 0 const found = LEAKED_INVOKE_BLOCK.test(text) LEAKED_INVOKE_BLOCK.lastIndex = 0 return found } +/** + * Strips well-formed tags repeatedly until the result stops changing. A single pass is unsafe: + * `<<invoke>>` reassembles into a live-looking tag after one replacement. + */ function stripTagsCompletely(text: string): string {🤖 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 `@src/api/providers/vscode-lm.ts` around lines 126 - 136, Move the repeated tag-stripping block comment from above hasCompleteInvokeBlock to directly above stripTagsCompletely, leaving hasCompleteInvokeBlock’s own documentation attached to that function.
332-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep
middleOutTruncatewithinmaxChars.When
maxCharsis smaller than the marker length, the function returns a marker longer thanmaxChars. Add the proposed guard to preserve the documented contract. The current production caller always uses a minimum target of 2000 characters.🤖 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 `@src/api/providers/vscode-lm.ts` around lines 332 - 347, Update middleOutTruncate to handle maxChars values smaller than the generated marker length, returning a result that never exceeds maxChars. Add the guard near reservedMarkerLength/keep calculation, while preserving the existing truncation behavior for the production caller’s minimum target of 2000 characters.
🤖 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 `@scripts/probe-vscode-lm-api/extension.js`:
- Around line 195-199: Update the lmprobe.run command registration and run flow
to prevent overlapping probe executions: track whether run() is active, reject a
second invocation or queue it until the first completes, and clear the active
state on both success and failure. Preserve the existing fatal.json error
handling while ensuring shared outputs such as summary.json and transcript files
are never written concurrently.
In `@src/api/providers/vscode-lm.ts`:
- Around line 765-776: Clamp the `messagesBudgetChars` value in the
context-window trimming block to a positive minimum before passing it to
`truncateToolResultsToFitWindow`. Use the existing `MIN_TOOL_RESULT_CHARS`
constant as the floor so non-positive derived budgets still trigger tool-result
trimming while preserving the current calculation for larger budgets.
---
Nitpick comments:
In `@src/api/providers/__tests__/vscode-lm.spec.ts`:
- Around line 323-346: Refactor the collect helper to call and return drain
after invoking streamTextParts(parts), removing the duplicated createMessage and
stream-consumption logic while preserving both helpers’ existing behavior.
In `@src/api/providers/vscode-lm.ts`:
- Around line 126-136: Move the repeated tag-stripping block comment from above
hasCompleteInvokeBlock to directly above stripTagsCompletely, leaving
hasCompleteInvokeBlock’s own documentation attached to that function.
- Around line 332-347: Update middleOutTruncate to handle maxChars values
smaller than the generated marker length, returning a result that never exceeds
maxChars. Add the guard near reservedMarkerLength/keep calculation, while
preserving the existing truncation behavior for the production caller’s minimum
target of 2000 characters.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b1dbef64-ed16-40b9-8198-e47cb90b5c08
📒 Files selected for processing (8)
.roo/skills/probe-vscode-lm-api/SKILL.mdscripts/probe-vscode-lm-api/extension.jsscripts/probe-vscode-lm-api/package.jsonscripts/probe-vscode-lm-api/probe-false-positives.spec.tssrc/api/providers/__tests__/vscode-lm.spec.tssrc/api/providers/vscode-lm.tssrc/api/transform/__tests__/vscode-lm-format.spec.tssrc/api/transform/vscode-lm-format.ts
Address CodeRabbit review: a system prompt or tool schema large enough to consume the derived char budget left messagesBudgetChars non-positive, which made truncateToolResultsToFitWindow a no-op exactly when the request was most oversized. Clamp to MIN_TOOL_RESULT_CHARS and cover it with a regression test. Also reattach a misplaced doc comment and dedupe a test helper.
|
Responses to the three nitpicks from CodeRabbit review 4920827327 (they were in the review body rather than inline threads, so replying here). All addressed in fbe5079 unless noted. 1. 2. 3. Per the repo's "Avoid unnecessary complexity" rule, a defensive branch for an input no caller can produce is speculative generality. The honest characterization is that |
3 participants
Port of simurg79/Roo-Code#12 into this repo. Credit to the original PR author.
What this changes
Hardens the VS Code Language Model provider (notably GitHub Copilot serving Anthropic Claude) against three failure modes.
1. Surrogate sanitization
A lone UTF-16 surrogate cannot be encoded as UTF-8, so the backend rejects the entire request with a 400.
sanitizeSurrogates()replaces unpaired surrogates with U+FFFD while preserving valid pairs (emoji, CJK ext.). Applied to string messages, tool results, and text parts.2. Leaked tool-call recovery
Some backends stream a tool call as raw
<invoke>XML instead of a structuredLanguageModelToolCallPart, leaving the turn with notool_useblock and stalling the task in a "no tools used" retry loop.extractLeakedToolCalls()andtrailingPartialToolMarkerLength()detect the markup mid-stream (including markers split across chunk boundaries) and replay it as a real tool call. This is deliberately conservative: only for<invoke>names matching a tool actually offered that turn, and only when tools were offered at all.3. Window-safe
tool_resulttruncationCopilot's backend trims over-window requests without preserving
tool_use/tool_resultpairing, orphaning atool_resultand causing a 400 (unexpected tool_use_id).truncateToolResultsToFitWindow()andmiddleOutTruncate()shrink oversizedtool_resultpayloads on our side (largest first, middle-out, pairing preserved) before sending.Adaptations made during the port
vscode-lm-format.tshad diverged from upstream, so insertion points were re-derived against the local structure.console.warndiagnostics (Task.ts,multi-search-replace.ts,ApplyDiffTool.ts) and its 3.53.1 -> 3.53.2 version bump were deliberately excluded.Verification
mainbaseline. All 22 new tests pass and no previously-passing test regressed. The 72 failures are pre-existing and identical to baseline (brokenvscodemocks in those specs, out of scope).--prune-suppressionsclean on all changed source files.src/eslint-suppressions.jsonverified content-identical and left unmodified.tsc --noEmitshows no new type errors (only the 3 pre-existing ones already present onmain).Files changed
Source and tests:
src/api/transform/vscode-lm-format.tssrc/api/providers/vscode-lm.tssrc/api/transform/__tests__/vscode-lm-format.spec.tssrc/api/providers/__tests__/vscode-lm.spec.tsProbe documentation and harness (added):
.roo/skills/probe-vscode-lm-api/SKILL.mdscripts/probe-vscode-lm-api/extension.jsscripts/probe-vscode-lm-api/package.jsonscripts/probe-vscode-lm-api/probe-false-positives.spec.tsEight files total. No changeset file is included, and no build/tooling configuration is modified.
Empirical validation of the leaked-tool-call path
Review feedback asked whether the recovery path could fire on markup the model merely quotes. To answer that with evidence rather than inference, a scratch extension was loaded into a real extension host and made 210 live
vscode.lmrequests (7 Copilot Claude models x 6 scenarios x 5 repeats, 0 errors).The raw transcripts were not retained in the repo. The probe harness and the replay spec live under
scripts/probe-vscode-lm-api/, and re-running the probe regenerates the evidence;.roo/skills/probe-vscode-lm-api/SKILL.mddocuments how to run it and records the measured findings below.<invokein textWhat was measured
LanguageModelToolCallPartand leaked nothing into text parts. This bounds the leak rate at a low value; it does not prove absence. 105 runs cannot exclude a rare or prompt-specific trigger.<function_calls>; 0 were bare. All 44 quoted cases (E+F) were bare; 0 were wrapped. In this sample, bare correlates with quoting and wrapped with genuine invocation — so requiring the<function_calls>wrapper would not have been the discriminator it appears to be.extractLeakedToolCalls()over all 58 responses containing<invokewithvalidToolNames = {read_file}: 9 recovered (all genuine wrapped invocations, arguments parsed correctly), 49 passed through as text, including all 44 bare quoted cases. The quoted/fenced guard is what does the work here, not the wrapper requirement.antml:prefix appeared in any of the 210 runs.Reviewer attention:
isQuotedAsCode()is a judgment call, not a measurementisQuotedAsCode()now suppresses recovery when narrative text follows the block on the same line. This behavioral change is a judgment call, not a measured result. The probe sample did not cover unfenced, backtick-free quoting, so there is no data in this PR that validates or refutes the heuristic.The residual ambiguity is unavoidable and worth stating plainly: a quoted block sitting alone on its own line remains indistinguishable from a genuine leak, and will still be treated as a recoverable tool call. This is the item most warranting reviewer scrutiny.
Limits of this evidence
The real-world shape of the leak that motivated this recovery code is inferred from third-party Anthropic-API reports (anthropics/claude-code#66153, #73808), not captured from
vscode-lm. Novscode-lmtranscript of the failure exists. An earlier claim in this PR's discussion that the real-world leak is a bare unwrapped<invoke>was unsubstantiated and is withdrawn. Copilot'svscode.lmendpoint also sits behind its own prompt assembly, so these results describe that surface rather than the raw Anthropic API.Summary by CodeRabbit
New Features
Bug Fixes