fix(parser): recover tool calls that have a trailing comma - #469
Conversation
The embedded-JSON scanner advanced a single byte after a balanced candidate
failed to parse, so it resumed scanning inside the object it had just
rejected. A trailing comma — a common LLM JSON quirk — therefore made
`{"tool":"set_timer","arguments":{"seconds":300},}` fall through to the inner
`{"seconds":300}` fragment, which the single-key normalizer turned into a
phantom tool named "seconds". The array form dropped every call after the
first.
When a balanced top-level candidate fails to parse, retry once after removing
trailing commas with a string-aware pass (commas inside string values are
preserved). The malformed call is recovered as the correct tool instead of a
sub-fragment; valid JSON is returned unchanged on the first attempt.
matedev01
left a comment
There was a problem hiding this comment.
Approve ✅
Clean, correct recovery. strip_trailing_commas is properly string- and escape-aware: " toggles in_string, a \ inside a string sets escape so \" doesn't close the string, and a comma is only dropped when its next non-whitespace char is }/] outside a string. It's also conservative — the repaired candidate is only returned when stripping actually changed something and the result parses, so it can't "recover" into a different call. And it's pure additions, no churn in the existing scan path.
The tests are on point: trailing comma in an object, trailing comma in an array (the "drops every call after the first" case), and the important one — "a, b,}" proving in-string commas and braces survive untouched — plus the valid-JSON-untouched guard.
Verified locally (pure parser string logic, no arch sensitivity): cargo test -p genie-core --lib tools::parser → 28 passed (the 4 new + existing, no regression).
One tiny non-blocking idea: a case with an escaped quote next to a trailing comma (e.g. {"x":"a\",}",}) would explicitly pin the escape handling — the code already gets it right, this would just lock it.
LGTM — needs the catch-up with main (currently behind).
Summary
The tool-call parser mis-handled a trailing comma — one of the most common malformed-JSON quirks local models emit.
{"tool":"set_timer","arguments":{"seconds":300},}was parsed as a phantom tool named"seconds", and a trailing comma in an array dropped every call after the first. This recovers the correct call instead.Changes
extract_embedded_jsonadvanced by a single byte after a balanced candidate failedserde_jsonvalidation, so it resumed scanning inside the object it had just rejected — and returned an inner fragment of the real call.extract_balanced_json_candidatenow retries once after stripping trailing commas via a string-aware pass (strip_trailing_commas), so commas inside string values are preserved. Valid JSON still returns unchanged on the first attempt; genuinely-invalid JSON (e.g.{"seconds":60*60*12}, issue fix(tools): don't leak unparsed tool-call JSON to the user (closes #378) #380) still returnsNoneand is caught byis_unparsed_tool_call.Real Behavior Proof
Tested profile / hardware (check all that apply):
laptopThis is a deterministic, hardware-independent change to
tools/parser.rs(the shared tool-call extraction used by both the runtime path and the BFCL eval path). No Jetson, audio, model, or network dependency — an x86_64 Linux dev box exercises the exact code path that runs on-device. Because the same parser backsparse_tool_calls_for_eval, this directly affects BFCL scoring fidelity: a trailing-comma response that previously scored as a wrong/missing call now scores as the correct call.What I ran
On x86_64 Linux (Ubuntu, kernel 6.8), rustc 1.96.0:
I added the four regression tests first and confirmed three of them fail on the current code (proving the bug) before applying the fix.
What I observed
extract_recovers_trailing_comma_object:left: "seconds"vsright: "set_timer".extract_recovers_trailing_comma_array:left: 1vsright: 2(second call dropped).extract_trailing_comma_recovery_preserves_string_commas: panicked (no JSON extracted).cargo test -p genie-core— 734 passed, 0 failed (plus integration/doc suites green). The parser module: 21 passed.extract_leaves_valid_json_untouchedconfirms valid JSON is returned byte-for-byte unchanged (no behavior change on the happy path).clippyclean in both default and--no-default-features;cargo fmt --checkclean.Test plan
cargo test -p genie-core --lib tools::parser— all 21 pass, including the four newextract_*tests.{"tool":"set_timer","arguments":{"seconds":300},}throughparse_tool_calls_for_evaland confirm oneset_timercall withseconds = 300(not a"seconds"tool).