Skip to content

fix(vscode-lm): sanitize surrogates, recover leaked tool calls, and window-safe tool_result truncation - #1188

Open
simurg79 wants to merge 9 commits into
Zoo-Code-Org:mainfrom
simurg79:port/vscode-lm-reliability
Open

fix(vscode-lm): sanitize surrogates, recover leaked tool calls, and window-safe tool_result truncation#1188
simurg79 wants to merge 9 commits into
Zoo-Code-Org:mainfrom
simurg79:port/vscode-lm-reliability

Conversation

@simurg79

@simurg79 simurg79 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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 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. 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_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.

Adaptations made during the port

  • vscode-lm-format.ts had diverged from upstream, so insertion points were re-derived against the local structure.
  • Log strings rebranded to "Zoo Code".
  • The upstream PR's TEMP console.warn diagnostics (Task.ts, multi-search-replace.ts, ApplyDiffTool.ts) and its 3.53.1 -> 3.53.2 version bump were deliberately excluded.

Verification

  • Vitest on the two specs: 25 passing vs. 3 on the main baseline. All 22 new tests pass and no previously-passing test regressed. The 72 failures are pre-existing and identical to baseline (broken vscode mocks in those specs, out of scope).
  • ESLint with --prune-suppressions clean on all changed source files. src/eslint-suppressions.json verified content-identical and left unmodified.
  • tsc --noEmit shows no new type errors (only the 3 pre-existing ones already present on main).

Files changed

Source and tests:

  • src/api/transform/vscode-lm-format.ts
  • src/api/providers/vscode-lm.ts
  • src/api/transform/__tests__/vscode-lm-format.spec.ts
  • src/api/providers/__tests__/vscode-lm.spec.ts

Probe documentation and harness (added):

  • .roo/skills/probe-vscode-lm-api/SKILL.md
  • scripts/probe-vscode-lm-api/extension.js
  • scripts/probe-vscode-lm-api/package.json
  • scripts/probe-vscode-lm-api/probe-false-positives.spec.ts

