diff --git a/scripts/run_external_review.py b/scripts/run_external_review.py index d80f6f177..c6e2f9d44 100755 --- a/scripts/run_external_review.py +++ b/scripts/run_external_review.py @@ -551,22 +551,19 @@ def _create_isolated_checkout( if add.returncode != 0: raise RuntimeError(f"worktree add failed: {add.stderr.strip()}") if staged_patch.strip(): - # TEXT stdin on purpose, symmetric with the text-mode capture that produced - # `staged_patch` (see the `git diff --cached --binary` capture sites and the - # test's helper): this exact pairing is the configuration Windows CI was - # green with through v6.87.5, and switching only this side to bytes broke - # CRLF worktrees there. On POSIX text mode is an identity. A staged BINARY - # file on a CRLF-translating platform can still fail the roundtrip — that - # failure is loud (RuntimeError with git's stderr), never silent. + # Capture and apply stay byte-paired: Windows text stdin translates LF, + # while text capture loses CRLF. Changing only stdin cannot repair bytes + # already normalized at capture. The public patch remains a str using + # the contributor snapshot's reversible UTF-8/surrogateescape contract. apply = subprocess.run( ["git", "apply", "--index", "--whitespace=nowarn", "--binary"], - cwd=str(checkout), input=staged_patch, - capture_output=True, text=True, timeout=120, + cwd=str(checkout), input=staged_patch.encode("utf-8", errors="surrogateescape"), + capture_output=True, timeout=120, ) if apply.returncode != 0: raise RuntimeError( "staged diff did not apply to the isolated checkout: " - f"{(apply.stderr or '').strip()}" + f"{(apply.stderr or b'').decode('utf-8', errors='replace').strip()}" ) return checkout_root, checkout @@ -1293,12 +1290,9 @@ def main() -> int: staged = ( str(contributor_snapshot["patch"]) if contributor_snapshot is not None - else subprocess.run( - ["git", "diff", "--cached", "--binary"], - cwd=str(REPO), - capture_output=True, - text=True, - ).stdout + else _git_bytes(["diff", "--cached", "--binary"]).decode( + "utf-8", errors="surrogateescape", + ) ) if not staged.strip(): message = ( @@ -1451,10 +1445,9 @@ def main() -> int: ["git", "add", "-A"], cwd=str(checkout), capture_output=True, text=True, timeout=120, ) - post_tree = subprocess.run( - ["git", "diff", "--cached", "--binary"], - cwd=str(checkout), capture_output=True, text=True, timeout=120, - ).stdout + post_tree = _git_bytes( + ["diff", "--cached", "--binary"], cwd=checkout, + ).decode("utf-8", errors="surrogateescape") if post_tree.strip() != staged.strip(): print( "WARN: the reviewed checkout tree drifted from the staged " @@ -1462,8 +1455,8 @@ def main() -> int: "worktree before committing what was reviewed.", file=sys.stderr, ) - (output_dir / "reviewed-tree-drift.diff").write_text( - post_tree, encoding="utf-8", + (output_dir / "reviewed-tree-drift.diff").write_bytes( + post_tree.encode("utf-8", errors="surrogateescape"), ) if args.contributor: outcome = { diff --git a/tests/test_external_review_patch_bytes.py b/tests/test_external_review_patch_bytes.py new file mode 100644 index 000000000..d8783c595 --- /dev/null +++ b/tests/test_external_review_patch_bytes.py @@ -0,0 +1,95 @@ +"""The wrapper replays exact Git patch bytes across newline conversion policies.""" + +import os +from pathlib import Path +import subprocess +import tempfile +from types import SimpleNamespace + +import pytest + +from scripts import run_external_review as runner +from ouroboros.tools import git as review_git + + +pytestmark = pytest.mark.serial + + +@pytest.mark.parametrize("eol", [b"\n", b"\r\n"], ids=["lf-blob", "crlf-blob"]) +@pytest.mark.parametrize("autocrlf", ["false", "true"]) +@pytest.mark.parametrize("windows_text", [False, True], ids=["native-stdio", "windows-text-stdio"]) +@pytest.mark.parametrize("drift", [False, True], ids=["same-tree", "drift-bytes"]) +def test_patch_roundtrip(tmp_path, monkeypatch, eol, autocrlf, windows_text, drift): + repo, output = tmp_path / "repo", tmp_path / "output" + repo.mkdir() + original_run = subprocess.run + + def git_bytes(*args, cwd=repo): + return original_run(["git", *args], cwd=cwd, check=True, capture_output=True).stdout + + for args in (("init",), ("config", "user.name", "Test"), + ("config", "user.email", "test@example.invalid"), + ("config", "core.autocrlf", autocrlf)): + git_bytes(*args) + # Explicit raw text permits either Git blob spelling under either user + # conversion preference. The ordinary no-attributes fixture is covered by + # test_external_review_pending_checkout, including on native Windows CI. + (repo / ".gitattributes").write_bytes(b"change.py -text\n") + (repo / "VERSION").write_bytes(b"1.0.0\n") + (repo / "change.py").write_bytes(b"value = 1" + eol) + git_bytes("add", ".") + git_bytes("commit", "-m", "base") + proposed = b"value = 2" + eol + (repo / "change.py").write_bytes(proposed) + git_bytes("add", "change.py") + expected_tree = git_bytes("write-tree") + observed = {} + checkouts = [] + + def run(args, *positional, **kwargs): + # POSIX normally masks Windows' text-pipe translation. Exercise that + # exact transformation only if a regression reintroduces text stdin; + # native Windows already performs it and must not translate twice. + if (windows_text and os.name != "nt" and list(args)[:2] == ["git", "apply"] + and kwargs.get("text") and isinstance(kwargs.get("input"), str)): + kwargs["input"] = kwargs["input"].replace("\n", "\r\n") + return original_run(args, *positional, **kwargs) + + def owned_temp(*, prefix): + root = Path(tempfile.mkdtemp(prefix=prefix, dir=tmp_path)) + checkouts.append(root) + return str(root) + + def cycle(ctx, _message, **kwargs): + assert kwargs["skip_advisory_review"] is False + assert git_bytes("write-tree", cwd=ctx.repo_dir) == expected_tree + assert (ctx.repo_dir / "change.py").read_bytes() == proposed + observed["cycle"] = True + if drift: + (ctx.repo_dir / "change.py").write_bytes(b"value = 3" + eol) + observed["drift"] = git_bytes("diff", "HEAD", "--binary", cwd=ctx.repo_dir) + return {"status": "blocked", "block_reason": "preflight", "message": "fixture"} + + monkeypatch.setattr(subprocess, "run", run) + monkeypatch.setattr(runner, "tempfile", SimpleNamespace(mkdtemp=owned_temp)) + monkeypatch.setattr(runner, "REPO", repo) + monkeypatch.setattr(runner, "_parse_args", lambda: SimpleNamespace( + contributor=False, commit_message="candidate", goal="", scope="", + output=str(output), drive_root=str(tmp_path / "data"), no_isolated_checkout=False)) + monkeypatch.setattr(runner, "_prepare_review_configuration", lambda _args: (None, "HEAD", {})) + monkeypatch.setattr(runner, "_advisory_unavailability_warning", lambda: "") + monkeypatch.setattr(review_git, "_run_non_committing_review_cycle", cycle) + try: + assert runner.main() == 3 + assert observed.get("cycle"), "Patch replay must reach the existing review cycle" + artifact = output / "reviewed-tree-drift.diff" + assert artifact.exists() is drift + if drift: + assert artifact.read_bytes() == observed["drift"] + assert all(not root.exists() for root in checkouts) + finally: + # Also clean a checkout when a regression fails before returning its + # handle to main; never leave fixture-created Git registrations behind. + for root in checkouts: + if root.exists(): + runner._remove_isolated_checkout(root, root / "repo") diff --git a/tests/test_external_review_script.py b/tests/test_external_review_script.py index e8ed3bbc1..062d27154 100644 --- a/tests/test_external_review_script.py +++ b/tests/test_external_review_script.py @@ -1223,10 +1223,11 @@ def test_normal_key_probe_stays_single_model_but_contributor_probes_all(monkeypa def _git(repo: Path, *args: str) -> str: + patch = args[:1] == ("diff",) and "--binary" in args proc = subprocess.run( - ["git", *args], cwd=str(repo), capture_output=True, text=True, check=True, + ["git", *args], cwd=str(repo), capture_output=True, text=not patch, check=True, ) - return proc.stdout + return proc.stdout.decode("utf-8", errors="surrogateescape") if patch else proc.stdout def test_isolated_checkout_freezes_the_reviewed_tree(tmp_path, monkeypatch):