fix(verify): translate git's decode fault and add a bytes mode (#377)

CHANGELOG.md

@@ -260,6 +260,18 @@ story <id>`, 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

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,16 +84,41 @@ 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]:
-    """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.
+    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
+    (#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.
+
+    `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
@@ -105,12 +130,14 @@ def _run_git(
         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"},
         )
     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
 
@@ -136,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 <repo> …` 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:

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,113 @@ 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
+
+
+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",
     [
@@ -881,36 +1019,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: