fix(llm): force UTF-8 encoding on _call_claude_cli subprocess + loud failure on chunk errors (#906)
* fix(extract): force UTF-8 encoding on subprocess + loud failure on chunk errors On Windows cp1252, subprocess.run(..., text=True) without encoding= raises UnicodeEncodeError for chars like → ✅ ≥ in chunk content. Both _call_claude_cli (llm.py:426) and the _call_llm claude-cli branch (llm.py:959) lacked encoding=. - Add encoding="utf-8" to both subprocess.run sites. - Track failed_chunks in extract_corpus_parallel merged result dict. - Print [graphify] WARNING: N/M semantic chunk(s) failed summary to stderr at end of run when any chunk failed, so silent partial failures are visible. - Add tests/test_charmap_encoding.py: 10 regression tests covering subprocess encoding kwarg, loud-failure summary, and substitution validation. Closes #3 * style: drop fork-local issue refs from llm/test comments
This commit is contained in:
Christopher Beaulieu
+33
-13
@@ -433,6 +433,7 @@ def _call_claude_cli(user_message: str, max_tokens: int = 8192) -> dict:
|
||||
input=user_message,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
encoding="utf-8", # Force UTF-8 — prevents UnicodeEncodeError on Windows cp1252
|
||||
timeout=600,
|
||||
check=False,
|
||||
)
|
||||
@@ -847,7 +848,11 @@ def extract_corpus_parallel(
|
||||
else:
|
||||
chunks = [files[i:i + chunk_size] for i in range(0, len(files), chunk_size)]
|
||||
|
||||
merged: dict = {"nodes": [], "edges": [], "hyperedges": [], "input_tokens": 0, "output_tokens": 0}
|
||||
merged: dict = {
|
||||
"nodes": [], "edges": [], "hyperedges": [],
|
||||
"input_tokens": 0, "output_tokens": 0,
|
||||
"failed_chunks": 0, # count of chunks that raised — loud failure on chunk errors
|
||||
}
|
||||
total = len(chunks)
|
||||
|
||||
def _run_one(idx: int, chunk: list[Path]) -> tuple[int, dict | None, Exception | None]:
|
||||
@@ -883,24 +888,38 @@ def extract_corpus_parallel(
|
||||
_, result, exc = _run_one(idx, chunk)
|
||||
if exc is not None:
|
||||
print(f"[graphify] chunk {idx + 1}/{total} failed: {exc}", file=sys.stderr)
|
||||
merged["failed_chunks"] += 1
|
||||
continue
|
||||
assert result is not None
|
||||
_merge_into(merged, result)
|
||||
if callable(on_chunk_done):
|
||||
on_chunk_done(idx, total, result)
|
||||
return merged
|
||||
else:
|
||||
with ThreadPoolExecutor(max_workers=workers) as pool:
|
||||
futures = [pool.submit(_run_one, idx, chunk) for idx, chunk in enumerate(chunks)]
|
||||
for future in as_completed(futures):
|
||||
idx, result, exc = future.result()
|
||||
if exc is not None:
|
||||
print(
|
||||
f"[graphify] chunk {idx + 1}/{total} failed: {exc}",
|
||||
file=sys.stderr,
|
||||
)
|
||||
merged["failed_chunks"] += 1
|
||||
continue
|
||||
assert result is not None
|
||||
_merge_into(merged, result)
|
||||
if callable(on_chunk_done):
|
||||
on_chunk_done(idx, total, result)
|
||||
|
||||
with ThreadPoolExecutor(max_workers=workers) as pool:
|
||||
futures = [pool.submit(_run_one, idx, chunk) for idx, chunk in enumerate(chunks)]
|
||||
for future in as_completed(futures):
|
||||
idx, result, exc = future.result()
|
||||
if exc is not None:
|
||||
print(f"[graphify] chunk {idx + 1}/{total} failed: {exc}", file=sys.stderr)
|
||||
continue
|
||||
assert result is not None
|
||||
_merge_into(merged, result)
|
||||
if callable(on_chunk_done):
|
||||
on_chunk_done(idx, total, result)
|
||||
# Loud failure summary — surface chunk failures at end so they're never
|
||||
# buried mid-log. Exit 0 preserved for caller compatibility; the
|
||||
# summary block makes the problem visible.
|
||||
if merged["failed_chunks"] > 0:
|
||||
print(
|
||||
f"[graphify] WARNING: {merged['failed_chunks']}/{total} semantic chunk(s) failed"
|
||||
" — see errors above. Partial results returned.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
return merged
|
||||
|
||||
|
||||
@@ -961,6 +980,7 @@ def _call_llm(prompt: str, *, backend: str, max_tokens: int = 200) -> str:
|
||||
input=prompt,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
encoding="utf-8", # Force UTF-8 — prevents UnicodeEncodeError on Windows cp1252
|
||||
timeout=600,
|
||||
check=False,
|
||||
)
|
||||
|
||||
@@ -0,0 +1,330 @@
|
||||
"""Regression tests for UnicodeEncodeError on Windows cp1252 console.
|
||||
|
||||
On Windows with the default cp1252 codepage, subprocess.run(..., text=True)
|
||||
without an explicit encoding= defaults to cp1252, causing UnicodeEncodeError
|
||||
when chunk content contains characters outside cp1252 (e.g. → ✅ ≥).
|
||||
|
||||
These tests mock subprocess.run to:
|
||||
a) Assert that the subprocess call is made with encoding="utf-8" (or
|
||||
equivalent environment forcing UTF-8), so non-ASCII chars never hit
|
||||
cp1252 encoding.
|
||||
b) Assert that extract_corpus_parallel reports loud failure (non-zero exit
|
||||
or summary block) when ≥1 chunk fails.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import sys
|
||||
from io import StringIO
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, call, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from graphify import llm
|
||||
|
||||
# ── Helpers ────────────────────────────────────────────────────────────────────
|
||||
|
||||
_UNICODE_CONTENT = "→ means implies. ✅ done. Score ≥ 90."
|
||||
|
||||
_ENVELOPE = {
|
||||
"type": "result",
|
||||
"subtype": "success",
|
||||
"is_error": False,
|
||||
"result": json.dumps({
|
||||
"nodes": [{"id": "n1", "label": "N1", "file_type": "document",
|
||||
"source_file": "u.md"}],
|
||||
"edges": [],
|
||||
"hyperedges": [],
|
||||
"input_tokens": 0,
|
||||
"output_tokens": 0,
|
||||
}),
|
||||
"stop_reason": "end_turn",
|
||||
"usage": {
|
||||
"input_tokens": 1,
|
||||
"output_tokens": 1,
|
||||
"cache_read_input_tokens": 0,
|
||||
"cache_creation_input_tokens": 0,
|
||||
},
|
||||
"modelUsage": {
|
||||
"claude-opus-4-7": {"inputTokens": 1, "outputTokens": 1},
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
# ── Test A: subprocess encoding ───────────────────────────────────────────────
|
||||
|
||||
class TestSubprocessEncoding:
|
||||
"""_call_claude_cli must pass encoding="utf-8" to subprocess.run so that
|
||||
non-ASCII content in chunk messages does not raise UnicodeEncodeError on
|
||||
Windows cp1252 consoles.
|
||||
"""
|
||||
|
||||
def _make_completed(self):
|
||||
"""Build a mock CompletedProcess with a valid JSON envelope."""
|
||||
return MagicMock(returncode=0, stdout=json.dumps(_ENVELOPE), stderr="")
|
||||
|
||||
def test_subprocess_called_with_utf8_encoding(self, monkeypatch):
|
||||
"""subprocess.run must be invoked with encoding='utf-8'."""
|
||||
completed = self._make_completed()
|
||||
monkeypatch.setattr(llm, "_response_is_hollow", lambda raw, parsed: False)
|
||||
with patch("shutil.which", return_value="/fake/bin/claude"), \
|
||||
patch("subprocess.run", return_value=completed) as mock_run:
|
||||
llm._call_claude_cli(_UNICODE_CONTENT, max_tokens=8192)
|
||||
_args, kwargs = mock_run.call_args
|
||||
assert kwargs.get("encoding") == "utf-8", (
|
||||
"subprocess.run must be called with encoding='utf-8'; "
|
||||
f"got encoding={kwargs.get('encoding')!r}"
|
||||
)
|
||||
|
||||
def test_subprocess_does_not_use_text_true_without_encoding(self, monkeypatch):
|
||||
"""text=True without encoding= relies on the locale codec (cp1252 on Windows).
|
||||
|
||||
Either encoding='utf-8' must be set (making text=True irrelevant) or
|
||||
text=True must be absent and input encoded to bytes explicitly.
|
||||
"""
|
||||
completed = self._make_completed()
|
||||
monkeypatch.setattr(llm, "_response_is_hollow", lambda raw, parsed: False)
|
||||
with patch("shutil.which", return_value="/fake/bin/claude"), \
|
||||
patch("subprocess.run", return_value=completed) as mock_run:
|
||||
llm._call_claude_cli(_UNICODE_CONTENT, max_tokens=8192)
|
||||
_args, kwargs = mock_run.call_args
|
||||
# If text=True is present, encoding must also be set to 'utf-8'.
|
||||
if kwargs.get("text") is True:
|
||||
assert kwargs.get("encoding") == "utf-8", (
|
||||
"text=True without encoding='utf-8' will use the locale codec "
|
||||
"(cp1252 on Windows), causing UnicodeEncodeError on → ✅ ≥"
|
||||
)
|
||||
else:
|
||||
# input must be bytes, not str
|
||||
inp = kwargs.get("input") or (mock_run.call_args[0][1:2] or [None])[0]
|
||||
assert isinstance(inp, bytes), (
|
||||
"Without text=True, input must be bytes pre-encoded to UTF-8."
|
||||
)
|
||||
|
||||
def test_unicode_chars_survive_subprocess_roundtrip(self, monkeypatch, tmp_path):
|
||||
"""Writing a file with → ✅ ≥ then passing its content through
|
||||
_call_claude_cli must not raise UnicodeEncodeError.
|
||||
"""
|
||||
f = tmp_path / "u.md"
|
||||
f.write_text(_UNICODE_CONTENT, encoding="utf-8")
|
||||
|
||||
completed = self._make_completed()
|
||||
monkeypatch.setattr(llm, "_response_is_hollow", lambda raw, parsed: False)
|
||||
with patch("shutil.which", return_value="/fake/bin/claude"), \
|
||||
patch("subprocess.run", return_value=completed):
|
||||
# Should not raise
|
||||
result = llm.extract_files_direct(
|
||||
files=[f], backend="claude-cli", root=tmp_path
|
||||
)
|
||||
assert len(result["nodes"]) >= 1
|
||||
|
||||
def test_call_llm_claude_cli_subprocess_encoding(self, monkeypatch):
|
||||
"""_call_llm with backend='claude-cli' must also use encoding='utf-8'."""
|
||||
completed = MagicMock(
|
||||
returncode=0,
|
||||
stdout=json.dumps({"result": "ok", "stop_reason": "end_turn"}),
|
||||
stderr="",
|
||||
)
|
||||
with patch("shutil.which", return_value="/fake/bin/claude"), \
|
||||
patch("subprocess.run", return_value=completed) as mock_run:
|
||||
llm._call_llm(_UNICODE_CONTENT, backend="claude-cli", max_tokens=200)
|
||||
_args, kwargs = mock_run.call_args
|
||||
assert kwargs.get("encoding") == "utf-8", (
|
||||
"_call_llm claude-cli subprocess must use encoding='utf-8'; "
|
||||
f"got encoding={kwargs.get('encoding')!r}"
|
||||
)
|
||||
|
||||
|
||||
# ── Test B: loud failure on chunk error ────────────────────────────────────────
|
||||
|
||||
class TestLoudChunkFailure:
|
||||
"""extract_corpus_parallel must surface chunk failures loudly — either via
|
||||
non-zero exit (exception raised from the function) or a printed summary
|
||||
block — rather than silently returning exit 0 with failures buried in logs.
|
||||
"""
|
||||
|
||||
def test_failure_count_in_merged_result(self, monkeypatch, tmp_path):
|
||||
"""When chunks fail, extract_corpus_parallel must record failed_chunks > 0
|
||||
in its return value.
|
||||
"""
|
||||
files = []
|
||||
for i in range(3):
|
||||
f = tmp_path / f"f{i}.py"
|
||||
f.write_text(f"x = {i}\n", encoding="utf-8")
|
||||
files.append(f)
|
||||
|
||||
monkeypatch.setattr(
|
||||
llm,
|
||||
"_extract_with_adaptive_retry",
|
||||
lambda *a, **kw: (_ for _ in ()).throw(RuntimeError("charmap error")),
|
||||
)
|
||||
|
||||
result = llm.extract_corpus_parallel(files, backend="claude-cli")
|
||||
assert result.get("failed_chunks", 0) > 0, (
|
||||
"extract_corpus_parallel must expose failed_chunks count in its "
|
||||
f"return dict; got: {result}"
|
||||
)
|
||||
|
||||
def test_summary_printed_when_chunks_fail(self, monkeypatch, tmp_path, capsys):
|
||||
"""A summary line must appear on stderr when ≥1 chunk fails."""
|
||||
files = []
|
||||
for i in range(2):
|
||||
f = tmp_path / f"g{i}.py"
|
||||
f.write_text(f"y = {i}\n", encoding="utf-8")
|
||||
files.append(f)
|
||||
|
||||
monkeypatch.setattr(
|
||||
llm,
|
||||
"_extract_with_adaptive_retry",
|
||||
lambda *a, **kw: (_ for _ in ()).throw(RuntimeError("charmap error")),
|
||||
)
|
||||
|
||||
llm.extract_corpus_parallel(files, backend="claude-cli")
|
||||
captured = capsys.readouterr()
|
||||
# The summary must mention how many chunks failed
|
||||
assert "failed" in captured.err.lower(), (
|
||||
"A failure summary must appear on stderr when chunks fail; "
|
||||
f"got stderr: {captured.err!r}"
|
||||
)
|
||||
|
||||
def test_no_false_alarm_when_all_chunks_succeed(self, monkeypatch, tmp_path, capsys):
|
||||
"""When all chunks succeed, failed_chunks must be 0 and no failure
|
||||
summary should appear.
|
||||
"""
|
||||
f = tmp_path / "ok.py"
|
||||
f.write_text("z = 1\n", encoding="utf-8")
|
||||
|
||||
good_result = {
|
||||
"nodes": [{"id": "n1", "label": "N1", "file_type": "code",
|
||||
"source_file": str(f)}],
|
||||
"edges": [], "hyperedges": [],
|
||||
"input_tokens": 1, "output_tokens": 1,
|
||||
"elapsed_seconds": 0.1,
|
||||
}
|
||||
monkeypatch.setattr(
|
||||
llm,
|
||||
"_extract_with_adaptive_retry",
|
||||
lambda *a, **kw: good_result,
|
||||
)
|
||||
|
||||
result = llm.extract_corpus_parallel([f], backend="claude-cli")
|
||||
assert result.get("failed_chunks", 0) == 0
|
||||
captured = capsys.readouterr()
|
||||
# "WARNING:" should NOT appear on a fully-successful run
|
||||
assert "WARNING:" not in captured.err or "0/" not in captured.err
|
||||
|
||||
|
||||
# ── Substitution validation (rsl-siege-manager path via Python) ────────────────
|
||||
|
||||
class TestSubstitutionValidation:
|
||||
"""Exercises the same code path as the rsl-siege-manager reproduction
|
||||
without requiring the `claude` CLI or its auth.
|
||||
|
||||
The reproduction scenario: a file containing → ✅ ≥ is read via _read_files
|
||||
and passed to _call_claude_cli as `user_message`. Prior to the fix, the
|
||||
subprocess.run call with text=True (no encoding=) would encode `user_message`
|
||||
using the locale codec (cp1252 on Windows), raising UnicodeEncodeError.
|
||||
|
||||
This test:
|
||||
1. Writes a temp file containing the exact unicode chars from the failing chunks.
|
||||
2. Calls _read_files to build the prompt string (same path as extract_files_direct).
|
||||
3. Confirms the prompt encodes cleanly to UTF-8 (the fix) but would fail cp1252.
|
||||
4. Mocks subprocess.run and confirms encoding='utf-8' is passed.
|
||||
"""
|
||||
|
||||
_UNICODE_CHARS = "→ means implies. ✅ done. Score ≥ 90. Threshold: ≥ 95%."
|
||||
|
||||
def test_read_files_produces_utf8_safe_prompt(self, tmp_path):
|
||||
"""_read_files must return a string that encodes cleanly to UTF-8."""
|
||||
f = tmp_path / "unicode_chunk.md"
|
||||
f.write_text(self._UNICODE_CHARS, encoding="utf-8")
|
||||
|
||||
prompt = llm._read_files([f], root=tmp_path)
|
||||
assert self._UNICODE_CHARS in prompt or "→" in prompt
|
||||
|
||||
# Must not raise with UTF-8
|
||||
encoded_utf8 = prompt.encode("utf-8")
|
||||
assert len(encoded_utf8) > 0
|
||||
|
||||
def test_cp1252_would_fail_but_utf8_succeeds(self, tmp_path):
|
||||
"""Demonstrate the exact failure mode that is now fixed.
|
||||
|
||||
The prompt string contains chars outside cp1252, so encoding
|
||||
to cp1252 raises UnicodeEncodeError while UTF-8 succeeds.
|
||||
"""
|
||||
f = tmp_path / "unicode_chunk.md"
|
||||
f.write_text(self._UNICODE_CHARS, encoding="utf-8")
|
||||
|
||||
prompt = llm._read_files([f], root=tmp_path)
|
||||
|
||||
# UTF-8 must succeed (our fix)
|
||||
try:
|
||||
prompt.encode("utf-8")
|
||||
except UnicodeEncodeError as e:
|
||||
raise AssertionError(
|
||||
f"UTF-8 encode must succeed but failed: {e}"
|
||||
) from e
|
||||
|
||||
# cp1252 must fail (confirming these chars are the failing surface)
|
||||
try:
|
||||
prompt.encode("cp1252")
|
||||
# If it doesn't fail, test content doesn't cover the issue —
|
||||
# fail loudly so the test author knows to update _UNICODE_CHARS.
|
||||
raise AssertionError(
|
||||
"Expected cp1252 encode to fail for chars → ✅ ≥, but it "
|
||||
"succeeded. Update _UNICODE_CHARS to include cp1252-incompatible "
|
||||
"characters."
|
||||
)
|
||||
except UnicodeEncodeError:
|
||||
pass # Expected — confirms these chars hit the pre-fix failure surface
|
||||
|
||||
def test_subprocess_encoding_kwarg_in_extract_files_direct(
|
||||
self, monkeypatch, tmp_path
|
||||
):
|
||||
"""End-to-end path: write unicode file → extract_files_direct → subprocess.
|
||||
|
||||
Subprocess must receive encoding='utf-8', not the locale default.
|
||||
"""
|
||||
f = tmp_path / "unicode_chunk.md"
|
||||
f.write_text(self._UNICODE_CHARS, encoding="utf-8")
|
||||
|
||||
_ENVELOPE_SIMPLE = {
|
||||
"type": "result", "subtype": "success", "is_error": False,
|
||||
"result": json.dumps({
|
||||
"nodes": [{"id": "u_chunk", "label": "Unicode Chunk",
|
||||
"file_type": "document",
|
||||
"source_file": "unicode_chunk.md"}],
|
||||
"edges": [], "hyperedges": [],
|
||||
"input_tokens": 1, "output_tokens": 1,
|
||||
}),
|
||||
"stop_reason": "end_turn",
|
||||
"usage": {
|
||||
"input_tokens": 1, "output_tokens": 1,
|
||||
"cache_read_input_tokens": 0, "cache_creation_input_tokens": 0,
|
||||
},
|
||||
"modelUsage": {
|
||||
"claude-opus-4-7": {"inputTokens": 1, "outputTokens": 1},
|
||||
},
|
||||
}
|
||||
completed = MagicMock(
|
||||
returncode=0, stdout=json.dumps(_ENVELOPE_SIMPLE), stderr=""
|
||||
)
|
||||
monkeypatch.setattr(llm, "_response_is_hollow", lambda raw, parsed: False)
|
||||
|
||||
with patch("shutil.which", return_value="/fake/bin/claude"), \
|
||||
patch("subprocess.run", return_value=completed) as mock_run:
|
||||
result = llm.extract_files_direct(
|
||||
files=[f], backend="claude-cli", root=tmp_path
|
||||
)
|
||||
|
||||
assert mock_run.called
|
||||
_args, kwargs = mock_run.call_args
|
||||
assert kwargs.get("encoding") == "utf-8", (
|
||||
"subprocess.run must be called with encoding='utf-8'; "
|
||||
f"got {kwargs.get('encoding')!r}"
|
||||
)
|
||||
# Confirm the unicode content was in the input (not truncated/replaced)
|
||||
inp = kwargs.get("input", "")
|
||||
assert "→" in inp or "✅" in inp or "≥" in inp
|
||||
assert len(result["nodes"]) >= 1
|
||||
Reference in New Issue
Block a user