fix(converter): don't let trailing braces corrupt valid JSON extraction#6521
fix(converter): don't let trailing braces corrupt valid JSON extraction#6521ErenAta16 wants to merge 2 commits into
Conversation
_JSON_PATTERN = re.compile(r"({.*})", re.DOTALL) greedily matched from
the first '{' to the LAST '}' anywhere in the string. If the LLM's
output contained a complete, valid JSON object followed by any
commentary that itself referenced curly braces (e.g. mentioning the
schema, a code snippet, or a set literal), the match swallowed that
trailing text too and json.loads failed on the resulting garbage, even
though the actual JSON object was perfectly well formed.
Replaced the regex-based extraction with json.JSONDecoder.raw_decode
starting at the first '{': it parses exactly one JSON value and stops
there regardless of what follows, so trailing braces elsewhere in the
string can no longer corrupt an otherwise-valid match. This preserves
the existing fall-through behavior for brace-delimited non-JSON
content (e.g. GraphQL schemas, fixed by crewAIInc#5461/crewAIInc#5460) since raw_decode
still raises JSONDecodeError on those.
Applied to both handle_partial_json and async_handle_partial_json,
which shared the same pattern. Also drops the now-unused re import.
Verified against the existing test suite (51/52 passing; the one
failure is a pre-existing, unrelated missing-litellm-dependency issue
in my sandbox, reproduced identically on unpatched main).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesPartial JSON Parsing
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai/tests/utilities/test_converter.py (1)
228-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding an async equivalent test.
Both
handle_partial_jsonandasync_handle_partial_jsonnow share_extract_first_json_object, so the extraction logic is covered. However, there's no async test for the trailing-commentary scenario. Adding one would provide symmetric coverage for the async path, which the PR objectives explicitly mention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/utilities/test_converter.py` around lines 228 - 242, Add an async counterpart to test_handle_partial_json_ignores_braces_in_trailing_commentary, using the same JSON input with brace-containing trailing commentary and awaiting async_handle_partial_json. Assert the returned value is a SimpleModel with the expected name and age to provide equivalent coverage for the async path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/crewai/tests/utilities/test_converter.py`:
- Around line 228-242: Add an async counterpart to
test_handle_partial_json_ignores_braces_in_trailing_commentary, using the same
JSON input with brace-containing trailing commentary and awaiting
async_handle_partial_json. Assert the returned value is a SimpleModel with the
expected name and age to provide equivalent coverage for the async path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ecb18e7-4a39-4ab6-abfe-3bfed1d78a33
📒 Files selected for processing (2)
lib/crewai/src/crewai/utilities/converter.pylib/crewai/tests/utilities/test_converter.py
Addresses CodeRabbit nitpick on crewAIInc#6521: async_handle_partial_json shares _extract_first_json_object with the sync path, so it should get the same regression coverage for the trailing-commentary bug.
|
Fair point - added test_async_handle_partial_json_ignores_braces_in_trailing_commentary alongside the sync one, since both paths share _extract_first_json_object. Full suite still at 52/53 (same pre-existing unrelated litellm-dependency failure as before). |
_JSON_PATTERN = re.compile(r"({.*})", re.DOTALL)inconverter.pygreedily matches from the first{to the last}anywhere in the string. If the LLM's output contains a complete, valid JSON object followed by commentary that itself references curly braces - mentioning the schema, a code snippet, a set literal, anything - the match swallows that trailing text too, andjson.loadsfails on the resulting garbage even though the actual JSON object was perfectly well formed:In
handle_partial_json, thatJSONDecodeErrortriggers an immediate fall through toconvert_with_instructions(a re-prompt), discarding output that was already correct.This is a different failure mode from #5460 (closed via #5461), which was about the regex false-positively matching brace-delimited text that isn't JSON at all (e.g. GraphQL schemas). This one is a false-negative: genuinely valid JSON gets corrupted because of what comes after it.
Fix
Replaced the regex-based extraction with
json.JSONDecoder(strict=False).raw_decode(result, start), starting at the first{.raw_decodeparses exactly one JSON value and stops there, so trailing braces elsewhere in the string can't corrupt an otherwise-valid match. The GraphQL fall-through from #5461 is preserved, sinceraw_decodestill raisesJSONDecodeErroron non-JSON brace-delimited content for the same reasonjson.loadsdid.Applied to both
handle_partial_jsonandasync_handle_partial_json, which shared the identical pattern. Also drops the now-unusedreimport and the_JSON_PATTERNconstant.Testing
Added
test_handle_partial_json_ignores_braces_in_trailing_commentarytotests/utilities/test_converter.py, covering the exact repro above.Ran the full
tests/utilities/test_converter.pysuite: 51/52 passing. The one failure (test_supports_function_calling_false) is pre-existing and unrelated - it needs the optionallitellmpackage, which isn't installed in my sandbox, and I confirmed it fails identically on unpatchedmainbefore making any changes.I did try a second test with brace-containing text on both sides of the target object, expecting it to also resolve correctly - it doesn't, because
raw_decodestarting at the first{will happily parse an earlier, self-contained JSON-looking object if one exists before the real one. That's a real limitation, but a different (arguably harder - which object is "the" answer?) problem than what this PR fixes, so I removed that test rather than either overclaiming or scope-creeping the fix. Happy to discuss if that's worth a follow-up.