Fix DirtyJson dropping array elements after an unquoted value - #1756
Open
Osamaali313 wants to merge 1 commit into
Open
Fix DirtyJson dropping array elements after an unquoted value#1756Osamaali313 wants to merge 1 commit into
Osamaali313 wants to merge 1 commit into
Conversation
_parse_unquoted_string stops the scan on the delimiter that ends the
bareword (`,`, `]`, `}`, `:`) but then unconditionally advanced one more
character, consuming that delimiter. In _parse_array_content the comma is
therefore already gone by the time the loop checks for it, so parsing
falls into the `elif self.current_char != "]"` branch and terminates the
array after the first unquoted element:
DirtyJson.parse_string("[foo, bar, baz]") # -> ['foo']
DirtyJson.parse_string('[foo, "bar"]') # -> ['foo']
Since DirtyJson is the repair layer for malformed model tool-call output,
any model emitting a bareword array element silently loses the remaining
arguments. Dropping the extra advance leaves the cursor on the delimiter
so the array/object loops handle it, and existing object recovery is
unaffected.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a DirtyJson parsing bug where arrays were truncated after the first bareword (unquoted) element because _parse_unquoted_string() consumed the delimiter (e.g., , or ]) that the array loop relies on.
Changes:
- Stop
_parse_unquoted_string()from advancing past the terminating delimiter so container parsers (_parse_array_content,_parse_object_content) can handle delimiters consistently. - Add a parametrized regression test covering unquoted array elements (leading, trailing, mixed quoted/unquoted, and single-element).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| helpers/dirty_json.py | Removes the extra cursor advance so unquoted values do not consume ,/]/} delimiters, preventing array truncation. |
| tests/test_dirty_json.py | Adds regression coverage to ensure all unquoted array elements are preserved. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DirtyJsonsilently drops every array element after the first unquoted (bareword) value:Arrays whose first element is quoted work by luck (
'["foo", bar]'->['foo', 'bar']), which hides the bug in most tests.Root cause
_parse_unquoted_stringstops its scan on the delimiter that ends the bareword (,,],},:), then runs an unconditionalself._advance()that consumes that delimiter:Back in
_parse_array_content, theif self.current_char == ","check then never sees the comma (it was already consumed), so control falls intoelif self.current_char != "]", which pops the stack and ends the array after the first bareword. Objects only survive because_parse_object_contenthas a fallthrough that re-enters.Fix
Drop the extra advance so the cursor is left on the delimiter, letting the array/object loops consume it as they already do for quoted values:
Impact
DirtyJsonis the repair layer for malformed model tool-call / response JSON (helpers/extract_tools.py,agent.py,mcp_handler.py, memory plugins). Models that emit bareword array elements — common outside strict OpenAI-style output — silently lost their remaining tool arguments. This is data loss, not a crash, so it went unnoticed.Tests
Added a parametrized
test_unquoted_array_elements_are_all_keptcovering leading/trailing/mixed bareword elements and the single-element case.All 7 pre-existing
dirty_jsontests still pass; object, number, and quoted-array parsing are unchanged.