From 7aa01484d261549d9d5fbc92ca019cd2299007b9 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 29 Jul 2026 10:07:59 -0700 Subject: [PATCH 1/2] fix(verify): translate git's decode fault into GitError (#377) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_run_git` decoded git's output strictly and translated only a timeout and a spawn OSError. A UnicodeDecodeError is a third fault raised before any return code exists, and being a ValueError it matched neither arm — so it escaped untyped past every `except GitError` guard, which is the one thing the chokepoint's docstring says must not happen. Reachable wherever git's own quoting is off: `-z` disables core.quotePath (dirty_paths, branch_incoming_paths, commit_paths), `worktree list --porcelain` never applied it, and `git diff` emits content verbatim. The test uses a real file with real bytes and real git: monkeypatching subprocess.run hands the code str objects and never runs the stdlib's decoding, so that version passes with the fix ablated. The #378 codec guard moves to the top of the file, since it now serves both. --- CHANGELOG.md | 12 +++++ src/bmad_loop/verify.py | 26 +++++++--- tests/test_verify.py | 102 ++++++++++++++++++++++++++++------------ 3 files changed, 103 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ccbf4f7..97fbb2d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -260,6 +260,18 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **A non-UTF-8 filename no longer crashes the run past every git guard (#377).** `verify._run_git` + is the sole git spawn point and exists to give git's faults a type its ~50 `except GitError` + guards can catch, but it decoded git's output strictly and translated only a timeout and a spawn + `OSError`. `UnicodeDecodeError` is a third fault raised before any return code exists, and being a + `ValueError` it matched neither arm — so one file whose name is not valid in the run's encoding + (POSIX filenames are arbitrary bytes) escaped untyped. Reachable wherever git's own quoting is + off: the `-z` callers (merge pre-flight collision detection), `worktree list --porcelain` — which + crashed the unguarded stale-run reconcile at every run and sweep start — and `git diff`, whose + content is verbatim, so a failed unit's forensic capture bypassed the valve that preserves its + uncaptured work. Such output is now a `GitError` like any other git failure. Making those paths + usable rather than merely non-fatal is a separate change. + - **One undecodable byte of verify output no longer crashes the run (#378).** Verify commands are arbitrary operator tools, and their captured output was decoded strictly — a child emitting bytes invalid in the run's encoding raised `UnicodeDecodeError` out of `run_verify_commands`, losing diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index ba87826d..dc063ba0 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -87,13 +87,23 @@ class GitSpawnError(GitError): def _run_git( cmd: list[str], repo: Path, *, env: dict[str, str] | None = None ) -> subprocess.CompletedProcess[str]: - """Sole spawn point for git subprocesses. Two failures are raised by - `subprocess.run` *before* any return code exists — a timeout (#156) and a - spawn-level OSError (#343) — so left uncaught either would bypass every - `except GitError` guard and crash the run. Both are translated here into - the GitError taxonomy — observation guards degrade, unguarded paths fail - typed — with the spawn class marked as `GitSpawnError` for the callers - that must distinguish an environment fault from git refusing. + """Sole spawn point for git subprocesses. Three failures are raised by + `subprocess.run` *before* any return code exists — a timeout (#156), a + spawn-level OSError (#343), and a strict-decode fault on the child's output + (#377) — so left uncaught any of them would bypass every `except GitError` + guard and crash the run. All are translated here into the GitError taxonomy + — observation guards degrade, unguarded paths fail typed — with the spawn + class marked as `GitSpawnError` for the callers that must distinguish an + environment fault from git refusing. + + The decode fault is real, not theoretical: POSIX filenames are arbitrary + bytes, and while `core.quotePath` C-quotes them to ASCII for ordinary + porcelain, `-z` disables that quoting (`dirty_paths`, `branch_incoming_paths`, + `commit_paths`), `worktree list --porcelain` never applied it, and `git diff` + emits file *content* verbatim (`capture_diff`) — so one latin-1 file is + enough. Translating is deliberately all this does; making such paths usable + (`errors="surrogateescape"`) is a separate call, since surrogates would then + flow into the UTF-8 journal and JSON writes downstream. Every git child runs with `LC_ALL=C` so messages stay stable English: the one place that inspects git *text* rather than a return code — `safe_rollback`'s @@ -111,6 +121,8 @@ def _run_git( ) except subprocess.TimeoutExpired as exc: raise GitError(f"git {cmd[3]} timed out after {_git_timeout_s}s in {repo}") from exc + except UnicodeDecodeError as exc: + raise GitError(f"git {cmd[3]} returned undecodable output in {repo}: {exc}") from exc except OSError as exc: raise GitSpawnError(f"git {cmd[3]} failed to spawn in {repo}: {exc}") from exc diff --git a/tests/test_verify.py b/tests/test_verify.py index fd0cbba1..7b08e897 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -32,6 +32,37 @@ def dev_result(sp): return {"workflow": "auto-dev", "spec_file": str(sp)} +def _codec_rejects_bad_byte() -> bool: + """Whether byte 0xff is undecodable in the codec ``text=True`` will use. + + Probes ``TextIOWrapper`` with ``encoding=None`` rather than naming a codec, + because that is the very default ``subprocess``'s text mode resolves for the + child's streams — asking the machinery beats predicting it from the locale. + """ + try: + io.TextIOWrapper(io.BytesIO(b"\xff")).read() + except UnicodeDecodeError: + return True + return False + + +# Guard for every test whose subject is a STRICT DECODE of subprocess output — +# the #378 verify-command pair below and the #377 git-chokepoint test above them. +# Byte 0xff is undecodable only in UTF-8/ASCII; every ISO-8859-x and cp125x codec +# maps all 256 byte values, so under such a locale the strict decode never raises +# and those tests pass *with the bug restored* — a silent vacuity rather than a +# failure (verified: under LC_ALL=et_EE.iso885915 the #378 ablation passes). No +# single byte is undecodable everywhere, so this skips instead, leaving each test +# either exercising the fault or saying plainly that it did not. CI is always on +# the exercising side: the Linux legs run UTF-8 and the Windows legs set +# PYTHONUTF8=1 (.github/workflows/ci.yml). +needs_strict_codec = pytest.mark.skipif( + not _codec_rejects_bad_byte(), + reason="host codec decodes 0xff (e.g. an ISO-8859-x locale), so nothing here " + "would exercise the strict decode this fix is about", +) + + def test_attempt_dirty_clean_tree(project): """At baseline with no changes — nothing for a rollback to undo.""" baseline = verify.rev_parse_head(project.project) @@ -211,6 +242,45 @@ def spying_run(cmd, **kwargs): assert seen["env"]["SENTINEL_X"] == "1" # caller's env preserved +# ---- the chokepoint's third pre-returncode fault, and its bytes mode (#377) + + +@needs_strict_codec +@pytest.mark.skipif( + sys.platform == "win32", reason="Windows filenames are UTF-16; no undecodable path exists" +) +def test_z_output_undecodable_path_becomes_git_error(project): + """#377: a filename carrying bytes invalid in the run's codec makes `_run_git`'s + strict decode raise UnicodeDecodeError — a third fault raised before any return + code exists, and the one the taxonomy missed. Being a ValueError it matched + neither `except` arm, so it sailed past every `except GitError` guard: here + `dirty_paths`, reached from `clean_incoming_collisions`' merge pre-flight, whose + callers guard exactly that type. + + The file is real and so is git. Monkeypatching `subprocess.run` would hand the + code str objects and never run the stdlib's decoding at all, so that version + passes identically with the fix ablated (tests/test_install.py:1996 documents + the same trap). + + Ablation: delete the `except UnicodeDecodeError` arm in `_run_git` and this + fails with the raw UnicodeDecodeError.""" + repo = project.project + # POSIX filenames are arbitrary bytes, so build the path as bytes — a str path + # would have to carry the byte as a surrogate and re-encode on the way out. + with open(os.fsencode(repo) + b"/weird-\xff-name.txt", "wb") as fh: + fh.write(b"content\n") + + with pytest.raises(verify.GitError, match="git status returned undecodable output"): + verify.dirty_paths(repo) + + # Why only the `-z` (and raw-diff) sites were ever exposed: plain porcelain + # C-quotes the same path down to ASCII. This pins git's `core.quotePath` + # default, not our fix — adding `-z` to one of the safe callers would + # reintroduce the fault silently, and this is what would notice. + rc, out = verify._git(repo, "status", "--porcelain") + assert rc == 0 and r"weird-\377-name.txt" in out + + @pytest.mark.parametrize( "fm,expected", [ @@ -881,36 +951,8 @@ def test_verify_commands_timeout_stays_charged(tmp_path, monkeypatch): # a test passes identically with the bug restored — see the #374 regression # test's docstring (tests/test_install.py) for the same distinction. # -# They are also codec-conditional, which is the subtler way they could go quiet. -# Byte 0xff is undecodable only in UTF-8/ASCII; every ISO-8859-x and cp125x -# codec maps all 256 byte values, so under such a locale the strict decode never -# raises and both tests pass *with the bug restored* — a silent vacuity rather -# than a failure (verified: under LC_ALL=et_EE.iso885915 the ablation passes). -# No single byte is undecodable everywhere, so the guard below skips instead, -# leaving the tests either exercising the fault or saying plainly that they did -# not. CI is always on the exercising side: the Linux legs run UTF-8 and the -# Windows legs set PYTHONUTF8=1 (.github/workflows/ci.yml). - - -def _codec_rejects_bad_byte() -> bool: - """Whether byte 0xff is undecodable in the codec ``text=True`` will use. - - Probes ``TextIOWrapper`` with ``encoding=None`` rather than naming a codec, - because that is the very default ``subprocess``'s text mode resolves for the - child's streams — asking the machinery beats predicting it from the locale. - """ - try: - io.TextIOWrapper(io.BytesIO(b"\xff")).read() - except UnicodeDecodeError: - return True - return False - - -needs_strict_codec = pytest.mark.skipif( - not _codec_rejects_bad_byte(), - reason="host codec decodes 0xff (e.g. an ISO-8859-x locale), so nothing here " - "would exercise the strict decode this fix is about", -) +# They are also codec-conditional, which is the subtler way they could go quiet; +# `needs_strict_codec` (top of file) is what keeps them honest about it. def _undecodable_cmd(tmp_path: Path, rc: int) -> str: From 76670d5c04edfc71bc4f449ae7ed832e5e5974de Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 29 Jul 2026 10:08:19 -0700 Subject: [PATCH 2/2] feat(verify): add a bytes mode to the git chokepoint (#377) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md requires every git subprocess to go through `_run_git`, but two properties kept a caller out: it hands back CompletedProcess[str], i.e. the strict decode, and it raises rather than returning a return code the caller reads as an answer — `git config --get` of an unset key exits 1, and that is the reply, not a fault. A keyword-only `binary` flag with @overload declarations gives the mode a type, and `git_bytes` is the public accessor so no other module reaches for the private name. Bytes callers keep what the chokepoint is for: the LC_ALL=C pin (#236) and the limits.git_timeout_s bound (#156), with the two faults that have no rc to return still raising. No caller on main yet — install._shield_git is the intended customer and lands with #385, whose docstring names this issue as its blocker. --- src/bmad_loop/verify.py | 46 +++++++++++++++++++++++++--- tests/test_verify.py | 68 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 4 deletions(-) diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index dc063ba0..94fc186c 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -16,7 +16,7 @@ from collections.abc import Callable from dataclasses import dataclass from pathlib import Path -from typing import Any +from typing import Any, Literal, overload from . import deferredwork from .bmadconfig import ProjectPaths @@ -84,9 +84,21 @@ class GitSpawnError(GitError): (#343). The underlying errno stays reachable via ``exc.__cause__.errno``.""" +@overload def _run_git( - cmd: list[str], repo: Path, *, env: dict[str, str] | None = None -) -> subprocess.CompletedProcess[str]: + cmd: list[str], repo: Path, *, env: dict[str, str] | None = ..., binary: Literal[False] = ... +) -> subprocess.CompletedProcess[str]: ... + + +@overload +def _run_git( + cmd: list[str], repo: Path, *, env: dict[str, str] | None = ..., binary: Literal[True] +) -> subprocess.CompletedProcess[bytes]: ... + + +def _run_git( + cmd: list[str], repo: Path, *, env: dict[str, str] | None = None, binary: bool = False +) -> subprocess.CompletedProcess[str] | subprocess.CompletedProcess[bytes]: """Sole spawn point for git subprocesses. Three failures are raised by `subprocess.run` *before* any return code exists — a timeout (#156), a spawn-level OSError (#343), and a strict-decode fault on the child's output @@ -105,6 +117,9 @@ class marked as `GitSpawnError` for the callers that must distinguish an (`errors="surrogateescape"`) is a separate call, since surrogates would then flow into the UTF-8 journal and JSON writes downstream. + `binary=True` skips the decode entirely and hands back the raw + `CompletedProcess[bytes]` — see `git_bytes`, the public accessor for it. + Every git child runs with `LC_ALL=C` so messages stay stable English: the one place that inspects git *text* rather than a return code — `safe_rollback`'s benign "pathspec did not match" tolerance — must not misread a translated @@ -115,7 +130,7 @@ class marked as `GitSpawnError` for the callers that must distinguish an return subprocess.run( cmd, capture_output=True, - text=True, + text=not binary, timeout=_git_timeout_s, env={**(env if env is not None else os.environ), "LC_ALL": "C"}, ) @@ -148,6 +163,29 @@ def _git_env(repo: Path, *args: str, env: dict[str, str]) -> tuple[int, str]: return proc.returncode, (proc.stdout + proc.stderr).strip() +def git_bytes(repo: Path, *args: str) -> subprocess.CompletedProcess[bytes]: + """Run one `git -C …` through the chokepoint, capturing raw BYTES. + + For the callers the `(rc, str)` wrappers above cannot serve, on two counts: + + * **bytes, not a strict decode.** POSIX filenames are arbitrary bytes, so a + repo path invalid in the locale codec reaches a `text=True` call on an + ordinary box. `_run_git` now translates that into `GitError` (#377) rather + than letting it escape untyped — but a caller whose contract is "answer the + question or skip silently" wants the bytes themselves, not a raised + taxonomy member. Decode at the point of use with `os.fsdecode`. + * **the returncode is an answer, not a fault.** The `CompletedProcess` is + returned whatever the rc, never `check=True`: `git config --get` of an unset + key exits 1, and that *is* the reply. Callers branch on `returncode`. + + Standing inside the chokepoint is what buys the rest: the `LC_ALL=C` pin so + git's message text stays stable English (#236), and the `_git_timeout_s` bound + the engine sets from `limits.git_timeout_s` (#156). The two faults with no rc + to return still raise — a timeout as `GitError`, a spawn failure as + `GitSpawnError` — since neither can be expressed as a `CompletedProcess`.""" + return _run_git(["git", "-C", str(repo), *args], repo, binary=True) + + def rev_parse_head(repo: Path) -> str: rc, out = _git(repo, "rev-parse", "HEAD") if rc != 0: diff --git a/tests/test_verify.py b/tests/test_verify.py index 7b08e897..a1bf0e9c 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -281,6 +281,74 @@ def test_z_output_undecodable_path_becomes_git_error(project): assert rc == 0 and r"weird-\377-name.txt" in out +def test_git_bytes_returns_bytes_and_reads_rc_as_an_answer(project): + """`git_bytes` hands back the CompletedProcess whatever the rc, with stdout as + raw bytes. Both halves are load-bearing for the bytes-mode callers: `git config + --get` of an unset key exits 1 and that non-zero rc *is* the reply (it is what + tells the git-add shield to enable the extension), so a chokepoint that raised + on it could not serve them; and bytes are what let a path invalid in the locale + codec through to an `os.fsdecode` at the point of use. + + Ablation: drop `binary=True` from `git_bytes` and the bytes assertions fail + against str.""" + unset = verify.git_bytes(project.project, "config", "--get", "bmadloop.definitelyunset") + assert unset.returncode == 1 # an answer, not a fault — and not raised + assert unset.stdout == b"" + + answered = verify.git_bytes(project.project, "rev-parse", "--abbrev-ref", "HEAD") + assert answered.returncode == 0 + assert answered.stdout.strip() == b"main" # bytes, not str + + +def test_git_bytes_inherits_locale_pin_and_timeout(project, monkeypatch): + """What standing inside the chokepoint buys the bytes callers, asserted rather + than assumed: the LC_ALL=C pin (#236) and the engine-set `limits.git_timeout_s` + bound (#156) reach subprocess.run unchanged, with text mode off.""" + seen: dict[str, object] = {} + real_run = subprocess.run + + def spying_run(cmd, **kwargs): + seen.update(kwargs) + return real_run(cmd, **kwargs) + + monkeypatch.setattr(verify.subprocess, "run", spying_run) + verify.configure_git_timeout(9) + try: + verify.git_bytes(project.project, "rev-parse", "HEAD") + finally: + verify.configure_git_timeout(verify.GIT_TIMEOUT_S) + + assert not seen["text"] + assert seen["timeout"] == 9 + assert seen["env"]["LC_ALL"] == "C" + + +def test_git_bytes_timeout_still_becomes_git_error(project, monkeypatch): + """Bytes mode skips the decode, not the taxonomy: a timeout has no return code + to hand back, so it still raises rather than becoming a CompletedProcess. + + Ablation: delete the `except subprocess.TimeoutExpired` arm and this fails with + the raw TimeoutExpired.""" + monkeypatch.setattr(verify.subprocess, "run", _timing_out_run) + with pytest.raises(verify.GitError, match=r"git config timed out after \d+s"): + verify.git_bytes(project.project, "config", "--get", "core.excludesFile") + + +def test_git_bytes_spawn_oserror_still_becomes_git_spawn_error(project, monkeypatch): + """Same for a spawn-level OSError — GitSpawnError, errno still reachable. + + Ablation: delete the `except OSError` arm and this fails with the raw OSError.""" + exc = OSError(24, "Too many open files") + + def failing_run(cmd, **kwargs): + raise exc + + monkeypatch.setattr(verify.subprocess, "run", failing_run) + with pytest.raises(verify.GitSpawnError, match="git config failed to spawn") as excinfo: + verify.git_bytes(project.project, "config", "--get", "core.excludesFile") + assert excinfo.value.__cause__ is exc + + @pytest.mark.parametrize( "fm,expected", [