From b5f728af6523b89125fcf505335a9bb445578111 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:18:16 -0700 Subject: [PATCH 1/2] fix(review): export the patch as bytes, not a JSON string (#1077) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/v2/review/patch answered 500 for any repository with one non-UTF-8 byte in a tracked file. GitPython decodes with surrogateescape — lossless in Python, but a lone surrogate cannot be serialised into JSON, so the response raised. The Export Patch button on /review was dead for those repos, with a 500 indistinguishable from the server being down. A patch is a file that gets fed back to `git apply`, so it has to be byte-faithful end to end. Fixing only the 500 would have left a patch that reached the browser and still failed to apply — the issue is explicit that both halves need doing together. Backend: - New git.get_patch_bytes() using GitPython's stdout_as_string=False, so there is no decode to undo. GitPython strips the trailing newline that `git diff` emits and `git apply` wants, so it is restored — my first version was byte-identical except for that one byte, caught only by diffing against git rather than checking the 0xe9 byte survived. - The endpoint returns application/octet-stream with the filename in Content-Disposition, where a file download already expects it. Frontend: - reviewApi.getPatch fetches arraybuffer and returns both the bytes and a decoded string; the string is for display and clipboard only. - ExportPatchModal builds its Blob from the bytes. It previously re-encoded the string as UTF-8, so even a successful response would not round-trip. jest.setup.js gains TextEncoder/TextDecoder from Node's util: jsdom ships neither, and every browser has them. Polyfilling the environment beats contorting production code around a test-environment gap — the file already does this for scrollIntoView. Two existing tests broke, and the second matters: - 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`, which this endpoint no longer calls — so the "unexpected core failure becomes a 500" guard had silently stopped covering this route while still passing. Now points at get_patch_bytes. Backend: 6431 passed, 49 skipped. Web-UI: 1278 passed, lint and build clean. --- codeframe/core/git.py | 35 ++++ codeframe/ui/routers/review_v2.py | 32 ++-- tests/ui/test_review_patch_bytes_1077.py | 153 ++++++++++++++++++ tests/ui/test_v2_untested_routers_947.py | 13 +- web-ui/jest.setup.js | 12 ++ .../review/ExportPatchModal.test.tsx | 46 ++++++ web-ui/src/__tests__/lib/api.contract.test.ts | 19 ++- web-ui/src/app/review/page.tsx | 3 + .../components/review/ExportPatchModal.tsx | 10 +- web-ui/src/lib/api.ts | 16 +- web-ui/src/types/index.ts | 7 + 11 files changed, 329 insertions(+), 17 deletions(-) create mode 100644 tests/ui/test_review_patch_bytes_1077.py diff --git a/codeframe/core/git.py b/codeframe/core/git.py index 24e2a07b..10681c7b 100644 --- a/codeframe/core/git.py +++ b/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. diff --git a/codeframe/ui/routers/review_v2.py b/codeframe/ui/routers/review_v2.py index 71918de2..81ed24ba 100644 --- a/codeframe/ui/routers/review_v2.py +++ b/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,16 +308,27 @@ 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 @@ -325,16 +336,19 @@ async def get_review_patch( 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: diff --git a/tests/ui/test_review_patch_bytes_1077.py b/tests/ui/test_review_patch_bytes_1077.py new file mode 100644 index 00000000..19042888 --- /dev/null +++ b/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 diff --git a/tests/ui/test_v2_untested_routers_947.py b/tests/ui/test_v2_untested_routers_947.py index a8074bd6..de419137 100644 --- a/tests/ui/test_v2_untested_routers_947.py +++ b/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", diff --git a/web-ui/jest.setup.js b/web-ui/jest.setup.js index 37a7063e..3c799e2b 100644 --- a/web-ui/jest.setup.js +++ b/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() { diff --git a/web-ui/src/__tests__/components/review/ExportPatchModal.test.tsx b/web-ui/src/__tests__/components/review/ExportPatchModal.test.tsx index 20daa04d..66c75783 100644 --- a/web-ui/src/__tests__/components/review/ExportPatchModal.test.tsx +++ b/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 = {}) { const onClose = jest.fn(); @@ -18,6 +26,7 @@ function setup(overrides: Record = {}) { 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(); diff --git a/web-ui/src/__tests__/lib/api.contract.test.ts b/web-ui/src/__tests__/lib/api.contract.test.ts index 7f95dd90..565e283f 100644 --- a/web-ui/src/__tests__/lib/api.contract.test.ts +++ b/web-ui/src/__tests__/lib/api.contract.test.ts @@ -397,9 +397,24 @@ describe('api.ts request contract', () => { expect(captured.params).toEqual({ workspace_path: '/ws', staged: true }); }); - it('getPatch → GET /api/v2/review/patch', async () => { - await reviewApi.getPatch('/ws'); + it('getPatch → GET /api/v2/review/patch as bytes', async () => { + // The endpoint returns application/octet-stream since #1077 — a patch is + // a file fed back to `git apply`, and JSON cannot carry a non-UTF-8 byte. + stubResponseData = new Uint8Array([0x2b, 0x63, 0x61, 0x66, 0xe9]).buffer; + + const result = await reviewApi.getPatch('/ws'); + expect(captured.url).toBe('/api/v2/review/patch'); + expect(result.bytes).toBe(stubResponseData); + // The display string is decoded from those bytes; the invalid byte + // becomes U+FFFD here, which is fine for a textarea and never downloaded. + expect(result.patch.startsWith('+caf')).toBe(true); + }); + + it('getPatch falls back to a default filename without Content-Disposition', async () => { + stubResponseData = new Uint8Array([0x61]).buffer; + const result = await reviewApi.getPatch('/ws'); + expect(result.filename).toBe('changes.patch'); }); it('generateCommitMessage → POST /api/v2/review/commit-message', async () => { diff --git a/web-ui/src/app/review/page.tsx b/web-ui/src/app/review/page.tsx index b5a9b9fb..0d414b4a 100644 --- a/web-ui/src/app/review/page.tsx +++ b/web-ui/src/app/review/page.tsx @@ -49,6 +49,7 @@ export default function ReviewPage() { // Modal states const [showPatchModal, setShowPatchModal] = useState(false); const [patchContent, setPatchContent] = useState(''); + const [patchBytes, setPatchBytes] = useState(null); const [patchFilename, setPatchFilename] = useState(''); const [showPRModal, setShowPRModal] = useState(false); const [prUrl, setPrUrl] = useState(''); @@ -166,6 +167,7 @@ export default function ReviewPage() { try { const result = await reviewApi.getPatch(workspacePath); setPatchContent(result.patch); + setPatchBytes(result.bytes); setPatchFilename(result.filename); setShowPatchModal(true); } catch (err) { @@ -373,6 +375,7 @@ export default function ReviewPage() { open={showPatchModal} onClose={() => setShowPatchModal(false)} patchContent={patchContent} + patchBytes={patchBytes} filename={patchFilename} /> diff --git a/web-ui/src/components/review/ExportPatchModal.tsx b/web-ui/src/components/review/ExportPatchModal.tsx index 70b84925..71dbc1c5 100644 --- a/web-ui/src/components/review/ExportPatchModal.tsx +++ b/web-ui/src/components/review/ExportPatchModal.tsx @@ -17,6 +17,8 @@ interface ExportPatchModalProps { open: boolean; onClose: () => void; patchContent: string; + /** The exact git bytes; downloads use these, not the string (#1077). */ + patchBytes: ArrayBuffer | null; filename: string; } @@ -24,6 +26,7 @@ export function ExportPatchModal({ open, onClose, patchContent, + patchBytes, filename, }: ExportPatchModalProps) { const [copied, setCopied] = useState(false); @@ -47,7 +50,10 @@ export function ExportPatchModal({ }, [patchContent]); const handleDownload = useCallback(() => { - const blob = new Blob([patchContent], { type: 'text/plain' }); + // From bytes, not the decoded string: re-encoding as UTF-8 corrupts any + // non-UTF-8 byte and the patch then fails `git apply` (#1077). + if (!patchBytes) return; + const blob = new Blob([patchBytes], { type: 'application/octet-stream' }); const url = URL.createObjectURL(blob); const a = document.createElement('a'); a.href = url; @@ -56,7 +62,7 @@ export function ExportPatchModal({ a.click(); document.body.removeChild(a); URL.revokeObjectURL(url); - }, [patchContent, filename]); + }, [patchBytes, filename]); return ( { if (!isOpen) onClose(); }}> diff --git a/web-ui/src/lib/api.ts b/web-ui/src/lib/api.ts index 1fdb616a..b7d48b7c 100644 --- a/web-ui/src/lib/api.ts +++ b/web-ui/src/lib/api.ts @@ -746,10 +746,22 @@ export const reviewApi = { }, getPatch: async (workspacePath: string, staged?: boolean): Promise => { - const response = await api.get('/api/v2/review/patch', { + // arraybuffer, not JSON (#1077): a patch is a file that gets fed back to + // `git apply`, and JSON cannot carry a non-UTF-8 byte — the endpoint used + // to 500 on one. The decoded string is for display only. + const response = await api.get('/api/v2/review/patch', { params: { workspace_path: workspacePath, ...(staged ? { staged } : {}) }, + responseType: 'arraybuffer', }); - return response.data; + + const disposition = response.headers['content-disposition'] ?? ''; + const match = /filename="?([^"]+)"?/.exec(disposition); + + return { + bytes: response.data, + patch: new TextDecoder().decode(response.data), + filename: match?.[1] ?? 'changes.patch', + }; }, generateCommitMessage: async (workspacePath: string, staged?: boolean): Promise => { diff --git a/web-ui/src/types/index.ts b/web-ui/src/types/index.ts index 4b34aa58..61ae4cf7 100644 --- a/web-ui/src/types/index.ts +++ b/web-ui/src/types/index.ts @@ -240,7 +240,14 @@ export interface DiffStatsResponse { } export interface PatchResponse { + /** Decoded for display and copy-to-clipboard. Lossy for non-UTF-8 bytes. */ patch: string; + /** + * The exact bytes git produced. Downloads must use these, not `patch` — + * re-encoding the string as UTF-8 breaks `git apply` for any diff containing + * a non-UTF-8 byte (#1077). + */ + bytes: ArrayBuffer; filename: string; } From fb5ca4d9c1d79e00f4cb8048dd9a7cd7d4ee92ec Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:37:14 -0700 Subject: [PATCH 2/2] fix(web-ui): recover the error detail from a binary response body (#1077) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding, and a regression this PR introduced. With responseType: 'arraybuffer', the ERROR body arrives as an ArrayBuffer too, so the interceptor's `data.detail` was undefined and the backend's reason collapsed into axios's generic "Request failed with status code 500". The Export Patch toast used to read "Failed to get patch: " — I fixed a 500 while degrading the message for every other failure on the route. Fixed in the shared interceptor, which is the only place it can be: by the time a per-call catch runs the buffer is gone and the rejection carries only the generic message. The check is Object.prototype.toString, not `instanceof ArrayBuffer`. The instanceof failed in jest because jsdom's ArrayBuffer and Node's are different realms — a test artifact, but it points at real fragility: a buffer from an iframe would fail the same check in a browser. Two tests: the git reason survives a binary error body, and a non-JSON binary body (an HTML gateway-timeout page) falls back to axios's message instead of throwing. Verified non-tautological by reverting the decode. Web-UI: 1280 passed, lint and build clean. Backend: 6431 passed, 49 skipped. --- web-ui/src/__tests__/lib/api.auth.test.ts | 36 +++++++++++++++++++++++ web-ui/src/lib/api.ts | 28 +++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/web-ui/src/__tests__/lib/api.auth.test.ts b/web-ui/src/__tests__/lib/api.auth.test.ts index 1635f791..4ad004a2 100644 --- a/web-ui/src/__tests__/lib/api.auth.test.ts +++ b/web-ui/src/__tests__/lib/api.auth.test.ts @@ -113,4 +113,40 @@ describe('api response interceptor — 401 handling', () => { ).rejects.toMatchObject({ detail: 'Boom', status_code: 500 }); expect(mockRedirectTo).not.toHaveBeenCalled(); }); + + it('recovers the detail from a binary error body (#1077)', async () => { + // getPatch uses responseType: 'arraybuffer', so its ERROR body arrives as + // an ArrayBuffer too. Without decoding it the backend's reason is dropped + // and the user sees axios's generic "Request failed with status code 500". + const body = JSON.stringify({ detail: 'Failed to get patch: not a git repository' }); + const bytes = new TextEncoder().encode(body).buffer; + + const handler = getResponseRejectHandler(); + + await expect( + handler({ + message: 'Request failed with status code 500', + response: { status: 500, data: bytes }, + } as unknown as AxiosError) + ).rejects.toMatchObject({ + detail: 'Failed to get patch: not a git repository', + status_code: 500, + }); + }); + + it('falls back to the axios message when a binary body is not JSON', async () => { + const bytes = new TextEncoder().encode('gateway timeout').buffer; + + const handler = getResponseRejectHandler(); + + await expect( + handler({ + message: 'Request failed with status code 504', + response: { status: 504, data: bytes }, + } as unknown as AxiosError) + ).rejects.toMatchObject({ + detail: 'Request failed with status code 504', + status_code: 504, + }); + }); }); diff --git a/web-ui/src/lib/api.ts b/web-ui/src/lib/api.ts index b7d48b7c..f8bb6da8 100644 --- a/web-ui/src/lib/api.ts +++ b/web-ui/src/lib/api.ts @@ -140,6 +140,29 @@ const createApiClient = (): AxiosInstance => { return config; }); + // A binary response (responseType: 'arraybuffer', used by getPatch since + // #1077) delivers its ERROR body as an ArrayBuffer too, so `data.detail` is + // undefined and the backend's reason collapses into axios's generic + // "Request failed with status code 500". Decode it back first. This has to + // live in the interceptor: by the time a per-call catch runs, the buffer is + // gone and the rejection carries only the generic message. + // Object.prototype.toString rather than `instanceof ArrayBuffer`: the check + // has to hold across realms, and an ArrayBuffer built in another one (Node's + // in jsdom, an iframe in a browser) fails the instanceof. + const isArrayBuffer = (v: unknown): v is ArrayBuffer => + Object.prototype.toString.call(v) === '[object ArrayBuffer]'; + + const extractDetail = (data: unknown): FastApiErrorDetail | undefined => { + if (isArrayBuffer(data)) { + try { + return JSON.parse(new TextDecoder().decode(data))?.detail; + } catch { + return undefined; // not JSON — fall back to axios's message + } + } + return (data as { detail?: FastApiErrorDetail } | undefined)?.detail; + }; + // Add response interceptor for error handling client.interceptors.response.use( (response) => response, @@ -156,7 +179,10 @@ const createApiClient = (): AxiosInstance => { // Transform error for consistent handling const apiError: ApiError = { - detail: normalizeErrorDetail(error.response?.data?.detail, error.message), + detail: normalizeErrorDetail( + extractDetail(error.response?.data), + error.message + ), status_code: error.response?.status, }; return Promise.reject(apiError);