fix(core): recover function-style XML tool calls - #13437

Reviewed the taught-dialect recovery at 2a064ee999a4 (base f2e069061ab4), parser source read from the head revision and exercised directly on the head build of xml-tool-call-fallback.ts (node 24, marked 15.0.12), with main as the control.

Verdict: request changes — 3 findings, no approval-blocking complaint about the feature itself.

The feature does what it claims: the full taught shape from prompts.ts (<tool_call> / <function=…> / <parameter=k>v</parameter>, prose interleaved, values on their own lines) recovers every call with the prose preserved, <example>-wrapped system-prompt echoes stay text, inline-code mentions do not suppress a later call, fenced documentation is skipped, edit payloads containing fence lines survive, __proto__ stays a real null-prototype key, and the invoke dialect is unchanged. The example scanner also held up under bold/blockquote/list/heading wrapping, quoted attributes containing >, nesting, and self-closing tags.

What needs work:

1. [P1] Truncated-parameter borrowing is still fail-open (inline, xml-tool-call-fallback.ts:207) — a block whose </parameter> and </function> are both missing swallows the next complete call's closing tags, so the guard sees an empty residue and dispatches a corrupted write_file whose content is the following call's markup; that call never runs and remainingText is empty. New versus main, which recovered nothing.
2. [P1] Unbounded synchronous lexer cost (inline, :155) — Lexer.lexInline runs on every candidate with no <example> early-out, no timeout and no try/catch; 4 KB / 8 KB of [a]( prose in a turn that also carries a call measured 3.1 s / 23 s.
3. [P3] Rejected blocks still feed the example scan (inline, :245) — masking uses only accepted blocks' ranges.

Every finding has a self-contained repro in the comment. Not reviewed here: the daemon/WebShell end-to-end path (your tmux evidence covers it) and the llm-chat.ts integration wiring, which is untouched by this diff.

APPROVE at c13c2d61, with one non-blocking suggestion about a degradation direction. Scope: I read xml-tool-call-fallback.ts in full at this head plus the whole of 2ce7b0c1's production diff. I did not work through the ~600 lines of test changes beyond confirming the parser suite runs in the green Test lane.

Worth stating first: 2ce7b0c1 (12:42) has no review coverage. @qwen-code-ci-bot's review was dismissed at 09:37, before that commit landed, so the hardening is unreviewed until now. 3 threads, 0 unresolved; CI is 22 pass / 0 fail with nothing in flight.

Finding 1's fix is an arithmetic invariant, and the rescan cannot loop

The borrowed-closer test is:

paramsBlock.match(PARAM_OPEN_PATTERN)?.length !==
  (outsideParameters.match(PARAM_OPEN_PATTERN)?.length ?? 0) +
  (paramsBlock.match(PARAMETER_PATTERN)?.length ?? 0)

Each complete PARAMETER_PATTERN match consumes exactly one open and one close tag, and outsideParameters is the body with those matches removed, so totalOpens === leftoverOpens + completeMatches holds for any well-formed block. It breaks precisely in the borrowed case: one match spans two open tags, so the left side exceeds the right and the block is rejected. Counting rather than pattern-matching the residual is what makes this work, and the comment says why the older residual-tag test could not see it — a borrowed close leaves no residual tag behind.

The rescan position is match.index + match[0].indexOf('>') + 1, which is strictly greater than match.index for any match (indexOf('>') cannot be negative here, since both alternatives in TOOL_CALL_PATTERN begin with a tag that contains one). So lastIndex always advances and the loop terminates; and because the assignment sits in the reject branch only, accepted blocks do not rescan. Letting a swallowed later block be found on its own merits is the right consequence — the alternative was losing a valid call because a preceding invalid one had eaten its close tag.

Finding 3's fix moves the masking input to the right place

parameterRanges is now built by scanning PARAMETER_PATTERN over the whole text before the block loop, instead of accumulating spans from accepted blocks. The comment states the failure it closes: a rejected block's parameter data went unmasked, so a literal example tag inside it could swallow a later valid call. Parameter spans are a property of the raw text, so deriving them from the raw text is the correct dependency direction — and it makes the masking independent of the guard's verdicts, which is what lets findings 1 and 3 compose instead of interact.

Finding 2's early return is sound

if (!text.includes('<example') && !text.includes('</example')) return [];

The tag scan that follows matches <\/?example(?=[\s/>])… and every candidate is re-checked against tagPositions, so with no literal example tag in the text the function could only ever return an empty list anyway. Skipping the lexer therefore changes nothing semantically and removes the super-linear cost on example-free responses, which is the common case. The stated measurement — 4 KB of repeated link openers costing ~3.0 s of synchronous work, with nothing bounding the model's output length — is the right reason to gate it.

One non-blocking suggestion: the lexer catch degrades in the unsafe direction

try {
  collectTags(Lexer.lexInline(prose), prose, 0);
} catch {
  tagPositions.clear();
}

Clearing yields "no example ranges", and exampleRanges is consumed as an exclusion filter (:282-287):

!exampleRanges.some(([exampleStart, exampleEnd]) => …)

With an empty list, some is false and no block is excluded. So on a lexer throw, a call written inside an explicit example wrapper becomes recoverable and executable, which is the opposite of the body's promise that "fenced or explicit example documentation … remains text".

The blast radius is narrower than it first looks, and this is why I am filing it as a suggestion rather than a blocker: the other half of that filter, positionInsideFence(text, …), does not depend on the lexer, so fenced content stays excluded even when the lexer throws. Only the explicit-wrapper half fails open. And reaching it requires Lexer.lexInline to throw at all — which the author judged possible enough to add the catch for, so the consequence of the catch is worth choosing deliberately rather than inheriting.

The conservative alternative costs nothing relative to today's main: when the lexer throws, bail out of recovery for this response entirely rather than continuing with no example ranges. "Recover nothing this turn" is exactly the behaviour main has now, so it cannot be a regression, whereas "recover everything, including documentation" is a new and silent one. If bailing out of the whole response is too blunt, treating the throw as "the entire text is unverifiable, so exclude all blocks" is the same shape.

A second note, also non-blocking

The rescan guarantees progress but not bounded total work: each rejected block restarts the block scan from just after its own open tag, so a response containing many rejected blocks is quadratic in the block loop. That is much milder than the lexer's super-linearity and the intent-ratio guard bounds how much text reaches here, but since finding 2 exists precisely because synchronous cost on model-controlled input mattered, the interaction is worth knowing about rather than discovering later.

Two things done right that are easy to miss

args is Object.create(null), so a model-supplied parameter name cannot pollute a prototype — consistent with how the rest of the repo handles network-derived keys. And the catch's comment records why a catch is needed now when it was not before ("the regex-only implementation this replaced could not throw"), which is the kind of provenance that stops the next reader deleting it as dead defensiveness.

CI and vote effect

22 pass, 0 fail, 9 skipped, nothing in progress — including Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke and both Desktop Shell legs. The body discloses a local run of 622/624 with two LlmChat cases timing out and passing on a serial single-worker rerun with unchanged limits; the green Test lane at this head is consistent with that being local contention rather than a defect, so I am not carrying it forward — but the two-file unit run is the one that exercises this parser, and it is worth knowing the full-suite result rests on CI rather than on a clean local run.

All three changed files are under packages/core/src/core/, which is CODEOWNERS-covered, 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 until one of them does.

Main flow looks right — the taught-dialect recovery, fence/<example> shields, native-call priority, and span removal all verified independently (details below). Two follow-ups inline, one worth closing before merge; nothing here questions the direction.

Verification against this head (c13c2d61a0), independent of the PR-thread evidence:

• Merge-reference TUI run, real built CLI in tmux against the repo's loopback fake OpenAI server, before/after differing only in this file (merge-base vs head, same build everywhere else). After: one real read_file executed, the next model request carried the matching tool receipt with the actual file bytes; before: same XML stayed dead text with zero tools. Fenced and <example>-wrapped variants of the same call executed zero tools and stayed intact text.
• Regression pin: the new taught-dialect suite is red on merge-base source (16 failed / 13 passed) and green at this head; full run of both changed test files 630/630; eslint clean on all three files.
• The three earlier round findings (borrowed closers, no-marker lexer cost, rejected-block masking) are confirmed fixed at this head, empirically where reproducible.

Count: 0 P0, 1 P1, 1 P2, 1 suggestion (a failed-stream no-dispatch pin is still missing — the streamError === null gate is pre-existing and correct by construction, but the PR body lists it as a test-plan item, so pinning it would close the loop; non-blocking).

Note for the gate: contributor PR touching packages/core, so per AGENTS.md this still needs a maintainer's review before merge regardless of my verdict.

Re-approving at 838292de. My approval at c13c2d61 was dismissed by the push. I reviewed the delta only — the three commits and their 82 lines of new tests — and two of them fix defects in code I approved. I verified both by executing the guards rather than by reading them, so the record here is measured rather than asserted.

I was wrong about the borrowed-closer arithmetic, and the new test is strictly better

In my previous review I called the open/close tag count "an arithmetic invariant" and wrote that counting rather than pattern-matching the residual was what made it work. That was wrong. I ran both guards over three inputs:

So the arithmetic had a false positive on exactly the case the new commit names — a value documenting this syntax — and the consequence was the bad one: an intact call was rejected, never ran, and left its raw markup visible. The replacement /<(?:function|invoke)(?:[\s=>]|$)/.test(paramsBlock) still rejects the borrowed-closer case, because a borrowed close only happens when the swallowed region contains a nested call, and a nested call carries its own opener. Same coverage of the case the arithmetic was added for, without firing on legitimate literal text. The new comment states the model correctly: an open tag inside an accepted value is that value's own text, not evidence of an unclosed parameter. The arithmetic was a proxy for "a nested call was swallowed", and proxies fire on things that merely resemble the target.

The openTagEnd fix closes a hole I had the observation for and did not draw the conclusion from

I noted last round that TOOL_CALL_PATTERN's invoke branch admits > inside the quoted name, and used that only to argue resumeAt is strictly greater than match.index, so the loop terminates. Progress was the wrong thing to be checking. Measured on <invoke name="a>b"> followed by further markup: indexOf('>') + 1 is 16, inside the quoted name, while openTagEnd returns 19, the real tag end. A rescan from 16 runs inside a name attribute the guard had just rejected, and can match and dispatch a complete call embedded there — which is what does not dispatch a complete call embedded in a rejected block name now pins.

The -1 fallback is also the right shape: when no unquoted > exists the tag end is not derivable, so resumeAt becomes match.index + match[0].length and the block is skipped whole rather than rescanned. "Do not rescan inside a rejected block unless the tag end is known" is a safer default than any computed guess.

The lexer cap, and what it changes about my earlier suggestion

The measured numbers in the comment are the justification, and they are the right kind: '*a '.repeat(n) after one example marker costs 238 ms at 2k units, 794 ms at 4k, 3.0 s at 8k and 11.7 s at 16k — a synchronous stall inside a streaming turn, with nothing upstream bounding a model turn's length.

The part I want to record is that the cap path fails closed. (!skipLexer && !tagPositions.has(tagPosition)) waives the lexer-confirmation requirement when the lexer was skipped, so every regex-matched tag is taken at face value and example ranges still form. The test pins this with a spy and a positive control — not.toHaveBeenCalled() over the cap, toHaveBeenCalled() under it — and asserts recovered === false on both sides, so "still honouring examples" is checked rather than assumed. A test that only asserted the over-cap behaviour would have passed with the lexer never called at all.

That narrows but does not close the suggestion from my last review. The catch branch still runs tagPositions.clear() while skipLexer stays false, so on a lexer throw the requirement is not waived and every tag is skipped: no example ranges, and the explicit-wrapper exclusion is lost while the fence exclusion survives. The exposure is smaller now, since the inputs most likely to make the lexer choke are the ones the cap diverts away from lexing. And this PR has demonstrated what the conservative degradation looks like, which makes the fix small: have the throw set the same waiver the cap sets, instead of clearing positions and keeping the requirement. Two lines, and it makes both degradations behave identically. Still non-blocking — it needs a throw to reach.

My other note is unchanged and still non-blocking: the reject path still rescans (lastIndex = resumeAt), so the position is now correct but the total work is still unbounded across many rejected blocks. Correctness was the thing that needed fixing; the shape can wait.

Gates

5 threads, 0 unresolved. CI 21 pass, 0 fail, 8 skipped, with only review-pr still running — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and web-shell E2E Smoke are green at this head, so the three new tests have executed in CI rather than only locally.

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

No blocking findings. Approval blockers: none.

Scope

Read xml-tool-call-fallback.ts (head) in full. Read callers in llm-chat.ts (lines 6417–6419). Verified marked is already a declared dependency. Diffed against all existing inline comments and review bodies.

NOT reviewed: test changes (~600 lines) beyond confirming the new tests exist and exercise the stated cases; Windows/Linux runtime behaviour (macOS tested by author per PR body).

What I checked

API/compatibility (class 2). extractXmlToolCalls and tryRecoverXmlToolCalls have unchanged signatures. containsXmlToolCalls broadens its match set (now also detects function-style XML), which is the intended semantic change. extractXmlToolCalls is not re-exported from the package's index.ts barrel, so no external consumer is affected. The two llm-chat.ts call sites at lines 6417–6419 pass through unchanged.

Contract internals. TOOL_CALL_PATTERN.lastIndex is reset to 0 at every entry point: containsXmlToolCalls, recoverableToolCallBlocks, and the prose-ratio check in tryRecoverXmlToolCalls. No re-entrancy risk in single-threaded JS.

Prose-ratio guard. TOOL_CALL_PATTERN.replace on the full text (including fenced/example blocks) computes the guard the same way as before for invoke-style and extends it to function-style. Blocks excluded by fence/example detection are still available for the ratio denominator.

Cross-check against existing reviews. Three P1 issues and one P2 found by yiliang114 and addressed in subsequent commits:

Non-blocking note from existing reviews (re-confirming): When Lexer.lexInline throws, the catch clears tagPositions and skipLexer remains false, so the tag scan finds no confirmed positions and produces no example ranges. Explicit-wrapper exclusion is silently lost on a lexer throw, though fence exclusion survives. The conservative fix (treat a lexer throw the same as the over-cap path: skip the lexer requirement rather than clear the confirmed set) remains a two-line change and the regression direction is safe — "recover nothing inside explicit examples this turn" rather than "recover everything including documentation". Non-blocking as before.

Quadratic reject path (still non-blocking): TOOL_CALL_PATTERN.lastIndex = resumeAt is strictly greater than match.index after openTagEnd fix, so the loop terminates. Total work across many rejected blocks is O(n²) in the block count, milder than the lexer's super-linearity and bounded by the intent-ratio guard. No action required here.

Reviewed with AI assistance.

Critical-only scan at head 838292de1171b9d51dde86f65b6df40c05226df8 — partial, budget-limited.

Verdict: COMMENT. No Critical is asserted against this diff. This is not an approval: the review ran out of budget before the historical-blocker gate could be closed, and the unconfirmed gates are listed below so the author and maintainer know exactly what was and was not checked.

What was completed

The whole production diff was read at this head — packages/core/src/core/xml-tool-call-fallback.ts, +224/-62, all 395 patch lines. Nothing blocking was found in it, and three properties were checked directly rather than assumed:

• The rescan loop terminates. resumeAt is match.index + tagEnd when openTagEnd derives a tag end, else match.index + match[0].length. Both are strictly greater than match.index, and the assignment sits only in the reject branch, so TOOL_CALL_PATTERN.lastIndex always advances and accepted blocks do not rescan.
• The offset remap for remainingText is sound. removedRanges inherits ascending order from the match loop, and the prefix-sum walk with the start > originalOffset break maps a wrapper's post-strip offset back to its original position correctly — which is what lets an originally-empty wrapper survive while a wrapper emptied by stripping does not.
• Parameter spans are derived from raw text, not from accepted blocks. parameterRanges is built by scanning PARAMETER_PATTERN over the whole text before the block loop, so masking and fence tracking no longer depend on the guard's verdicts. That is the right dependency direction and is what lets the borrowed-closer rejection and the masking compose.

Gates not confirmed — the reason this is not an approval

1. The three earlier Criticals were not verified at this head. The qwen-code-review-bot review that assesses them (5418141544) was written against c13c2d61, which is two commits behind the current head, and it is DISMISSED. Its reasoning — the borrowed-closer count invariant, the rescan advancement, the early example return — reads as sound, and the rescan and masking halves are independently confirmed above, but inheriting a verdict from an older head over a dismissed review is exactly what this channel's rules do not permit. The qwen-code-ci-bot review that originally filed those findings was dismissed before 2ce7b0c1 landed, so its findings are not readable from the review collection either.
2. The two approvals at this head were not read. qwen-code-review-bot (5424307028) and chiga0 (5424322087) both approved at 838292de. Their scope disclosures would say what was covered; without them I cannot borrow their coverage.
3. The ~539 lines of test changes were not read, beyond noting that Test (ubuntu-latest, Node 22.x) is green. For a parser whose whole job is deciding which model-authored markup becomes an executable call, a false green in the new cases would be the most likely place for a defect to hide.
4. Ten inline author threads on this file were not read.

One item worth a maintainer's eye, explicitly not filed as a Critical

computeExampleRanges degrades in the unsafe direction on a lexer throw: the catch clears tagPositions, the tag scan re-checks every candidate against that now-empty set, so exampleRanges comes back empty and no block is excluded — a call written inside an explicit example wrapper becomes recoverable and executable, which is the opposite of the documented promise. qwen-code-review-bot already graded this non-blocking at c13c2d61 and it is unchanged at this head. I am not filing it as a Critical because I could not establish within budget that Lexer.lexInline can actually throw here, and an unproven reachability chain is not a reportable finding. Recording it so the choice is visible rather than implicit. Note the direction is not uniform: the skipLexer path over MAX_LEXER_SCAN_LENGTH leaves tagPositions empty too, but there the regex-matched tags are taken at face value, which yields more exclusions rather than fewer.

CI

Healthy at this head — Test (ubuntu-latest, Node 22.x), Lint & Static and the integration and desktop lanes report pass, with no failure attributable to this PR.

Next step

Nothing here asks for a code change. A reviewer with budget to spare should close gate 1 by re-checking the three earlier findings against 838292de in source, and gate 3 by confirming the new parser cases actually fail when the guards are reverted; once those two are closed this is approvable on the strength of the production read above.

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

My previous review on this PR was a budget-limited COMMENT at 838292de. It asserted no Critical and listed four gates it had not closed. One commit has landed since; this review closes those gates at the current head.

The delta since the last review is the fix for the one item I declined to file

f3dd8cf8 is +3/-4 in xml-tool-call-fallback.ts. It is exactly the conservative degradation both prior reviewers specified for the lexer catch:

let skipLexer = text.length > MAX_LEXER_SCAN_LENGTH;
…
} catch {
  // Preserve example filtering with the same fallback as oversized text.
  skipLexer = true;
}

