Skip to content

[P2.24] GET /api/v2/review/patch 500s on a diff containing a non-UTF-8 byte #1077

Description

@frankbria

Found while fixing the sibling defect in artifacts.export_patch (#1029, PR #1073). Different code path, so it was out of scope there.

Reproduced

A repo whose working tree contains one non-UTF-8 byte — a Latin-1 comment, a stray byte from a bad merge:

(repo / "notes.txt").write_bytes(b"caf\xe9 latin-1\n")   # committed as b"original\n"
client.get("/api/v2/review/patch")
# status: 500
# body:   Internal Server Error

The Export Patch button on /review is therefore dead for that repository, with no actionable error.

Mechanism

core/git.get_patch goes through GitPython, not subprocess, so it was not part of #1029's audit and is not locale-dependent. GitPython decodes with surrogateescape:

git.get_patch(ws)  ->  '+caf\udce9 latin-1'
raw git bytes      ->  b'+caf\xe9 latin-1'

That is genuinely lossless in Python — "\udce9".encode("utf-8", "surrogateescape") gives b"\xe9" back. The failure is one layer up: a lone surrogate cannot be encoded into JSON, so serialising PatchResponse.patch raises and FastAPI returns 500.

Why it is P2, not P1

It needs a non-UTF-8 byte in a tracked file, which is uncommon, and the CLI path (cf patch export) is unaffected — #1029 made that one byte-faithful. So there is a working way to get the patch. But the web button is simply broken for those repos, and a 500 is indistinguishable from the server being down.

Suggested direction

The response carries a file, so it should be byte-faithful end to end, the same conclusion #1029 reached for the CLI:

  • return the patch as application/octet-stream (or base64 in the JSON) rather than a JSON string, and have ExportPatchModal build its Blob from those bytes; or
  • if it must stay a JSON string, encode with surrogateescape and hand the frontend something it can turn back into the original bytes.

Note web-ui/src/components/review/ExportPatchModal.tsx:50 currently does new Blob([patchContent], {type: 'text/plain'}), so even a successful response re-encodes as UTF-8 — a patch that reached the browser intact would still not round-trip through git apply. Both halves need doing together.

Acceptance criteria

  • GET /api/v2/review/patch returns 200 for a diff containing a non-UTF-8 byte
  • The file the browser downloads is byte-identical to git diff --patch --full-index, asserted by applying it back with git apply and comparing bytes (the test shape used in tests/core/test_locale_decoding_1029.py::TestAnExportedPatchIsByteFaithful)
  • ExportPatchModal builds its Blob from bytes, not from a re-encoded string
  • A plain ASCII diff still exports unchanged

Evidence

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions