fix(core): keep an XML tool call quoted in a parameter value inert - #13515

Reviewed the full diff (base 4ce35fc…head 76513d8) and the complete module at head. No blocking findings; correct as far as I can verify. Evidence below.

How it works. The fix is purely additive: closedParameterSpans pairs parameter tags by depth, and any TOOL_CALL_PATTERN match whose opener starts inside a closed value span is skipped before the guards run. Every remaining branch is base code, so the change can only suppress dispatches, never add one — I confirmed by tracing both added branches and by the red/green runs below.

Verification I ran myself (in a worktree at the head commit, npm run build first):

• packages/core fallback suite: 82/82 green at head, including the untouched borrowed-closers witness.
• Red-first reproduced: head test file against the base source yields exactly the two new tests failing and the 80 pre-existing ones passing.
• eslint (changed files) and tsc --noEmit (packages/core): clean.
• Boundary probes beyond the added tests (run once, then deleted): <function=name> / <parameter=name> dialect parity; a real sibling call after a closed value still dispatches; an unclosed outer parameter owns nothing (the truncated-block rescan path stays intact); an opener at the exact span-end dispatches; a nested parameter pair inside a value does not reopen it. All 5 pass.

Consumers. Sole production consumer is llm-chat.ts (containsXmlToolCalls gates tryRecoverXmlToolCalls); the fix strictly reduces recovered calls in quoted-markup text, which is the intended security improvement there — no consumer relies on getting more calls. containsXmlToolCalls is unchanged, so the gate/recovery flow is coherent: unsupported matches now leave the text as prose, exactly as the new first test asserts via remainingText.

Tests. Both added tests pass the value gate: the first reproduces the #13492 incident (fails pre-fix, passes post-fix), the second pins that the skip is scoped to the owning value — a broad skip would fail it. The first also asserts the tryRecoverXmlToolCalls contract (remainingText preserved).

Two non-blocking notes: (1) CI was still mid-run on the head commit when I reviewed (ubuntu Test and Lint & Static in progress; macOS/Windows Test skipped by matrix), so merge should wait for green rather than my review. (2) Per the repo's two-tier rule for packages/core this is a small-scope core change (52 production lines); my confidence is high from the evidence above, but maintainer sign-off is still required — treating this comment as the requested gate evaluation, not an approval.

Tier: Standard. Pure additive change (92 lines, 0 deleted) to the XML tool-call fallback parser. No public API changes.

Scope. xml-tool-call-fallback.ts (new PARAMETER_TAG_PATTERN regex, closedParameterSpans() helper, guard in recoverableToolCallBlocks) and its test file. Sole production consumer is llm-chat.ts, which is unchanged. Not reviewed: macOS/Windows runtime behavior (PR flags both as untested; unit-only scope).

Findings

No blocking findings.

What was checked