The old branch ran tagPositions.clear() while skipLexer stayed false, so the tag scan's (!skipLexer && !tagPositions.has(tagPosition)) was true for every candidate, exampleRanges came back empty, and the explicit-wrapper exclusion was silently lost — a call documented inside <example> became recoverable and executable. Setting the waiver instead of clearing the set makes the throw path behave identically to the over-cap path: regex-matched tags are taken at face value, example ranges still form, and the wrapper exclusion survives.

Three properties of the replacement checked directly:

• No stale-partial-set hazard. If collectTags throws midway, tagPositions may hold partial entries. skipLexer = true short-circuits the &&, so the set is not consulted at all — partial state is inert.
• The mutation is visible where it is read. skipLexer is function-scoped let and the consuming while loop runs after the try/catch, so the assignment lands before any read.
• The degradation direction is safe. Face-value tag acceptance can only yield more example ranges, hence more blocks excluded. "Recover nothing inside explicit examples this turn" matches main's behaviour and cannot be a regression; the old direction could have been.

The rewritten test is stronger than the mock it replaces. It drives a real lexer overflow ('*'.repeat(4096) either side of prose) instead of a stubbed throw, bounds the example range with a real </example>, and asserts the safe outcome on both sides: only read_file is recovered with {file_path: 'b.ts'}, and remainingText still contains the full documentation. expect(text.length).toBeLessThan(64 * 1024) is the load-bearing control — it proves the test exercises the catch path rather than the cap path, which now behave identically and would otherwise be indistinguishable.

