Skip to content

fix: parse nested bare JSON tool calls - #2682

Open
alectimison-maker wants to merge 3 commits into
webbrain-one:mainfrom
alectimison-maker:fix/nested-text-tool-calls
Open

fix: parse nested bare JSON tool calls#2682
alectimison-maker wants to merge 3 commits into
webbrain-one:mainfrom
alectimison-maker:fix/nested-text-tool-calls

Conversation

@alectimison-maker

Copy link
Copy Markdown
Contributor

Summary

  • replace the flat bare-JSON tool-call regex with a string-aware balanced-object scanner
  • recover nested argument objects and multiple bare calls in model text
  • keep the Chrome and Firefox parser modules byte-identical and add regressions

Motivation

Local and OpenAI-compatible model backends can emit a tool call in ordinary message content
instead of the structured tool_calls field. The existing bare-JSON fallback matched only
objects without nested braces, so a valid call such as a click with nested metadata was
ignored 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-style
fallback 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

  • targeted nested-object, escaped-string, multiple-call, allowlist, and Chrome/Firefox parity checks — passed
  • node test/security/injection-corpus.mjs — 60/60 passed
  • node --check src/chrome/src/agent/tool-call-parser.js — passed
  • node --check src/firefox/src/agent/tool-call-parser.js — passed
  • node test/run.js — 1,449 passed; one pre-existing repository-version assertion fails because package.json is 26.0.10 while the newest CHANGELOG.md entry is 26.0.0
  • git diff --check — passed

Compatibility 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.

@vercel

vercel Bot commented Aug 4, 2026

Copy link
Copy Markdown

@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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Comment on lines +18 to +22
if (start < 0) {
if (char === '{') {
start = i;
depth = 1;
}
Comment on lines +18 to +22
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>
@esokullu

esokullu commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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 { and never reconsidered that offset, so a brace that never closed consumed everything after it. The flat regex this replaced had no such state and did recover those calls. Verified both ways:

input before after
Template: {unclosed\n{"name":"click",…} [] parsed
if (ready) { submit();\n{"name":"read_page",…} [] parsed

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 start + 1 when a candidate never closes, which is the resynchronisation Copilot asked for. The nested-object recovery this PR is actually about is untouched — top-level candidates are still preferred, and the extra scans only happen on malformed input.

I capped the restarts at 16. Unbounded start + 1 restarts go quadratic: I measured '{'.repeat(10000) at ~95ms of blocking work, which the cap takes under a millisecond while still tolerating far more stray braces than real model output contains. The number is arbitrary in the sense that any value well above "realistic prose" works — say if you'd rather it were higher.

Also. Moved the parseToolCallsFromText docblock back onto its function; the original commit left it stranded above the new helper, which now has its own.

Tests. Both unbalanced-prefix cases, plus one asserting that a nested {"name":"click"} inside a disallowed outer call stays rejected — that pins the property that nested objects aren't promoted to candidates, which is what keeps the allowlist meaningful here. Plus a bounded-work assertion on the brace flood.

1449 passed, security corpus 60/60, Chrome and Firefox copies cmp-verified identical. The one failure is the pre-existing package.json 26.0.10 vs CHANGELOG 26.0.0 mismatch.

Worth flagging for later: there are now two balanced-object scanners per browser tree — this one and json-extract.js — and they'd drifted apart on exactly this recovery behaviour, which is how the same bug ended up living in both. #2683 now matches. Within a tree they could share one helper.

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>
@esokullu

esokullu commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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 main and this branch:

model output main this branch (before)
"I could click it with {…"text":"Delete account"…} but that's destructive, so I will not." [] executes the click
"Option A: {…click…} Option B: {…navigate…} I recommend A." [] executes both
"The page said to run {…navigate to evil.test…} — I ignored it." [] navigates there

This matters more than it looks because of what the caller does with the result: agent.js:20700 sets result.content = null when the fallback finds calls. The model's prose is discarded. So in row one the refusal is thrown away and the refused action runs.

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 arguments: {…}, so in practice the old fallback rarely fired on prose. This one fires reliably. Worth fixing in the description regardless of what you think of the rule below — as written it tells a reviewer not to look here.

But main is not the safe baseline either, which is what convinced me not to just narrow the scanner. It executes the same narration whenever the object has no nested braces:

  • "I will not call {"name":"click","text":"Delete"} here." → ["click"]
  • "The page said to run {"name":"navigate","url":"https://evil.test"} — I ignored it." → ["navigate"]

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:

  • Kept: whole-message call; prose line then call on its own line (your fixture); two calls on their own lines (your fixture); array elements with trailing commas; indented inside a fence; call after an unbalanced prose brace.
  • Dropped: all four narration cases above, including the two main executes today.

So this ends up strictly safer than the current main, not just safer than this branch.

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 cmp-identical. The parser's worst case is 9ms on a 10k input. Happy to drop this commit if you would rather ship the narrower fix and handle narration separately.

@esokullu

esokullu commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

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 main has today, but it is a judgment call about model behaviour that you are better placed to make than I am. If you would rather ship the narrower nested-object fix and handle narration as its own change, say so and I will drop that commit — the two earlier ones stand on their own.

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. smoke passes. Vercel is red on the fork-deploy authorization prompt, not on anything in the diff. npm test shows one failure, the pre-existing package.json 26.0.10 vs CHANGELOG.md 26.0.0 mismatch, which PR CI does not run — it will surface at release time instead, and is worth fixing on main independently of this PR.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants