Skip to content

fix: accept markdown-fenced JSON in validate_json_format#1768

Open
Chessing234 wants to merge 2 commits into
allenai:mainfrom
Chessing234:fix/json-format-markdown-fences
Open

fix: accept markdown-fenced JSON in validate_json_format#1768
Chessing234 wants to merge 2 commits into
allenai:mainfrom
Chessing234:fix/json-format-markdown-fences

Conversation

@Chessing234

@Chessing234 Chessing234 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Strip json / fences before json.loads, matching IFEvalG.

Test plan

  • Unit tests for bare and fenced JSON
  • GPU_TESTS=bypass

Chessing234 and others added 2 commits July 18, 2026 19:00
IFEval allows ```json fences; strip them before json.loads.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request updates the validate_json_format function in open_instruct/if_functions.py to strip markdown code fences before parsing JSON, and adds comprehensive unit tests in open_instruct/test_if_functions.py. The reviewer feedback suggests replacing the chained .removeprefix() and .removesuffix() calls with a robust, case-insensitive regular expression to better handle mixed-case language identifiers (e.g., jSoN) and avoid potential parsing failures.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +388 to +396
value = (
text.strip()
.removeprefix("```json")
.removeprefix("```Json")
.removeprefix("```JSON")
.removeprefix("```")
.removesuffix("```")
.strip()
)

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.

medium

Using chained .removeprefix() and .removesuffix() calls can be fragile and lead to bugs. For example, if the model outputs a mixed-case identifier like jSoN, the specific prefixes (json, Json, JSON) won't match, but the fallback .removeprefix() for ```` will match and strip only the backticks, leaving the malformed string jSoN\n{"a": 1} which fails JSON parsing.\n\nUsing a single case-insensitive regular expression is much more robust, cleaner, and handles any whitespace or casing variations automatically.

    match = re.match(r"^```(?:json)?\s*(.*?)\s*```$", text.strip(), re.DOTALL | re.IGNORECASE)
    value = match.group(1) if match else text.strip()

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.

1 participant