mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
Fix headroom learn crashing/no-op on Windows from missing UTF-8 encoding (#1239)
## Description Fixes #1202. On a Windows (cp1252) locale, `headroom learn` cannot complete a run: the whole pipeline opens transcript files and pipes analyzer prompts without `encoding="utf-8"`, so any non-ASCII byte (em-dashes, arrows — ubiquitous in code and prose) breaks it. Same bug class already fixed for `headroom wrap` (#65, #1126) and the dashboard (#533), never swept through `learn`. Three independent failure points, each hidden behind the previous: 1. **Reading transcripts** — six bare `open()` calls in the learn plugins. The **Codex** JSONL scanner caught only `OSError`, so a `UnicodeDecodeError` propagated and **aborted the whole cross-agent run**; the **Claude** scanner caught it and **silently dropped the session**. `analyzer.py` also read the user's own CLAUDE.md/MEMORY.md with no encoding. 2. **Analyzer subprocess** — `subprocess.run`/`Popen(..., text=True)` with no encoding raised `UnicodeEncodeError` on the piped prompt; it was swallowed, so the run produced **0 recommendations** with no obvious failure. 3. **`--apply` merge** — `writer.py` read the existing context file with strict `encoding="utf-8"`, which aborts on a single stray legacy byte. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - `learn/plugins/{claude,codex,gemini}.py`: add `encoding="utf-8", errors="replace"` to the six transcript `open()` calls. - `learn/analyzer.py`: same on the `read_text` of the user's context files and on both analyzer subprocess calls (`subprocess.run` and `Popen`). - `learn/writer.py`: add `_read_text_tolerant` — decode the to-be-rewritten context file as UTF-8, falling back to UTF-8-with-replacement on a stray byte (a whole-file cp1252 fallback is wrong: it mojibakes genuine UTF-8 em-dashes); the subsequent `write_text(encoding="utf-8")` self-heals the file. - `cli/learn.py`: wrap `plugin.scan_project` so one unreadable agent/project is skipped with a warning instead of aborting the whole run. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check`) - [ ] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ pytest tests/test_learn/test_writer.py tests/test_learn/test_plugin_encoding.py -q 22 passed $ ruff check headroom/learn/plugins/*.py headroom/learn/analyzer.py \ headroom/learn/writer.py headroom/cli/learn.py tests/test_learn/test_*.py All checks passed! ``` New tests are **red on the old code, green with the fix**: - `test_plugin_encoding.py` — a transcript with a stray `0x9d` byte (undefined in cp1252 *and* an invalid UTF-8 start byte, so a bare `open()` fails on any locale): the Codex scanner no longer raises, the Claude scanner now recovers the session instead of dropping it. - `test_writer.py::TestEncodingResilience` — `_read_text_tolerant` preserves valid UTF-8 (no mojibake) and `--apply` merges over a file with a stray byte. ## Real Behavior Proof - Environment: Windows 11, Python 3.10, against the real learn plugins/writer (no live LLM backend; the decode failures occur before any backend call). - Exact command / steps: write a Claude transcript and a Codex rollout JSONL containing a valid em-dash/arrow line plus a stray `0x9d` byte, then call `ClaudeCodePlugin._scan_session` / `CodexPlugin._scan_jsonl_session`; for the writer, `write_bytes` an `AGENTS.md` with a stray `0x97` and run `_merge_into_file`. - Observed result: **before** the fix → `CodexPlugin._scan_jsonl_session` raises `UnicodeDecodeError: 'utf-8' codec can't decode byte 0x9d` (aborts the run) and `ClaudeCodePlugin._scan_session` returns `None` (session dropped); **after** → Codex completes, Claude returns the `SessionData` (`total_input_tokens == 5`), and `_merge_into_file` keeps `Notes — existing` with no mojibake. - Not tested: a full end-to-end `headroom learn --apply` against live agent histories + a real LLM backend (verified at the plugin/writer level, which is where the decode failures live). ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review
This commit is contained in:
parent
7c26a54d53
commit
6129808462
8 changed files with 131 additions and 12 deletions
|
|
@ -236,9 +236,15 @@ def learn(
|
|||
click.echo(f"Path: {proj.project_path}")
|
||||
click.echo(f"{'=' * 60}")
|
||||
|
||||
sessions = plugin.scan_project(
|
||||
proj, max_workers=max_workers, include_subagents=not main_only
|
||||
)
|
||||
try:
|
||||
sessions = plugin.scan_project(
|
||||
proj, max_workers=max_workers, include_subagents=not main_only
|
||||
)
|
||||
except Exception as exc:
|
||||
# One unreadable agent/project must not abort the whole
|
||||
# cross-agent run; skip it with a warning and continue.
|
||||
click.echo(f" Skipping (could not scan sessions): {exc}")
|
||||
continue
|
||||
if not sessions:
|
||||
click.echo(" No conversation data found.")
|
||||
continue
|
||||
|
|
|
|||
|
|
@ -198,7 +198,7 @@ def _build_prior_patterns_section(project: ProjectInfo) -> str:
|
|||
for label, path in candidates:
|
||||
if path is None or not path.exists():
|
||||
continue
|
||||
block = extract_marker_block(path.read_text())
|
||||
block = extract_marker_block(path.read_text(encoding="utf-8", errors="replace"))
|
||||
if block:
|
||||
parts.append((label, block))
|
||||
|
||||
|
|
@ -486,6 +486,8 @@ def _call_cli_llm(digest: str, model: str) -> dict:
|
|||
input=prompt,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
encoding="utf-8",
|
||||
errors="replace",
|
||||
timeout=hard_cap,
|
||||
)
|
||||
except FileNotFoundError:
|
||||
|
|
@ -541,6 +543,8 @@ def _call_claude_cli_streaming(
|
|||
stdout=subprocess.PIPE,
|
||||
stderr=subprocess.PIPE,
|
||||
text=True,
|
||||
encoding="utf-8",
|
||||
errors="replace",
|
||||
bufsize=1, # line-buffered
|
||||
)
|
||||
except FileNotFoundError:
|
||||
|
|
|
|||
|
|
@ -167,7 +167,7 @@ class ClaudeCodePlugin(LearnPlugin, ConversationScanner):
|
|||
msg_index = 0
|
||||
|
||||
try:
|
||||
with open(jsonl_path) as f:
|
||||
with open(jsonl_path, encoding="utf-8", errors="replace") as f:
|
||||
for line in f:
|
||||
try:
|
||||
d = json.loads(line)
|
||||
|
|
|
|||
|
|
@ -126,7 +126,7 @@ class CodexPlugin(LearnPlugin, ConversationScanner):
|
|||
def _scan_json_session(self, json_path: Path) -> SessionData | None:
|
||||
"""Parse a single Codex session file."""
|
||||
try:
|
||||
with open(json_path) as f:
|
||||
with open(json_path, encoding="utf-8", errors="replace") as f:
|
||||
data = json.load(f)
|
||||
except (OSError, json.JSONDecodeError) as e:
|
||||
logger.debug("Failed to read Codex session %s: %s", json_path, e)
|
||||
|
|
@ -209,7 +209,7 @@ class CodexPlugin(LearnPlugin, ConversationScanner):
|
|||
msg_index = 0
|
||||
|
||||
try:
|
||||
with open(jsonl_path) as f:
|
||||
with open(jsonl_path, encoding="utf-8", errors="replace") as f:
|
||||
for line in f:
|
||||
try:
|
||||
entry = json.loads(line)
|
||||
|
|
|
|||
|
|
@ -143,7 +143,7 @@ class GeminiPlugin(LearnPlugin, ConversationScanner):
|
|||
def _scan_json_session(self, json_path: Path) -> SessionData | None:
|
||||
"""Parse a Gemini JSON session file."""
|
||||
try:
|
||||
with open(json_path) as f:
|
||||
with open(json_path, encoding="utf-8", errors="replace") as f:
|
||||
data = json.load(f)
|
||||
except (OSError, json.JSONDecodeError) as e:
|
||||
logger.debug("Failed to read Gemini session %s: %s", json_path, e)
|
||||
|
|
@ -167,7 +167,7 @@ class GeminiPlugin(LearnPlugin, ConversationScanner):
|
|||
messages: list[dict] = []
|
||||
|
||||
try:
|
||||
with open(jsonl_path) as f:
|
||||
with open(jsonl_path, encoding="utf-8", errors="replace") as f:
|
||||
for line in f:
|
||||
try:
|
||||
entry = json.loads(line)
|
||||
|
|
@ -314,7 +314,7 @@ class GeminiPlugin(LearnPlugin, ConversationScanner):
|
|||
def _detect_project_path(self, session_path: Path) -> Path | None:
|
||||
"""Try to detect the project path from a session file."""
|
||||
try:
|
||||
with open(session_path) as f:
|
||||
with open(session_path, encoding="utf-8", errors="replace") as f:
|
||||
data = json.load(f)
|
||||
except (OSError, json.JSONDecodeError):
|
||||
return None
|
||||
|
|
|
|||
|
|
@ -26,6 +26,23 @@ _MARKER_PATTERN = re.compile(
|
|||
)
|
||||
|
||||
|
||||
def _read_text_tolerant(file_path: Path) -> str:
|
||||
"""Read an existing context file that we are about to rewrite as UTF-8.
|
||||
|
||||
These files are predominantly valid UTF-8 but may carry a stray legacy
|
||||
byte (e.g. a cp1252 em-dash ``0x97``). Strict UTF-8 decoding aborts the
|
||||
whole ``--apply`` on a single such byte, so fall back to UTF-8 with
|
||||
replacement: this preserves the valid UTF-8 content — a full-file cp1252
|
||||
fallback would instead turn every genuine UTF-8 em-dash into mojibake —
|
||||
and the subsequent ``write_text(encoding="utf-8")`` self-heals the file.
|
||||
"""
|
||||
raw = file_path.read_bytes()
|
||||
try:
|
||||
return raw.decode("utf-8")
|
||||
except UnicodeDecodeError:
|
||||
return raw.decode("utf-8", errors="replace")
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Abstract Writer
|
||||
# =============================================================================
|
||||
|
|
@ -153,7 +170,7 @@ def _merge_recommendations(
|
|||
"""
|
||||
if not file_path.exists():
|
||||
return new_recommendations
|
||||
prior = _parse_prior_recommendations(file_path.read_text(encoding="utf-8"))
|
||||
prior = _parse_prior_recommendations(_read_text_tolerant(file_path))
|
||||
if not prior:
|
||||
return new_recommendations
|
||||
new_sections = {r.section for r in new_recommendations}
|
||||
|
|
@ -166,7 +183,7 @@ def _merge_into_file(file_path: Path, new_recommendations: list[Recommendation])
|
|||
merged = _merge_recommendations(file_path, new_recommendations)
|
||||
section = _build_section(merged)
|
||||
if file_path.exists():
|
||||
existing = file_path.read_text(encoding="utf-8")
|
||||
existing = _read_text_tolerant(file_path)
|
||||
if _MARKER_START in existing:
|
||||
return _MARKER_PATTERN.sub(lambda _match: section, existing)
|
||||
return existing.rstrip() + "\n\n" + section + "\n"
|
||||
|
|
|
|||
51
tests/test_learn/test_plugin_encoding.py
Normal file
51
tests/test_learn/test_plugin_encoding.py
Normal file
|
|
@ -0,0 +1,51 @@
|
|||
"""Regression tests for #1202 — the ``learn`` session scanners must read agent
|
||||
transcripts as UTF-8 with replacement, so a stray non-UTF-8 byte cannot abort
|
||||
(or silently drop) a scan.
|
||||
|
||||
``0x9d`` is undefined in cp1252 *and* an invalid UTF-8 start byte, so a bare
|
||||
``open()`` fails on it regardless of the host locale. Before the fix this made
|
||||
the Codex JSONL scanner raise ``UnicodeDecodeError`` (the scan caught only
|
||||
``OSError``), aborting the whole cross-agent run, while the Claude scanner
|
||||
caught it and silently dropped the session.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from pathlib import Path
|
||||
|
||||
from headroom.learn.models import SessionData
|
||||
from headroom.learn.plugins.claude import ClaudeCodePlugin
|
||||
from headroom.learn.plugins.codex import CodexPlugin
|
||||
|
||||
|
||||
def _stray_byte_line() -> bytes:
|
||||
# A line that is neither valid UTF-8 nor decodable in cp1252.
|
||||
return b"\x9d arrow \xe2\x86\x92 junk\n"
|
||||
|
||||
|
||||
def test_claude_scan_recovers_session_with_stray_byte(tmp_path: Path) -> None:
|
||||
jsonl = tmp_path / "session.jsonl"
|
||||
valid = json.dumps(
|
||||
{"type": "assistant", "message": {"usage": {"input_tokens": 5}}, "text": "em — arrow →"}
|
||||
)
|
||||
jsonl.write_bytes(valid.encode() + b"\n" + _stray_byte_line())
|
||||
|
||||
result = ClaudeCodePlugin(claude_dir=tmp_path)._scan_session(jsonl)
|
||||
|
||||
# Before the fix this returned None (session silently dropped); now the
|
||||
# valid line is read and the stray-byte line is skipped, not fatal.
|
||||
assert result is not None
|
||||
assert result.session_id == "session"
|
||||
assert result.total_input_tokens == 5
|
||||
|
||||
|
||||
def test_codex_jsonl_scan_does_not_crash_on_stray_byte(tmp_path: Path) -> None:
|
||||
jsonl = tmp_path / "rollout.jsonl"
|
||||
meta = json.dumps({"type": "session_meta", "payload": {"id": "abc"}})
|
||||
jsonl.write_bytes(meta.encode() + b"\n" + _stray_byte_line())
|
||||
|
||||
# Before the fix this raised UnicodeDecodeError and aborted the run.
|
||||
result = CodexPlugin()._scan_jsonl_session(jsonl)
|
||||
|
||||
assert result is None or isinstance(result, SessionData)
|
||||
|
|
@ -2,6 +2,8 @@
|
|||
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from headroom.learn.models import ProjectInfo, Recommendation, RecommendationTarget
|
||||
from headroom.learn.writer import (
|
||||
_MARKER_END,
|
||||
|
|
@ -9,6 +11,7 @@ from headroom.learn.writer import (
|
|||
ClaudeCodeWriter,
|
||||
_merge_into_file,
|
||||
_parse_prior_recommendations,
|
||||
_read_text_tolerant,
|
||||
extract_marker_block,
|
||||
)
|
||||
|
||||
|
|
@ -308,3 +311,41 @@ class TestExtractMarkerBlock:
|
|||
block = extract_marker_block(content)
|
||||
assert block is not None
|
||||
assert block == f"{_MARKER_START}\n{_MARKER_END}"
|
||||
|
||||
|
||||
class TestEncodingResilience:
|
||||
"""Regression tests for #1202 — ``learn --apply`` must not crash merging into
|
||||
an existing context file that carries a stray non-UTF-8 byte (e.g. a legacy
|
||||
cp1252 em-dash ``0x97``)."""
|
||||
|
||||
def test_read_text_tolerant_preserves_valid_utf8(self, tmp_path):
|
||||
path = tmp_path / "AGENTS.md"
|
||||
path.write_text("Use em-dashes — and arrows →.", encoding="utf-8")
|
||||
assert _read_text_tolerant(path) == "Use em-dashes — and arrows →."
|
||||
|
||||
def test_read_text_tolerant_survives_stray_legacy_byte(self, tmp_path):
|
||||
# Predominantly valid UTF-8 (genuine em-dash E2 80 94) plus one stray
|
||||
# cp1252 em-dash byte (0x97) that strict UTF-8 cannot decode.
|
||||
path = tmp_path / "AGENTS.md"
|
||||
path.write_bytes("real em-dash — here\n".encode() + b"legacy \x97 byte\n")
|
||||
|
||||
# The old strict read aborts the whole --apply on that single byte.
|
||||
with pytest.raises(UnicodeDecodeError):
|
||||
path.read_text(encoding="utf-8")
|
||||
|
||||
text = _read_text_tolerant(path)
|
||||
# Valid UTF-8 content is preserved (no cp1252 "â€" mojibake) and the
|
||||
# stray byte is replaced rather than fatal.
|
||||
assert "real em-dash — here" in text
|
||||
assert "\x97" not in text
|
||||
assert "â€" not in text
|
||||
|
||||
def test_merge_into_file_applies_over_file_with_stray_byte(self, tmp_path):
|
||||
path = tmp_path / "AGENTS.md"
|
||||
path.write_bytes("# Notes — existing\n".encode() + b"stray \x97 byte\n")
|
||||
recs = [_rec(RecommendationTarget.CONTEXT_FILE, "Environment", "- Use uv")]
|
||||
|
||||
merged = _merge_into_file(path, recs)
|
||||
|
||||
assert "Use uv" in merged
|
||||
assert "Notes — existing" in merged
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue