fix(vscode-lm): sanitize surrogates, recover leaked tool calls, and window-safe tool_result truncation - #12
Merged
Merged
Conversation
…w-safe tool_result truncation - sanitizeSurrogates(): replace unpaired UTF-16 surrogate code units with U+FFFD so the Copilot/Anthropic backend no longer 400s on requests that cannot be encoded as valid UTF-8. - extractLeakedToolCalls()/trailingPartialToolMarkerLength(): recover tool calls that some VS Code LM backends stream as plain <invoke> XML instead of structured tool-call parts, so tasks no longer stall in a 'no tools used' retry loop. - truncateToolResultsToFitWindow()/middleOutTruncate(): shrink oversized tool_result payloads on our side with tool_use/tool_result pairing preserved, so an over-window request is not trimmed by Copilot's backend into an orphaned tool_result 400. - Add diagnostics for the 'no tools used' path (Task.ts) and apply_diff no-op/all-or-nothing behavior (multi-search-replace.ts, ApplyDiffTool.ts). - Bump version 3.53.1 -> 3.53.2.
DaubnerF
pushed a commit
to DaubnerF/Zoo-Code
that referenced
this pull request
Sep 20, 2026
…o-Code-Org#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 Zoo-Code-Org#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 <bertanari@microsoft.com> Co-authored-by: PR1188 Local Tester <tester@local.invalid> Co-authored-by: Elliott de Launay <edelauna@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reliability fixes for the VS Code Language Model (
vscode-lm) provider — primarily for GitHub Copilot serving Anthropic Claude models — plus diagnostics for the "no tools used" path andapply_diff. Bumps the extension version to3.53.2.Changes
VS Code LM provider hardening
vscode-lm-format.ts): newsanitizeSurrogates()replaces unpaired UTF-16 surrogate code units withU+FFFD. A lone surrogate cannot be encoded as UTF-8, so the backend otherwise rejects the whole request with a 400 (string contains an unpaired UTF-16 surrogate code point). Valid surrogate pairs (emoji, CJK ext.) are preserved. Applied to string messages, tool results, and text parts.vscode-lm.ts): some backends stream a tool call as plain<invoke>XML instead of a structuredLanguageModelToolCallPart, so the turn contains notool_useblock and the task stalls in a "no tools used" retry loop.extractLeakedToolCalls()+trailingPartialToolMarkerLength()detect the leaked markup mid-stream (including markers split across chunk boundaries) and replay it as a real tool call. Recovery is conservative: only<invoke>blocks whose name matches a tool actually offered this turn become calls; everything else passes through as text. Only active when tools were offered.tool_resulttruncation (vscode-lm.ts): Copilot's backend trims over-window requests without preservingtool_use/tool_resultpairing, orphaning atool_resultand triggering a 400 (unexpected tool_use_id).truncateToolResultsToFitWindow()+middleOutTruncate()shrink oversizedtool_resultpayloads on our side (largest first, middle-out, pairing preserved) before sending. Onlytool_resulttext is truncated — nevertool_use, assistant text, summaries, or environment details — and only when the request would otherwise exceed the budget.Diagnostics
Task.ts: structured logging on the "no tools used" path, including alooksLikeToolCallAsTextheuristic to make provider tool-call leakage diagnosable.multi-search-replace.ts/ApplyDiffTool.ts: temporaryconsole.warndiagnostics (markedTEMP) around identical SEARCH/REPLACE detection, the all-or-nothing apply gate, andfailPartssurfacing.Tests
New unit tests cover
sanitizeSurrogates, valid/invalid surrogate handling throughconvertToVsCodeLmMessages,extractLeakedToolCalls,trailingPartialToolMarkerLength,middleOutTruncate, andtruncateToolResultsToFitWindow.Notes
The
apply_diff/Task.tslogging is diagnostic (some blocks are markedTEMP) and intended for investigation, not permanent behavior.