PARAMETER_TAG_PATTERN regex — /<\/?parameter(?=[\s=>])(?:[^>"']|"[^"]*"|'[^']*')*>/g. The (?=[\s=>]) lookahead correctly prevents matches on longer tag names like <parameterize>. Quoted attribute values are handled by the alternation. lastIndex = 0 is reset at closedParameterSpans entry, preventing stale state between calls. Self-closing <parameter/> is not matched (the lookahead expects \s, =, or >); self-closing parameters are not part of this dialect, so this is correct.

closedParameterSpans depth logic — The stack-based pairing is correct: open tags push their start position; close tags pop and record a [start, end) span where end = tagMatch.index + tagMatch[0].length (position just past the > of the close tag). Unmatched open tags stay on the stack and contribute no span, which preserves the existing borrowed-closer rescan path: an outer block whose parameter is never closed does not suppress a swallowed sibling call.

Span check boundary — matchStart >= start && matchStart < end uses an exclusive end, correctly excluding a call that starts immediately after </parameter>. Traced the geometry for both added tests: the inner run_shell_command opener falls inside the content value span in both; the read_file opener in the second test falls after the span end and dispatches normally.

Rescan interaction — When the outer write_file block is rejected and TOOL_CALL_PATTERN.lastIndex is set to resumeAt, the next exec() finds the inner run_shell_command block. Its matchStart is inside the content value span — continue fires. Since exec() already advanced lastIndex past that match, the next iteration searches from after the inner block's end, finds no further <invoke> before the outer block's real close, and returns zero dispatched calls. The sibling read_file in the second test starts outside all value spans and is dispatched correctly.

Tests — Both added tests are effective. The first reproduces the #13492 incident precisely (fails against base, passes at head). The second pins that the skip is scoped to the owning value — a broad guard would fail it. Deleting the valueSpans.some(...) guard makes both red. The five pre-existing borrowed-closer witnesses are untouched and still pass.

Consumer check — llm-chat.ts is the sole production consumer. The fix strictly reduces recovered calls for quoted-markup text. No consumer depends on getting more calls back from the fallback; the change is a security improvement in the safe direction.

Cross-check — yiliang114 reviewed at prior head 76513d8d52b0 (COMMENTED, no approval): no issues flagged, five boundary probes confirmed. My analysis confirms the same conclusions. No misses found.

No blocking findings. Approval blockers: none.

Reviewed with AI assistance.

Critical-only scan at head 9cb4847d9e0677f69559238d144877670e4bc5a9 — complete. Verdict: APPROVE.

No Critical found, and there is no historical blocking issue to clear: the PR has zero review threads, no prior review findings from any reviewer, and the one existing review entry is the author's own self-assessment, explicitly graded "no blocking findings" and explicitly not offered as an approval.

The defect being closed is a real execute-from-prose path, not a cosmetic one

Before this change, a write_file whose content documented this dialect with an unfenced example call had the example dispatched. The first new test reproduces the reported incident, and the value that ran was run_shell_command with command: "rm -rf /tmp/x". Text a model emits as documentation executed as a tool call, while the write the user actually asked for stayed behind as prose. That is the worst available direction for a fallback whose entire job is deciding which model-authored markup becomes an action.

The mechanism the body describes checks out against the source. The outer block's lazily matched body ends at the quoted block's closer, so its parameter region holds a call opener the parameters did not consume and the borrowed-closer guard rejects the block — correctly, by its own rule. The truncated-block rescan then restarted just after the outer open tag, where the nested complete call matched on its own merits. Flat lazy PARAMETER_PATTERN cannot distinguish that from a genuinely swallowed following call, because a nested element consumes its enclosing value's own terminator. Pairing by depth supplies the missing fact: whether a value owns the region.

The change is monotonic in the safe direction, which bounds its blast radius

closedParameterSpans feeds exactly one new branch, and that branch only ever continues. It cannot admit a match the guards would have rejected, cannot alter parameterRanges, computeExampleRanges or positionInsideFence, and cannot change what remainingText retains beyond leaving more of it. So the only failure mode available to this diff is a false negative — a real call left as prose — never a false dispatch. I confirmed this by reading both added branches rather than inferring it from their shape.

That makes the false-negative surface the thing worth probing, and it is narrow:

• Single ordinary call: the invoke's matchStart precedes its own parameter span, so matchStart >= start is false and it is recovered.
• Sibling calls: the second starts after the first value's close and before its own parameter open. Pinned by the second new test.
• A value documenting a stray parameter close tag: that close pops the value's own open, so the value's span ends early and the enclosing call's matchStart still precedes it. The enclosing call is recovered, and a later real call falls outside the shortened span.
• A value documenting a stray parameter open tag: never popped, so it owns nothing. This is precisely the truncated-block case, and leaving it owning nothing is what keeps the existing rescan witness intact.

The residual false negative needs an unmatched open before a real call together with an unmatched close after it, and it degrades to this file's behaviour on main — no recovery. That is the safe side of the trade.

Loop safety, regex state and cost

• The bare continue cannot stall the scan. TOOL_CALL_PATTERN.exec on a /g regex advances lastIndex past the match on success, and a match here necessarily spans literal open and close tags, so it is never zero-length. This contrasts deliberately with the reject branch, which sets lastIndex = resumeAt in order to rescan; skipping whole is the right choice here, since rescanning into a value is what dispatched the quoted call in the first place.
• Module-level regex state is reset. PARAMETER_TAG_PATTERN.lastIndex = 0 at the top of closedParameterSpans, matching how the file already treats TOOL_CALL_PATTERN and PARAMETER_PATTERN at every entry point. No re-entrancy or cross-call leakage.
• The quoted-run admission is consistent with the file's existing tag scanning. (?:[^>"']|"[^"]*"|'[^']*')* mirrors the tags pattern in computeExampleRanges, so a > inside an attribute value cannot end a tag early — the same hazard openTagEnd already closes for the rescan position.
• No new stall hazard. closedParameterSpans is one linear scan. The per-match valueSpans.some(...) is numeric comparison only, while the same loop already calls positionInsideFence per block, which slices and splits the entire prefix. The addition is asymptotically no worse than code already in the loop and considerably cheaper per unit.

Both tests carry their weight

The first asserts extractXmlToolCalls returns an empty array and that tryRecoverXmlToolCalls reports recovered: false with remainingText equal to the whole input — so it pins the recovery contract, not just the extraction. The second asserts exactly one recovered call, read_file with args: { p: 'b.ts' }. That single assertion does three jobs at once: the quoted run_shell_command is not recovered, the outer write_file is still rejected by the borrowed-closer guard, and a real sibling outside the owning value survives. A blanket skip would fail it, which is what makes it a genuine scope control rather than a restatement of the first test.

Dependency and CI

This PR builds on #13437, which merged at f3dd8cf8; the patch context sits inside that PR's openTagEnd, so the base is merged code rather than an open sibling. The delta between the head the author self-reviewed and this head is four commits of merges from main touching unrelated packages — neither of this PR's two files changed, so its own contribution is byte-identical to what that review covered.

Lint & Static, Integration Tests (no-AK, No Sandbox) and both Desktop Shell legs pass at this head. Test (ubuntu-latest, Node 22.x) was still pending at review time; under this channel's rules a pending Test lane is not a gate and I did not poll it, so my verdict rests on the source read above rather than on that lane. It is the one check worth seeing land before merge.