Gate 1 — the five historical blocking findings are fixed at this head

All five review threads are resolved, and I verified each in source at f3dd8cf8 rather than inheriting a verdict from an older head:

Independently re-confirmed at this head: the rescan loop terminates, since resumeAt is match.index + tagEnd (tagEnd ≥ 1) or match.index + match[0].length, both strictly greater than match.index, and the assignment sits only in the reject branch so accepted blocks do not rescan. The remainingText offset remap is sound — removedRanges inherits ascending order from the match loop and the prefix-sum walk breaks on start > originalOffset, which is what lets an originally-empty wrapper survive while one emptied by stripping does not. args remains Object.create(null), so a model-supplied parameter name cannot pollute a prototype.

Gates 2 and 3 — coverage I previously could not borrow

The two approvals at 838292de were read in full. qwen-code-review-bot executed the guards over concrete inputs rather than reading them, and correctly retracted its own earlier "arithmetic invariant" claim by showing the old count rejected a parameter value that literally contained <parameter name="x"> — a false positive in the bad direction, since an intact call was rejected and left raw markup visible. chiga0 independently reached the same fixed/not-fixed verdict on all four findings and disclosed its own scope, including that it did not review the ~600 lines of tests. Their coverage is consistent with what I verified in source above; nothing in it is contradicted.

