Repository navigation
Commit 08d05eb
fix(vscode-lm): add guarded recovery parser and schema conversion (#1188)
* fix(vscode-lm): sanitize surrogates, recover leaked tool calls, and window-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.
* test(vscode-lm): cover leaked tool-call salvage and tool_result truncation 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).
* fix(vscode-lm): guard leaked tool-call recovery against quoted markup
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.
* chore(knip): exclude .roo skill assets from unused-file analysis
Skill directories hold reference scripts and captured artifacts that are intentionally never imported by the build.
* fix(vscode-lm): address review feedback on leaked tool-call recovery
- dispose the probe CancellationTokenSource in a finally block
* docs(skill): drop probe transcripts from repo
Remove the ~120KB raw probe transcript corpus from the vscode-lm probe skill; keep the measured findings and their stated limits in SKILL.md.
* chore: move probe skill scripts under scripts/, drop .roo knip ignore
* fix(vscode-lm): harden quoted-markup detection and bound the salvage 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.
* test(vscode-lm): assert the salvage buffer flushes mid-stream
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.
* test(vscode-lm): cover fence-close branch of isInsideCodeFence
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.
* fix(vscode-lm): clamp messages budget to a positive floor
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.
* fix(vscode-lm): require function_calls wrapper and sanitize tool-call args
Addresses taltas review feedback on PR #1188.
* chore: remove probe skill and harness from PR
* docs: drop dangling probe skill path from vscode-lm comment
* fix(vscode-lm): harden streaming tool-call recovery and token budgeting
Recover only wrapped function_calls/invoke markup leaked into text parts; bare unwrapped invoke is passed through unchanged. Add narrow top-level schema-aware parameter conversion and an approximate output-budget guard, with expanded provider unit tests.
* fix: derive mutation gate base from the PR merge commit's first parent
GitHub checks out the synthetic pull request merge commit as github.sha, but pull_request.base.sha is frozen when the event is created. Once main advances, the stale base made the changed-code mutation gate attribute unrelated upstream-only files to the pull request (3294 changed executable lines across 87 files instead of 361 across the 2 files the PR actually touches).
Resolve the base from the checked-out head's first parent when the head is a merge commit, leaving non-merge heads and the merge_group path unchanged. Head stays github.sha so selector coordinates remain aligned with the checked-out tree.
* fix(vscode-lm): admit requests against the raw context budget, not the trimming floor
The clamp to MIN_TOOL_RESULT_CHARS exists only to keep tool_result trimming productive; using it for the final admission check let a request through whenever the raw budget was positive but below the floor, sending an over-window request. Judge admission against the raw budget and cover the boundary with a regression test.
Also guarantee temp-repository cleanup in the two stryker-diff pull-request-selection tests via try/finally, and move the system-prompt surrogate sanitization test out of the leaked streaming recovery group.
* fix(vscode-lm): accept an explicit null for a nullable leaked-tool parameter
declaredParamType stripped "null" from a declared ["T","null"] union, so convertLeakedParamValue rejected a literal JSON null and failed the whole leaked block closed to text. It now reports that null is permitted and the conversion consults that flag. A non-nullable object still rejects null, and a declared string keeps the literal text "null".
Also assert the streamed text chunk in the accepted-budget test, which previously drained the stream and only checked the sendRequest call.
* fix(vscode-lm): support null-only parameter schemas in leaked tool-call recovery
Handle both structured type: "null" and array type: ["null"] forms in declaredParamType so recovery emits JSON null, while continuing to fail closed for non-null values. Adds unit coverage for both helper forms and a createMessage runtime regression test with a mocked VS Code LM host.
* refactor(vscode-lm): narrow this branch to leaked tool-call recovery
Surrogate sanitization and context-window tool_result truncation are being proposed as independent changes, so remove them here. Recovery does not depend on either: it keeps the original unsanitized system-prompt boundary and no longer references the truncation helpers. Retains the null-only parameter schema fix and the stryker-diff CI prerequisite.
* refactor(vscode-lm): drop surrogate sanitization from the transform layer
Sanitization is proposed independently, so restore src/api/transform to origin/main here. Recovery does not use it; the full provider and transform suites pass without it.
* refactor(vscode-lm): defer leaked tool-call streaming integration
Keeps the complete leaked tool-call parser and its direct tests, but removes the createMessage streaming integration and its integration tests so the changed-code mutation gate stays within its per-run mutant budget. createMessage is restored byte-for-byte to the base implementation, so the parser is present but not yet activated; a follow-up change re-enables it.
* test: local validation commit for PR1188 mutation selection
* test: corrected contracts for PR1188 mutation validation
* test(vscode-lm): behavioral coverage for leaked tool-call parsing
* test(vscode-lm): cover consecutive recovered invoke blocks in one wrapper
* refactor(vscode-lm): simplify leaked tool-call parser internals
Replaces the per-call regex factories with module-scope literals scanned via matchAll, which
iterates a private clone and so cannot strand a shared lastIndex when a parameter scan stops
early. Resolves the null-only declaration in its own branch instead of a never-satisfied table
entry, and accumulates leftover text as a single string now that every segment produced by a
recovery carried the same flag. Behavior is unchanged.
* refactor(vscode-lm): own each leaked-parse regex at its call site
Each pattern is declared where it is used instead of behind a module-scope factory. matchAll
iterates a private clone, so a scan that stops early when a parameter fails its schema cannot
strand a shared lastIndex. Also drops a nullable flag that the null-only branch already settles.
Behavior is unchanged.
* docs(vscode-lm): tighten leaked-parse rationale comments
Corrects a stale note that described the null-only union as forcing a JSON parse, which the
null-only branch now settles directly, and merges two overlapping quoting-cue comments. Also
stops reporting a nullable flag for a null-only type, where it is never read.
* fix(vscode-lm): keep the resolved nullable flag computed
A literal here is unobservable, since the null-only branch settles that case before the flag is
read; the computed value keeps the resolver honest about what the union actually declared.
* fix(vscode-lm): require a whitespace-only suffix on closing code fences
* test(vscode-lm): pin whitespace-only closing fence suffix behavior
* perf(vscode-lm): make leaked tool-call scanning linear in message length
* test(vscode-lm): cover incremental scanner fence, tag, and quoting-cue boundaries
* refactor(vscode-lm): track fence state per line to remove unobservable scanner states
* test(vscode-lm): pin fence suffix handling and cross-chunk line-start tracking
* test(vscode-lm): pin line-start tracking across a deferred newline
* refactor(vscode-lm): drop redundant scan offsets from quoting state
* docs: record string-parsing performance guidance in AGENTS.md
* Revert "docs: record string-parsing performance guidance in AGENTS.md"
This reverts commit 875b0b8.
* test: replace flaky timing-based scaling assertions with deterministic work counter
The two leaked tool-call scaling tests measured wall-clock elapsed time and asserted the 4x-input ratio stayed under 10. On shared CI runners GC pauses and contention breached that even though complexity is linear (observed 14.55 and 10.02). Count characters the parser scans instead: exact, machine-independent, and still ~16x under a reintroduced quadratic prefix re-scan.
* fix(vscode-lm): keep invoke bodies out of quoting state and fail closed on unclosed params
* fix(vscode-lm): ignore function_calls wrapper tags inside quoted code
A <function_calls> opener shown inside a code fence or inline-code span armed the wrapped-only gate, so a later bare <invoke> was replayed as a real tool call. Wrapper tags are now read in source order and only outside quoted code.
* test: correct stale-base expectation for merge-result base resolution
selectFromGit now normalizes a stale base to the merge commit's first parent, so an intervening base-branch file is no longer charged to the pull request.
* fix: track inline code span width when detecting quoted wrapper tags
Backtick parity treated an even-width code span as two toggles, so a quoted <function_calls> example armed wrapped-only recovery and a later bare invoke was replayed as a live tool call. Both quoting checks now share one CommonMark-correct helper that closes a span only on an equal-width backtick run.
* test(vscode-lm): make inline code-span closure tests mutation-sensitive
The wider-backtick-run test placed the wrapper between the opener and the wider run, so its verdict was decided on a prefix ending before that run and no closure-rule mutation could change the outcome. Move the wider run ahead of the wrapper, and add a mixed wider/narrower run case plus a closed-span arming case so the span width tracking in insideInlineSpanAt is actually exercised.
* fix(vscode-lm): fail closed on nested parameter tags and malformed backtick fences
Reject a leaked parameter value that itself contains parameter markup, and do not open a backtick fence whose info string contains a backtick (CommonMark 0.31.2). Adds a regression test pinning that an inline-code run before the wrapper is not a fence.
* fix(vscode-lm): fail closed on unparseable parameter openers in inter-match gaps
parseLeakedInvokeParams validated only matched values and the trailing suffix, so a <parameter opener the strict pattern could not parse was silently skipped and a later well-formed parameter still recovered - dispatching a tool call with an argument the model wrote silently missing. Guard the gap before each match as well.
* fix(vscode-lm): honour wrapper closers in fences and block argument rebinding
Two defects in the leaked tool-call parser introduced by this PR.
The wrapper-tag scan skipped closers as well as openers inside a code
fence, so a quoted closing wrapper tag left the wrapper armed and a later
bare invoke was recovered, breaking the bare-invoke contract. Closers are
now honoured unconditionally; only openers stay fence-gated.
Injected parameter markup in a value split an invoke into adjacent
well-formed matches, letting a later match silently rebind an earlier
argument (e.g. path safe.txt to /evil) and dispatch it. Repeated
parameter names now fail closed.
Also strengthens four suppression tests to assert verbatim leftoverText
passthrough, not just an empty call list.
* fix(vscode-lm): fail closed on recovered parameters absent from the tool schema
---------
Co-authored-by: Bertan Ari <[email protected]>
Co-authored-by: PR1188 Local Tester <[email protected]>
Co-authored-by: Elliott de Launay <[email protected]>1 parent 914f0c4 commit 08d05eb
4 files changed
Lines changed: 1775 additions & 3 deletions
File tree
- scripts
- src/api/providers
- __tests__
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
259 | 259 | | |
260 | 260 | | |
261 | 261 | | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
262 | 271 | | |
263 | 272 | | |
264 | 273 | | |
| 274 | + | |
265 | 275 | | |
266 | 276 | | |
267 | 277 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
95 | 95 | | |
96 | 96 | | |
97 | 97 | | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
98 | 178 | | |
99 | 179 | | |
100 | 180 | | |
| |||
475 | 555 | | |
476 | 556 | | |
477 | 557 | | |
| 558 | + | |
| 559 | + | |
478 | 560 | | |
479 | 561 | | |
480 | | - | |
| 562 | + | |
481 | 563 | | |
482 | 564 | | |
483 | 565 | | |
| |||
0 commit comments