Both changed files are under packages/core/src/core/, so require_code_owner_review applies. This approval is submitted as qqqys.

APPROVE at 9cb4847d. Gates: 0 review threads (nothing unresolved), this is the first approval on the PR, mergeable=MERGEABLE, and CI is 20 pass / 0 fail / 53 skipped. Two lanes — Test (ubuntu-latest, Node 22.x) and Hosted process fault gates / MySQL 8.4 / Java 21 — have been stuck IN_PROGRESS since 2026-10-06T13:53Z, three days, and three review-pr automation lanes are cancelled. That is runner infrastructure, not a code signal, and this PR touches no Java. Rather than cite the stuck lane I replaced it: I ran the suite myself against this exact head and got 82/82 passing, which reproduces the body's "After" evidence independently.

I reviewed this file in #13437, so I read the change against the two guards that landed there — the quote-aware openTagEnd and the nested-opener test that replaced the borrowed-closer arithmetic. This is the third member of that family, and it is the one that closes a hole the second guard opened.

The defect was real, and I measured it rather than accepting the description

I built a differential harness in an isolated scratch directory over the head source and a variant with only the valueSpans skip removed, so each difference is attributable to this change:

Row A is the point of the PR and it is worse than "a call was misattributed": before this change the fallback dispatched run_shell_command with command: "rm -rf /tmp/x" taken from a value that was documenting syntax. Row B shows the skip is scoped to the span that owns the markup rather than to the rest of the turn, which is the property that makes it safe to add at all. Rows E and F are the controls I cared about most, because an unbalanced quote in a value is ordinary prose and a common apostrophe must not disturb pairing — neither does, since PARAMETER_TAG_PATTERN's quoted-run alternation only has to survive the tag, not the value.

C is a regression I can reproduce, and the obvious fix makes things worse

Pairing is positional: openStarts.pop() matches the most recent unmatched opener, and a closer carries no name, so nothing can validate the pair. When openers and closers do not balance, a stray closer pops an opener belonging to an earlier, never-closed element and the span it creates reaches across everything in between — including a well-formed call. Measured input, an unclosed content, then an intact read_file, then one extra closer:

<invoke name="write_file">
<parameter name="content">unclosed value
</invoke>
<invoke name="read_file"><parameter name="p">b.ts</parameter></invoke>
</parameter>

Head returns (none); without the skip it returns read_file. So a legitimate call is silently dropped and left as prose, with no error to the model — the same failure class #13437's nested-opener rewrite existed to eliminate.

Two things keep this non-blocking. The polarity is safe: it drops a call rather than executing one, in input that is already malformed enough that the fallback is running at all. And the intuitive hardening does not work — I tried clearing the stack at each </invoke> or </function> so a pending opener cannot reach across a completed call, and that breaks A: the quoted call carries its own </invoke> inside the value, so the reset pops content, content then owns nothing, and rm -rf /tmp/x is dispatched again. Worth recording so nobody spends a round on it. What I would ask for instead is cheap: a test pinning the current unbalanced-closer behaviour, so a future change to the pairing cannot widen the span silently. The docstring's claim that an unclosed opener "owns nothing" is true only while no stray closer arrives later, and D versus C is exactly that boundary.

A verified negative: the outer write_file is not cheaply recoverable, and trying corrupts the write

Case A now yields no calls at all, so the write the model actually asked for still does not land — #13492's symptom was "dropped the write and ran the example", and this fixes the dangerous half. Since closedParameterSpans already computes which region a value owns, the tempting follow-up is to reuse it to exempt quoted regions from the nested-opener guard and recover the outer call. I tested that instead of proposing it: neutralising only that guard does recover write_file, but with a truncated content —

"content": "Usage:\n<invoke name=\"run_shell_command\"><parameter name=\"command\">rm -rf /tmp/x"

The value's own </parameter></invoke>\n tail is gone, because TOOL_CALL_PATTERN is lazy so match[0] ends at the quoted call's closer. That would be a silent corrupted write reporting success — strictly worse than the inert outcome this PR produces. So the guard's rejection is load-bearing here, the ownership information does not substitute for depth-aware block matching, and leaving the rescan as it was is the right call. Recovering the outer call needs </invoke> pairing at the block level, which is its own change; if #13492's reporter needs the write to land, that is the issue to file.

Scope of my read

Both diffs in full, plus recoverableToolCallBlocks, closedParameterSpans, PARAMETER_TAG_PATTERN and the guard at head. I did not review computeExampleRanges or positionInsideFence beyond confirming the skip happens before them and that valueSpans is derived from raw text like parameterRanges is, so a rejected block's data stays masked. Execution was in an isolated scratch tree; the repository checkout it borrowed node_modules from was not modified.

Vote effect

Both files are under packages/core/src/core/, so require_code_owner_review applies: one of wenshao, tanzhenxin, LaZzyMan, doudouOUC or qqqys must approve. This approval satisfies required_approving_review_count: 1 but not the code-owner requirement, so reviewDecision stays REVIEW_REQUIRED and mergeStateStatus stays BLOCKED until one of them submits.