On tests: the production file is read in full at this head and I found no provable blocking defect in an uncovered path, and Test (ubuntu-latest, Node 22.x) is green at this head, so the new parser cases executed in CI rather than only locally. Per this channel's rules a missing test is not itself a Critical, and I have no evidence of a false green here.

Gate 4 — inline threads

All five inline threads read; zero unresolved. None was closed on a resolved flag alone — each was checked against the current source as tabulated above.

CI

Healthy at this head. Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke and both Desktop Shell legs pass. The only failures are review-pr and fallback-comment, which are this repository's review-automation lane and not product checks; no failure is attributable to this PR's code.

Note for the maintainer, not a blocker

The reject path still rescans, so total work across many rejected blocks is quadratic in the block count. Position is now correct and the intent-ratio guard bounds how much text reaches here, so this is a shape concern rather than a correctness one. Both prior reviewers graded it non-blocking and I agree.

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

No blocking findings. Approval blockers: none.

Re-review delta

My round-1 review (838292de) was dismissed when head moved to f3dd8cf89b14. The delta is one commit: +3/-4 in xml-tool-call-fallback.ts, the catch block fix I listed as a non-blocking recommendation.

Prior non-blocking note — FIXED. The catch branch in computeExampleRanges now sets skipLexer = true rather than calling tagPositions.clear(). As I noted last round, clearing the set while skipLexer remains false caused every subsequent tag to fail the tagPositions.has() check, producing empty example ranges and silently losing explicit-wrapper exclusion on a lexer throw. The fix makes the throw path behave identically to the over-cap path: example ranges still form from the regex scan, the wrapper exclusion survives. Confirmed at head by reading xml-tool-call-fallback.ts lines 752–757.

