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

[frankbria]

Closes #1077.

Both halves, because either alone is useless

The 500 and the browser-side re-encode are the same bug wearing two hats: a
patch is a file that gets fed back to git apply. Fixing only the crash
would have produced a patch that arrives intact and still fails to apply.

Backend — new git.get_patch_bytes() using GitPython's
stdout_as_string=False, so there is no surrogateescape decode to undo. The
endpoint returns application/octet-stream, with the filename moved to
Content-Disposition where a file download already expects it.

Frontend — reviewApi.getPatch fetches arraybuffer and returns the bytes
alongside a decoded string used only for the textarea and clipboard.
ExportPatchModal builds its Blob from the bytes.

Caught by verifying rather than assuming

GitPython strips the trailing newline. My first version was byte-identical
to git diff except for one missing \n — which git apply wants. I only
found it because I compared against git diff directly instead of checking that
the 0xe9 byte survived. Restored, and asserted.

Evidence

The test that matters applies the exported patch to a clean clone and compares
bytes:

subprocess.run(["git", "apply", str(patch_file)], cwd=clone)
assert (clone / "notes.txt").read_bytes() == final   # b"caf\xe9 latin-1\n"

GET /review/patch on a Latin-1 diff	200 (was 500)
response vs git diff --patch --full-index	byte-identical, ASCII and non-UTF-8
git apply round-trip	reproduces the original bytes exactly
downloaded Blob	[0x2b, 0x63, 0x61, 0x66, 0xe9, 0x0a] — 0xe9 intact, not 0xc3 0xa9
plain ASCII diff	unchanged

The Blob test is verified non-tautological: restoring the old
new Blob([patchContent]) fails it.

Two existing tests broke, and the second is the interesting one

• test_patch_filename_is_derived_from_the_branch read filename from the JSON
  body; it now reads Content-Disposition.
• test_a_raising_core_call_is_a_500 patched git.get_patch — a function
  this endpoint no longer calls. So the "unexpected core failure becomes a 500"
  guard returned 200 and had silently stopped covering this route. Repointed
  at get_patch_bytes.

