fix(review): export the patch as bytes, not a JSON string (#1077)

codeframe/core/git.py

@@ -492,6 +492,41 @@ def _index_diff_sections(diff_text: str) -> dict[str, str]:
     return sections
 
 
+def get_patch_bytes(workspace: Workspace, staged: bool = False) -> bytes:
+    """Get the patch as the exact bytes git produced (#1077).
+
+    A patch is byte-faithful by definition — it gets written out and fed to
+    ``git apply``. ``get_patch`` returns a str because GitPython decodes with
+    ``surrogateescape``, which is lossless in Python but cannot be serialised
+    into JSON: a lone surrogate raises, and the endpoint answered 500 for any
+    repository with one non-UTF-8 byte in a tracked file.
+
+    ``stdout_as_string=False`` skips the decode entirely, so nothing has to be
+    put back together afterwards.
+    """
+    repo = _get_repo(workspace)
+
+    def _restore_trailing_newline(out: bytes) -> bytes:
+        # GitPython strips the final newline from command output. `git apply`
+        # wants it, and `git diff` emits it, so put it back — otherwise the
+        # exported patch differs from git's own by exactly one byte.
+        if out and not out.endswith(b"\n"):
+            return out + b"\n"
+        return out
+
+    try:
+        if not repo.head.is_valid():
+            return b""
+        args = ["--patch", "--full-index"]
+        if staged:
+            args.insert(0, "--cached")
+        return _restore_trailing_newline(
+            repo.git.diff(*args, stdout_as_string=False)
+        )
+    except git.GitCommandError as e:
+        raise ValueError(f"Failed to get patch: {e}")
+
+
 def get_patch(workspace: Workspace, staged: bool = False) -> str:
     """Get patch-formatted diff for export.
 

codeframe/ui/routers/review_v2.py

@@ -7,7 +7,7 @@
 import logging
 from typing import Literal, Optional
 
-from fastapi import APIRouter, Depends, HTTPException, Request
+from fastapi import APIRouter, Depends, HTTPException, Request, Response
 from fastapi.concurrency import run_in_threadpool
 from pydantic import BaseModel, Field
 
@@ -308,33 +308,47 @@ async def get_review_diff(
         raise HTTPException(status_code=500, detail=str(e))
 
 
-@router.get("/patch", response_model=PatchResponse)
+@router.get("/patch")
 @rate_limit_standard()
 async def get_review_patch(
     request: Request,
     staged: bool = False,
     workspace: Workspace = Depends(get_v2_workspace),
-) -> PatchResponse:
+) -> Response:
     """Get patch-formatted diff for export.
 
-    Returns the diff in patch format suitable for `git apply`.
+    Returns the raw bytes `git diff --patch --full-index` produced, as
+    application/octet-stream.
+
+    Not a JSON string (#1077): GitPython decodes with surrogateescape, and a
+    lone surrogate cannot be serialised into JSON — so one non-UTF-8 byte in a
+    tracked file made this endpoint 500, with the /review Export Patch button
+    dead for that repository. A patch is a file that gets fed back to
+    `git apply`, so it has to be byte-faithful end to end; JSON cannot carry
+    those bytes without an encoding step on both sides.
+
+    The suggested filename moves to Content-Disposition, where a file download
+    already expects it.
 
     Args:
         request: HTTP request for rate limiting
         staged: If True, show staged changes; if False, show unstaged
         workspace: v2 Workspace
 
     Returns:
-        PatchResponse with patch content and suggested filename
+        The patch bytes, with a Content-Disposition filename
     """
     try:
-        patch_content = await run_in_threadpool(git.get_patch, workspace, staged=staged)
+        patch_bytes = await run_in_threadpool(
+            git.get_patch_bytes, workspace, staged=staged
+        )
         branch = await run_in_threadpool(git.get_current_branch, workspace)
         filename = f"{branch.replace('/', '-')}.patch"
 
-        return PatchResponse(
-            patch=patch_content,
-            filename=filename,
+        return Response(
+            content=patch_bytes,
+            media_type="application/octet-stream",
+            headers={"Content-Disposition": f'attachment; filename="{filename}"'},
         )
 
     except ValueError as e:

tests/ui/test_review_patch_bytes_1077.py

@@ -0,0 +1,153 @@
+"""#1077 — GET /api/v2/review/patch 500'd on a diff containing a non-UTF-8 byte.
+
+`core/git.get_patch` goes through GitPython, which decodes with
+``surrogateescape``. That is lossless in Python, but a lone surrogate cannot be
+serialised into JSON — so the response raised and FastAPI answered 500. The
+`Export Patch` button on `/review` was simply dead for any repository with one
+Latin-1 comment or a stray byte from a bad merge.
+
+A patch is a file that gets fed back to ``git apply``, so it has to be
+byte-faithful end to end. The endpoint now returns the raw bytes as
+application/octet-stream.
+"""
+
+import subprocess
+
+import pytest
+from fastapi import FastAPI
+from fastapi.testclient import TestClient
+
+from codeframe.core import git
+from codeframe.core.workspace import create_or_load_workspace
+
+pytestmark = pytest.mark.v2
+
+
+def _init_repo(path, final: bytes):
+    subprocess.run(["git", "init", "-q"], cwd=path, check=True)
+    subprocess.run(["git", "config", "user.email", "t@t.test"], cwd=path, check=True)
+    subprocess.run(["git", "config", "user.name", "T"], cwd=path, check=True)
+    target = path / "notes.txt"
+    target.write_bytes(b"original\n")
+    subprocess.run(["git", "add", "-A"], cwd=path, check=True)
+    subprocess.run(["git", "commit", "-qm", "init"], cwd=path, check=True)
+    target.write_bytes(final)
+    return path
+
+
+@pytest.fixture
+def latin1_repo(tmp_path):
+    """A tracked file edited to contain a byte that is not valid UTF-8."""
+    return _init_repo(tmp_path, b"caf\xe9 latin-1\n")
+
+
+@pytest.fixture
+def ascii_repo(tmp_path):
+    return _init_repo(tmp_path, b"plain ascii change\n")
+
+
+def _client(repo):
+    from codeframe.ui.dependencies import get_v2_workspace
+    from codeframe.ui.routers import review_v2
+
+    workspace = create_or_load_workspace(repo)
+    app = FastAPI()
+    app.include_router(review_v2.router)
+    app.dependency_overrides[get_v2_workspace] = lambda: workspace
+    return TestClient(app), workspace
+
+
+class TestTheEndpointNoLongerCrashes:
+    def test_a_non_utf8_diff_is_200(self, latin1_repo):
+        """AC: 200 for a diff containing a non-UTF-8 byte."""
+        client, _ = _client(latin1_repo)
+        res = client.get("/api/v2/review/patch")
+        assert res.status_code == 200, res.text
+
+    def test_the_raw_byte_survives_the_response(self, latin1_repo):
+        client, _ = _client(latin1_repo)
+        res = client.get("/api/v2/review/patch")
+        assert b"\xe9" in res.content
+        # Not the UTF-8 re-encoding of U+00E9, which is what a JSON round-trip
+        # or a str->bytes step would have produced.
+        assert b"\xc3\xa9" not in res.content
+
+    def test_the_filename_moved_to_content_disposition(self, latin1_repo):
+        client, _ = _client(latin1_repo)
+        res = client.get("/api/v2/review/patch")
+        assert "attachment" in res.headers["content-disposition"]
+        assert ".patch" in res.headers["content-disposition"]
+
+
+class TestItIsByteIdenticalToGit:
+    """AC: byte-identical to `git diff --patch --full-index`."""
+
+    @pytest.mark.parametrize("fixture", ["latin1_repo", "ascii_repo"])
+    def test_the_response_matches_git(self, fixture, request):
+        repo = request.getfixturevalue(fixture)
+        client, _ = _client(repo)
+
+        expected = subprocess.run(
+            ["git", "diff", "--patch", "--full-index"],
+            cwd=repo,
+            capture_output=True,
+        ).stdout
+
+        assert client.get("/api/v2/review/patch").content == expected
+
+    @pytest.mark.parametrize("fixture", ["latin1_repo", "ascii_repo"])
+    def test_core_get_patch_bytes_matches_git(self, fixture, request):
+        repo = request.getfixturevalue(fixture)
+        workspace = create_or_load_workspace(repo)
+
+        expected = subprocess.run(
+            ["git", "diff", "--patch", "--full-index"],
+            cwd=repo,
+            capture_output=True,
+        ).stdout
+
+        assert git.get_patch_bytes(workspace) == expected
+
+
+class TestTheExportedPatchApplies:
+    """AC: assert by applying it back with `git apply` and comparing bytes.
+
+    The shape used by tests/core/test_locale_decoding_1029.py — a patch that
+    cannot be applied is not a patch, however well-formed it looks.
+    """
+
+    @pytest.mark.parametrize(
+        "fixture,final",
+        [("latin1_repo", b"caf\xe9 latin-1\n"), ("ascii_repo", b"plain ascii change\n")],
+    )
+    def test_git_apply_reproduces_the_original_bytes(
+        self, fixture, final, request, tmp_path
+    ):
+        repo = request.getfixturevalue(fixture)
+        client, _ = _client(repo)
+        patch_bytes = client.get("/api/v2/review/patch").content
+
+        # A clean clone at the base commit, then apply the exported patch.
+        clone = tmp_path / "clone"
+        subprocess.run(
+            ["git", "clone", "-q", str(repo), str(clone)], check=True, capture_output=True
+        )
+        patch_file = tmp_path / "exported.patch"
+        patch_file.write_bytes(patch_bytes)
+
+        applied = subprocess.run(
+            ["git", "apply", str(patch_file)], cwd=clone, capture_output=True
+        )
+        assert applied.returncode == 0, applied.stderr
+
+        assert (clone / "notes.txt").read_bytes() == final
+
+
+class TestAnAsciiDiffIsUnchanged:
+    """AC: a plain ASCII diff still exports unchanged."""
+
+    def test_it_still_contains_the_change(self, ascii_repo):
+        client, _ = _client(ascii_repo)
+        body = client.get("/api/v2/review/patch").content
+        assert b"plain ascii change" in body
+        assert b"diff --git" in body

tests/ui/test_v2_untested_routers_947.py

@@ -481,13 +481,19 @@ def test_diff_reports_per_file_stats(self, client, repo):
         assert [f["path"] for f in body["changed_files"]] == ["README.md"]
 
     def test_patch_filename_is_derived_from_the_branch(self, client, repo):
+        """The filename moved to Content-Disposition with #1077.
+
+        The response is application/octet-stream now — a patch is a file, and
+        JSON cannot carry a non-UTF-8 byte without an encoding step.
+        """
         (repo / "README.md").write_text("# changed\n")
         branch = _git("git", "branch", "--show-current", cwd=repo).stdout.strip()
 
         res = client.get("/api/v2/review/patch")
 
         assert res.status_code == 200, res.text
-        assert res.json()["filename"] == f"{branch.replace('/', '-')}.patch"
+        expected = f"{branch.replace('/', '-')}.patch"
+        assert expected in res.headers["content-disposition"]
 
     def test_a_generated_commit_message_is_not_empty(self, client, repo):
         (repo / "README.md").write_text("# changed\n")
@@ -691,7 +697,10 @@ class TestUnexpectedCoreFailuresBecome500s:
         ("codeframe.core.git", "get_current_branch", "GET", "/api/v2/git/branch", None),
         ("codeframe.core.git", "is_clean", "GET", "/api/v2/git/clean", None),
         ("codeframe.core.git", "get_diff_stats", "GET", "/api/v2/review/diff", None),
-        ("codeframe.core.git", "get_patch", "GET", "/api/v2/review/patch", None),
+        # get_patch_bytes, not get_patch: the endpoint returns raw bytes since
+        # #1077, and naming the old function left this route unguarded while
+        # the test still passed.
+        ("codeframe.core.git", "get_patch_bytes", "GET", "/api/v2/review/patch", None),
         (
             "codeframe.core.git",
             "generate_commit_message",

web-ui/jest.setup.js

@@ -3,6 +3,18 @@ import '@testing-library/jest-dom';
 // jsdom does not implement scrollIntoView
 window.HTMLElement.prototype.scrollIntoView = jest.fn();
 
+// jsdom ships no TextEncoder/TextDecoder, which every browser has. Node's are
+// the same WHATWG implementation, so this makes the environment match reality
+// rather than working around the gap in production code (#1077).
+import { TextDecoder, TextEncoder } from 'util';
+
+if (typeof global.TextDecoder === 'undefined') {
+  global.TextDecoder = TextDecoder;
+}
+if (typeof global.TextEncoder === 'undefined') {
+  global.TextEncoder = TextEncoder;
+}
+
 // Mock next/navigation
 jest.mock('next/navigation', () => ({
   useRouter() {

web-ui/src/__tests__/components/review/ExportPatchModal.test.tsx

@@ -10,6 +10,14 @@ import userEvent from '@testing-library/user-event';
 import { ExportPatchModal } from '@/components/review/ExportPatchModal';
 
 const PATCH = 'diff --git a/x b/x\n+added line\n';
+// The raw bytes the endpoint returns, written out literally: jsdom has no
+// TextEncoder, and a literal array shows the point anyway — byte 0xe9 is not
+// valid UTF-8, and the download must carry it through unchanged (#1077).
+const PATCH_BYTES = new Uint8Array([
+  0x2b, 0x63, 0x61, 0x66, // "+caf"
+  0xe9, // the non-UTF-8 byte
+  0x0a, // "\n"
+]).buffer;
 
 function setup(overrides: Record<string, unknown> = {}) {
   const onClose = jest.fn();
@@ -18,6 +26,7 @@ function setup(overrides: Record<string, unknown> = {}) {
       open
       onClose={onClose}
       patchContent={PATCH}
+      patchBytes={PATCH_BYTES}
       filename="changes.patch"
       {...overrides}
     />
@@ -84,6 +93,43 @@ describe('ExportPatchModal — copy to clipboard', () => {
 });
 
 describe('ExportPatchModal — download', () => {
+  it('downloads the raw bytes, not the re-encoded display string (#1077)', async () => {
+    const createObjectURL = jest.fn().mockReturnValue('blob:fake');
+    Object.assign(URL, { createObjectURL, revokeObjectURL: jest.fn() });
+
+    const realCreate = document.createElement.bind(document);
+    jest.spyOn(document, 'createElement').mockImplementation((tag: string) => {
+      const el = realCreate(tag);
+      if (tag === 'a') {
+        jest.spyOn(el as HTMLAnchorElement, 'click').mockImplementation(() => {});
+      }
+      return el;
+    });
+
+    // jsdom's Blob has no arrayBuffer(), so capture what the constructor was
+    // handed — which is the claim under test: bytes, not the display string.
+    const RealBlob = global.Blob;
+    const parts: unknown[][] = [];
+    // @ts-expect-error - replacing the constructor for the assertion below
+    global.Blob = function (p: unknown[], opts?: BlobPropertyBag) {
+      parts.push(p);
+      return new RealBlob(p as BlobPart[], opts);
+    };
+
+    setup();
+    await userEvent.click(screen.getByRole('button', { name: /download/i }));
+
+    global.Blob = RealBlob;
+
+    const bytes = new Uint8Array(parts[0][0] as ArrayBuffer);
+    // 0xe9 survives. Building the Blob from `patchContent` would have
+    // re-encoded it as UTF-8 (0xc3 0xa9) and broken `git apply`.
+    expect(Array.from(bytes)).toEqual([0x2b, 0x63, 0x61, 0x66, 0xe9, 0x0a]);
+
+    (document.createElement as jest.Mock).mockRestore();
+  });
+
+
   it('builds a Blob download under the given filename and revokes the URL', async () => {
     const createObjectURL = jest.fn().mockReturnValue('blob:fake');
     const revokeObjectURL = jest.fn();