Three additional properties confirmed at this head:

• No stale-partial-set hazard: skipLexer = true short-circuits the &&, so any partial entries in tagPositions from a midway throw are never consulted.
• skipLexer is function-scoped let; the assignment lands before the consuming loop.
• Degradation direction is safe: "treat this turn as over-cap" is strictly more conservative than "silently lose all example ranges".

Round-1 findings status

All prior findings from yiliang114 (borrowed-closer dispatch, unbounded lexer cost, rejected-block masking, openTagEnd attribute parsing) and the one non-blocking note from my round-1 review are confirmed fixed at this head. No new issues introduced by the delta.

Scope (cumulative)

Read xml-tool-call-fallback.ts at head in full (round 1). Read llm-chat.ts callers at lines 6417–6419 (round 1). Verified marked ^15.0.12 is a production dep (round 1). Delta commit read in full this round.

NOT reviewed: test changes beyond confirming they exercise the stated cases; Windows/Linux runtime behaviour.

Reviewed with AI assistance.

Verdict: request changes — one blocking finding in the new recovery path. The rest of this head checks out.

Verified clean at f3dd8cf8 before the blocker: taught-dialect recovery for wrapped/bare/split calls; span removal (strictly ascending, non-overlapping ranges — no prose eaten); the llm-chat.ts gating (recovery only on a clean stream with no native calls, and the recovered functionCall flows through the normal scheduler/confirmation path — no approval bypass); fence/example shields under the real lexer; and the f3dd8cf8 lexer-failure change itself, which is strictly better than the tagPositions.clear() it replaced (that one dropped example shielding entirely). CI is green at this head.