That is the second time in two issues that redirecting an endpoint's call path
quietly detached a safety test from the thing it guards (the first was two
path-containment tests in #1066). Worth watching for.

jest.setup.js

Gains TextEncoder/TextDecoder from Node's util. jsdom ships neither and
every browser has them, so this makes the test environment match reality rather
than working around the gap in production code — the file already polyfills
scrollIntoView in the same style.

Acceptance criteria

• 200 for a diff containing a non-UTF-8 byte
• Byte-identical to git diff --patch --full-index, asserted via git apply
• ExportPatchModal builds its Blob from bytes
• A plain ASCII diff still exports unchanged

Backend: 6431 passed, 49 skipped. Web-UI: 1278 passed, lint and build clean.

Breaking change

GET /api/v2/review/patch no longer returns JSON. Any consumer reading
response.patch needs the arraybuffer treatment — the in-repo client is updated
here, and that reshaping is the fix, not a side effect.

[github-actions]

Claude finished @frankbria's task in 5m 41s —— View job

---

GLM precision review — done

Scoped to concrete defects only (logic, crashes, security, data loss, contract breaks). Style, architecture, naming, and test coverage are covered by the general review.

1 finding (minor):

Severity	File:line	Finding
minor	web-ui/src/lib/api.ts:754	responseType: 'arraybuffer' makes non-2xx bodies reach the global interceptor as an ArrayBuffer, so error.response.data.detail is undefined and the backend's specific error ("Failed to get patch: …") degrades to axios's generic "Request failed with status code 500" in the export toast. Regression vs. the prior JSON path. Fix belongs in the shared interceptor (decode arraybuffer error bodies), not this line.

Verified clean (no defect):

• git.get_patch_bytes — stdout_as_string=False returns raw bytes; _restore_trailing_newline is idempotent and no-ops on empty output; GitCommandError→ValueError→400, broad Exception→500 still wired on the route.
• Content-Disposition filename is derived from a git ref name; git's ref-format forbids CR/LF/control chars, so no header-response-splitting vector via branch.
• new TextDecoder().decode(response.data) is non-fatal → invalid bytes become U+FFFD (display only); downloads use bytes, not the string.
• handleDownload's if (!patchBytes) return is a silent no-op in theory, but getPatch always returns a non-null ArrayBuffer and setPatchBytes runs before the modal opens — no reachable null-download in production.
• PatchResponse Pydantic model in review_v2.py:100 is now dead code (endpoint dropped response_model), but that is a cleanup item, not a defect.
• No other consumer of the endpoint's old JSON shape was missed — the only caller (reviewApi.getPatch) is updated here; CLI uses core directly.

[claude]

Review

The core fix is sound: get_patch_bytes() correctly avoids the surrogateescape round-trip by using stdout_as_string=False, the trailing-newline restoration is a real gotcha and is handled + asserted, and the frontend change to build the download Blob from raw bytes instead of the decoded string closes the loop. Test coverage for the byte-fidelity claim (git apply round-trip on a clean clone) is a good verification strategy, and catching that test_a_raising_core_call_is_a_500 had silently stopped guarding this route is a genuinely useful catch.

Two things worth a look before merge:

1. Content-Disposition filename isn't sanitized against the branch name — likely regression for non-ASCII branches (codeframe/ui/routers/review_v2.py:351)

headers={"Content-Disposition": f'attachment; filename="{filename}"'},

filename is built directly from branch.replace('/', '-'). Two issues:

• Starlette encodes header values as Latin-1 when building the ASGI response (Response.init_headers → value.encode("latin-1")). Any branch name containing a character outside Latin-1 — e.g. CJK, most emoji, feature/日本語 — will raise inside the try block and now turn into an unhandled-looking 500 via the generic except Exception handler. Previously this worked fine because the filename traveled inside a JSON body, which handles UTF-8 natively. This looks like a real regression for any non-ASCII branch name, not just an edge case.
• Git branch names are allowed to contain a literal " (it's not in git-check-ref-format's disallowed set: control chars, space, ~^:?*[\\, leading/trailing /, .., @{). A branch like foo"bar would break the quoted-string syntax of the header (e.g. a branch such as foo"; filename="evil.exe is a legal ref name and would corrupt/spoof the filename parameter downstream). Worth using something like urllib.parse.quote() / a filename*=UTF-8''... fallback, or at minimum stripping/escaping " and non-ASCII from the derived filename.

2. get_patch_bytes requires a valid HEAD even for the unstaged diff, unlike its siblings (codeframe/core/git.py, new get_patch_bytes)

if not repo.head.is_valid():
    return b""
args = ["--patch", "--full-index"]
if staged:
    args.insert(0, "--cached")

This check runs before branching on staged. But get_diff() (used by the diff-review view) and the old get_patch() only require a valid HEAD for the staged case — the unstaged case is repo.git.diff() compared against the index directly, which needs no HEAD and works fine in a repo with no commits yet:

# get_diff, for comparison
if staged:
    if repo.head.is_valid():
        return repo.git.diff("--cached")
    return ""
else:
    return repo.git.diff()   # <-- no HEAD check

So in a workspace with no commits yet but staged + further-modified files (plausible right after cf init, before the first commit), GET /review/diff?staged=false will show the real diff, while GET /review/patch?staged=false will now silently return empty bytes — an inconsistency between the two views of the same state, and a divergence from git diff --patch --full-index run directly (which doesn't care about HEAD either). None of the new tests in test_review_patch_bytes_1077.py exercise a repo without an initial commit — all fixtures commit before mutating — so this gap isn't caught. Worth either dropping the unconditional HEAD check (mirroring get_diff's per-branch logic) or confirming this is intentional and covering it with a test.

Minor: PatchResponse (the Pydantic model, codeframe/ui/routers/review_v2.py:100) is no longer used anywhere in the file now that the endpoint returns a raw Response — looks like dead code left over from the refactor.

Nothing else stood out — error handling, rate limiting, and the auth wiring on this router are all unchanged and fine. The jest polyfill and contract-test updates look correct and match the stated goal (environment matching reality rather than working around it in prod code).

[github-actions]

Claude finished @frankbria's task in 3m 6s —— View job

---

GLM precision review — done

Re-scoped to the new commit fb5ca4d (the synchronize that responded to the prior interceptor finding). Concrete defects only.

✅ No new defects introduced by fb5ca4d. The interceptor fix is correct and complete:

• getPatch (web-ui/src/lib/api.ts:778) is the only axios call with responseType: 'arraybuffer' — the useTerminalSocket.ts match is a WebSocket binaryType, unrelated. So the shared-interceptor change affects only this route's error path; no other consumer relied on a raw-buffer error shape.
• extractDetail preserves the prior JSON path verbatim (the non-arraybuffer branch is the same data?.detail). undefined/null/string/object bodies all behave as before; a validation-error array as detail still flows correctly into normalizeErrorDetail.
• JSON.parse(new TextDecoder().decode(data))?.detail is safe across every parsed shape (object→string, primitive/null→undefined, syntax error→caught→fallback to axios's message). The two new tests cover both the recovered-detail and the non-JSON-fallback paths.
• The 401 redirect branch is keyed on error.response.status, not body type — unaffected.
• handleExportPatch (review/page.tsx:174) reads (err as ApiError).detail, which the fix now populates — the git reason is restored to the export toast. Loop closed.

Still open (not re-posted — already on the PR from the prior claude review, and this commit doesn't touch these files):

• Content-Disposition filename derived directly from branch → Latin-1 encode crash for non-ASCII branch names (CJK/emoji) + "-quoting break for legal refs like foo"bar (codeframe/ui/routers/review_v2.py:351).
• get_patch_bytes requires a valid HEAD even for the unstaged patch, diverging from get_diff and from bare git diff in a not-yet-committed repo (codeframe/core/git.py, new get_patch_bytes).

Both live in files this commit didn't modify, so they're unaffected by fb5ca4d.

[claude]

Review

The core byte-fidelity fix is solid: get_patch_bytes() avoids the surrogateescape round-trip with stdout_as_string=False, the trailing-newline restoration is a real gotcha that's correctly handled and asserted, and the git apply-round-trip test in test_review_patch_bytes_1077.py is exactly the right way to verify a byte-faithful claim rather than eyeballing it.

The second commit (fb5ca4d) correctly fixes the regression flagged by both prior reviews (the GLM bot and my own): decoding ArrayBuffer error bodies in the shared interceptor so the backend's specific error message survives responseType: 'arraybuffer', with tests for both the JSON-in-binary case and the non-JSON (e.g. gateway timeout HTML) fallback. Good catch using Object.prototype.toString instead of instanceof ArrayBuffer for the cross-realm issue.

The other two findings from my previous review on this PR are still open — I re-checked the current diff and neither has been addressed:

1. Content-Disposition filename isn't sanitized (codeframe/ui/routers/review_v2.py, get_review_patch)

filename = f"{branch.replace('/', '-')}.patch"
...
headers={"Content-Disposition": f'attachment; filename="{filename}"'},

Still unchanged. Two issues stand:

• Starlette encodes header values as Latin-1; a branch name with any non-Latin-1 character (CJK, emoji, feature/日本語) raises inside the handler, and the endpoint's broad except Exception turns that into a 500 — a regression versus the old JSON path, which handled UTF-8 fine.
• " is a legal character in a git ref name (not excluded by git-check-ref-format), so a branch like foo"; filename="evil.exe breaks the quoted-string syntax of the header and can spoof/corrupt the filename parameter downstream.

None of the new tests exercise a non-ASCII or quote-containing branch name, so this gap isn't caught by the suite either.

2. get_patch_bytes requires a valid HEAD even for the unstaged diff (codeframe/core/git.py)

if not repo.head.is_valid():
    return b""
args = ["--patch", "--full-index"]
if staged:
    args.insert(0, "--cached")

The HEAD check runs unconditionally, before branching on staged. get_diff() (and the old get_patch()) only require a valid HEAD for the staged case — the unstaged case is repo.git.diff() against the index, which works fine with zero commits. So in a fresh workspace before the first commit, GET /review/diff?staged=false shows the real diff while GET /review/patch?staged=false silently returns empty bytes — an inconsistency between the two views, uncaught because every fixture in test_review_patch_bytes_1077.py commits before mutating.

Minor, still applies: PatchResponse (the Pydantic model in review_v2.py) is dead code now that the endpoint returns a raw Response with no response_model — unused, per the GLM bot's finding too.

Everything else lines up with what's described in the PR body: ExportPatchModal builds its Blob from patchBytes (not the decoded string), reviewApi.getPatch correctly parses the filename out of Content-Disposition with a sane fallback, the jest.setup.js polyfill is a reasonable fix for a jsdom gap rather than the alternative of contorting production code, and the two broken pre-existing tests were updated for the right reasons (one is a genuine regression-test drift, called out well in the PR description).

Given #1 and #2 are still outstanding from the prior round and are real correctness/security-adjacent gaps (header injection potential, silent data loss for a plausible pre-first-commit workflow), I'd hold off on merging until at least the filename sanitization is addressed — the HEAD-check inconsistency is lower urgency but should get a tracking note if not fixed here.