From 26518bc35ea9c1110fda40c335f740f063dfb65b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 26 Aug 2026 10:36:55 -0700 Subject: [PATCH 1/2] Fix a Real UnicodeDecodeError Crash Review Found on the Promotion PR Per qodo's fresh review of the develop -> main promotion diff: 1. hub_tracked() requested NUL-delimited raw bytes from git ls-tree -z but decoded them with subprocess's text=True (locale decoding), so a tracked filename with a byte invalid in that locale raised UnicodeDecodeError before the NUL-split ever ran, aborting the audit rather than enumerating the path. Read raw bytes instead and decode each record with os.fsdecode() (surrogateescape), matching the rest of Python's filesystem APIs. Verified: reproduced the crash with the old code against a synthetic non-UTF-8 filename, confirmed the fix enumerates it correctly, and added the case as a permanent --selftest regression (fails with a Traceback when the fix is reverted). 2. hub_only_paths()'s new docstring used a semicolon as prose punctuation. Split into two sentences. 3. canonical_blob_sha()'s new docstring explained git rev-parse tree-ish resolution mechanics rather than stating the callable's behavior contract. Trimmed to the contract. --- spec/audit.py | 65 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 54 insertions(+), 11 deletions(-) diff --git a/spec/audit.py b/spec/audit.py index d2eb2756..af9201fc 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -32,6 +32,7 @@ import hashlib import itertools import json +import os import pathlib import re import subprocess @@ -103,25 +104,27 @@ def hub_tracked(rev=None): nothing" and silently clear every hub-only finding. """ walk_rev = _hub_main_rev() if rev is None else rev + # No text=True: -z's pathnames are raw bytes, and locale decoding here would crash on an invalid byte before the NUL-split below ever runs. + # Decode each record with os.fsdecode() instead, the same surrogateescape policy the rest of Python's filesystem APIs use. r = subprocess.run( ["git", "ls-tree", "-r", "-z", walk_rev], cwd=ROOT, capture_output=True, - text=True, check=False, ) if r.returncode != 0: + stderr = r.stderr.decode("utf-8", errors="replace").strip() raise RuntimeError( - f"git ls-tree -r {walk_rev} failed in {ROOT}: {r.stderr.strip() or 'non-zero exit'}" + f"git ls-tree -r {walk_rev} failed in {ROOT}: {stderr or 'non-zero exit'}" ) paths = set() - for record in r.stdout.split("\0"): + for record in r.stdout.split(b"\0"): if not record: continue - meta, _, path = record.partition("\t") + meta, _, path = record.partition(b"\t") mode = meta.split(None, 1)[0] if meta else None - if mode in ("100644", "100755"): - paths.add(path) + if mode in (b"100644", b"100755"): + paths.add(os.fsdecode(path)) return frozenset(paths) @@ -140,7 +143,7 @@ def hub_only_paths(spec, rev=None): So only a `retire` disposition in spec/divergences.json asserts a deletion, and an untriaged hit asks for the file to be read. - `rev` is passed through to `hub_tracked()`; see its docstring. + `rev` is passed through to `hub_tracked()`. See its docstring. """ declared = {e["path"] for e in spec["files"]["baseline"]} tree_paths = set() @@ -198,10 +201,9 @@ def repo_tree(slug, ground_head): def canonical_blob_sha(path): """The hub's git blob identity for path, from the same resolved `main` commit `_git_revisions()` and `_hub_main_rev()` walk, not from ROOT's checked-out working tree, - which is not necessarily `main` (ptr727/ProjectTemplate#1017 review). `git rev-parse` resolves - a `:` tree-ish straight to the blob object id, so no separate read-and-hash step is - needed. Raises OSError, matching a filesystem read's own contract, when path is absent from - that commit or is not a regular file there. + which is not necessarily `main` (ptr727/ProjectTemplate#1017 review). Raises OSError, matching + a filesystem read's own contract, when path is absent from that commit or is not a regular + file there. """ rev = _hub_main_rev() result = subprocess.run( @@ -3577,6 +3579,47 @@ def _selftest(): f" {'ok ' if got == want else 'FAIL'} want={want!s:<24} got={got!s:<24} _git_revisions: default rev reads origin's main, not the checked-out branch" ) + # hub_tracked(): a tracked filename with a byte invalid in the locale encoding must round-trip + # rather than crash git ls-tree -z is NUL-delimited raw bytes, and decoding it as text before + # the NUL-split (the bug review caught) raises UnicodeDecodeError instead of enumerating the path. + with tempfile.TemporaryDirectory() as tmp_root: + tmp_root_path = pathlib.Path(tmp_root) + for cmd in ( + ["git", "init", "-q", "-b", "main"], + ["git", "config", "user.email", "test@test.invalid"], + ["git", "config", "user.name", "test"], + ): + subprocess.run(cmd, cwd=tmp_root_path, check=True, capture_output=True) + bad_name = os.fsdecode(b"bad-\xff-name.txt") + try: + (tmp_root_path / bad_name).write_text("x") + except OSError: + # This host's filesystem or encoding cannot represent the byte: skip rather than + # aborting the whole --selftest run over an environment limitation, not a code fault. + print(" skip hub_tracked: non-UTF-8 filename (host cannot create it)") + else: + subprocess.run(["git", "add", "-A"], cwd=tmp_root_path, check=True, capture_output=True) + subprocess.run( + ["git", "commit", "-q", "-m", "add invalid-utf8 name"], + cwd=tmp_root_path, + check=True, + capture_output=True, + ) + saved_root = ROOT + ROOT = tmp_root_path + try: + hub_tracked.cache_clear() + tracked = hub_tracked(rev="HEAD") + finally: + ROOT = saved_root + hub_tracked.cache_clear() + got = bad_name in tracked + if not got: + ok = False + print( + f" {'ok ' if got else 'FAIL'} want=True got={got!s:<24} hub_tracked: a non-UTF-8 filename round-trips instead of crashing" + ) + # Region extraction and hashing: a forked github-release block must hash differently from the canonical. region = split_jobs(rel_ok).get("github-release") forked_region = split_jobs( From 999364663694665ac8b071fdacf28a36a28765fe Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 26 Aug 2026 10:47:38 -0700 Subject: [PATCH 2/2] Fix Real Bugs Review Found in This PR's Own Fixes Per review on PR #1028 (CodeRabbit + qodo): 1. My own os.fsdecode() fix for the UnicodeDecodeError crash used a platform-dependent error handler: surrogateescape on POSIX, surrogatepass on Windows, which still raises on an arbitrary invalid byte there. Switched to an explicit path.decode("utf-8", errors="surrogateescape") in both hub_tracked() and the new --selftest fixture, which never raises on any platform regardless of host locale or OS. The fixture also picks a byte sequence the active locale encoding actually rejects (0xFF is invalid under UTF-8 but valid under Latin-1/CP1252), rather than assuming one fixed byte, skipping only if no candidate is rejected. 2. canonical_blob_sha() only checked git rev-parse's exit status, so a directory path silently returned a tree object id instead of raising OSError, contradicting its own documented regular-file contract. git rev-parse : resolves a tree exactly as readily as a blob. Switched to git ls-tree, checked its mode the same way _git_revisions()/hub_tracked() already do (100644/100755 only), verified live: a real file still resolves its correct blob sha, a directory now raises OSError instead of silently succeeding. 3. Comment-style: two wrapped multi-line sentences and one lowercase sentence opener, fixed to match this repo's one-sentence-per-line convention. --- spec/audit.py | 105 +++++++++++++++++++++++++++++--------------------- 1 file changed, 62 insertions(+), 43 deletions(-) diff --git a/spec/audit.py b/spec/audit.py index af9201fc..34f1d064 100755 --- a/spec/audit.py +++ b/spec/audit.py @@ -32,7 +32,7 @@ import hashlib import itertools import json -import os +import locale import pathlib import re import subprocess @@ -105,7 +105,7 @@ def hub_tracked(rev=None): """ walk_rev = _hub_main_rev() if rev is None else rev # No text=True: -z's pathnames are raw bytes, and locale decoding here would crash on an invalid byte before the NUL-split below ever runs. - # Decode each record with os.fsdecode() instead, the same surrogateescape policy the rest of Python's filesystem APIs use. + # Decode each record with a fixed utf-8/surrogateescape policy instead of os.fsdecode(), whose error handler is platform-dependent (surrogatepass on Windows) and still crashes on an arbitrary invalid byte there. r = subprocess.run( ["git", "ls-tree", "-r", "-z", walk_rev], cwd=ROOT, @@ -124,7 +124,7 @@ def hub_tracked(rev=None): meta, _, path = record.partition(b"\t") mode = meta.split(None, 1)[0] if meta else None if mode in (b"100644", b"100755"): - paths.add(os.fsdecode(path)) + paths.add(path.decode("utf-8", errors="surrogateescape")) return frozenset(paths) @@ -206,8 +206,9 @@ def canonical_blob_sha(path): file there. """ rev = _hub_main_rev() + # Read via git ls-tree, not git rev-parse :, which resolves a directory to a tree object id just as readily as a file to a blob one, silently breaking the regular-file promise above. result = subprocess.run( - ["git", "rev-parse", f"{rev}:{path}"], + ["git", "ls-tree", rev, "--", path], cwd=ROOT, capture_output=True, text=True, @@ -215,7 +216,13 @@ def canonical_blob_sha(path): ) if result.returncode != 0: raise OSError(f"{path} is unreadable from the hub's main at {rev}: {result.stderr.strip()}") - return result.stdout.strip() + entry = result.stdout.strip() + if not entry: + raise OSError(f"{path} is absent from the hub's main at {rev}") + mode, _, sha = entry.partition("\t")[0].split() + if mode not in ("100644", "100755"): + raise OSError(f"{path} is not a regular file at {rev} (mode {mode})") + return sha def tree_path_included(path, patterns): @@ -3579,46 +3586,58 @@ def _selftest(): f" {'ok ' if got == want else 'FAIL'} want={want!s:<24} got={got!s:<24} _git_revisions: default rev reads origin's main, not the checked-out branch" ) - # hub_tracked(): a tracked filename with a byte invalid in the locale encoding must round-trip - # rather than crash git ls-tree -z is NUL-delimited raw bytes, and decoding it as text before - # the NUL-split (the bug review caught) raises UnicodeDecodeError instead of enumerating the path. - with tempfile.TemporaryDirectory() as tmp_root: - tmp_root_path = pathlib.Path(tmp_root) - for cmd in ( - ["git", "init", "-q", "-b", "main"], - ["git", "config", "user.email", "test@test.invalid"], - ["git", "config", "user.name", "test"], - ): - subprocess.run(cmd, cwd=tmp_root_path, check=True, capture_output=True) - bad_name = os.fsdecode(b"bad-\xff-name.txt") + # hub_tracked(): a tracked filename with a byte the active locale rejects must round-trip rather than crash. + # git ls-tree -z is NUL-delimited raw bytes, and decoding it as text before the NUL-split (the bug review caught) raises UnicodeDecodeError instead of enumerating the path. + encoding = locale.getpreferredencoding(False) + invalid_bytes = None + for candidate in (b"\xff", b"\x80\x81", b"\xfe\xff"): try: - (tmp_root_path / bad_name).write_text("x") - except OSError: - # This host's filesystem or encoding cannot represent the byte: skip rather than - # aborting the whole --selftest run over an environment limitation, not a code fault. - print(" skip hub_tracked: non-UTF-8 filename (host cannot create it)") - else: - subprocess.run(["git", "add", "-A"], cwd=tmp_root_path, check=True, capture_output=True) - subprocess.run( - ["git", "commit", "-q", "-m", "add invalid-utf8 name"], - cwd=tmp_root_path, - check=True, - capture_output=True, - ) - saved_root = ROOT - ROOT = tmp_root_path + candidate.decode(encoding) + except UnicodeDecodeError: + invalid_bytes = candidate + break + if invalid_bytes is None: + # No candidate is actually invalid under this host's active encoding: skip rather than asserting a regression the fixture cannot exercise here. + print(f" skip hub_tracked: no candidate byte sequence is invalid under {encoding!r}") + else: + bad_name = "bad-" + invalid_bytes.decode("utf-8", errors="surrogateescape") + "-name.txt" + with tempfile.TemporaryDirectory() as tmp_root: + tmp_root_path = pathlib.Path(tmp_root) + for cmd in ( + ["git", "init", "-q", "-b", "main"], + ["git", "config", "user.email", "test@test.invalid"], + ["git", "config", "user.name", "test"], + ): + subprocess.run(cmd, cwd=tmp_root_path, check=True, capture_output=True) try: - hub_tracked.cache_clear() - tracked = hub_tracked(rev="HEAD") - finally: - ROOT = saved_root - hub_tracked.cache_clear() - got = bad_name in tracked - if not got: - ok = False - print( - f" {'ok ' if got else 'FAIL'} want=True got={got!s:<24} hub_tracked: a non-UTF-8 filename round-trips instead of crashing" - ) + (tmp_root_path / bad_name).write_text("x") + except OSError: + # This host's filesystem cannot represent the byte: skip rather than aborting the whole --selftest run over an environment limitation, not a code fault. + print(" skip hub_tracked: non-UTF-8 filename (host cannot create it)") + else: + subprocess.run( + ["git", "add", "-A"], cwd=tmp_root_path, check=True, capture_output=True + ) + subprocess.run( + ["git", "commit", "-q", "-m", "add invalid-utf8 name"], + cwd=tmp_root_path, + check=True, + capture_output=True, + ) + saved_root = ROOT + ROOT = tmp_root_path + try: + hub_tracked.cache_clear() + tracked = hub_tracked(rev="HEAD") + finally: + ROOT = saved_root + hub_tracked.cache_clear() + got = bad_name in tracked + if not got: + ok = False + print( + f" {'ok ' if got else 'FAIL'} want=True got={got!s:<24} hub_tracked: a non-UTF-8 filename round-trips instead of crashing" + ) # Region extraction and hashing: a forked github-release block must hash differently from the canonical. region = split_jobs(rel_ok).get("github-release")