diff --git a/packages/review-editor/hooks/useFileSemanticChanges.ts b/packages/review-editor/hooks/useFileSemanticChanges.ts index 6acb84de..627e2409 100644 --- a/packages/review-editor/hooks/useFileSemanticChanges.ts +++ b/packages/review-editor/hooks/useFileSemanticChanges.ts @@ -9,32 +9,60 @@ import { isOrphanChange } from '../dock/panels/semanticDiffShared'; /** * Single shared fetch of the semantic diff, cached by the active patch so every * file-header badge reuses one request (sem already caches per-patch server-side). + * + * Failures are retried with backoff and never memoized — in PR mode the server + * parks this request while the checkout warms up (a clone that can take + * minutes), and a connection killed mid-wait must not pin every badge to empty + * for the rest of the session. */ let cacheKey: string | null = null; let cachePromise: Promise | null = null; +const RETRY_DELAYS_MS = [5_000, 15_000, 30_000]; + +async function fetchSemanticDiff(): Promise { + const res = await fetch('/api/semantic-diff'); + if (!res.ok) throw new Error('Semantic diff failed'); + return res.json() as Promise; +} + function loadSemanticDiff(rawPatch: string): Promise { if (cacheKey === rawPatch && cachePromise) return cachePromise; cacheKey = rawPatch; - cachePromise = fetch('/api/semantic-diff') - .then((res) => { - if (!res.ok) throw new Error('Semantic diff failed'); - return res.json() as Promise; - }) - .catch((error): SemanticDiffResponse => ({ - status: 'error', - reason: 'fetch-failed', - message: error instanceof Error ? error.message : String(error), - })) - .then((data) => { - // Logged once per patch (the promise is cached) rather than per badge, so a - // systemic failure leaves a trace instead of every badge vanishing silently. - if (data.status !== 'ok') { - console.error('Failed to load semantic diff for file badges:', data.message ?? data.reason ?? data.status); + + const attempt = async (): Promise => { + for (let i = 0; ; i++) { + let result: SemanticDiffResponse; + try { + result = await fetchSemanticDiff(); + } catch (error) { + result = { + status: 'error', + reason: 'fetch-failed', + message: error instanceof Error ? error.message : String(error), + }; } - return data; - }); - return cachePromise; + // 'unavailable' means sem isn't installed — retrying won't change that. + if (result.status !== 'error' || i >= RETRY_DELAYS_MS.length) return result; + await new Promise((resolve) => setTimeout(resolve, RETRY_DELAYS_MS[i])); + if (cacheKey !== rawPatch) return result; // patch changed mid-retry; let the new fetch win + } + }; + + const promise = attempt().then((data) => { + // Logged once per patch (the promise is cached) rather than per badge, so a + // systemic failure leaves a trace instead of every badge vanishing silently. + if (data.status !== 'ok') { + console.error('Failed to load semantic diff for file badges:', data.message ?? data.reason ?? data.status); + } + if (data.status === 'error' && cacheKey === rawPatch && cachePromise === promise) { + cacheKey = null; + cachePromise = null; + } + return data; + }); + cachePromise = promise; + return promise; } export interface FileSemanticChanges { diff --git a/packages/server/annotate.ts b/packages/server/annotate.ts index 4c99eea7..5cd36116 100644 --- a/packages/server/annotate.ts +++ b/packages/server/annotate.ts @@ -178,6 +178,9 @@ export async function startAnnotateServer( server = Bun.serve({ hostname: getServerHostname(), port: configuredPort, + // Bun's default 10s idleTimeout kills AI SSE streams that stall + // between bytes (e.g. while a permission prompt waits on the user). + idleTimeout: 0, async fetch(req, server) { const url = new URL(req.url); diff --git a/packages/server/goal-setup.ts b/packages/server/goal-setup.ts index 57d9c99c..9854635a 100644 --- a/packages/server/goal-setup.ts +++ b/packages/server/goal-setup.ts @@ -110,6 +110,8 @@ export async function startGoalSetupServer( server = Bun.serve({ hostname: getServerHostname(), port: configuredPort, + // Bun's default 10s idleTimeout kills long-running requests. + idleTimeout: 0, async fetch(req) { const url = new URL(req.url); diff --git a/packages/server/index.ts b/packages/server/index.ts index a2c68b0b..c3363e84 100644 --- a/packages/server/index.ts +++ b/packages/server/index.ts @@ -215,6 +215,9 @@ export async function startPlannotatorServer( server = Bun.serve({ hostname: getServerHostname(), port: configuredPort, + // Bun's default 10s idleTimeout kills AI SSE streams that stall + // between bytes (e.g. while a permission prompt waits on the user). + idleTimeout: 0, async fetch(req, server) { const url = new URL(req.url); diff --git a/packages/server/review.ts b/packages/server/review.ts index f07d3008..91c93c14 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -657,6 +657,10 @@ export async function startReviewServer( server = Bun.serve({ hostname: getServerHostname(), port: configuredPort, + // Bun's default 10s idleTimeout kills requests that legitimately park: + // PR-mode endpoints await the background checkout warmup (a clone that + // can take minutes) and AI SSE streams can stall between bytes. + idleTimeout: 0, async fetch(req, server) { const url = new URL(req.url); diff --git a/packages/shared/pr-github.test.ts b/packages/shared/pr-github.test.ts index 4dfc1c49..4f7d9713 100644 --- a/packages/shared/pr-github.test.ts +++ b/packages/shared/pr-github.test.ts @@ -197,6 +197,9 @@ describe("reconstructGhPatch", () => { expect(lines[0]).toBe("diff --git a/before.ts b/after.ts"); expect(parseDiffGitHeader(lines[0])).toEqual({ oldPath: "before.ts", newPath: "after.ts" }); expect(parseDiffMetadataPathLines(lines)).toEqual({ oldPath: "before.ts", newPath: "after.ts" }); + // Pierre's parser classifies renames off the similarity line — a patched + // rename must carry a sub-100% score or it renders as a plain change. + expect(lines[1]).toBe("similarity index 99%"); }); test("pure rename (no patch field) emits a header-only section", () => { @@ -204,7 +207,9 @@ describe("reconstructGhPatch", () => { { filename: "after.ts", previous_filename: "before.ts", status: "renamed" }, ]); - expect(patch).toBe("diff --git a/before.ts b/after.ts\nrename from before.ts\nrename to after.ts\n"); + expect(patch).toBe( + "diff --git a/before.ts b/after.ts\nsimilarity index 100%\nrename from before.ts\nrename to after.ts\n", + ); }); test("entry without patch (binary / per-file too large) doesn't corrupt the next file's section", () => { @@ -272,6 +277,7 @@ describe("reconstructGhPatch", () => { { filename: "copy.ts", previous_filename: "orig.ts", status: "copied", patch: "@@ -1 +1 @@\n-a\n+b" }, ]); + expect(patch).toContain("similarity index 99%"); expect(patch).toContain("copy from orig.ts"); expect(patch).toContain("copy to copy.ts"); expect(patch.split("\n")[0]).toBe("diff --git a/orig.ts b/copy.ts"); diff --git a/packages/shared/pr-github.ts b/packages/shared/pr-github.ts index 1ca1de80..095f854b 100644 --- a/packages/shared/pr-github.ts +++ b/packages/shared/pr-github.ts @@ -103,10 +103,17 @@ export function reconstructGhPatch(files: GitHubFileEntry[]): string { const isDeleted = f.status === "removed"; let header = `diff --git ${headerPathToken("a", oldPath)} ${headerPathToken("b", newPath)}`; - if (f.status === "renamed") { - header += `\nrename from ${metadataPathToken(oldPath)}\nrename to ${metadataPathToken(newPath)}`; - } else if (f.status === "copied") { - header += `\ncopy from ${metadataPathToken(oldPath)}\ncopy to ${metadataPathToken(newPath)}`; + if (f.status === "renamed" || f.status === "copied") { + // Git always prints a similarity score before rename/copy lines, and + // diff parsers (e.g. Pierre's) key rename classification off it — + // without the line a rename renders as a plain change with no old path. + // The files API doesn't expose the score: a patch-less entry is by + // definition a 100% match; for patched entries emit a synthetic <100% + // value (consumers only branch on 100% vs not). + header += f.patch ? "\nsimilarity index 99%" : "\nsimilarity index 100%"; + header += f.status === "renamed" + ? `\nrename from ${metadataPathToken(oldPath)}\nrename to ${metadataPathToken(newPath)}` + : `\ncopy from ${metadataPathToken(oldPath)}\ncopy to ${metadataPathToken(newPath)}`; } if (isNew) { header += "\nnew file mode 100644"; diff --git a/packages/shared/pr-gitlab.test.ts b/packages/shared/pr-gitlab.test.ts index 9dedf08a..5904afbd 100644 --- a/packages/shared/pr-gitlab.test.ts +++ b/packages/shared/pr-gitlab.test.ts @@ -152,6 +152,38 @@ describe("fetchGlMR raw_diffs fallback", () => { expect(calls.some((c) => c.includes("/diffs?per_page=100"))).toBe(true); }); + test("reconstructed renames carry a similarity line so parsers classify them as renames", async () => { + const entries = JSON.stringify([ + { + old_path: "src/old.ts", + new_path: "src/new.ts", + new_file: false, + deleted_file: false, + renamed_file: true, + diff: "", // pure rename — GitLab sends an empty diff + }, + { + old_path: "src/before.ts", + new_path: "src/after.ts", + new_file: false, + deleted_file: false, + renamed_file: true, + diff: "@@ -1 +1 @@\n-a\n+b\n", + }, + ]); + const { runtime } = gitlabRuntime({ + rawDiffs: { exitCode: 1, stderr: "404 Not Found" }, + diffs: { exitCode: 0, stdout: entries }, + }); + const result = await fetchGlMR(runtime, REF); + expect(result.rawPatch).toContain( + "diff --git a/src/old.ts b/src/new.ts\nsimilarity index 100%\nrename from src/old.ts\nrename to src/new.ts", + ); + expect(result.rawPatch).toContain( + "diff --git a/src/before.ts b/src/after.ts\nsimilarity index 99%\nrename from src/before.ts\nrename to src/after.ts", + ); + }); + test("falls back when raw_diffs returns empty (oversized MR)", async () => { const { runtime, calls } = gitlabRuntime({ rawDiffs: { exitCode: 0, stdout: "" }, diff --git a/packages/shared/pr-gitlab.ts b/packages/shared/pr-gitlab.ts index ab8edb3e..e0125072 100644 --- a/packages/shared/pr-gitlab.ts +++ b/packages/shared/pr-gitlab.ts @@ -63,6 +63,10 @@ function reconstructPatch(diffs: GitLabDiffEntry[]): string { let header = `diff --git a/${displayOld} b/${displayNew}`; if (d.renamed_file) { + // Diff parsers (e.g. Pierre's) key rename classification off the + // similarity line; the API doesn't expose the score, so emit 100% for + // pure renames (empty diff) and a synthetic <100% otherwise. + header += d.diff.trim() === "" ? "\nsimilarity index 100%" : "\nsimilarity index 99%"; header += `\nrename from ${d.old_path}\nrename to ${d.new_path}`; } if (d.new_file) {