Eight 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.lm requests (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.md documents how to run it and records the measured findings below.

Scenario Setup Runs <invoke in text
A tools declared + agent system prompt 35 0
B tools declared, no system prompt 35 0
C tools declared + ~300KB filler context 35 0
D no tools, model asked to emit the markup 35 14
E asked to quote the markup in prose 35 23
F asked to quote the markup in a code fence 35 21

What was measured

  • The leak did not reproduce. 105/105 tool-declared runs (A+B+C) emitted a proper LanguageModelToolCallPart and 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.
  • Wrapped vs. bare inverts the intuition. All 14 genuine emitted invocations (D) were wrapped in <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.
  • Zero false positives. Replaying extractLeakedToolCalls() over all 58 responses containing <invoke with validToolNames = {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.
  • No antml: prefix appeared in any of the 210 runs.

Reviewer attention: isQuotedAsCode() is a judgment call, not a measurement

isQuotedAsCode() 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. No vscode-lm transcript 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's vscode.lm endpoint also sits behind its own prompt assembly, so these results describe that surface rather than the raw Anthropic API.

…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.
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The VS Code LM provider recovers text-emitted tool calls, truncates oversized tool results, and sanitizes invalid surrogate characters. A probe extension and transcript fixtures record observed model behavior.

Changes

VS Code LM robustness

Layer / File(s) Summary
Surrogate sanitization
src/api/transform/vscode-lm-format.ts, src/api/transform/__tests__/vscode-lm-format.spec.ts
Adds surrogate sanitization for message and tool-result text. Tests cover valid and invalid UTF-16 sequences.
Leaked tool-call recovery
src/api/providers/vscode-lm.ts, src/api/providers/__tests__/vscode-lm.spec.ts
Buffers partial invoke markup, validates offered tools, excludes quoted markup, preserves ordinary text, and emits structured tool-call events.
Tool-result context trimming
src/api/providers/vscode-lm.ts, src/api/providers/__tests__/vscode-lm.spec.ts
Calculates an input budget and applies middle-out truncation to oversized tool results while preserving structure and non-text content.

VS Code LM behavior probe

Layer / File(s) Summary
Probe extension workflow
.roo/skills/probe-vscode-lm-api/scripts/*, .roo/skills/probe-vscode-lm-api/SKILL.md
Discovers Claude models, runs probe scenarios, captures streamed responses, writes transcripts, and documents execution.
Probe fixtures and false-positive analysis
.roo/skills/probe-vscode-lm-api/scripts/probe-false-positives.spec.ts, .roo/skills/probe-vscode-lm-api/transcripts/*, knip.json
Adds transcript fixtures and a harness that classifies recovered calls and passthrough results. Knip ignores the probe directory.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

Suggested reviewers: hannesrudolph

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant createMessage
  participant VSCodeLM
  participant extractLeakedToolCalls
  Client->>createMessage: message request
  createMessage->>createMessage: truncate oversized tool results
  createMessage->>VSCodeLM: sanitized 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
Loading
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description gives detailed implementation and verification information but omits the required linked issue, checklist, and several template sections. Add the approved GitHub issue reference, complete the pre-submission checklist, and include the required template sections or mark them as not applicable.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the three primary fixes: surrogate sanitization, leaked tool-call recovery, and safe tool-result truncation.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/api/transform/__tests__/vscode-lm-format.spec.ts (1)

333-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test the conversion boundary.

These tests only exercise sanitizeSurrogates. They do not prove that convertToVsCodeLmMessages sanitizes 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

📥 Commits

Reviewing files that changed from the base of the PR and between 276e425 and b4e1727.

📒 Files selected for processing (4)
  • src/api/providers/__tests__/vscode-lm.spec.ts
  • src/api/providers/vscode-lm.ts
  • src/api/transform/__tests__/vscode-lm-format.spec.ts
  • src/api/transform/vscode-lm-format.ts

Comment thread src/api/transform/vscode-lm-format.ts
@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.24138% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/api/providers/vscode-lm.ts 92.00% 5 Missing and 13 partials ⚠️

📢 Thoughts on this report? Let us know!

@github-actions github-actions Bot added the awaiting-review PR changes are ready and waiting for maintainer re-review label Aug 7, 2026
…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 edelauna left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contirbution

Comment thread src/api/providers/vscode-lm.ts Outdated
Comment on lines +781 to +791
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 }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +114 to +118
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]) })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +279 to +319
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 () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 github-actions Bot added awaiting-author PR is waiting for the author to address requested changes and removed awaiting-review PR changes are ready and waiting for maintainer re-review labels Aug 8, 2026
Bertan Ari added 2 commits August 8, 2026 12:28
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 306976d and ed3e8ec.

📒 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.json
  • src/api/providers/__tests__/vscode-lm.spec.ts
  • src/api/providers/vscode-lm.ts
  • src/api/transform/__tests__/vscode-lm-format.spec.ts

Comment thread scripts/probe-vscode-lm-api/extension.js
Comment thread .roo/skills/probe-vscode-lm-api/SKILL.md Outdated
Comment thread src/api/providers/vscode-lm.ts
Comment thread src/api/providers/vscode-lm.ts
- dispose the probe CancellationTokenSource in a finally block
Comment thread src/api/providers/vscode-lm.ts Fixed
@simurg79

simurg79 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@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 (RequestReviewsByLogin denied), so flagging here instead — could you re-review when you get a chance? Note item r3741434464 involved a behavioral decision (extending the quoted-markup guard to unfenced prose) that's worth a look.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/api/providers/vscode-lm.ts (1)

167-192: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve 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 as nearRecovery. 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

📥 Commits

Reviewing files that changed from the base of the PR and between cbac74d and 220ee89.

📒 Files selected for processing (4)
  • .roo/skills/probe-vscode-lm-api/SKILL.md
  • .roo/skills/probe-vscode-lm-api/scripts/extension.js
  • src/api/providers/__tests__/vscode-lm.spec.ts
  • src/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

Comment thread src/api/providers/vscode-lm.ts Outdated
@github-actions github-actions Bot removed the awaiting-author PR is waiting for the author to address requested changes label Aug 9, 2026
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 github-actions Bot added the awaiting-author PR is waiting for the author to address requested changes label Aug 9, 2026
@github-actions github-actions Bot added awaiting-author PR is waiting for the author to address requested changes and removed awaiting-author PR is waiting for the author to address requested changes labels Aug 9, 2026
Bertan Ari added 2 commits August 10, 2026 16:46
…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.
@simurg79
simurg79 requested a review from edelauna August 11, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-author PR is waiting for the author to address requested changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants