fix(agent): Autofix trailing commas in LLM-generated JSON#619
fix(agent): Autofix trailing commas in LLM-generated JSON#619selamw1 wants to merge 4 commits intogoogle:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request aims to fix JSON decoding errors from LLM-generated content by removing trailing commas. The approach of using a regular expression is sound. However, the implemented regex is not fully robust and can fail in cases of multiple trailing commas. I've suggested a more robust regex that handles these cases correctly.
| raise ValueError("Cleaned JSON string is empty.") | ||
|
|
||
| # Autofix: Remove trailing commas from arrays and objects | ||
| json_string_cleaned = re.sub(r",\s*([\]}])", r"\1", json_string_cleaned) |
There was a problem hiding this comment.
The current regex only handles a single trailing comma. It would fail to fix a string with multiple trailing commas, like [1, 2,,], which would be converted to the still-invalid [1, 2,]. Using a positive lookahead is more robust and handles multiple trailing commas correctly in a single pass.
| json_string_cleaned = re.sub(r",\s*([\]}])", r"\1", json_string_cleaned) | |
| json_string_cleaned = re.sub(r",(?=\s*[\]}])", "", json_string_cleaned) |
b2afa00 to
b08a1da
Compare
| raise ValueError("Cleaned JSON string is empty.") | ||
|
|
||
| # Autofix: Remove trailing commas from arrays and objects | ||
| json_string_cleaned = re.sub(r",(?=\s*[\]}])", "", json_string_cleaned) |
There was a problem hiding this comment.
thanks for this
can you put it in the SDK here:
https://github.com/google/A2UI/blob/main/a2a_agents/python/a2ui_agent/src/a2ui/extension/send_a2ui_to_client_toolset.py#L265
and add some tests
I think flow would be
if not jsonschema.validate():
run fixers
if trailing comma fixer does something
emit warning
jsonschema.validate
There was a problem hiding this comment.
Thanks, I've adjusted the code into the SDK and followed the suggested flow. I also added unit tests to verify the fix and the warning message.
91a7032 to
e259644
Compare
0253f9a to
a87a01b
Compare
wrenj
left a comment
There was a problem hiding this comment.
thanks looks good, just please add bck in the list wrapping logic
|
|
||
| # Auto-wrap single object in list | ||
| if not isinstance(a2ui_json_payload, list): | ||
| logger.info( |
There was a problem hiding this comment.
lets keep the list wrapping logic
|
wrenj's approval is correct — just needs the list wrapping restored. The regex handles multiple trailing commas fine (each Once the auto-wrap logic is back, this is good to merge. 👍 |
revert sample-agentand add fix to adk-agent for malformed JSON Add robust JSON autofix and tests Add robust JSON autofix logic Implement robust JSON autofix with tests
ad45351 to
e99612c
Compare
|
Thanks! preserved the list wrapping logic in both paths. |
Fixes
json.JSONDecodeErrorcaused by LLMs returning JSON with trailing commas.Changes
RestaurantAgentto strip trailing commas before callingjson.loads().Fixes
Resolves #480