fix(llm): recover JSON from reasoning-first model replies (#2882) - #2890
fix(llm): recover JSON from reasoning-first model replies (#2882)#2890rajarshidattapy wants to merge 3 commits into
Conversation
…abs#2882) Models such as nvidia/nemotron-* and gemma narrate before they answer, and `_parse_llm_json` dropped the whole chunk when they did: [graphify] LLM returned invalid JSON, skipping chunk (first 200 chars: 'Let me analyze the provided source files to extract a knowledge graph...') Three separate weaknesses, all hit by that shape: - The fence handler took the FIRST ``` anywhere in the text and cut at the LAST one, then OVERWROTE the working text with the result. A narration that opens a fence of its own (```text, ```python) therefore mangled the answer's ```json block before it was ever parsed. Now every fenced block is tried as a candidate against the original text, JSON-tagged and untagged fences first. - The balanced-brace fallback tried only the first `{` in the text and gave up on the first candidate that failed to parse. Narration is full of braces, so the answer sitting further down was never reached. Each plausible start is tried now, prioritized by whether an extraction key appears nearby and bounded so a pathological reply cannot make recovery quadratic. - An object that parses but carries none of nodes/edges/hyperedges — a restated schema, for instance — used to win simply by coming first. It is now kept only as a last resort, so the narration cannot shadow the answer that follows it. Also strip <think>/<thinking>/<reasoning> blocks up front, which is where deepseek-r1 and qwq put their chain of thought. A reply with no answer anywhere still degrades to the empty fragment, so the hollow detector takes over exactly as before.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Rewrites _parse_llm_json to strip <think>/reasoning blocks and recover the extraction fragment through layered strategies — bare object, every fenced block, then balanced-object candidates ranked by proximity to nodes/edges/hyperedges keys — each run against the original text so narration can't shadow the real answer. Adds helpers _balanced_object and _json_object_candidates (capped at _MAX_OBJECT_CANDIDATES) and keeps a keyless-but-valid object only as a last-resort fallback. Adds tests/test_llm_parser_reasoning.py covering reasoning-first models that narrate, double-fence, or emit decoy JSON objects before answering.
No blocking issues surfaced. 8 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 725 functions depend on the 177 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
extract_corpus_parallel()— 26 callers, 10 callees - new:
dispatch_command()— 2 callers, 117 callees - new:
_call_llm()— 11 callers, 18 callees - new:
_call_openai_compat()— 23 callers, 8 callees - …and 16 more — each is listed as a finding
Verification — 725 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 456 function(s) in the blast radius were not formally verified this run
Formal verification
No difference found (not proven): No behavior difference found in \_parse\_llm\_json (not a proof).
The verifier ran both versions of \_parse\_llm\_json on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 1 grounded finding(s) anchored inline below; 23 more finding(s) on lines outside this diff (see the check run).
| return preferred + rest | ||
|
|
||
|
|
||
| def _parse_llm_json(raw: str) -> dict: |
There was a problem hiding this comment.
_parse_llm_json()
22 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Review flagged _parse_llm_json as a health regression on afferent coupling. The coupling itself is not new — 5 direct callers, all the `_call_*` backends, and the signature and contract are unchanged (str in, fragment dict out). What did change is that a function everything depends on grew three inline recovery strategies and a closure over a `fallback` cell. Lift the search half into `_json_fragment_candidates()`, a generator that yields fenced blocks then balanced objects, most-likely first. What is left is a flat loop: parse each candidate, take the first that carries the extraction keys, keep a keyless object only as a last resort. The closure and its `nonlocal` are gone and the function is 88 lines down to 58. Behaviour is identical — same candidate order, same preference rule, same empty-fragment degradation. All 201 parser and backend tests pass unchanged.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Rewrites _parse_llm_json to survive reasoning-first models that narrate before answering: strips <think> blocks, scans every fenced block and every balanced {...} object (not just the first fence/brace), and prefers candidates carrying nodes/edges/hyperedges keys so schema restatements in the preamble can't shadow the real answer. Adds helpers _balanced_object, _json_object_candidates, and _json_fragment_candidates with a candidate cap to keep recovery from going quadratic on brace-heavy replies. Adds tests/test_llm_parser_reasoning.py covering CoT preambles, narration fences, <think> stripping, and prose objects (#2882).
Worth a look
- Fence content is preferred even when a non-fragment fence precedes a bare JSON answer with fragment keys —
graphify/llm.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 726 functions depend on the 178 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
extract_corpus_parallel()— 26 callers, 10 callees - new:
dispatch_command()— 2 callers, 117 callees - new:
_call_llm()— 11 callers, 18 callees - new:
_call_openai_compat()— 23 callers, 8 callees - …and 15 more — each is listed as a finding
Verification — 726 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 457 function(s) in the blast radius were not formally verified this run
Formal verification
No difference found (not proven): No behavior difference found in \_parse\_llm\_json (not a proof).
The verifier ran both versions of \_parse\_llm\_json on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 23 more finding(s) on lines outside this diff (see the check run).
Review raised fence-vs-bare ordering. The case as worded is already handled
— the fragment-key preference crosses the fence/brace boundary, so a
non-fragment fence does not beat a bare answer below it, verified against
five orderings. But probing it turned up a real one next door.
A model that restates the required shape before answering —
The schema is:
```json
{"nodes": [], "edges": []}
```
{"nodes": [{"id": "real"}], "edges": []}
produces a candidate that carries the extraction keys and no content.
Preferring any candidate that merely HAS those keys handed the restatement
the win, which is the same shadowing this change exists to prevent, one
level deeper than a keyless prose object.
Add a tier: keys with content wins outright; keys without content is held
as second best; a non-fragment object stays the weakest fallback. A
genuinely empty extraction is still returned — the model looked and found
nothing is a valid answer, and it must keep reading as an empty fragment so
the hollow detector takes over.
Four tests, two of which fail without the change.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Rewrites _parse_llm_json to survive reasoning-first model replies: strips <think> blocks, then tries the whole reply plus every fenced block and balanced {...} object via new helpers (_json_fragment_candidates, _json_object_candidates, _balanced_object), preferring candidates that carry nodes/edges/hyperedges over schema restatements and non-fragment objects. Fixes #2882, where a brace in the narration or a preamble fence caused the real answer to be dropped and the chunk skipped. Adds tests/test_llm_parser_reasoning.py covering the preamble, narration-fence, and think-block cases.
Worth a look
- Fenced-block preamble JSON can shadow the real answer via fence ordering —
graphify/llm.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 733 functions depend on the 185 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
extract_corpus_parallel()— 26 callers, 10 callees - new:
dispatch_command()— 2 callers, 117 callees - new:
_call_llm()— 11 callers, 18 callees - new:
_call_openai_compat()— 23 callers, 8 callees - …and 15 more — each is listed as a finding
Verification — 733 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 464 function(s) in the blast radius were not formally verified this run
Formal verification
No difference found (not proven): No behavior difference found in \_parse\_llm\_json (not a proof).
The verifier ran both versions of \_parse\_llm\_json on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 23 more finding(s) on lines outside this diff (see the check run).
|
Shipped in v0.9.48 via authorship-preserving cherry-pick, plus a senior tweak: the winner gate now runs on sanitized content so a bare-string id sketch cannot shadow the real answer. Thanks @rajarshidattapy! Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.48 |
2 participants
Fixes #2882.
What was happening
nvidia/nemotron-*(and gemma, and any reasoning-first model) narrates before it answers._parse_llm_jsondropped the entire chunk when it did:The parser already had a fence-stripper and a balanced-brace fallback, so the JSON was often right there in the response. Three separate weaknesses stopped it being found — each one is enough on its own to lose the chunk:
1. The fence handler was destructive and picked the wrong fence. It took the first
```anywhere in the text, cut at the last one, and then overwrote the working text with the result. A narration that opens a fence of its own —```text,```python— mangled the answer's```jsonblock before it was ever parsed. Every fenced block is now tried as a candidate against the original text, JSON-tagged and untagged fences first.2. The balanced-brace fallback tried exactly one candidate. It scanned to the first
{and, if that object failed to parse,break— done. Narration is full of braces ("The shape I must emit is { ... }", bullet lists, inline examples), so the answer further down was never reached. Every plausible start is tried now.3. A prose object could shadow the answer. A restated schema like
{"description": "graph fragment"}parses fine and comes first. An object carrying none ofnodes/edges/hyperedgesis now kept only as a last resort.Plus:
<think>/<thinking>/<reasoning>blocks are stripped up front, which is where deepseek-r1 and qwq put their chain of thought — prose that is nothing but braces to a brace scanner.Bounded, not just tolerant
Candidate starts are prioritized by whether an extraction key appears within 200 characters, and the likely/unlikely lists are capped separately at 64. That matters: a single shared cap would let a wall of noise braces crowd out an answer that comes after it, which is exactly the pathological case a cap is supposed to protect against. Covered by
test_candidate_scan_is_bounded.Unchanged behaviour
A reply with genuinely no JSON anywhere still returns the empty fragment and logs the same line, so
_response_is_hollowtakes over exactly as before. The 10 MB_LLM_JSON_MAX_BYTESguard is untouched.Tests
tests/test_llm_parser_reasoning.py— the two verbatim response shapes from the issue, a<think>-block reply, the schema-shadowing case, the bounded scan, and the graceful-degradation case. Five of the six fail without the change. The existing 107 parser/backend tests pass unchanged, includingtest_preamble_then_fence_is_parsedandtest_fence_without_closing_backticks.Related
The same responses were also being fed into adaptive retry as
finish_reason="length"and bisected — visible in the issue's log aschunk of 19 truncated at depth 2, splitting into halves of 9 and 10, which is #2880. This PR makes the response parse in the first place; #2880 stops the ones that still cannot parse from costing 15 calls. They are independent and compose.