The blocker is inline at line 282, reproduced by executing this head's extractXmlToolCalls directly:

<function=write_file><parameter=file_path>a.txt</parameter><parameter=content>How to call: <function=read_file><parameter=file_path>b.ts</parameter></function> done</parameter></function>
→ [{ name: "read_file", args: { file_path: "b.ts" } }]   // write_file never runs

Trace: the outer match ends at the documented call's </function> → the guard sees a <function opener anywhere in paramsBlock (values included) and rejects the intact block → the rescan restarts at resumeAt, just past the outer open tag, i.e. back inside the parameter values → the documented read_file matches clean and is accepted. None of the three documentation guards cover that spot: fence tracking skips lines inside parameter ranges, computeExampleRanges skips <example> tags inside parameter ranges, and the fence check runs on outsideParameters with values stripped. The block's own comment ("an open tag inside an accepted value is that value's own text") describes exactly the case the guard doesn't distinguish — the pinned test only covers a <parameter> mention, not a complete call, and fencing the documented call inside the value doesn't protect it either.

This blocks merge because the natural "write_file documents the dialect" case executes the wrong tool: the intended write is silently lost, its markup is left mangled in remainingText, and an arbitrary documented call is dispatched instead (approval-gated, but no longer reading as documentation to the approver).

Two non-blocking notes: the skipLexer face-value example pairing has a specific inline-code hole (inline at line 190 — sharper instance of the regex-only degradation direction flagged in an earlier round), and positionInsideFence re-scans text[0..index] per example tag and per block while the 64KiB cap covers only the lexer pass — a very large turn dense with <example> tags multiplies that O(N) rescan. Follow-up material, not blockers.