Skip to content

fix(llm): recover malformed JSON in the openai-compatible provider (#3683) - #3685

Open
chiruu12 wants to merge 3 commits into
vectorize-io:mainfrom
chiruu12:fix/3683-openai-compatible-parse-llm-json
Open

fix(llm): recover malformed JSON in the openai-compatible provider (#3683)#3685
chiruu12 wants to merge 3 commits into
vectorize-io:mainfrom
chiruu12:fix/3683-openai-compatible-parse-llm-json

Conversation

@chiruu12

Copy link
Copy Markdown

Fixes #3683.

openai_compatible_llm.py parsed structured output with bare json.loads and, on failure, retried by re-sending a byte-identical request. parse_llm_json was never reached, so a recoverable response was never repaired and a deterministic one cost max_retries + 1 full generations before the chunk was dropped.

What changed

Both parse sites in the file now fall back to parse_llm_json once a fresh generation can no longer help: the chat.completions path and the native Ollama /api/chat path. That mirrors what litellm_llm.py has done since #2547/#2544, including its preference for a clean re-roll before repair.

The retry ladder also stops early when a parse failure repeats at the same (msg, lineno, colno). Every attempt re-sends the same request, so a failure at the same position will keep repeating. Two attempts establish that, and the rest of the budget goes to repair instead of more generations. A failure at a new position still earns a fresh generation, so flaky output keeps the behaviour it has today.

Tests

Added to tests/test_openai_compatible_response_hardening.py. All mocked, no network:

  • a repairable response is recovered instead of dropped
  • an identical repeat stops re-rolling after 2 attempts instead of 4
  • a failure at a new position still re-rolls
  • the same recovery on the native Ollama path

Three of those fail on current main. The fourth pins the re-roll behaviour this change keeps.

ruff check, ruff format --check and ty check are clean. 295 passed and 5 skipped across every test file that touches the provider.

Not here

The issue also suggests feeding the invalid output back as a repair turn, and a per-document dropped-chunk counter on the retain response. Both change behaviour past parsing, so I left them on the issue.

Copilot AI lite review requested due to automatic review settings August 20, 2026 20:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@r266-tech r266-tech 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.

At head SHA 65161aa3e21fc5f3dba3d95b1bc8d3a67181a86a, the retry short-circuit treats any two failures with the same JSONDecodeError message, line, and column as a deterministic repeat without checking that the response text is unchanged. Two different stochastic outputs can therefore be classified as a deterministic repeat and trigger parse_llm_json on the second attempt; if repair cannot recover that output, the exception exits the retry loop and skips the remaining generation budget. A later valid response is therefore dropped instead of being attempted.

Please only short-circuit when the failed response content is also identical, or otherwise retain a retry when content differs, and add a regression test with two distinct responses sharing the same JSONDecodeError signature followed by a valid response.

The bound verification observed head SHA 65161aa3e21fc5f3dba3d95b1bc8d3a67181a86a, and git diff --check passed. The focused pytest command was not independently run because uv was denied permission to create its cache directory in the read-only environment.

@chiruu12

Copy link
Copy Markdown
Author

Right, that holds. Two different malformed responses can share a JSONDecodeError message, line and column, so the check called them a repeat and spent the rest of the retry budget on repair instead of on the generation that would have worked.

The tracker now holds the response content, and the short-circuit only fires when the text comes back unchanged. Identical content produces an identical decode error, so the position fields carried nothing the content did not already carry. The dataclass is gone with them.

Added test_two_responses_failing_alike_are_not_treated_as_one: {"aa": 1,} then {"bb": 2,}, both failing at line 1 column 10, then a valid response. Fails on 65161aa, passes now. 369 passed and 5 skipped across the 26 test files that touch the provider.

@koriyoshi2041 koriyoshi2041 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.

Verified the update at 2927e8e164aa6c33fcf06aefa1e80d4ffc0a1fc9. The repeat check now keys on response content in both compatible and native Ollama paths, so distinct malformed outputs with the same decode position retain the remaining generation attempt. The focused response-hardening suite passes 13/13; Ruff check/format and the exact update diff check are clean.

@koriyoshi2041

Copy link
Copy Markdown
Contributor

Verified the update at 2927e8e164aa6c33fcf06aefa1e80d4ffc0a1fc9. The repeat check now keys on response content in both compatible and native Ollama paths, so distinct malformed outputs with the same decode position retain the remaining generation attempt. The focused response-hardening suite passes 13/13; Ruff check/format and the exact update diff check are clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants