fix(review): preserve staged patch bytes across platforms

Keep capture, replay, comparison and drift artifacts byte-paired through the existing reversible codec. Cover LF and CRLF blobs, autocrlf settings, Windows text pipes and unchanged custody handling.

Co-authored-by: Ouroboros <311266734+ouroboros-agent@users.noreply.github.com>

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 = {

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")

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):