fix: parse nested bare JSON tool calls - #2682
Conversation
|
@alectimison-maker is attempting to deploy a commit to the esokullu's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
🟡 Changes recommended
An unmatched opening brace before a valid tool call prevents the scanner from recovering that call.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Updates fallback parsing to support nested bare-JSON tool calls while preserving browser parity.
Changes:
- Adds a string-aware balanced-object scanner.
- Adds nested and multiple-call regression tests.
- Keeps Chrome and Firefox implementations identical.
File summaries
| File | Description |
|---|---|
src/chrome/src/agent/tool-call-parser.js |
Adds balanced JSON extraction. |
src/firefox/src/agent/tool-call-parser.js |
Mirrors Chrome parser changes. |
test/run.js |
Tests nested arguments and call ordering. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if (start < 0) { | ||
| if (char === '{') { | ||
| start = i; | ||
| depth = 1; | ||
| } |
| if (start < 0) { | ||
| if (char === '{') { | ||
| start = i; | ||
| depth = 1; | ||
| } |
The balanced-object scanner opened a candidate at the first `{` and never
reconsidered that position, so a brace that never closed consumed the rest
of the text and any real tool call after it was lost. The flat regex this
replaced had no such state and did recover those calls.
Models routinely wrap a bare tool call in prose braces, template
placeholders, or a code snippet, which is exactly the population this
fallback serves. Scanning now resumes one character past an unbalanced
opener, capped at 16 restarts so a pathological "{{{{..." response stays
linear over the 10,000-character budget.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed a commit fixing the unbalanced-brace regression. Copilot flagged the same thing on both parser files, so this closes that out. The problem. The scanner opened a candidate at the first
Prose braces, template placeholders and code snippets ahead of a bare tool call are exactly what this fallback exists to survive, so it mattered. The fix. Scanning resumes at I capped the restarts at 16. Unbounded Also. Moved the Tests. Both unbalanced-prefix cases, plus one asserting that a nested 1449 passed, security corpus 60/60, Chrome and Firefox copies Worth flagging for later: there are now two balanced-object scanners per browser tree — this one and |
Recovering nested bare calls also made narrated ones reachable. A model
that writes "I could click it with {...} but that is destructive" or
quotes a call it found in page content now had that call parsed — and
the caller replaces content with the parsed calls (result.content =
null), so the sentence declining the action was discarded and only the
action survived. The old flat regex missed those by accident, because
real calls carry nested arguments; it executed the same narration the
moment the object had no nested braces.
A model that is calling a tool puts the JSON on its own line; a model
talking about a call embeds it in a sentence. Bare candidates are now
accepted only when they are the whole of their line, ignoring whitespace
and a single trailing comma so array-shaped output still parses.
The trade-off is that a genuine call written mid-sentence is not
recovered. That is the safer side here: this fallback exists for models
that emit a call instead of prose.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Follow-up: pushed an own-line rule for bare candidates. This one is a design change rather than a bug fix, so it needs your read. What I found. Recovering nested bare objects also made narrated calls reachable. Testing the same inputs against
This matters more than it looks because of what the caller does with the result: One correction to the PR description. It says this "matches the previous flat-object behavior." It doesn't. The old regex matched only brace-free objects, and essentially every real call carries But
So the real defect is older than this PR and lives in both: the parser cannot tell a call the model committed to from one it merely described. This PR widens its reach; it didn't create it. The rule. A model that is calling a tool emits the JSON on its own line. A model talking about a call embeds it in a sentence. Bare candidates are now accepted only when they are the whole of their line, ignoring surrounding whitespace and a single trailing comma so array-shaped output still parses. Wrapped and XML formats are untouched — they are explicit, so narration was never ambiguous there. Verified both directions:
So this ends up strictly safer than the current Limits, so you can judge it fairly. It is a heuristic on eight hand-picked cases, not a corpus. A model that lists options on separate lines would still get both executed. A genuine call written mid-sentence is now lost — I took that trade deliberately, since this fallback exists for models that emit a call instead of prose, but it is a real behaviour change and it is the part most likely to bite. It also brushes against something you tested for on purpose: multiple bare calls still parse, but only because your fixture puts them on separate lines. If you meant to support several calls inline, this rule contradicts that and should be revisited. 1449 passed, corpus 60/60, both trees |
|
Status notes to go with the commits above, so the review state is on the record rather than in a side channel. Who has actually looked at this. Nobody has approved. Copilot commented and its finding is addressed, but that review does not count toward merge requirements. Everything on this branch beyond your original two commits was written by me and reviewed by me — the unbalanced-brace fix, the restart cap, and the own-line rule. No independent eyes. That is worth weighing given the last of those is a behaviour change to the trust boundary rather than a bug fix. What needs your decision, not just your review. The own-line rule narrows what counts as a tool call. I believe it is right, and it closes holes One thing only you can fix. The PR description still says the residual risk "matches the previous flat-object behavior." It does not, and that sentence is the one most likely to stop the next reviewer from checking the narration case. I cannot edit the PR body. Verification, stated plainly. The own-line rule was validated against eight inputs I picked myself — six that must parse, four that must not. That is not a corpus, and I would not describe it as proof. Two gaps I know about: options listed on separate lines still both execute, and a genuine call written mid-sentence is now dropped. CI. I have kept these notes here rather than duplicating them on #2678 and #2683, since this is the PR carrying a design decision. The same "no approval, author has not reviewed the pushed commits" applies to both of those. |
Summary
Motivation
Local and OpenAI-compatible model backends can emit a tool call in ordinary message content
instead of the structured
tool_callsfield. The existing bare-JSON fallback matched onlyobjects without nested braces, so a valid call such as a
clickwith nested metadata wasignored and could surface as final assistant text instead of executing.
Design
The fallback now scans the already bounded model text for balanced JSON objects while respecting
quoted strings and escapes. Each candidate still has to parse as JSON and pass the existing tool
allowlist. Wrapped JSON, XML,
call:name{}handling, the 10,000-character cap, and the OpenAI-stylefallback output shape are unchanged.
The scanner is used only when the earlier wrapped/XML parsers did not find a call. A greedy regex
was rejected because it can merge adjacent calls, and parsing the entire response was rejected
because models commonly include prose around tool-call JSON.
Testing
node test/security/injection-corpus.mjs— 60/60 passednode --check src/chrome/src/agent/tool-call-parser.js— passednode --check src/firefox/src/agent/tool-call-parser.js— passednode test/run.js— 1,449 passed; one pre-existing repository-version assertion fails becausepackage.jsonis26.0.10while the newestCHANGELOG.mdentry is26.0.0git diff --check— passedCompatibility and risks
No public API or provider behavior changes for already supported formats. The parser remains
bounded and allowlisted. The main residual risk is accepting a valid allowlisted JSON object
embedded in explanatory model text; that is the intended purpose of this fallback and matches
the previous flat-object behavior.
Scope
This does not add new tool-call syntaxes, change provider normalization, or relax parser limits.