From 6129808462f4f48347283f34f96b950243c8accf Mon Sep 17 00:00:00 2001 From: jichaowang02-lang Date: Sun, 21 Jun 2026 18:37:37 +0100 Subject: [PATCH] Fix `headroom learn` crashing/no-op on Windows from missing UTF-8 encoding (#1239) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- headroom/cli/learn.py | 12 ++++-- headroom/learn/analyzer.py | 6 ++- headroom/learn/plugins/claude.py | 2 +- headroom/learn/plugins/codex.py | 4 +- headroom/learn/plugins/gemini.py | 6 +-- headroom/learn/writer.py | 21 +++++++++- tests/test_learn/test_plugin_encoding.py | 51 ++++++++++++++++++++++++ tests/test_learn/test_writer.py | 41 +++++++++++++++++++ 8 files changed, 131 insertions(+), 12 deletions(-) create mode 100644 tests/test_learn/test_plugin_encoding.py diff --git a/headroom/cli/learn.py b/headroom/cli/learn.py index 2b69d89c9..6d5a19981 100644 --- a/headroom/cli/learn.py +++ b/headroom/cli/learn.py @@ -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 diff --git a/headroom/learn/analyzer.py b/headroom/learn/analyzer.py index e939aea3c..110ba6046 100644 --- a/headroom/learn/analyzer.py +++ b/headroom/learn/analyzer.py @@ -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: diff --git a/headroom/learn/plugins/claude.py b/headroom/learn/plugins/claude.py index 8cc5f7dcf..c790bfdf1 100644 --- a/headroom/learn/plugins/claude.py +++ b/headroom/learn/plugins/claude.py @@ -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) diff --git a/headroom/learn/plugins/codex.py b/headroom/learn/plugins/codex.py index 8b70ae748..4908aa74d 100644 --- a/headroom/learn/plugins/codex.py +++ b/headroom/learn/plugins/codex.py @@ -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) diff --git a/headroom/learn/plugins/gemini.py b/headroom/learn/plugins/gemini.py index 7f8a1b901..15e589a68 100644 --- a/headroom/learn/plugins/gemini.py +++ b/headroom/learn/plugins/gemini.py @@ -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 diff --git a/headroom/learn/writer.py b/headroom/learn/writer.py index ddad3e529..ade64a02a 100644 --- a/headroom/learn/writer.py +++ b/headroom/learn/writer.py @@ -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" diff --git a/tests/test_learn/test_plugin_encoding.py b/tests/test_learn/test_plugin_encoding.py new file mode 100644 index 000000000..6c15ea7ae --- /dev/null +++ b/tests/test_learn/test_plugin_encoding.py @@ -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) diff --git a/tests/test_learn/test_writer.py b/tests/test_learn/test_writer.py index 80b98ad40..8e4ef956a 100644 --- a/tests/test_learn/test_writer.py +++ b/tests/test_learn/test_writer.py @@ -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