fix(vscode-lm): sanitize surrogates, recover leaked tool calls, and window-safe tool_result truncation - #1188
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe VS Code LM provider now recovers text-emitted tool calls, trims oversized tool results before requests, and sanitizes invalid surrogate characters during message conversion. Tests cover parsing, truncation, context limits, and surrogate handling. ChangesVS Code LM robustness
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant createMessage
participant VSCodeLM
participant extractLeakedToolCalls
Client->>createMessage: message request
createMessage->>createMessage: truncate oversized tool results
createMessage->>VSCodeLM: converted messages
VSCodeLM-->>createMessage: streamed text chunks
createMessage->>extractLeakedToolCalls: buffered invoke markup
extractLeakedToolCalls-->>createMessage: prose and validated tool calls
createMessage-->>Client: text and tool_call events
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/api/providers/__tests__/vscode-lm.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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
| export function sanitizeSurrogates(text: string): string { | ||
| if (!text) { | ||
| return text | ||
| } | ||
| return text.replace(/[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(?<![\uD800-\uDBFF])[\uDC00-\uDFFF]/g, "\uFFFD") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Sanitize systemPrompt at the VS Code API boundary.
convertToVsCodeLmMessages only processes messages. src/api/providers/vscode-lm.ts creates LanguageModelChatMessage.Assistant(systemPrompt) directly. If systemPrompt contains a lone surrogate, the VS Code LM backend can still reject the request.
Sanitize systemPrompt before constructing that assistant message. Add a provider regression test for this path.
Proposed fix
- vscode.LanguageModelChatMessage.Assistant(systemPrompt),
+ vscode.LanguageModelChatMessage.Assistant(sanitizeSurrogates(systemPrompt)),🤖 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/vscode-lm-format.ts` around lines 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.
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.
| 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?
| 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)?
| 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?
2 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 four changed 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
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.tsNo changeset file is included.
Summary by CodeRabbit