mirror of
https://github.com/safishamsi/graphify.git
synced 2026-07-22 07:11:47 +00:00
Merge pull request #1063 from christophepub/fix/claude-cli-hollow-responses-v2
Fix hollow-response loop in claude-cli backend (re-targeted to v8, clean diff, with tests)
This commit is contained in:
Safi
+79
-15
@@ -194,16 +194,63 @@ def _parse_llm_json(raw: str) -> dict:
|
||||
file=sys.stderr,
|
||||
)
|
||||
return {"nodes": [], "edges": [], "hyperedges": []}
|
||||
if raw.startswith("```"):
|
||||
raw = raw.split("```", 2)[1]
|
||||
if raw.startswith("json"):
|
||||
raw = raw[4:]
|
||||
raw = raw.rsplit("```", 1)[0]
|
||||
# Strategy 1: strip whitespace, then handle markdown fences anywhere in the
|
||||
# text (not only at offset 0 — the original code only stripped fences when
|
||||
# `raw.startswith("```")`, missing the common case where Claude prepends a
|
||||
# preamble like "Here's the extracted entities:\n\n```json\n{...}\n```").
|
||||
stripped = raw.strip()
|
||||
fence_start = stripped.find("```")
|
||||
if fence_start != -1:
|
||||
after_fence = stripped[fence_start + 3 :]
|
||||
# Optional language tag (json, JSON, javascript, etc.) up to newline.
|
||||
nl = after_fence.find("\n")
|
||||
if nl != -1 and after_fence[:nl].strip().lower() in {"json", "javascript", "js", ""}:
|
||||
after_fence = after_fence[nl + 1 :]
|
||||
fence_end = after_fence.rfind("```")
|
||||
if fence_end != -1:
|
||||
stripped = after_fence[:fence_end].strip()
|
||||
else:
|
||||
stripped = after_fence.strip()
|
||||
try:
|
||||
return json.loads(raw.strip())
|
||||
except json.JSONDecodeError as exc:
|
||||
print(f"[graphify] LLM returned invalid JSON, skipping chunk: {exc}", file=sys.stderr)
|
||||
return {"nodes": [], "edges": [], "hyperedges": []}
|
||||
return json.loads(stripped)
|
||||
except json.JSONDecodeError:
|
||||
pass
|
||||
# Strategy 2: extract the first balanced JSON object found anywhere in
|
||||
# the text. Handles the case where Claude wraps the JSON in prose without
|
||||
# any markdown fence ("The extracted graph is { ... }. Hope this helps!").
|
||||
start = stripped.find("{")
|
||||
if start != -1:
|
||||
depth = 0
|
||||
in_string = False
|
||||
escape = False
|
||||
for i in range(start, len(stripped)):
|
||||
ch = stripped[i]
|
||||
if escape:
|
||||
escape = False
|
||||
continue
|
||||
if ch == "\\":
|
||||
escape = True
|
||||
continue
|
||||
if ch == '"':
|
||||
in_string = not in_string
|
||||
continue
|
||||
if in_string:
|
||||
continue
|
||||
if ch == "{":
|
||||
depth += 1
|
||||
elif ch == "}":
|
||||
depth -= 1
|
||||
if depth == 0:
|
||||
try:
|
||||
return json.loads(stripped[start : i + 1])
|
||||
except json.JSONDecodeError:
|
||||
break
|
||||
print(
|
||||
f"[graphify] LLM returned invalid JSON, skipping chunk "
|
||||
f"(first 200 chars: {raw[:200]!r})",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return {"nodes": [], "edges": [], "hyperedges": []}
|
||||
|
||||
|
||||
def _response_is_hollow(raw_content: str | None, parsed: dict) -> bool:
|
||||
@@ -452,13 +499,30 @@ def _call_claude_cli(user_message: str, max_tokens: int = 8192, *, deep_mode: bo
|
||||
"https://claude.ai/code and run `claude` once to authenticate."
|
||||
)
|
||||
|
||||
# Use --system-prompt (replaces) instead of --append-system-prompt (adds
|
||||
# to Claude Code's default coding-agent prompt). The default prompt
|
||||
# pushes the model towards markdown + prose explanations, which conflict
|
||||
# with the "raw JSON only" extraction instruction and cause ~30-50% of
|
||||
# responses to come back wrapped in ```json fences or prefixed with a
|
||||
# preamble — both of which fail the strict json.loads in _parse_llm_json.
|
||||
# Replacing the default prompt eliminates the conflict at the source.
|
||||
# Side benefit: cache-creation tokens per call drop ~19% in practice.
|
||||
cli_args = [
|
||||
"claude", "-p",
|
||||
"--output-format", "json",
|
||||
"--no-session-persistence",
|
||||
"--system-prompt", _extraction_system(deep=deep_mode),
|
||||
]
|
||||
# claude-cli defaults to Opus, which is overkill for the structured-JSON
|
||||
# extraction graphify performs. GRAPHIFY_CLAUDE_CLI_MODEL=haiku (or
|
||||
# sonnet, or a full model ID like claude-haiku-4-5-20251001) lets users
|
||||
# opt into a cheaper / faster model. Default behaviour unchanged when
|
||||
# the env var is unset.
|
||||
cli_model = os.environ.get("GRAPHIFY_CLAUDE_CLI_MODEL", "").strip()
|
||||
if cli_model:
|
||||
cli_args.extend(["--model", cli_model])
|
||||
proc = subprocess.run(
|
||||
[
|
||||
"claude", "-p",
|
||||
"--output-format", "json",
|
||||
"--no-session-persistence",
|
||||
"--append-system-prompt", _extraction_system(deep=deep_mode),
|
||||
],
|
||||
cli_args,
|
||||
input=user_message,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
|
||||
@@ -0,0 +1,146 @@
|
||||
"""Tests for `_parse_llm_json` robustness and the `_call_claude_cli`
|
||||
subprocess argv shape introduced in the hollow-response fix.
|
||||
|
||||
These tests cover:
|
||||
- The four parser failure modes described in PR #1062
|
||||
- The switch from --append-system-prompt to --system-prompt
|
||||
- The GRAPHIFY_CLAUDE_CLI_MODEL env-var passthrough
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
from graphify import llm
|
||||
|
||||
|
||||
# ---------- _parse_llm_json: the four canonical failure modes ----------
|
||||
|
||||
|
||||
def test_preamble_then_fence_is_parsed():
|
||||
"""Claude often prefixes the JSON with a short preamble before the
|
||||
```json fence. The original parser only stripped fences at offset 0,
|
||||
so any preamble caused json.loads to fail and the chunk to be
|
||||
dropped as a hollow response. The robust parser handles fences
|
||||
anywhere in the text."""
|
||||
raw = (
|
||||
"Here are the extracted entities:\n\n"
|
||||
'```json\n{"nodes": [{"id": "a"}], "edges": []}\n```'
|
||||
)
|
||||
result = llm._parse_llm_json(raw)
|
||||
assert result["nodes"] == [{"id": "a"}]
|
||||
assert result["edges"] == []
|
||||
|
||||
|
||||
def test_prose_wrapped_json_without_fence_is_parsed():
|
||||
"""Some models return prose around bare JSON with no markdown fence.
|
||||
The balanced-brace fallback extracts the first complete object."""
|
||||
raw = (
|
||||
'The extracted graph is {"nodes": [{"id": "b"}], "edges": []}. '
|
||||
"Hope this helps!"
|
||||
)
|
||||
result = llm._parse_llm_json(raw)
|
||||
assert result["nodes"] == [{"id": "b"}]
|
||||
|
||||
|
||||
def test_raw_json_still_works():
|
||||
"""Regression: clean JSON input (the original happy path) must keep
|
||||
parsing exactly as before."""
|
||||
raw = '{"nodes": [], "edges": [], "hyperedges": []}'
|
||||
result = llm._parse_llm_json(raw)
|
||||
assert result == {"nodes": [], "edges": [], "hyperedges": []}
|
||||
|
||||
|
||||
def test_total_refusal_returns_empty_fragment():
|
||||
"""When the model refuses or returns unrelated prose, the parser
|
||||
must degrade gracefully — return the empty fragment so the hollow
|
||||
detector takes over, never raise."""
|
||||
raw = "I cannot extract structured data from this content."
|
||||
result = llm._parse_llm_json(raw)
|
||||
assert result == {"nodes": [], "edges": [], "hyperedges": []}
|
||||
|
||||
|
||||
# ---------- _parse_llm_json: secondary cases worth pinning ----------
|
||||
|
||||
|
||||
def test_fence_with_uppercase_language_tag():
|
||||
raw = '```JSON\n{"nodes": [{"id": "x"}], "edges": []}\n```'
|
||||
result = llm._parse_llm_json(raw)
|
||||
assert result["nodes"] == [{"id": "x"}]
|
||||
|
||||
|
||||
def test_fence_without_closing_backticks():
|
||||
"""Truncated response: the model started the fence but ran out of
|
||||
tokens before closing it. We should still recover the JSON body."""
|
||||
raw = '```json\n{"nodes": [{"id": "y"}], "edges": []}'
|
||||
result = llm._parse_llm_json(raw)
|
||||
assert result["nodes"] == [{"id": "y"}]
|
||||
|
||||
|
||||
def test_empty_response_returns_empty_fragment():
|
||||
assert llm._parse_llm_json("") == {"nodes": [], "edges": [], "hyperedges": []}
|
||||
|
||||
|
||||
# ---------- _call_claude_cli: argv shape ----------
|
||||
|
||||
|
||||
def _make_envelope(result_obj: dict) -> str:
|
||||
return json.dumps({
|
||||
"type": "result",
|
||||
"subtype": "success",
|
||||
"is_error": False,
|
||||
"result": json.dumps(result_obj),
|
||||
"usage": {"input_tokens": 1, "output_tokens": 1,
|
||||
"cache_creation_input_tokens": 0, "cache_read_input_tokens": 0},
|
||||
"modelUsage": {"claude-opus-4-7": {}},
|
||||
"stop_reason": "end_turn",
|
||||
})
|
||||
|
||||
|
||||
@patch("shutil.which", return_value="/usr/local/bin/claude")
|
||||
@patch("subprocess.run")
|
||||
def test_uses_system_prompt_not_append(mock_run, _which):
|
||||
"""The hollow-response root cause was --append-system-prompt
|
||||
layering graphify's extraction prompt on top of Claude Code's
|
||||
default interactive-agent prompt. The fix switches to
|
||||
--system-prompt (replace) to eliminate the conflict."""
|
||||
mock_run.return_value.returncode = 0
|
||||
mock_run.return_value.stdout = _make_envelope({"nodes": [], "edges": [], "hyperedges": []})
|
||||
mock_run.return_value.stderr = ""
|
||||
llm._call_claude_cli("payload")
|
||||
argv = mock_run.call_args.args[0]
|
||||
assert "--system-prompt" in argv, f"--system-prompt missing from argv: {argv}"
|
||||
assert "--append-system-prompt" not in argv, (
|
||||
"--append-system-prompt should have been replaced — it's the root "
|
||||
"cause of the hollow-response loop"
|
||||
)
|
||||
|
||||
|
||||
@patch("shutil.which", return_value="/usr/local/bin/claude")
|
||||
@patch("subprocess.run")
|
||||
def test_model_env_var_adds_model_flag(mock_run, _which, monkeypatch):
|
||||
"""GRAPHIFY_CLAUDE_CLI_MODEL must be forwarded to claude -p --model."""
|
||||
monkeypatch.setenv("GRAPHIFY_CLAUDE_CLI_MODEL", "haiku")
|
||||
mock_run.return_value.returncode = 0
|
||||
mock_run.return_value.stdout = _make_envelope({"nodes": [], "edges": [], "hyperedges": []})
|
||||
mock_run.return_value.stderr = ""
|
||||
llm._call_claude_cli("payload")
|
||||
argv = mock_run.call_args.args[0]
|
||||
assert "--model" in argv
|
||||
assert argv[argv.index("--model") + 1] == "haiku"
|
||||
|
||||
|
||||
@patch("shutil.which", return_value="/usr/local/bin/claude")
|
||||
@patch("subprocess.run")
|
||||
def test_no_model_flag_when_env_var_unset(mock_run, _which, monkeypatch):
|
||||
"""Default behaviour: when the env var is not set, --model is not
|
||||
added so claude-cli's own default kicks in."""
|
||||
monkeypatch.delenv("GRAPHIFY_CLAUDE_CLI_MODEL", raising=False)
|
||||
mock_run.return_value.returncode = 0
|
||||
mock_run.return_value.stdout = _make_envelope({"nodes": [], "edges": [], "hyperedges": []})
|
||||
mock_run.return_value.stderr = ""
|
||||
llm._call_claude_cli("payload")
|
||||
argv = mock_run.call_args.args[0]
|
||||
assert "--model" not in argv
|
||||
Reference in New Issue
Block a user