From 7205cfeee87314f689f027fad74ee74aeacf7303 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 18:25:19 -0700 Subject: [PATCH 01/14] feat(review): large GitHub PR fallback + non-blocking PR checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two PR-mode improvements: 1. Large GitHub PRs no longer fail to load. When `gh pr diff` is refused (HTTP 406 for oversized diffs), fetchGhPR pages through the pulls files API and stitches the per-file patches into a unified diff — mirroring the existing GitLab raw_diffs fallback. Path quoting matches git's exact rules (bare spaces unquoted) so downstream parsers round-trip; truncation at the API's 3000-file cap is surfaced, never silent. 2. The --local worktree/clone no longer blocks startup. The review server opens as soon as the platform diff arrives; the checkout warms in the background as a seeded not-ready pool entry. Consumers that need real files (agent jobs, full-stack diff, code-nav, semantic diff, AI sessions) await pool.ensure(), with creations serialized so concurrent fetches can't clobber the shared FETCH_HEAD. Cross-repo clone steps converted from spawnSync to async spawns; warmup children are killed on exit (plus `git worktree prune`) so aborted sessions can't leak stale registrations; failed checkouts degrade honestly (no agent runs in the wrong directory claiming local access) with a 30s retry cooldown. --- apps/hook/server/index.ts | 222 +++++++++++------- apps/pi-extension/server/serverReview.ts | 13 +- apps/pi-extension/vendor.sh | 2 +- packages/server/review.ts | 164 ++++++++++--- packages/shared/cli-pagination.ts | 56 +++++ packages/shared/pr-github.test.ts | 279 +++++++++++++++++++++++ packages/shared/pr-github.ts | 114 ++++++++- packages/shared/pr-gitlab.ts | 58 +---- packages/shared/worktree-pool.test.ts | 158 +++++++++++++ packages/shared/worktree-pool.ts | 48 +++- 10 files changed, 932 insertions(+), 182 deletions(-) create mode 100644 packages/shared/cli-pagination.ts create mode 100644 packages/shared/pr-github.test.ts diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 1185d0bcd..d0f44dfec 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -93,8 +93,7 @@ import { import { stripAtPrefix, resolveAtReference } from "@plannotator/shared/at-reference"; import { htmlToMarkdown } from "@plannotator/shared/html-to-markdown"; import { urlToMarkdown, isConvertedSource } from "@plannotator/shared/url-to-markdown"; -import { fetchRef, createWorktree, removeWorktree, ensureObjectAvailable } from "@plannotator/shared/worktree"; -import { createWorktreePool, type WorktreePool } from "@plannotator/shared/worktree-pool"; +import { createWorktreePool, type WorktreePool, type PoolEntry } from "@plannotator/shared/worktree-pool"; import { parsePRUrl, checkPRAuth, fetchPR, getCliName, getCliInstallUrl, getMRLabel, getMRNumberLabel, getDisplayRepo } from "@plannotator/server/pr"; import { writeRemoteShareLink } from "@plannotator/server/share-url"; import { resolveMarkdownFile, resolveUserPath, hasMarkdownFiles } from "@plannotator/shared/resolve-file"; @@ -550,7 +549,12 @@ if (args[0] === "sessions") { process.exit(1); } - // --local: create a local checkout with the PR head for full file access + // --local: create a local checkout with the PR head for full file access. + // The checkout is built in the BACKGROUND — the platform diff is already + // in hand, so the review server starts immediately. The pool entry starts + // ready:false and flips to ready when the warmup completes; consumers that + // need real files (agent jobs, full-stack diff, code-nav) await it via + // pool.ensure(). if (useLocal && prMetadata) { // Hoisted so catch block can clean up partially-created directories let localPath: string | undefined; @@ -594,101 +598,151 @@ if (args[0] === "sessions") { } } catch { /* not in a git repo — cross-repo path */ } - if (isSameRepo) { - // ── Same-repo: fast worktree path ── - console.error("Fetching PR branch and creating local worktree..."); - // Fetch base branch so origin/ is current for agent diffs. - // Ensure baseSha is available (may fetch, which overwrites FETCH_HEAD). - // Both MUST happen before the PR head fetch since FETCH_HEAD is what - // createWorktree uses — the PR head fetch must be last. - await fetchRef(gitRuntime, prMetadata.baseBranch, { cwd: repoDir }); - await ensureObjectAvailable(gitRuntime, prMetadata.baseSha, { cwd: repoDir }); - // Fetch PR head LAST — sets FETCH_HEAD to the PR tip for createWorktree. - await fetchRef(gitRuntime, fetchRefStr, { cwd: repoDir }); - - await createWorktree(gitRuntime, { - ref: "FETCH_HEAD", - path: localPath, - detach: true, - cwd: repoDir, + // Capture closure values — the warmup outlives this block. + const warmupPath = localPath; + const warmupSessionDir = sessionDir; + const { baseBranch, baseSha, url: prUrl } = prMetadata; + const platform = prMetadata.platform; + const host = prMetadata.host; + const prRepo = platform === "github" + ? `${prMetadata.owner}/${prMetadata.repo}` + : prMetadata.projectPath; + // Validate repo identifier to prevent flag injection via crafted URLs + if (/^-/.test(prRepo)) throw new Error(`Invalid repository identifier: ${prRepo}`); + + // Async spawn for background steps — spawnSync would block the event + // loop and freeze the review server while cloning. Children are + // tracked so a process exit mid-warmup can kill them instead of + // letting an orphaned clone/fetch resurrect the removed session dir + // or register a stale worktree after we're gone. + const warmupProcs = new Set>(); + const runStep = async ( + cmd: string[], + opts: { cwd?: string; env?: Record } = {}, + ): Promise<{ exitCode: number; stderr: string }> => { + const proc = Bun.spawn(cmd, { + cwd: opts.cwd, + env: opts.env, + stdout: "ignore", + stderr: "pipe", }); - - worktreeCleanup = async () => { - if (worktreePool) await worktreePool.cleanup(gitRuntime); - try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} - }; - process.once("exit", () => { - // Best-effort sync cleanup: remove each pool worktree from git, then rm session dir - try { - for (const entry of worktreePool?.entries() ?? []) { - Bun.spawnSync(["git", "worktree", "remove", "--force", entry.path], { cwd: repoDir }); - } - } catch {} - try { Bun.spawnSync(["rm", "-rf", sessionDir]); } catch {} - }); - } else { - // ── Cross-repo: shallow clone + fetch PR head ── - const prRepo = prMetadata.platform === "github" - ? `${prMetadata.owner}/${prMetadata.repo}` - : prMetadata.projectPath; - // Validate repo identifier to prevent flag injection via crafted URLs - if (/^-/.test(prRepo)) throw new Error(`Invalid repository identifier: ${prRepo}`); - const cli = prMetadata.platform === "github" ? "gh" : "glab"; - const host = prMetadata.host; - // gh/glab repo clone doesn't accept --hostname; set GH_HOST/GITLAB_HOST env instead - const isDefaultHost = host === "github.com" || host === "gitlab.com"; - const cloneEnv = isDefaultHost ? undefined : { - ...process.env, - ...(prMetadata.platform === "github" ? { GH_HOST: host } : { GITLAB_HOST: host }), - }; - - // Step 1: Fast skeleton clone (no checkout, depth 1 — minimal data transfer) - console.error(`Cloning ${prRepo} (shallow)...`); - const cloneResult = Bun.spawnSync( - [cli, "repo", "clone", prRepo, localPath, "--", "--depth=1", "--no-checkout"], - { stderr: "pipe", env: cloneEnv }, - ); - if (cloneResult.exitCode !== 0) { - throw new Error(`${cli} repo clone failed: ${new TextDecoder().decode(cloneResult.stderr).trim()}`); + warmupProcs.add(proc); + try { + const [stderr, exitCode] = await Promise.all([ + new Response(proc.stderr).text(), + proc.exited, + ]); + return { exitCode, stderr }; + } finally { + warmupProcs.delete(proc); } + }; + + const warmup: Promise = isSameRepo + ? (async () => { + // ── Same-repo: fast worktree path (tracked spawns — see above) ── + // Fetch base branch so origin/ is current for agent + // diffs. Ensure baseSha is available (may fetch, which overwrites + // FETCH_HEAD). Both MUST happen before the PR head fetch since + // FETCH_HEAD is what worktree add uses — PR head fetch is last. + const baseFetchRes = await runStep(["git", "fetch", "origin", "--", baseBranch], { cwd: repoDir }); + if (baseFetchRes.exitCode !== 0) throw new Error(`git fetch origin ${baseBranch} failed: ${baseFetchRes.stderr.trim()}`); + // Best-effort baseSha availability — mirrors ensureObjectAvailable + const catRes = await runStep(["git", "cat-file", "-t", baseSha], { cwd: repoDir }); + if (catRes.exitCode !== 0) await runStep(["git", "fetch", "origin", "--", baseSha], { cwd: repoDir }); + const headFetchRes = await runStep(["git", "fetch", "origin", "--", fetchRefStr], { cwd: repoDir }); + if (headFetchRes.exitCode !== 0) throw new Error(`git fetch origin ${fetchRefStr} failed: ${headFetchRes.stderr.trim()}`); + + const addRes = await runStep(["git", "worktree", "add", "--detach", warmupPath, "FETCH_HEAD"], { cwd: repoDir }); + if (addRes.exitCode !== 0) throw new Error(`git worktree add failed: ${addRes.stderr.trim()}`); + return { path: warmupPath, prUrl, number: prNumber, ready: true }; + })() + : (async () => { + // ── Cross-repo: shallow clone + fetch PR head ── + const cli = platform === "github" ? "gh" : "glab"; + // gh/glab repo clone doesn't accept --hostname; set GH_HOST/GITLAB_HOST env instead + const isDefaultHost = host === "github.com" || host === "gitlab.com"; + const cloneEnv = isDefaultHost ? undefined : { + ...process.env, + ...(platform === "github" ? { GH_HOST: host } : { GITLAB_HOST: host }), + } as Record; + + // Step 1: Fast skeleton clone (no checkout, depth 1 — minimal data transfer) + const cloneResult = await runStep( + [cli, "repo", "clone", prRepo, warmupPath, "--", "--depth=1", "--no-checkout"], + { env: cloneEnv }, + ); + if (cloneResult.exitCode !== 0) { + throw new Error(`${cli} repo clone failed: ${cloneResult.stderr.trim()}`); + } - // Step 2: Fetch only the PR head ref (targeted, much faster than full fetch) - console.error("Fetching PR branch..."); - const fetchResult = Bun.spawnSync( - ["git", "fetch", "--depth=200", "origin", fetchRefStr], - { cwd: localPath, stderr: "pipe" }, - ); - if (fetchResult.exitCode !== 0) throw new Error(`Failed to fetch PR head ref: ${new TextDecoder().decode(fetchResult.stderr).trim()}`); - - // Step 3: Checkout PR head (critical — if this fails, worktree is empty) - const checkoutResult = Bun.spawnSync(["git", "checkout", "FETCH_HEAD"], { cwd: localPath, stderr: "pipe" }); - if (checkoutResult.exitCode !== 0) { - throw new Error(`git checkout FETCH_HEAD failed: ${new TextDecoder().decode(checkoutResult.stderr).trim()}`); - } + // Step 2: Fetch only the PR head ref (targeted, much faster than full fetch) + const fetchResult = await runStep( + ["git", "fetch", "--depth=200", "origin", fetchRefStr], + { cwd: warmupPath }, + ); + if (fetchResult.exitCode !== 0) throw new Error(`Failed to fetch PR head ref: ${fetchResult.stderr.trim()}`); + + // Step 3: Checkout PR head (critical — if this fails, worktree is empty) + const checkoutResult = await runStep(["git", "checkout", "FETCH_HEAD"], { cwd: warmupPath }); + if (checkoutResult.exitCode !== 0) { + throw new Error(`git checkout FETCH_HEAD failed: ${checkoutResult.stderr.trim()}`); + } - // Best-effort: create base refs so `git diff main...HEAD` and `git diff origin/main...HEAD` work - const baseFetch = Bun.spawnSync(["git", "fetch", "--depth=200", "origin", prMetadata.baseSha], { cwd: localPath, stderr: "pipe" }); - if (baseFetch.exitCode !== 0) console.error("Warning: failed to fetch baseSha, agent diffs may be inaccurate"); - Bun.spawnSync(["git", "branch", "--", prMetadata.baseBranch, prMetadata.baseSha], { cwd: localPath, stderr: "pipe" }); - Bun.spawnSync(["git", "update-ref", `refs/remotes/origin/${prMetadata.baseBranch}`, prMetadata.baseSha], { cwd: localPath, stderr: "pipe" }); + // Best-effort: create base refs so `git diff main...HEAD` and `git diff origin/main...HEAD` work + const baseFetch = await runStep(["git", "fetch", "--depth=200", "origin", baseSha], { cwd: warmupPath }); + if (baseFetch.exitCode !== 0) console.error("Warning: failed to fetch baseSha, agent diffs may be inaccurate"); + await runStep(["git", "branch", "--", baseBranch, baseSha], { cwd: warmupPath }); + await runStep(["git", "update-ref", `refs/remotes/origin/${baseBranch}`, baseSha], { cwd: warmupPath }); - worktreeCleanup = () => { try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} }; - process.once("exit", () => { - try { Bun.spawnSync(["rm", "-rf", sessionDir]); } catch {} - }); - } + return { path: warmupPath, prUrl, number: prNumber, ready: true }; + })(); // --local only provides a sandbox path for agent processes. // Do NOT set gitContext — that would contaminate the diff pipeline. agentCwd = localPath; - // Create worktree pool with the initial PR as the first entry + // Pool starts with the initial PR as a not-ready entry; the seeded + // warmup flips it to ready (or leaves it not-ready on failure). worktreePool = createWorktreePool( { sessionDir, repoDir, isSameRepo }, - { path: localPath, prUrl: prMetadata.url, number: prNumber, ready: true }, + { path: localPath, prUrl, number: prNumber, ready: false }, + warmup, ); - console.error(`Local checkout ready at ${localPath}`); + worktreeCleanup = async () => { + if (isSameRepo && worktreePool) await worktreePool.cleanup(gitRuntime); + try { rmSync(warmupSessionDir, { recursive: true, force: true }); } catch {} + }; + process.once("exit", () => { + // Best-effort sync cleanup: kill in-flight warmup children first so + // an orphaned clone/fetch can't write into the dir we're removing, + // then remove each pool worktree from git, then rm session dir. + for (const proc of warmupProcs) { try { proc.kill(); } catch {} } + if (isSameRepo) { + try { + for (const entry of worktreePool?.entries() ?? []) { + Bun.spawnSync(["git", "worktree", "remove", "--force", entry.path], { cwd: repoDir }); + } + } catch {} + // Clear any registration left by a worktree add that completed + // after the kill (or by a not-ready entry the loop can't see). + try { Bun.spawnSync(["git", "worktree", "prune"], { cwd: repoDir }); } catch {} + } + try { Bun.spawnSync(["rm", "-rf", warmupSessionDir]); } catch {} + }); + + console.error(isSameRepo + ? "Preparing local worktree in the background..." + : `Cloning ${prRepo} (shallow) in the background...`); + warmup.then( + () => console.error(`Local checkout ready at ${warmupPath}`), + (err) => { + console.error("Warning: local checkout failed — features needing local files (agents, full-stack diff) are limited"); + console.error(err instanceof Error ? err.message : String(err)); + try { rmSync(warmupSessionDir, { recursive: true, force: true }); } catch {} + }, + ); } catch (err) { console.error(`Warning: --local failed, falling back to remote diff`); console.error(err instanceof Error ? err.message : String(err)); diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index ef73e8354..10b7d1d1e 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -1245,8 +1245,17 @@ export async function startReviewServer(options: { return; } else if (await agentJobs.handle(req, res, url)) { return; - } else if (url.pathname.startsWith("/api/ai/") && await handlePiAIRequest(req, res, url, aiRuntime)) { - return; + } else if (url.pathname.startsWith("/api/ai/")) { + // AI sessions pin their cwd at creation — make sure the PR checkout + // exists first so sessions never root in a transient fallback + // (mirrors the Bun server; no-op while the pool entry is ready). + if (req.method === "POST" && url.pathname === "/api/ai/session" && options.worktreePool && prMeta) { + try { await options.worktreePool.ensure(reviewRuntime, prMeta); } catch { /* capability degrades below */ } + } + if (await handlePiAIRequest(req, res, url, aiRuntime)) return; + // Unmatched /api/ai/* paths fall through to the app shell, same as + // the original dispatch chain. + html(res, options.htmlContent); } else if (url.pathname === "/api/exit" && req.method === "POST") { deleteDraft(draftKey); resolveDecision({ approved: false, feedback: '', annotations: [], exit: true }); diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index 457babe65..9a218ae86 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -7,7 +7,7 @@ cd "$(dirname "$0")" rm -rf generated mkdir -p generated generated/ai/providers -for f in feedback-templates prompts review-core diff-paths jj-core vcs-core review-args storage draft project pr-types pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common favicon code-file resolve-file config external-annotation agent-jobs worktree worktree-pool html-to-markdown url-to-markdown tour annotate-args at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff; do +for f in feedback-templates prompts review-core diff-paths cli-pagination jj-core vcs-core review-args storage draft project pr-types pr-provider pr-stack pr-github pr-gitlab checklist integrations-common repo reference-common favicon code-file resolve-file config external-annotation agent-jobs worktree worktree-pool html-to-markdown url-to-markdown tour annotate-args at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff; do src="../../packages/shared/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/shared/%s.ts\n' "$f" | cat - "$src" > "generated/$f.ts" done diff --git a/packages/server/review.ts b/packages/server/review.ts index 8f7215a20..f07d30089 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -13,6 +13,7 @@ import { isRemoteSession, getServerHostname, getServerPort } from "./remote"; import type { Origin } from "@plannotator/shared/agents"; import { type DiffType, type GitContext, runVcsDiff, getVcsFileContentsForDiff, getVcsDiffFingerprint, canStageFiles, stageFile, unstageFile, resolveVcsCwd, validateFilePath, getVcsContext, detectRemoteDefaultCompareTarget, gitRuntime } from "./vcs"; import { basename } from "node:path"; +import { existsSync } from "node:fs"; import { parseWorktreeDiffType, resolveBaseBranch } from "@plannotator/shared/review-core"; import { createDefaultSemanticDiffRuntime, @@ -188,6 +189,56 @@ export async function startReviewServer( let currentBase = options.initialBase || detectedCompareTarget(); let baseEverSwitched = false; + // --- PR local checkout resolution ----------------------------------------- + // The pool's initial entry may still be warming up: the checkout is built in + // the background so the server can start on the platform diff alone. Three + // states matter: + // ready entry → use its path + // entry, not ready → the path does not exist on disk yet (or warmup + // failed) — never hand it out; options.agentCwd points + // at the same not-yet-created path + // no entry → PR not in the pool (e.g. cross-repo pr-switch) — + // legacy fallback to the initial checkout (agentCwd) + // The initial checkout path is only trustworthy once it actually exists — + // the warmup may not have created it yet, or may have failed and removed it. + const agentCwdIfExists = (): string | undefined => + options.agentCwd && existsSync(options.agentCwd) ? options.agentCwd : undefined; + const resolvePRLocalCwd = (meta: PRMetadata | undefined = prMetadata): string | undefined => { + const pool = options.worktreePool; + if (pool && meta) { + const entry = pool.get(meta.url); + if (entry?.ready) return entry.path; + if (entry) return undefined; + } + return agentCwdIfExists(); + }; + // Failure memo: a persistently-failing checkout (network down, ref denied) + // must not turn every code-nav hover / agent launch into a multi-second + // re-fetch against origin. Failed URLs are skipped for a cooldown window. + const prLocalFailureMemo = new Map(); + const PR_LOCAL_RETRY_COOLDOWN_MS = 30_000; + // Await the current PR's checkout: blocks on the in-flight warmup, retries + // failed same-repo creations, returns undefined when no checkout can exist. + const ensurePRLocalCwd = async (meta: PRMetadata | undefined = prMetadata): Promise => { + const pool = options.worktreePool; + if (pool && meta) { + const hadEntry = pool.has(meta.url); + const failedAt = prLocalFailureMemo.get(meta.url); + if (failedAt && Date.now() - failedAt < PR_LOCAL_RETRY_COOLDOWN_MS) { + return hadEntry ? undefined : agentCwdIfExists(); + } + try { + const entry = await pool.ensure(gitRuntime, meta); + prLocalFailureMemo.delete(meta.url); + return entry.path; + } catch { + prLocalFailureMemo.set(meta.url, Date.now()); + return hadEntry ? undefined : agentCwdIfExists(); + } + } + return options.agentCwd; + }; + // --- Diff staleness fingerprint ------------------------------------------- // Captured beside every patch snapshot (startup + every switch endpoint); // GET /api/diff/fresh recomputes and compares so the client can show a @@ -206,10 +257,7 @@ export async function startReviewServer( } // Full-stack: three-dot diff against the local checkout — fingerprint // (merge-base, HEAD), which changes exactly when the patch can. - const fullStackCwd = - (options.worktreePool && prMetadata - ? options.worktreePool.resolve(prMetadata.url) - : undefined) ?? options.agentCwd; + const fullStackCwd = resolvePRLocalCwd(); if (!prMetadata) return null; return await getPRFullStackFingerprint(gitRuntime, prMetadata, fullStackCwd); } @@ -253,11 +301,21 @@ export async function startReviewServer( const resolveAgentCwd = (): string => { if (workspace) return workspace.root; if (options.worktreePool && prMetadata) { - const poolPath = options.worktreePool.resolve(prMetadata.url); - if (poolPath) return poolPath; + return resolvePRLocalCwd() + ?? resolveVcsCwd(currentDiffType as DiffType, gitContext?.cwd) + ?? process.cwd(); } return options.agentCwd ?? resolveVcsCwd(currentDiffType as DiffType, gitContext?.cwd) ?? process.cwd(); }; + // Async sibling of resolveAgentCwd: waits for the current PR's checkout + // warmup instead of falling back while it is still being created. + const resolveAgentCwdReady = async (): Promise => { + if (options.worktreePool && prMetadata) { + const poolPath = await ensurePRLocalCwd(); + if (poolPath) return poolPath; + } + return resolveAgentCwd(); + }; const getWorkspacePromptContext = (): WorkspaceReviewPromptContext | undefined => { if (!workspace) return undefined; return workspace.getPromptContext(); @@ -266,8 +324,11 @@ export async function startReviewServer( const resolveSemanticDiffCwd = (): string => { if (workspace) return workspace.root; if (options.worktreePool && prMetadata) { - const poolPath = options.worktreePool.resolve(prMetadata.url); + const poolPath = resolvePRLocalCwd(); if (poolPath) return poolPath; + // Checkout warming up — probe sem availability in the scratch dir; the + // real run below awaits the checkout before resolving its cwd. + if (options.worktreePool.has(prMetadata.url)) return semanticDiffScratchCwd; } if (options.agentCwd) return options.agentCwd; if (gitContext) { @@ -308,6 +369,8 @@ export async function startReviewServer( }; const getSemanticDiff = async (url: URL): Promise => { + // Semantic diff reads real files — wait out the checkout warmup in PR mode. + if (isPRMode && options.worktreePool) await ensurePRLocalCwd(); const cwd = resolveSemanticDiffCwd(); const fileExts = semanticDiffFileExtsFromSearchParams(url.searchParams); const cacheKey = semanticDiffCacheKey({ rawPatch: currentPatch, cwd, fileExts }); @@ -330,13 +393,33 @@ export async function startReviewServer( getCwd: resolveAgentCwd, async buildCommand(provider, config) { - const cwd = resolveAgentCwd(); + // Snapshot ALL launch-relevant state before any await: waiting out the + // checkout warmup below yields to other requests (e.g. pr-switch), and + // the job's cwd, prompt, and PR attribution must describe the same PR. + const launchMetadata = prMetadata; + const launchPatch = currentPatch; + const launchDiffType = currentDiffType; + const launchBase = currentBase; + const launchScope = currentPRDiffScope; + + // Agents run inside the PR checkout — wait out the background warmup so + // the spawn-time getCwd() below resolves to a path that exists. + const cwd = options.worktreePool && launchMetadata + ? (await ensurePRLocalCwd(launchMetadata)) ?? resolveAgentCwd() + : await resolveAgentCwdReady(); const workspacePrompt = getWorkspacePromptContext(); - const hasAgentLocalAccess = !!workspacePrompt || !!options.worktreePool || !!options.agentCwd || !!gitContext; + // Honest local-access claim: in PR mode the checkout must actually be + // available (warmup done, not failed) — the prompt tells the agent it + // can read PR files, so a bare pool/agentCwd existence check would have + // it confidently reviewing whatever directory it landed in. + const hasAgentLocalAccess = !!workspacePrompt || !!gitContext || + (options.worktreePool && launchMetadata + ? resolvePRLocalCwd(launchMetadata) !== undefined + : !!options.agentCwd); const userMessageOptions = { - defaultBranch: currentBase, + defaultBranch: launchBase, hasLocalAccess: hasAgentLocalAccess, - prDiffScope: currentPRDiffScope, + prDiffScope: launchScope, ...(workspacePrompt && { workspace: workspacePrompt }), }; @@ -344,28 +427,28 @@ export async function startReviewServer( // downstream "Copy All" produces the same markdown as /api/feedback // would right now, even if the reviewer switches modes/bases later. // Skipped in PR mode (prMetadata carries equivalent context). - const worktreeParts = String(currentDiffType).startsWith("worktree:") - ? parseWorktreeDiffType(currentDiffType as DiffType) + const worktreeParts = String(launchDiffType).startsWith("worktree:") + ? parseWorktreeDiffType(launchDiffType as DiffType) : null; - const launchPrUrl = prMetadata?.url; - const launchDiffScope = isPRMode ? currentPRDiffScope : undefined; + const launchPrUrl = launchMetadata?.url; + const launchDiffScope = isPRMode ? launchScope : undefined; const diffContext: AgentJobInfo["diffContext"] | undefined = workspacePrompt - ? { mode: String(currentDiffType), worktreePath: null } - : prMetadata + ? { mode: String(launchDiffType), worktreePath: null } + : launchMetadata ? undefined : { - mode: (worktreeParts?.subType ?? currentDiffType) as string, - base: currentBase, + mode: (worktreeParts?.subType ?? launchDiffType) as string, + base: launchBase, worktreePath: worktreeParts?.path ?? null, }; if (provider === "tour") { const built = await tour.buildCommand({ cwd, - patch: currentPatch, - diffType: currentDiffType as DiffType, + patch: launchPatch, + diffType: launchDiffType as DiffType, options: userMessageOptions, - prMetadata, + prMetadata: launchMetadata, config, }); return built ? { ...built, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext } : built; @@ -374,10 +457,10 @@ export async function startReviewServer( const userMessage = workspacePrompt ? buildAgentReviewUserMessageForTarget({ kind: "workspace", - patch: currentPatch, + patch: launchPatch, workspace: workspacePrompt, }) - : buildAgentReviewUserMessage(currentPatch, currentDiffType as DiffType, userMessageOptions, prMetadata); + : buildAgentReviewUserMessage(launchPatch, launchDiffType as DiffType, userMessageOptions, launchMetadata); const jobLabel = workspacePrompt ? "Workspace Review" : "Code Review"; if (provider === "codex") { @@ -795,7 +878,14 @@ export async function startReviewServer( ); } - const fullStackCwd = (options.worktreePool && prMetadata ? options.worktreePool.resolve(prMetadata.url) : undefined) ?? options.agentCwd; + // Blocks on the background checkout warmup if it's still running. + const fullStackCwd = await ensurePRLocalCwd(); + if (!fullStackCwd) { + return Response.json( + { error: "Local checkout is unavailable — full stack diff cannot run" }, + { status: 400 }, + ); + } const result = await runPRFullStackDiff(gitRuntime, prMetadata, fullStackCwd); if (result.error) { @@ -996,7 +1086,7 @@ export async function startReviewServer( // Full-stack PR mode uses local git for file expansion because // the patch is no longer the platform's layer diff. - const fileContentCwd = (options.worktreePool && prMetadata) ? options.worktreePool.resolve(prMetadata.url) : options.agentCwd; + const fileContentCwd = resolvePRLocalCwd(); if ( isPRMode && currentPRDiffScope === "full-stack" && @@ -1062,7 +1152,14 @@ export async function startReviewServer( { status: 400 }, ); } - const navCwd = resolveAgentCwd(); + // PR mode: the checkout must actually exist — ripgrep over a + // fallback directory returns confidently-wrong results. + const navCwd = options.worktreePool && prMetadata + ? await ensurePRLocalCwd() + : await resolveAgentCwdReady(); + if (!navCwd) { + return Response.json({ error: "Local checkout unavailable" }, { status: 400 }); + } const changedFiles = extractChangedFiles(currentPatch); return handleCodeNavResolve(req, navCwd, changedFiles); } @@ -1081,7 +1178,12 @@ export async function startReviewServer( return Response.json({ error: "Invalid path" }, { status: 400 }); } try { - const navCwd = resolveAgentCwd(); + const navCwd = options.worktreePool && prMetadata + ? await ensurePRLocalCwd() + : await resolveAgentCwdReady(); + if (!navCwd) { + return Response.json({ error: "Local checkout unavailable" }, { status: 400 }); + } const content = await Bun.file(`${navCwd}/${filePath}`).text(); return Response.json({ content }); } catch { @@ -1306,6 +1408,12 @@ export async function startReviewServer( if (url.pathname.startsWith("/api/ai/")) { const handler = aiRuntime.endpoints[url.pathname as keyof AIEndpoints]; if (handler) { + // AI sessions pin their cwd at creation — wait out the PR + // checkout warmup so a session opened in the first seconds + // isn't rooted in a transient fallback directory for life. + if (req.method === "POST" && url.pathname === "/api/ai/session" && options.worktreePool && prMetadata) { + await ensurePRLocalCwd(); + } if (url.pathname === AI_QUERY_ENDPOINT) { server.timeout(req, 0); } diff --git a/packages/shared/cli-pagination.ts b/packages/shared/cli-pagination.ts new file mode 100644 index 000000000..1c54bf0e9 --- /dev/null +++ b/packages/shared/cli-pagination.ts @@ -0,0 +1,56 @@ +/** + * Parse output of `gh api --paginate` / `glab api --paginate`. + * + * Both CLIs concatenate pages as adjacent JSON arrays (`[...][...]`) which is + * not valid JSON. Walk the output, split it into top-level arrays, and merge + * them. Single-page output (the common case) round-trips through the same path. + */ +export function parsePaginatedArray(stdout: string): T[] { + const trimmed = stdout.trim(); + if (!trimmed) return []; + + const slices: string[] = []; + let depth = 0; + let inString = false; + let escape = false; + let start = -1; + + for (let i = 0; i < trimmed.length; i++) { + const c = trimmed[i]; + if (inString) { + if (escape) { + escape = false; + } else if (c === "\\") { + escape = true; + } else if (c === '"') { + inString = false; + } + continue; + } + if (c === '"') { + inString = true; + continue; + } + if (c === "[" || c === "{") { + if (depth === 0 && c === "[") start = i; + depth++; + } else if (c === "]" || c === "}") { + depth--; + if (depth === 0 && c === "]" && start !== -1) { + slices.push(trimmed.slice(start, i + 1)); + start = -1; + } + } + } + + if (slices.length === 0) { + return JSON.parse(trimmed) as T[]; + } + + const merged: T[] = []; + for (const slice of slices) { + const page = JSON.parse(slice) as T[]; + if (Array.isArray(page)) merged.push(...page); + } + return merged; +} diff --git a/packages/shared/pr-github.test.ts b/packages/shared/pr-github.test.ts new file mode 100644 index 000000000..4dfc1c49f --- /dev/null +++ b/packages/shared/pr-github.test.ts @@ -0,0 +1,279 @@ +import { describe, expect, test, spyOn } from "bun:test"; +import { fetchGhPR, reconstructGhPatch, type GitHubFileEntry } from "./pr-github"; +import { parseDiffGitHeader, parseDiffFilePathLines, parseDiffMetadataPathLines } from "./diff-paths"; +import type { PRRuntime } from "./pr-types"; + +const REF = { platform: "github" as const, host: "github.com", owner: "o", repo: "r", number: 123 }; + +const VIEW_JSON = JSON.stringify({ + id: "PR_node123", + title: "Big change", + author: { login: "dev" }, + baseRefName: "main", + headRefName: "feature", + baseRefOid: "a".repeat(40), + headRefOid: "b".repeat(40), + url: "https://github.com/o/r/pull/123", +}); + +/** + * Mock gh runtime. Routes by subcommand; records every invocation so tests can + * assert on exactly which commands ran (and which didn't). + */ +function githubRuntime(opts: { + prDiff: { stdout?: string; stderr?: string; exitCode: number }; + files?: { stdout?: string; stderr?: string; exitCode: number }; + view?: { stdout?: string; stderr?: string; exitCode: number }; +}): { runtime: PRRuntime; calls: string[] } { + const calls: string[] = []; + const runtime: PRRuntime = { + async runCommand(command, args) { + calls.push([command, ...args].join(" ")); + if (args[0] === "pr" && args[1] === "diff") { + return { stdout: opts.prDiff.stdout ?? "", stderr: opts.prDiff.stderr ?? "", exitCode: opts.prDiff.exitCode }; + } + if (args[0] === "pr" && args[1] === "view") { + return { stdout: opts.view?.stdout ?? VIEW_JSON, stderr: opts.view?.stderr ?? "", exitCode: opts.view?.exitCode ?? 0 }; + } + if (args[0] === "repo" && args[1] === "view") { + return { stdout: "main\n", stderr: "", exitCode: 0 }; + } + if (args[0] === "api" && args[1]?.includes("/compare/")) { + return { stdout: `${"c".repeat(40)}\n`, stderr: "", exitCode: 0 }; + } + if (args[0] === "api" && args[1]?.includes("/pulls/123/files")) { + return { stdout: opts.files?.stdout ?? "", stderr: opts.files?.stderr ?? "", exitCode: opts.files?.exitCode ?? 1 }; + } + return { stdout: "", stderr: `unexpected command: ${args.join(" ")}`, exitCode: 1 }; + }, + }; + return { runtime, calls }; +} + +describe("fetchGhPR", () => { + test("uses gh pr diff verbatim when it succeeds and never touches the files API", async () => { + const patch = "diff --git a/x.ts b/x.ts\n--- a/x.ts\n+++ b/x.ts\n@@ -1 +1 @@\n-a\n+b\n"; + const { runtime, calls } = githubRuntime({ prDiff: { exitCode: 0, stdout: patch } }); + + const result = await fetchGhPR(runtime, REF); + + expect(result.rawPatch).toBe(patch); + expect(result.metadata).toMatchObject({ + platform: "github", + number: 123, + baseBranch: "main", + headBranch: "feature", + mergeBaseSha: "c".repeat(40), + }); + expect(calls.some((c) => c.includes("/pulls/123/files"))).toBe(false); + }); + + test("falls back to the paginated files API when gh pr diff fails (oversized PR)", async () => { + // Two concatenated pages — the actual shape `gh api --paginate` emits. + const page1 = JSON.stringify([ + { filename: "src/a.ts", status: "modified", patch: "@@ -1 +1 @@\n-old\n+new" }, + ]); + const page2 = JSON.stringify([ + { filename: "src/b.ts", status: "added", patch: "@@ -0,0 +1 @@\n+hello" }, + ]); + const { runtime, calls } = githubRuntime({ + prDiff: { exitCode: 1, stderr: "diff exceeded the maximum number of lines (20000)" }, + files: { exitCode: 0, stdout: page1 + page2 }, + }); + + const result = await fetchGhPR(runtime, REF); + + expect(calls).toContain("gh api repos/o/r/pulls/123/files?per_page=100 --paginate"); + expect(result.rawPatch).toContain("diff --git a/src/a.ts b/src/a.ts"); + expect(result.rawPatch).toContain("+new"); + expect(result.rawPatch).toContain("diff --git a/src/b.ts b/src/b.ts"); + expect(result.rawPatch).toContain("new file mode 100644"); + // Metadata path is unaffected by the fallback. + expect(result.metadata).toMatchObject({ number: 123, mergeBaseSha: "c".repeat(40) }); + }); + + test("passes --hostname to the files API on GitHub Enterprise", async () => { + const { runtime, calls } = githubRuntime({ + prDiff: { exitCode: 1, stderr: "406" }, + files: { exitCode: 0, stdout: JSON.stringify([{ filename: "a.ts", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }]) }, + }); + + await fetchGhPR(runtime, { ...REF, host: "ghe.corp.com" }); + + const filesCall = calls.find((c) => c.includes("/pulls/123/files")); + expect(filesCall).toContain("--hostname ghe.corp.com"); + }); + + test("surfaces both errors when gh pr diff and the files API both fail", async () => { + const { runtime } = githubRuntime({ + prDiff: { exitCode: 1, stderr: "diff too large" }, + files: { exitCode: 1, stderr: "files boom" }, + }); + + await expect(fetchGhPR(runtime, REF)).rejects.toThrow(/diff too large.*files boom|files boom.*diff too large/s); + }); + + test("throws a clear empty-diff error when the files API returns no entries", async () => { + const { runtime } = githubRuntime({ + prDiff: { exitCode: 1, stderr: "406" }, + files: { exitCode: 0, stdout: "[]" }, + }); + + await expect(fetchGhPR(runtime, REF)).rejects.toThrow(/PR diff is empty/); + }); + + test("warns when the files API returns fewer files than the PR reports (3000-file cap)", async () => { + const view = JSON.parse(VIEW_JSON); + view.changedFiles = 3500; + const { runtime } = githubRuntime({ + prDiff: { exitCode: 1, stderr: "406" }, + files: { exitCode: 0, stdout: JSON.stringify([{ filename: "a.ts", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }]) }, + view: { exitCode: 0, stdout: JSON.stringify(view) }, + }); + + const errSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const result = await fetchGhPR(runtime, REF); + expect(result.rawPatch).toContain("diff --git a/a.ts b/a.ts"); // partial diff still served + const warned = errSpy.mock.calls.some((args) => String(args[0]).includes("3500 changed files")); + expect(warned).toBe(true); + } finally { + errSpy.mockRestore(); + } + }); + + test("metadata failure wins over diff failure — no fallback attempted", async () => { + const { runtime, calls } = githubRuntime({ + prDiff: { exitCode: 1, stderr: "406" }, + files: { exitCode: 0, stdout: "[]" }, + view: { exitCode: 1, stderr: "no such PR" }, + }); + + await expect(fetchGhPR(runtime, REF)).rejects.toThrow(/Failed to fetch PR metadata/); + expect(calls.some((c) => c.includes("/pulls/123/files"))).toBe(false); + }); +}); + +describe("reconstructGhPatch", () => { + test("modified file round-trips through the real diff header parsers", () => { + const patch = reconstructGhPatch([ + { filename: "src/app.ts", status: "modified", patch: "@@ -1,2 +1,2 @@\n-const a = 1;\n+const a = 2;\n context" }, + ]); + + const lines = patch.split("\n"); + expect(lines[0]).toBe("diff --git a/src/app.ts b/src/app.ts"); + expect(parseDiffGitHeader(lines[0])).toEqual({ oldPath: "src/app.ts", newPath: "src/app.ts" }); + expect(parseDiffFilePathLines(lines)).toEqual({ oldPath: "src/app.ts", newPath: "src/app.ts" }); + expect(patch).toContain("\n--- a/src/app.ts\n+++ b/src/app.ts\n@@ -1,2 +1,2 @@\n"); + expect(patch.endsWith("\n")).toBe(true); + }); + + test("added file uses /dev/null for the old side and new file mode", () => { + const patch = reconstructGhPatch([ + { filename: "new.ts", status: "added", patch: "@@ -0,0 +1 @@\n+x" }, + ]); + + expect(patch).toContain("diff --git a/new.ts b/new.ts"); + expect(patch).toContain("new file mode 100644"); + expect(patch).toContain("\n--- /dev/null\n+++ b/new.ts\n"); + }); + + test("removed file uses /dev/null for the new side and deleted file mode", () => { + const patch = reconstructGhPatch([ + { filename: "gone.ts", status: "removed", patch: "@@ -1 +0,0 @@\n-x" }, + ]); + + expect(patch).toContain("diff --git a/gone.ts b/gone.ts"); + expect(patch).toContain("deleted file mode 100644"); + expect(patch).toContain("\n--- a/gone.ts\n+++ /dev/null\n"); + }); + + test("renamed file emits rename metadata that the real parser extracts", () => { + const patch = reconstructGhPatch([ + { filename: "after.ts", previous_filename: "before.ts", status: "renamed", patch: "@@ -1 +1 @@\n-a\n+b" }, + ]); + + const lines = patch.split("\n"); + 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" }); + }); + + test("pure rename (no patch field) emits a header-only section", () => { + const patch = 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"); + }); + + test("entry without patch (binary / per-file too large) doesn't corrupt the next file's section", () => { + const patch = reconstructGhPatch([ + { filename: "huge.json", status: "modified" }, + { filename: "small.ts", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }, + ]); + + // Every diff --git header must start at the beginning of its own line — + // this is what the UI's file splitter (split on /^diff --git /) relies on. + const headerLines = patch.split("\n").filter((l) => l.startsWith("diff --git ")); + expect(headerLines).toEqual([ + "diff --git a/huge.json b/huge.json", + "diff --git a/small.ts b/small.ts", + ]); + expect(patch).toContain("diff --git a/huge.json b/huge.json\ndiff --git a/small.ts"); + }); + + test("terminates a patch that lacks a trailing newline (GitHub omits it)", () => { + const patch = reconstructGhPatch([ + { filename: "a.ts", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }, + { filename: "b.ts", status: "modified", patch: "@@ -1 +1 @@\n-c\n+d" }, + ]); + + expect(patch).toContain("+b\ndiff --git a/b.ts b/b.ts"); + }); + + test("leaves paths with bare spaces unquoted — git parity, so the header parser round-trips them", () => { + // Git only C-quotes paths containing quotes/backslashes/control chars. + // Over-quoting (e.g. quoting spaces) breaks parseDiffGitHeader's regex + // branch and silently drops files downstream. + const patch = reconstructGhPatch([ + { filename: "docs/my file.md", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }, + ]); + + const headerLine = patch.split("\n")[0]; + expect(headerLine).toBe("diff --git a/docs/my file.md b/docs/my file.md"); + expect(parseDiffGitHeader(headerLine)).toEqual({ oldPath: "docs/my file.md", newPath: "docs/my file.md" }); + }); + + test("pure rename with a space in the new name still yields parseable paths (file must not vanish)", () => { + // Regression: GitHub omits `patch` for 100%-similarity renames; if the + // header is unparseable the UI's file splitter drops the file silently. + const patch = reconstructGhPatch([ + { filename: "docs/road map.md", previous_filename: "docs/roadmap.md", status: "renamed" }, + ]); + + const headerLine = patch.split("\n")[0]; + expect(headerLine).toBe("diff --git a/docs/roadmap.md b/docs/road map.md"); + expect(parseDiffGitHeader(headerLine)).toEqual({ oldPath: "docs/roadmap.md", newPath: "docs/road map.md" }); + }); + + test("C-quotes paths containing double quotes, matching git, and the parser round-trips them", () => { + const patch = reconstructGhPatch([ + { filename: 'he"llo.ts', status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }, + ]); + + const headerLine = patch.split("\n")[0]; + expect(headerLine).toBe('diff --git "a/he\\"llo.ts" "b/he\\"llo.ts"'); + expect(parseDiffGitHeader(headerLine)).toEqual({ oldPath: 'he"llo.ts', newPath: 'he"llo.ts' }); + }); + + test("copied file emits copy metadata", () => { + const patch = reconstructGhPatch([ + { filename: "copy.ts", previous_filename: "orig.ts", status: "copied", patch: "@@ -1 +1 @@\n-a\n+b" }, + ]); + + 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 0330a87e9..1ca1de806 100644 --- a/packages/shared/pr-github.ts +++ b/packages/shared/pr-github.ts @@ -6,6 +6,7 @@ import type { PRRuntime, PRMetadata, PRContext, PRReviewThread, PRThreadComment, PRReviewFileComment, CommandResult, PRStackTree, PRStackNode, PRListItem } from "./pr-types"; import { encodeApiFilePath } from "./pr-types"; +import { parsePaginatedArray } from "./cli-pagination"; // GitHub-specific PRRef shape (used internally) interface GhPRRef { @@ -59,6 +60,74 @@ export async function getGhUser(runtime: PRRuntime, host: string): Promise(filesResult.stdout); + rawPatch = reconstructGhPatch(fileEntries); + if (!rawPatch.trim()) { + throw new Error( + "PR diff is empty — it may be too large to fetch via the GitHub API. Review it on the GitHub web UI.", + ); + } + // The files API silently caps at 3000 files — never present a truncated + // review as complete. + const expectedFiles = (JSON.parse(viewResult.stdout) as { changedFiles?: number }).changedFiles; + if (typeof expectedFiles === "number" && fileEntries.length < expectedFiles) { + console.error( + `Warning: PR reports ${expectedFiles} changed files but the GitHub files API returned ${fileEntries.length} (the API caps at 3000). The review is missing the remainder.`, + ); + } + } + const raw = JSON.parse(viewResult.stdout) as { id: string; title: string; @@ -141,7 +239,7 @@ export async function fetchGhPR( url: raw.url, }; - return { metadata, rawPatch: diffResult.stdout }; + return { metadata, rawPatch }; } // --- PR Context --- diff --git a/packages/shared/pr-gitlab.ts b/packages/shared/pr-gitlab.ts index 59c0b98be..ab8edb3ec 100644 --- a/packages/shared/pr-gitlab.ts +++ b/packages/shared/pr-gitlab.ts @@ -43,62 +43,8 @@ interface GitLabDiffEntry { renamed_file: boolean; } -/** - * Parse output of `glab api --paginate`. - * - * glab concatenates pages as adjacent JSON arrays (`[...][...]`) which is not - * valid JSON. Walk the output, split it into top-level arrays, and merge them. - * Single-page output (the common case) round-trips through the same path. - */ -export function parsePaginatedArray(stdout: string): T[] { - const trimmed = stdout.trim(); - if (!trimmed) return []; - - const slices: string[] = []; - let depth = 0; - let inString = false; - let escape = false; - let start = -1; - - for (let i = 0; i < trimmed.length; i++) { - const c = trimmed[i]; - if (inString) { - if (escape) { - escape = false; - } else if (c === "\\") { - escape = true; - } else if (c === '"') { - inString = false; - } - continue; - } - if (c === '"') { - inString = true; - continue; - } - if (c === "[" || c === "{") { - if (depth === 0 && c === "[") start = i; - depth++; - } else if (c === "]" || c === "}") { - depth--; - if (depth === 0 && c === "]" && start !== -1) { - slices.push(trimmed.slice(start, i + 1)); - start = -1; - } - } - } - - if (slices.length === 0) { - return JSON.parse(trimmed) as T[]; - } - - const merged: T[] = []; - for (const slice of slices) { - const page = JSON.parse(slice) as T[]; - if (Array.isArray(page)) merged.push(...page); - } - return merged; -} +export { parsePaginatedArray } from "./cli-pagination"; +import { parsePaginatedArray } from "./cli-pagination"; /** * Reconstruct a unified patch from GitLab's merge_request diffs API response. diff --git a/packages/shared/worktree-pool.test.ts b/packages/shared/worktree-pool.test.ts index 63773042a..01856caf4 100644 --- a/packages/shared/worktree-pool.test.ts +++ b/packages/shared/worktree-pool.test.ts @@ -160,3 +160,161 @@ describe("worktree-pool", () => { expect(fetchPRHead?.[3]).toBe("refs/merge-requests/42/head"); }); }); + +// --- Seeded background warmup (non-blocking PR checkout) --------------------- + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (err: unknown) => void; + const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); + return { promise, resolve, reject }; +} + +const PR3_URL = "https://github.com/acme/widgets/pull/3"; + +function notReadyInitial() { + return { path: "/tmp/session/pool/pr-3", prUrl: PR3_URL, number: 3, ready: false }; +} + +describe("worktree-pool seeded warmup", () => { + test("resolve returns undefined while the warmup is in flight, then the path once it lands", async () => { + const warmup = deferred<{ path: string; prUrl: string; number: number; ready: boolean }>(); + const pool = createWorktreePool( + { sessionDir: "/tmp/session", repoDir: "/repo", isSameRepo: true }, + notReadyInitial(), + warmup.promise, + ); + + expect(pool.resolve(PR3_URL)).toBeUndefined(); + expect(pool.has(PR3_URL)).toBe(true); // entry exists — consumers can distinguish "warming up" from "unknown" + + warmup.resolve({ path: "/tmp/session/pool/pr-3", prUrl: PR3_URL, number: 3, ready: true }); + await warmup.promise; + await Bun.sleep(0); // let the pool's bookkeeping handlers run + + expect(pool.resolve(PR3_URL)).toBe("/tmp/session/pool/pr-3"); + expect(pool.get(PR3_URL)?.ready).toBe(true); + }); + + test("ensure during warmup awaits the seeded promise instead of starting a duplicate creation", async () => { + const { runtime, commands } = fakeRuntime(); + const warmup = deferred<{ path: string; prUrl: string; number: number; ready: boolean }>(); + const pool = createWorktreePool( + { sessionDir: "/tmp/session", repoDir: "/repo", isSameRepo: true }, + notReadyInitial(), + warmup.promise, + ); + + const ensured = pool.ensure(runtime, makeMetadata(3)); + await Bun.sleep(0); + expect(commands.length).toBe(0); // no parallel creation kicked off + + warmup.resolve({ path: "/tmp/session/pool/pr-3", prUrl: PR3_URL, number: 3, ready: true }); + const entry = await ensured; + + expect(entry.path).toBe("/tmp/session/pool/pr-3"); + expect(entry.ready).toBe(true); + expect(commands.length).toBe(0); // creation was the warmup's job, not ensure's + }); + + test("failed warmup keeps the entry not-ready so resolve never hands out the dead path", async () => { + const warmup = deferred<{ path: string; prUrl: string; number: number; ready: boolean }>(); + const pool = createWorktreePool( + { sessionDir: "/tmp/session", repoDir: "/repo", isSameRepo: true }, + notReadyInitial(), + warmup.promise, + ); + + warmup.reject(new Error("network down")); + await Bun.sleep(0); + + expect(pool.resolve(PR3_URL)).toBeUndefined(); + expect(pool.has(PR3_URL)).toBe(true); + }); + + test("same-repo ensure retries creation after a failed warmup", async () => { + const { runtime, commands } = fakeRuntime(); + const warmup = deferred<{ path: string; prUrl: string; number: number; ready: boolean }>(); + const pool = createWorktreePool( + { sessionDir: "/tmp/session", repoDir: "/repo", isSameRepo: true }, + notReadyInitial(), + warmup.promise, + ); + + warmup.reject(new Error("network down")); + await Bun.sleep(0); + + const entry = await pool.ensure(runtime, makeMetadata(3)); + + expect(entry.ready).toBe(true); + expect(pool.resolve(PR3_URL)).toBe("/tmp/session/pool/pr-3"); + // The retry ran the full creation sequence itself this time. + expect(commands.some(c => c[0] === "worktree" && c[1] === "add")).toBe(true); + }); + + test("cross-repo ensure rejects after a failed warmup — pool cannot rebuild a clone", async () => { + const { runtime } = fakeRuntime(); + const warmup = deferred<{ path: string; prUrl: string; number: number; ready: boolean }>(); + const pool = createWorktreePool( + { sessionDir: "/tmp/session", repoDir: "/repo", isSameRepo: false }, + notReadyInitial(), + warmup.promise, + ); + + warmup.reject(new Error("clone failed")); + await Bun.sleep(0); + + await expect(pool.ensure(runtime, makeMetadata(3))).rejects.toThrow(); + }); + + test("creation for another PR queues behind the in-flight warmup (FETCH_HEAD serialization)", async () => { + const { runtime, commands } = fakeRuntime(); + const warmup = deferred<{ path: string; prUrl: string; number: number; ready: boolean }>(); + const pool = createWorktreePool( + { sessionDir: "/tmp/session", repoDir: "/repo", isSameRepo: true }, + notReadyInitial(), + warmup.promise, + ); + + // pr-switch arrives while the initial checkout is still being fetched + const switched = pool.ensure(runtime, makeMetadata(5)); + await Bun.sleep(0); + expect(commands.length).toBe(0); // PR #5's fetch must not clobber the warmup's FETCH_HEAD + + warmup.resolve({ path: "/tmp/session/pool/pr-3", prUrl: PR3_URL, number: 3, ready: true }); + const entry = await switched; + + expect(entry.path).toBe("/tmp/session/pool/pr-5"); + expect(commands[commands.length - 1]).toEqual(["worktree", "add", "--detach", "/tmp/session/pool/pr-5", "FETCH_HEAD"]); + }); + + test("concurrent creations for different PRs never interleave their git commands", async () => { + // Runtime that yields between commands — interleaving would surface here + // if creations ran concurrently instead of through the serialization chain. + const commands: string[][] = []; + const runtime: ReviewGitRuntime = { + async runGit(args) { + await Bun.sleep(1); + commands.push(args); + return { stdout: "", stderr: "", exitCode: 0 }; + }, + async readTextFile() { return null; }, + }; + const pool = createWorktreePool({ sessionDir: "/tmp/session", repoDir: "/repo", isSameRepo: true }); + + const [a, b] = await Promise.all([ + pool.ensure(runtime, makeMetadata(3)), + pool.ensure(runtime, makeMetadata(4, "feature/pr-3")), + ]); + + expect(a.path).toBe("/tmp/session/pool/pr-3"); + expect(b.path).toBe("/tmp/session/pool/pr-4"); + + // PR #3's entire sequence (ending in worktree add) must complete before + // any PR #4 command runs. + const firstPr4Index = commands.findIndex(c => c.join(" ").includes("pr-4") || c.join(" ").includes("refs/pull/4")); + const pr3AddIndex = commands.findIndex(c => c[0] === "worktree" && c[3] === "/tmp/session/pool/pr-3"); + expect(pr3AddIndex).toBeGreaterThanOrEqual(0); + expect(firstPr4Index).toBeGreaterThan(pr3AddIndex); + }); +}); diff --git a/packages/shared/worktree-pool.ts b/packages/shared/worktree-pool.ts index 72b1effd1..61d429b3b 100644 --- a/packages/shared/worktree-pool.ts +++ b/packages/shared/worktree-pool.ts @@ -36,11 +36,44 @@ export interface WorktreePool { cleanup(runtime: ReviewGitRuntime): Promise; } -export function createWorktreePool(config: WorktreePoolConfig, initial?: PoolEntry): WorktreePool { +export function createWorktreePool( + config: WorktreePoolConfig, + initial?: PoolEntry, + initialPending?: Promise, +): WorktreePool { const pool = new Map(); const pending = new Map>(); + // FETCH_HEAD is shared per-repo state: a creation's PR-head fetch must not + // run while another creation (or the seeded background warmup) is between + // its own fetch and `git worktree add`. Serialize all creations through + // this chain. + let creationChain: Promise = Promise.resolve(); if (initial) pool.set(initial.prUrl, initial); + // Seeded background warmup: the initial entry starts ready:false while the + // caller builds its checkout (fetch/clone) off the request path. ensure() + // awaits the in-flight warmup instead of starting a duplicate creation. + // On failure the entry is KEPT as ready:false — resolve() stays undefined so + // consumers never receive a path that was never created; same-repo ensure() + // can retry creation once the failed warmup is cleared from pending. + if (initial && initialPending) { + const tracked = initialPending.then( + (entry) => { + pool.set(initial.prUrl, entry); + return entry; + }, + (err) => { + pending.delete(initial.prUrl); + throw err; + }, + ); + pending.set(initial.prUrl, tracked); + creationChain = tracked.catch(() => {}); + tracked + .then(() => pending.delete(initial.prUrl)) + .catch(() => {}); // warmup may complete with nobody awaiting it + } + return { get(prUrl) { return pool.get(prUrl); }, has(prUrl) { return pool.has(prUrl); }, @@ -60,7 +93,7 @@ export function createWorktreePool(config: WorktreePoolConfig, initial?: PoolEnt throw new Error("Cross-repo pool cannot create worktrees for other PRs"); } - const promise = (async (): Promise => { + const create = async (): Promise => { const number = metadata.platform === "github" ? metadata.number : metadata.iid; const worktreePath = join(config.sessionDir, "pool", `pr-${number}`); const refSpec = metadata.platform === "github" @@ -81,7 +114,10 @@ export function createWorktreePool(config: WorktreePoolConfig, initial?: PoolEnt const entry: PoolEntry = { path: worktreePath, prUrl: metadata.url, number, ready: true }; pool.set(metadata.url, entry); return entry; - })(); + }; + + const promise = creationChain.then(create, create); + creationChain = promise.catch(() => {}); pending.set(metadata.url, promise); try { @@ -94,6 +130,12 @@ export function createWorktreePool(config: WorktreePoolConfig, initial?: PoolEnt entries() { return pool.values(); }, async cleanup(runtime) { + // Wait out in-flight creations first: a warmup or queued ensure() that + // finishes after the pool is cleared would resurrect its entry and + // orphan the worktree it just built. + while (pending.size > 0) { + await Promise.all([...pending.values()].map((p) => p.catch(() => {}))); + } for (const entry of pool.values()) { await removeWorktree(runtime, entry.path, { force: true, cwd: config.repoDir }); } From a2d19a4e61aa4f0bab7280bc25dc17229d7c735b Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 20:13:34 -0700 Subject: [PATCH 02/14] fix(review): survive long PR checkout warmups + classify reconstructed renames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stress-testing against oven-sh/bun#30412 (2,188 files) surfaced three bugs: - Bun.serve's default 10s idleTimeout killed /api/semantic-diff while it parked on the background checkout warmup (a clone that can take minutes). Disable the idle timeout on all servers — AI SSE streams can also stall >10s between bytes while a permission prompt waits. - The file-badge hook memoized that failed fetch in a module-level cache keyed by patch, pinning every badge to empty until a hard refresh. Never cache failures; retry with backoff (5s/15s/30s). - reconstructGhPatch/reconstructPatch omitted the `similarity index` line, which Pierre's parser keys rename classification off — pure renames rendered as blank plain changes with no old path. Emit 100% for patch-less renames/copies (exactly accurate) and a synthetic 99% for patched ones (consumers only branch on 100% vs not). --- .../hooks/useFileSemanticChanges.ts | 64 +++++++++++++------ packages/server/annotate.ts | 3 + packages/server/goal-setup.ts | 2 + packages/server/index.ts | 3 + packages/server/review.ts | 4 ++ packages/shared/pr-github.test.ts | 8 ++- packages/shared/pr-github.ts | 15 +++-- packages/shared/pr-gitlab.test.ts | 32 ++++++++++ packages/shared/pr-gitlab.ts | 4 ++ 9 files changed, 112 insertions(+), 23 deletions(-) diff --git a/packages/review-editor/hooks/useFileSemanticChanges.ts b/packages/review-editor/hooks/useFileSemanticChanges.ts index 6acb84de4..627e24098 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 4c99eea7f..5cd36116d 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 57d9c99cd..9854635a5 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 a2c68b0b3..c3363e841 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 f07d30089..91c93c148 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 4dfc1c49f..4f7d97135 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 1ca1de806..095f854b1 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 9dedf08ae..5904afbda 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 ab8edb3ec..e0125072d 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) { From a1b2df45ace84dede2eaabeb6af8b25868fae910 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 21:03:40 -0700 Subject: [PATCH 03/14] feat(review): local full-diff upgrade for PRs whose API diff is truncated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On oversized PRs the platform APIs withhold per-file patch content entirely (bun#30412: 1,066 of 2,188 files came back with status added/modified, zeroed counts, and no patch). Those files rendered as empty stubs with no diff. - fetchGhPR/fetchGlMR flag the result `patchIncomplete` when patch-less non-rename entries exist or the 3000-file cap truncates the listing. - New runPRLayerLocalDiff (pr-stack.ts) recomputes the exact layer diff in the local checkout: platform merge-base + head SHA two-dot diff (three-dot vs baseSha fallback), fetch-by-SHA for objects missing from shallow clones, -l0 so rename detection doesn't silently degrade on huge PRs. - The review UI shows a "Partial diff · Load full diff" notice in layer scope; clicking re-requests the layer scope and the server swaps in the recomputed full diff (waiting out the background clone if needed). - PR scope/switch state writes are epoch-guarded: a request parked on the checkout warmup can no longer overwrite a newer scope select or pr-switch. - draftKey follows the upgraded patch so annotation drafts survive pr-switch round-trips; recompute failures surface in the response error field. - Pi server mirrors all of it, including an agentCwd fallback so the upgrade works for PRs switched-to under a cross-repo clone pool. --- apps/hook/server/index.ts | 6 + apps/pi-extension/plannotator-browser.ts | 3 + apps/pi-extension/server/serverReview.ts | 107 +++++++++++++- packages/review-editor/App.tsx | 27 +++- packages/review-editor/hooks/usePRSession.ts | 9 ++ packages/review-editor/hooks/usePRStack.ts | 1 + packages/server/pr.ts | 2 +- packages/server/review.ts | 100 ++++++++++++- packages/shared/pr-github.test.ts | 49 +++++++ packages/shared/pr-github.ts | 29 +++- packages/shared/pr-gitlab.test.ts | 44 +++++- packages/shared/pr-gitlab.ts | 25 +++- packages/shared/pr-provider.ts | 2 +- packages/shared/pr-stack.test.ts | 143 ++++++++++++++++++- packages/shared/pr-stack.ts | 74 ++++++++++ 15 files changed, 599 insertions(+), 22 deletions(-) diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index d0f44dfec..69eb08af6 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -505,6 +505,7 @@ if (args[0] === "sessions") { let diffError: string | undefined; let gitContext: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; + let prPatchIncomplete = false; let initialDiffType: DiffType | WorkspaceDiffType | undefined; let agentCwd: string | undefined; let worktreePool: WorktreePool | undefined; @@ -544,6 +545,7 @@ if (args[0] === "sessions") { rawPatch = pr.rawPatch; gitRef = `${getMRLabel(prRef)} ${getMRNumberLabel(prRef)}`; prMetadata = pr.metadata; + prPatchIncomplete = pr.patchIncomplete ?? false; } catch (err) { console.error(err instanceof Error ? err.message : "Failed to fetch PR"); process.exit(1); @@ -797,6 +799,7 @@ if (args[0] === "sessions") { diffType: workspace ? (initialDiffType ?? workspace.diffType) : gitContext ? (initialDiffType ?? "unstaged") : undefined, gitContext, prMetadata, + prPatchIncomplete, workspace, agentCwd, worktreePool, @@ -1362,6 +1365,7 @@ if (args[0] === "sessions") { let userDiffType: DiffType | WorkspaceDiffType | undefined; let gitContext: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; + let prPatchIncomplete = false; let workspace: Awaited> | undefined; let agentCwd: string | undefined; @@ -1387,6 +1391,7 @@ if (args[0] === "sessions") { rawPatch = pr.rawPatch; gitRef = `${getMRLabel(prRef)} ${getMRNumberLabel(prRef)}`; prMetadata = pr.metadata; + prPatchIncomplete = pr.patchIncomplete ?? false; } catch (err) { console.error(err instanceof Error ? err.message : `Failed to fetch ${getMRLabel(prRef)} ${getMRNumberLabel(prRef)}`); process.exit(1); @@ -1440,6 +1445,7 @@ if (args[0] === "sessions") { diffType: isPRMode ? undefined : userDiffType, gitContext, prMetadata, + prPatchIncomplete, workspace, agentCwd, sharingEnabled: bridgeSharingEnabled, diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index 4c322da95..e9396e194 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -253,6 +253,7 @@ export async function startCodeReviewBrowserSession( let diffError: string | undefined; let gitCtx: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; + let prPatchIncomplete = false; let diffType: DiffType | WorkspaceDiffType | undefined; let agentCwd: string | undefined; let initialBase: string | undefined; @@ -291,6 +292,7 @@ export async function startCodeReviewBrowserSession( rawPatch = pr.rawPatch; gitRef = `${getMRLabel(prRef)} ${getMRNumberLabel(prRef)}`; prMetadata = pr.metadata; + prPatchIncomplete = pr.patchIncomplete ?? false; if (shouldUseLocalPrCheckout(options)) { // Create local worktree for agent file access (--local is the default for PR reviews) @@ -473,6 +475,7 @@ export async function startCodeReviewBrowserSession( gitContext: gitCtx, initialBase, prMetadata, + prPatchIncomplete, workspace, agentCwd, worktreePool, diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index 10b7d1d1e..8901964dc 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -38,6 +38,7 @@ import { resolveStackInfo, resolvePRFullStackBaseRef, runPRFullStackDiff, + runPRLayerLocalDiff, type PRDiffScope, } from "../generated/pr-stack.js"; @@ -198,6 +199,12 @@ export async function startReviewServer(options: { shareBaseUrl?: string; pasteApiUrl?: string; prMetadata?: PRMetadata; + /** + * The initial layer patch is missing per-file content (platform APIs + * withhold patches on very large PRs). Enables the local recompute upgrade + * once a pool checkout is ready. + */ + prPatchIncomplete?: boolean; /** Working directory for agent processes (e.g., --local worktree). Independent of diff pipeline. */ agentCwd?: string; /** Local parent directory containing multiple child VCS repositories. */ @@ -228,8 +235,22 @@ export async function startReviewServer(options: { let prListCache: import("../generated/pr-types.js").PRListItem[] | null = null; let prListCacheTime = 0; - const prSwitchCache = new Map(); - if (isPRMode && prMeta) prSwitchCache.set(prMeta.url, { metadata: prMeta, rawPatch: options.rawPatch }); + // Platform APIs withhold per-file patches on very large PRs. When the layer + // patch is incomplete, a local recompute (exact merge-base diff, no size + // limits) becomes available once a pool checkout exists — the layer + // fingerprint flips to drive the refresh notice, and the pr-diff-scope + // "layer" branch performs the upgrade. Tracked per-PR across pr-switch. + // Gated on the pool: without a local checkout the upgrade can never run, + // so the client must not be offered one. + let layerPatchIncomplete = (options.prPatchIncomplete ?? false) && isPRMode && !!options.worktreePool; + const prSwitchCache = new Map(); + if (isPRMode && prMeta) { + prSwitchCache.set(prMeta.url, { + metadata: prMeta, + rawPatch: options.rawPatch, + patchIncomplete: layerPatchIncomplete, + }); + } const prStackTreeCache = new Map(); // Fetch full stack tree (best-effort — always try in PR mode so root PRs @@ -281,6 +302,10 @@ export async function startReviewServer(options: { let originalPRGitRef = options.gitRef; let originalPRError = options.error; let currentPRDiffScope: PRDiffScope = "layer"; + // Monotonic guard for PR scope/switch state writes. Scope requests now park + // on long awaits (checkout warmup, full recompute) — a request that resumed + // after a NEWER scope select or pr-switch must not overwrite their state. + let prScopeEpoch = 0; // Tracks the base branch the user picked from the UI. Agent review prompts // read this (not gitContext.defaultBranch) so they analyze the same diff // the reviewer is currently looking at. Honors an explicit initialBase from @@ -301,8 +326,13 @@ export async function startReviewServer(options: { if (workspace) return await workspace.getFingerprint(); if (isPRMode) { if (currentPRDiffScope === "layer") { - // Platform-computed diff — immutable locally; recaptured on pr-switch. - return `pr-layer:${prMeta?.url ?? ""}`; + // Platform-computed diff — immutable locally. The :incomplete + // suffix keeps the baseline honest across the local-recompute + // upgrade (the upgrade recaptures without it); the upgrade notice + // itself is client-driven via prPatchIncomplete, not this probe. + // Recaptured on pr-switch. + const suffix = layerPatchIncomplete ? ":incomplete" : ""; + return `pr-layer:${prMeta?.url ?? ""}${suffix}`; } // Full-stack: three-dot diff against the local checkout — fingerprint // (merge-base, HEAD), which changes exactly when the patch can. @@ -662,6 +692,7 @@ export async function startReviewServer(options: { prDiffScope: currentPRDiffScope, prDiffScopeOptions, }), + ...(isPRMode && layerPatchIncomplete && { prPatchIncomplete: true }), ...(isPRMode && initialViewedFiles.length > 0 && { viewedFiles: initialViewedFiles }), ...(currentError && { error: currentError }), semanticDiff: await getSemanticDiffAdvert(), @@ -788,17 +819,77 @@ export async function startReviewServer(options: { return; } + const scopeEpoch = ++prScopeEpoch; + // A newer scope select or pr-switch landed while this request was + // parked on an await: drop this request's writes and return the + // newest state so the client converges on it. + const respondSuperseded = async () => { + const semanticDiff = await getSemanticDiffAdvert(); + json(res, { + rawPatch: currentPatch, + gitRef: currentGitRef, + prDiffScope: currentPRDiffScope, + ...(layerPatchIncomplete ? { prPatchIncomplete: true } : {}), + ...(currentError ? { error: currentError } : {}), + semanticDiff, + }); + }; + if (body.scope === "layer") { + // Upgrade path: the platform withheld per-file content for this + // PR (too large). Once a pool checkout exists, recompute the + // exact layer diff locally and replace the truncated API + // reconstruction. Snapshot the PR before the await — a pr-switch + // landing mid-recompute must not have its patch overwritten with + // the previous PR's diff. + const upgradeMeta = prMeta; + let upgradeError: string | undefined; + if (layerPatchIncomplete && options.worktreePool && upgradeMeta) { + let upgradeCwd: string | undefined; + try { + upgradeCwd = (await options.worktreePool.ensure(reviewRuntime, upgradeMeta)).path; + } catch { + // Pool can't make a worktree (e.g. cross-repo pool after a + // pr-switch). The initial clone is still the right repo — + // pr-switch enforces same-project — and the recompute diffs + // explicit SHAs (fetching missing ones), so fall back to it. + upgradeCwd = options.agentCwd && existsSync(options.agentCwd) ? options.agentCwd : undefined; + } + if (upgradeCwd && prMeta === upgradeMeta) { + const result = await runPRLayerLocalDiff(reviewRuntime, upgradeMeta, upgradeCwd); + if (prMeta === upgradeMeta) { + if (!result.error) { + originalPRPatch = result.patch; + originalPRError = undefined; + layerPatchIncomplete = false; + prSwitchCache.set(upgradeMeta.url, { + metadata: upgradeMeta, + rawPatch: result.patch, + patchIncomplete: false, + }); + } else { + upgradeError = `Could not recompute the full diff locally: ${result.error}`; + console.error(`Local PR diff recompute failed: ${result.error}`); + } + } + } + } + if (scopeEpoch !== prScopeEpoch) return respondSuperseded(); currentPatch = originalPRPatch; currentGitRef = originalPRGitRef; currentError = originalPRError; currentPRDiffScope = "layer"; + // The upgrade changed the patch this session serves; drafts must + // key off it so a pr-switch round-trip (which rehashes from the + // cache) resolves to the same key. + if (!layerPatchIncomplete) draftKey = contentHash(currentPatch); captureDiffFingerprint(); json(res, { rawPatch: currentPatch, gitRef: currentGitRef, prDiffScope: currentPRDiffScope, - ...(currentError ? { error: currentError } : {}), + ...(layerPatchIncomplete ? { prPatchIncomplete: true } : {}), + ...((currentError ?? upgradeError) ? { error: currentError ?? upgradeError } : {}), semanticDiff: await getSemanticDiffAdvert(), }); return; @@ -818,6 +909,7 @@ export async function startReviewServer(options: { return; } + if (scopeEpoch !== prScopeEpoch) return respondSuperseded(); currentPatch = result.patch; currentGitRef = result.label; currentError = undefined; @@ -847,6 +939,9 @@ export async function startReviewServer(options: { const cached = prSwitchCache.get(body.url); const pr = cached ?? await fetchPR(newRef); if (!cached) prSwitchCache.set(body.url, pr); + // Bump the scope epoch so a scope request parked on a long await + // cannot overwrite this switch. + prScopeEpoch++; prMeta = pr.metadata; prRef = prRefFromMetadata(pr.metadata); currentPatch = pr.rawPatch; @@ -856,6 +951,7 @@ export async function startReviewServer(options: { originalPRGitRef = currentGitRef; originalPRError = undefined; currentPRDiffScope = "layer"; + layerPatchIncomplete = (pr.patchIncomplete ?? false) && !!options.worktreePool; draftKey = contentHash(pr.rawPatch); prListCache = null; captureDiffFingerprint(); @@ -907,6 +1003,7 @@ export async function startReviewServer(options: { prStackTree, prDiffScope: currentPRDiffScope, prDiffScopeOptions, + ...(layerPatchIncomplete ? { prPatchIncomplete: true } : {}), repoInfo, ...(switchedViewedFiles.length > 0 && { viewedFiles: switchedViewedFiles }), ...(currentError ? { error: currentError } : {}), diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index ccd7cf03f..5a638c197 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -208,7 +208,7 @@ const ReviewApp: React.FC = () => { document.title = repoInfo ? `${repoInfo.display} · Code Review` : "Code Review"; }, [repoInfo]); - const { prMetadata, prStackInfo, prStackTree, prDiffScope, prDiffScopeOptions, updatePRSession } = usePRSession(); + const { prMetadata, prStackInfo, prStackTree, prDiffScope, prDiffScopeOptions, prPatchIncomplete, updatePRSession } = usePRSession(); const { withPRContext } = useAnnotationFactory(prMetadata, prStackInfo ? prDiffScope : undefined); const prStackCallbacksRef = useRef(null); @@ -921,6 +921,7 @@ const ReviewApp: React.FC = () => { prStackTree?: PRStackTree | null; prDiffScope?: PRDiffScope; prDiffScopeOptions?: PRDiffScopeOption[]; + prPatchIncomplete?: boolean; platformUser?: string; viewedFiles?: string[]; error?: string; @@ -966,6 +967,7 @@ const ReviewApp: React.FC = () => { ...(data.prStackTree !== undefined && { prStackTree: data.prStackTree }), ...(data.prDiffScope && { prDiffScope: data.prDiffScope }), ...(data.prDiffScopeOptions && { prDiffScopeOptions: data.prDiffScopeOptions }), + ...(data.prMetadata && { prPatchIncomplete: data.prPatchIncomplete === true }), }); if (data.platformUser) setPlatformUser(data.platformUser); // Initialize viewed files from GitHub's state (set before draft restore so draft takes precedence) @@ -1275,6 +1277,9 @@ const ReviewApp: React.FC = () => { ...(data.prStackTree !== undefined && { prStackTree: data.prStackTree }), ...(data.prDiffScope && { prDiffScope: data.prDiffScope }), ...(data.prDiffScopeOptions && { prDiffScopeOptions: data.prDiffScopeOptions }), + // Scope/switch responses authoritatively report partiality; absence + // means the patch is complete (e.g. after the local recompute upgrade). + prPatchIncomplete: data.prPatchIncomplete === true, }); if (data.repoInfo) setRepoInfo(data.repoInfo); if (data.prMetadata) { @@ -2174,6 +2179,26 @@ const ReviewApp: React.FC = () => { )} + {/* Partial PR diff notice — the platform withheld per-file + content (PR too large). "Load full diff" re-requests the + layer scope; the server recomputes the exact diff from the + local checkout (waiting out the warmup if needed). */} + {prPatchIncomplete && prDiffScope === 'layer' && !isSwitchingPRScope && ( +
+ + Partial diff + + Partial + +
+ )} + {/* Diff staleness notice — files changed since this snapshot was computed (agent editing mid-review). Non-blocking; the user refreshes when ready. */} diff --git a/packages/review-editor/hooks/usePRSession.ts b/packages/review-editor/hooks/usePRSession.ts index 3401aa549..866394a7f 100644 --- a/packages/review-editor/hooks/usePRSession.ts +++ b/packages/review-editor/hooks/usePRSession.ts @@ -8,6 +8,12 @@ export interface PRSessionState { prStackTree: PRStackTree | null; prDiffScope: PRDiffScope; prDiffScopeOptions: PRDiffScopeOption[]; + /** + * The platform withheld per-file content for this PR (too large). A local + * recompute is offered via the partial-diff notice; cleared when a server + * response stops reporting the flag. + */ + prPatchIncomplete: boolean; } export interface PRSessionUpdate { @@ -16,6 +22,7 @@ export interface PRSessionUpdate { prStackTree?: PRStackTree | null; prDiffScope?: PRDiffScope; prDiffScopeOptions?: PRDiffScopeOption[]; + prPatchIncomplete?: boolean; } export function usePRSession() { @@ -25,6 +32,7 @@ export function usePRSession() { prStackTree: null, prDiffScope: 'layer', prDiffScopeOptions: [], + prPatchIncomplete: false, }); const updatePRSession = useCallback((update: PRSessionUpdate) => { @@ -35,6 +43,7 @@ export function usePRSession() { if (update.prStackTree !== undefined) next.prStackTree = update.prStackTree; if (update.prDiffScope !== undefined) next.prDiffScope = update.prDiffScope; if (update.prDiffScopeOptions !== undefined) next.prDiffScopeOptions = update.prDiffScopeOptions; + if (update.prPatchIncomplete !== undefined) next.prPatchIncomplete = update.prPatchIncomplete; return next; }); }, []); diff --git a/packages/review-editor/hooks/usePRStack.ts b/packages/review-editor/hooks/usePRStack.ts index 9cc22c76e..dc418d747 100644 --- a/packages/review-editor/hooks/usePRStack.ts +++ b/packages/review-editor/hooks/usePRStack.ts @@ -10,6 +10,7 @@ export interface PRSwitchResponse { prStackTree?: unknown; prDiffScope?: PRDiffScope; prDiffScopeOptions?: unknown[]; + prPatchIncomplete?: boolean; repoInfo?: unknown; viewedFiles?: string[]; error?: string; diff --git a/packages/server/pr.ts b/packages/server/pr.ts index 20f547b86..97bae2024 100644 --- a/packages/server/pr.ts +++ b/packages/server/pr.ts @@ -89,7 +89,7 @@ export function getPRUser(ref: PRRef): Promise { export function fetchPR( ref: PRRef, -): Promise<{ metadata: PRMetadata; rawPatch: string }> { +): Promise<{ metadata: PRMetadata; rawPatch: string; patchIncomplete?: boolean }> { return fetchPRCore(runtime, ref); } diff --git a/packages/server/review.ts b/packages/server/review.ts index 91c93c148..46c865823 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -32,6 +32,7 @@ import { resolveStackInfo, resolvePRFullStackBaseRef, runPRFullStackDiff, + runPRLayerLocalDiff, checkoutPRHead, type PRDiffScope, } from "@plannotator/shared/pr-stack"; @@ -109,6 +110,12 @@ export interface ReviewServerOptions { opencodeClient?: OpencodeClient; /** PR metadata when reviewing a pull request (PR mode) */ prMetadata?: PRMetadata; + /** + * The initial layer patch is missing per-file content (platform APIs + * withhold patches on very large PRs). Enables the local recompute upgrade + * once a pool checkout is ready. + */ + prPatchIncomplete?: boolean; /** Working directory for agent processes (e.g., --local worktree). Independent of diff pipeline. */ agentCwd?: string; /** Per-PR worktree pool. When set, pr-switch creates worktrees instead of checking out. */ @@ -176,10 +183,28 @@ export async function startReviewServer( let originalPRGitRef = options.gitRef; let originalPRError = options.error; let currentPRDiffScope: PRDiffScope = "layer"; + // Monotonic guard for PR scope/switch state writes. Scope requests now park + // on long awaits (checkout warmup, full recompute) — a request that resumed + // after a NEWER scope select or pr-switch must not overwrite their state. + let prScopeEpoch = 0; + // Platform APIs withhold per-file patches on very large PRs. When the layer + // patch is incomplete, a local recompute (exact merge-base diff, no size + // limits) becomes available once the checkout warmup finishes — the layer + // fingerprint flips to drive the refresh notice, and the pr-diff-scope + // "layer" branch performs the upgrade. Tracked per-PR across pr-switch. + // Gated on the pool: without a local checkout the upgrade can never run, + // so the client must not be offered one. + let layerPatchIncomplete = (options.prPatchIncomplete ?? false) && isPRMode && !!options.worktreePool; let prListCache: PRListItem[] | null = null; let prListCacheTime = 0; - const prSwitchCache = new Map(); - if (isPRMode && prMetadata) prSwitchCache.set(prMetadata.url, { metadata: prMetadata, rawPatch: options.rawPatch }); + const prSwitchCache = new Map(); + if (isPRMode && prMetadata) { + prSwitchCache.set(prMetadata.url, { + metadata: prMetadata, + rawPatch: options.rawPatch, + patchIncomplete: layerPatchIncomplete, + }); + } const prStackTreeCache = new Map(); // Tracks the base branch the user picked from the UI. Agent review prompts // read this (not gitContext.defaultBranch) so they analyze the same diff @@ -251,9 +276,13 @@ export async function startReviewServer( if (workspace) return await workspace.getFingerprint(); if (isPRMode) { if (currentPRDiffScope === "layer") { - // Platform-computed diff — immutable locally. Recaptured on - // pr-switch; remote-side PR updates are out of scope here. - return `pr-layer:${prMetadata?.url ?? ""}`; + // Platform-computed diff — immutable locally. The :incomplete + // suffix keeps the baseline honest across the local-recompute + // upgrade (the upgrade recaptures without it); the upgrade notice + // itself is client-driven via prPatchIncomplete, not this probe. + // Recaptured on pr-switch; remote-side PR updates are out of scope. + const suffix = layerPatchIncomplete ? ":incomplete" : ""; + return `pr-layer:${prMetadata?.url ?? ""}${suffix}`; } // Full-stack: three-dot diff against the local checkout — fingerprint // (merge-base, HEAD), which changes exactly when the patch can. @@ -715,6 +744,7 @@ export async function startReviewServer( prDiffScope: currentPRDiffScope, prDiffScopeOptions, }), + ...(isPRMode && layerPatchIncomplete && { prPatchIncomplete: true }), ...(isPRMode && initialViewedFiles.length > 0 && { viewedFiles: initialViewedFiles }), ...(currentError && { error: currentError }), semanticDiff: await getSemanticDiffAdvert(), @@ -859,17 +889,68 @@ export async function startReviewServer( return Response.json({ error: "Invalid PR diff scope" }, { status: 400 }); } + const scopeEpoch = ++prScopeEpoch; + // A newer scope select or pr-switch landed while this request + // was parked on an await: drop this request's writes and return + // the newest state so the client converges on it. + const supersededResponse = async () => { + const semanticDiff = await getSemanticDiffAdvert(); + return Response.json({ + rawPatch: currentPatch, + gitRef: currentGitRef, + prDiffScope: currentPRDiffScope, + ...(layerPatchIncomplete && { prPatchIncomplete: true }), + ...(currentError && { error: currentError }), + semanticDiff, + }); + }; + if (body.scope === "layer") { + // Upgrade path: the platform withheld per-file content for + // this PR (too large). Once the local checkout is ready, + // recompute the exact layer diff locally and replace the + // truncated API reconstruction. Snapshot the PR before the + // await — a pr-switch landing mid-recompute must not have its + // patch overwritten with the previous PR's diff. + const upgradeMetadata = prMetadata; + let upgradeError: string | undefined; + if (layerPatchIncomplete && options.worktreePool && upgradeMetadata) { + const upgradeCwd = await ensurePRLocalCwd(upgradeMetadata); + if (upgradeCwd && prMetadata === upgradeMetadata) { + const result = await runPRLayerLocalDiff(gitRuntime, upgradeMetadata, upgradeCwd); + if (prMetadata === upgradeMetadata) { + if (!result.error) { + originalPRPatch = result.patch; + originalPRError = undefined; + layerPatchIncomplete = false; + prSwitchCache.set(upgradeMetadata.url, { + metadata: upgradeMetadata, + rawPatch: result.patch, + patchIncomplete: false, + }); + } else { + upgradeError = `Could not recompute the full diff locally: ${result.error}`; + console.error(`Local PR diff recompute failed: ${result.error}`); + } + } + } + } + if (scopeEpoch !== prScopeEpoch) return supersededResponse(); currentPatch = originalPRPatch; currentGitRef = originalPRGitRef; currentError = originalPRError; currentPRDiffScope = "layer"; + // The upgrade changed the patch this session serves; drafts + // must key off it so a pr-switch round-trip (which rehashes + // from the cache) resolves to the same key. + if (!layerPatchIncomplete) draftKey = contentHash(currentPatch); captureDiffFingerprint(); return Response.json({ rawPatch: currentPatch, gitRef: currentGitRef, prDiffScope: currentPRDiffScope, - ...(currentError && { error: currentError }), + ...(layerPatchIncomplete && { prPatchIncomplete: true }), + ...((currentError ?? upgradeError) && { error: currentError ?? upgradeError }), semanticDiff: await getSemanticDiffAdvert(), }); } @@ -896,6 +977,7 @@ export async function startReviewServer( return Response.json({ error: result.error }, { status: 400 }); } + if (scopeEpoch !== prScopeEpoch) return supersededResponse(); currentPatch = result.patch; currentGitRef = result.label; currentError = undefined; @@ -958,7 +1040,9 @@ export async function startReviewServer( const pr = cached ?? await fetchPR(newRef); if (!cached) prSwitchCache.set(body.url, pr); - // Update mutable server state + // Update mutable server state. Bump the scope epoch so a scope + // request parked on a long await cannot overwrite this switch. + prScopeEpoch++; prMetadata = pr.metadata; prRef = prRefFromMetadata(pr.metadata); currentPatch = pr.rawPatch; @@ -968,6 +1052,7 @@ export async function startReviewServer( originalPRGitRef = currentGitRef; originalPRError = undefined; currentPRDiffScope = "layer"; + layerPatchIncomplete = (pr.patchIncomplete ?? false) && !!options.worktreePool; draftKey = contentHash(pr.rawPatch); prListCache = null; captureDiffFingerprint(); @@ -1031,6 +1116,7 @@ export async function startReviewServer( prStackTree, prDiffScope: currentPRDiffScope, prDiffScopeOptions, + ...(layerPatchIncomplete && { prPatchIncomplete: true }), repoInfo, ...(switchedViewedFiles.length > 0 && { viewedFiles: switchedViewedFiles }), ...(currentError ? { error: currentError } : {}), diff --git a/packages/shared/pr-github.test.ts b/packages/shared/pr-github.test.ts index 4f7d97135..35ee988c9 100644 --- a/packages/shared/pr-github.test.ts +++ b/packages/shared/pr-github.test.ts @@ -88,10 +88,58 @@ describe("fetchGhPR", () => { expect(result.rawPatch).toContain("+new"); expect(result.rawPatch).toContain("diff --git a/src/b.ts b/src/b.ts"); expect(result.rawPatch).toContain("new file mode 100644"); + // Every entry carried a patch — nothing is missing, no upgrade needed. + expect(result.patchIncomplete).toBeFalsy(); // Metadata path is unaffected by the fallback. expect(result.metadata).toMatchObject({ number: 123, mergeBaseSha: "c".repeat(40) }); }); + test("flags the patch incomplete when GitHub omits content for non-rename entries", async () => { + // The real shape from oversized PRs: status added/modified with zeroed + // counts and no patch field at all. + const entries = JSON.stringify([ + { filename: "src/big.rs", status: "added" }, + { filename: "src/also.zig", status: "modified" }, + { filename: "src/ok.ts", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }, + ]); + const { runtime } = githubRuntime({ + prDiff: { exitCode: 1, stderr: "406" }, + files: { exitCode: 0, stdout: entries }, + }); + + const errSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const result = await fetchGhPR(runtime, REF); + expect(result.patchIncomplete).toBe(true); + const warned = errSpy.mock.calls.some((args) => String(args[0]).includes("omitted diff content for 2 file(s)")); + expect(warned).toBe(true); + } finally { + errSpy.mockRestore(); + } + }); + + test("pure renames without patches are complete information — not flagged", async () => { + const entries = JSON.stringify([ + { filename: "src/new.ts", previous_filename: "src/old.ts", status: "renamed" }, + { filename: "src/ok.ts", status: "modified", patch: "@@ -1 +1 @@\n-a\n+b" }, + ]); + const { runtime } = githubRuntime({ + prDiff: { exitCode: 1, stderr: "406" }, + files: { exitCode: 0, stdout: entries }, + }); + + const result = await fetchGhPR(runtime, REF); + expect(result.patchIncomplete).toBeFalsy(); + }); + + test("never flags the verbatim gh pr diff path as incomplete", async () => { + const patch = "diff --git a/x.ts b/x.ts\n--- a/x.ts\n+++ b/x.ts\n@@ -1 +1 @@\n-a\n+b\n"; + const { runtime } = githubRuntime({ prDiff: { exitCode: 0, stdout: patch } }); + + const result = await fetchGhPR(runtime, REF); + expect(result.patchIncomplete).toBeFalsy(); + }); + test("passes --hostname to the files API on GitHub Enterprise", async () => { const { runtime, calls } = githubRuntime({ prDiff: { exitCode: 1, stderr: "406" }, @@ -135,6 +183,7 @@ describe("fetchGhPR", () => { try { const result = await fetchGhPR(runtime, REF); expect(result.rawPatch).toContain("diff --git a/a.ts b/a.ts"); // partial diff still served + expect(result.patchIncomplete).toBe(true); // 3000-file cap → upgrade offered const warned = errSpy.mock.calls.some((args) => String(args[0]).includes("3500 changed files")); expect(warned).toBe(true); } finally { diff --git a/packages/shared/pr-github.ts b/packages/shared/pr-github.ts index 095f854b1..04bb7e5b5 100644 --- a/packages/shared/pr-github.ts +++ b/packages/shared/pr-github.ts @@ -135,10 +135,26 @@ export function reconstructGhPatch(files: GitHubFileEntry[]): string { return parts.join(""); } +/** + * True when a files-API entry should carry a patch but doesn't. Patch-less + * renames/copies are 100%-similarity moves (complete information); patch-less + * added/removed/modified entries mean GitHub gave up computing that file's + * diff (on very large PRs it omits the patch AND zeroes the counts). + * + * Known ambiguity: when GitHub withholds content it does so across all + * statuses, so a rename-WITH-edits whose patch was withheld is + * indistinguishable from a pure move and renders as one. In practice such PRs + * always have withheld non-rename entries too, so the flag (and the local + * recompute that fixes everything) still triggers. + */ +function entryMissingContent(f: GitHubFileEntry): boolean { + return !f.patch && f.status !== "renamed" && f.status !== "copied" && f.status !== "unchanged"; +} + export async function fetchGhPR( runtime: PRRuntime, ref: GhPRRef, -): Promise<{ metadata: PRMetadata; rawPatch: string }> { +): Promise<{ metadata: PRMetadata; rawPatch: string; patchIncomplete?: boolean }> { const repo = repoFlag(ref); // Fetch diff, metadata, and repository defaults in parallel. @@ -170,6 +186,7 @@ export async function fetchGhPR( // "diff exceeded the maximum number of lines"); in that case fetch the same // diff file-by-file from the paginated files API and stitch it back together. let rawPatch: string; + let patchIncomplete = false; if (diffResult.exitCode === 0) { rawPatch = diffResult.stdout; } else { @@ -197,6 +214,14 @@ export async function fetchGhPR( console.error( `Warning: PR reports ${expectedFiles} changed files but the GitHub files API returned ${fileEntries.length} (the API caps at 3000). The review is missing the remainder.`, ); + patchIncomplete = true; + } + const missingContent = fileEntries.filter(entryMissingContent).length; + if (missingContent > 0) { + console.error( + `Warning: GitHub omitted diff content for ${missingContent} file(s) (PR too large). They appear in the review without hunks; the full diff can be recomputed locally once the checkout is ready.`, + ); + patchIncomplete = true; } } @@ -246,7 +271,7 @@ export async function fetchGhPR( url: raw.url, }; - return { metadata, rawPatch }; + return { metadata, rawPatch, ...(patchIncomplete && { patchIncomplete }) }; } // --- PR Context --- diff --git a/packages/shared/pr-gitlab.test.ts b/packages/shared/pr-gitlab.test.ts index 5904afbda..de58cf9ce 100644 --- a/packages/shared/pr-gitlab.test.ts +++ b/packages/shared/pr-gitlab.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, test } from "bun:test"; +import { describe, expect, spyOn, test } from "bun:test"; import { fetchGlMR, parsePaginatedArray } from "./pr-gitlab"; import type { PRRuntime } from "./pr-types"; @@ -194,6 +194,48 @@ describe("fetchGlMR raw_diffs fallback", () => { expect(calls.some((c) => c.includes("/diffs?per_page=100"))).toBe(true); }); + test("flags the patch incomplete when GitLab withholds content for a modified file", async () => { + const entries = JSON.stringify([ + { + old_path: "src/big.ts", + new_path: "src/big.ts", + new_file: false, + deleted_file: false, + renamed_file: false, + diff: "", // modified file with no content = withheld + }, + { + old_path: "src/old.ts", + new_path: "src/new.ts", + new_file: false, + deleted_file: false, + renamed_file: true, + diff: "", // pure rename — complete information, must not flag + }, + ]); + const { runtime } = gitlabRuntime({ + rawDiffs: { exitCode: 1, stderr: "404 Not Found" }, + diffs: { exitCode: 0, stdout: entries }, + }); + + const errSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const result = await fetchGlMR(runtime, REF); + expect(result.patchIncomplete).toBe(true); + } finally { + errSpy.mockRestore(); + } + }); + + test("does not flag a fallback where every entry carries content", async () => { + const { runtime } = gitlabRuntime({ + rawDiffs: { exitCode: 1, stderr: "404 Not Found" }, + diffs: { exitCode: 0, stdout: DIFF_ENTRIES_JSON }, + }); + const result = await fetchGlMR(runtime, REF); + expect(result.patchIncomplete).toBeFalsy(); + }); + test("throws a clear empty-diff error when both raw_diffs and diffs are empty", async () => { const { runtime } = gitlabRuntime({ rawDiffs: { exitCode: 0, stdout: "" }, diff --git a/packages/shared/pr-gitlab.ts b/packages/shared/pr-gitlab.ts index e0125072d..abe815fe6 100644 --- a/packages/shared/pr-gitlab.ts +++ b/packages/shared/pr-gitlab.ts @@ -114,10 +114,20 @@ export async function getGlUser(runtime: PRRuntime, host: string): Promise { +): Promise<{ metadata: PRMetadata; rawPatch: string; patchIncomplete?: boolean }> { const encoded = encodeProject(ref.projectPath); // Primary: raw_diffs — preserves Git's binary-marker shape and includes @@ -138,6 +148,7 @@ export async function fetchGlMR( // returns empty (very large MRs that exceed its safety limit). Reconstruct a // unified patch from the JSON entries — the long-standing pre-raw_diffs path. let rawPatch: string; + let patchIncomplete = false; if (diffResult.exitCode === 0 && diffResult.stdout.trim()) { rawPatch = diffResult.stdout; } else { @@ -150,12 +161,20 @@ export async function fetchGlMR( const fbErr = fallback.stderr.trim() || `exit code ${fallback.exitCode}`; throw new Error(`Failed to fetch MR diff (raw_diffs: ${rawErr}; diffs: ${fbErr}).`); } - rawPatch = reconstructPatch(parsePaginatedArray(fallback.stdout)); + const entries = parsePaginatedArray(fallback.stdout); + rawPatch = reconstructPatch(entries); if (!rawPatch.trim()) { throw new Error( "MR diff is empty — the diff may be too large to fetch via the GitLab API. Review it on the GitLab web UI.", ); } + const missingContent = entries.filter(entryMissingContent).length; + if (missingContent > 0) { + console.error( + `Warning: GitLab omitted diff content for ${missingContent} file(s) (MR too large). They appear in the review without hunks; the full diff can be recomputed locally once the checkout is ready.`, + ); + patchIncomplete = true; + } } const raw = JSON.parse(viewResult.stdout) as { @@ -199,7 +218,7 @@ export async function fetchGlMR( url: raw.web_url, }; - return { metadata, rawPatch }; + return { metadata, rawPatch, ...(patchIncomplete && { patchIncomplete }) }; } // --- MR Context --- diff --git a/packages/shared/pr-provider.ts b/packages/shared/pr-provider.ts index 5680a7b91..ad33e92fb 100644 --- a/packages/shared/pr-provider.ts +++ b/packages/shared/pr-provider.ts @@ -35,7 +35,7 @@ export async function getUser(runtime: PRRuntime, ref: PRRef): Promise { +): Promise<{ metadata: PRMetadata; rawPatch: string; patchIncomplete?: boolean }> { if (ref.platform === "github") return fetchGhPR(runtime, ref); return fetchGlMR(runtime, ref); } diff --git a/packages/shared/pr-stack.test.ts b/packages/shared/pr-stack.test.ts index 040d0cbcd..071300741 100644 --- a/packages/shared/pr-stack.test.ts +++ b/packages/shared/pr-stack.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import type { PRMetadata } from "./pr-types"; import type { GitCommandResult, ReviewGitRuntime } from "./review-core"; -import { runPRFullStackDiff } from "./pr-stack"; +import { runPRFullStackDiff, runPRLayerLocalDiff } from "./pr-stack"; function result(stdout = "", stderr = "", exitCode = 0): GitCommandResult { return { stdout, stderr, exitCode }; @@ -102,3 +102,144 @@ describe("runPRFullStackDiff", () => { expect(diff.error).toContain("Could not find origin/main or local main"); }); }); + +describe("runPRLayerLocalDiff", () => { + const MERGE_BASE = "a".repeat(40); + const BASE = "b".repeat(40); + const HEAD = "c".repeat(40); + const layerMetadata: PRMetadata = { + ...metadata, + baseSha: BASE, + headSha: HEAD, + mergeBaseSha: MERGE_BASE, + }; + + function layerRuntime(opts: { + missingObjects?: Set; + /** Objects that become available after a `fetch origin -- ` */ + fetchable?: Set; + diffStdout?: string; + diffExitCode?: number; + diffStderr?: string; + }): { runtime: ReviewGitRuntime; calls: string[][] } { + const calls: string[][] = []; + const missing = new Set(opts.missingObjects ?? []); + return { + calls, + runtime: { + async runGit(args) { + calls.push(args); + if (args[0] === "cat-file") { + const sha = args.at(-1)!; + return missing.has(sha) ? result("", "missing", 1) : result("commit"); + } + if (args[0] === "fetch") { + const sha = args.at(-1)!; + if (opts.fetchable?.has(sha)) missing.delete(sha); + return result(); + } + if (args[0] === "diff") { + return result( + opts.diffStdout ?? "diff --git a/x.ts b/x.ts\n", + opts.diffStderr ?? "", + opts.diffExitCode ?? 0, + ); + } + return result("", "unexpected", 1); + }, + async readTextFile() { + return null; + }, + }, + }; + } + + test("diffs the platform merge-base against the PR head (exact layer diff)", async () => { + const { runtime, calls } = layerRuntime({}); + const diff = await runPRLayerLocalDiff(runtime, layerMetadata, "/tmp/checkout"); + + expect(diff.error).toBeUndefined(); + expect(diff.patch).toBe("diff --git a/x.ts b/x.ts\n"); + expect(calls.at(-1)).toEqual([ + "diff", + "--no-ext-diff", + "--find-renames", + "-l0", + "--src-prefix=a/", + "--dst-prefix=b/", + "--end-of-options", + MERGE_BASE, + HEAD, + ]); + }); + + test("fetches a missing merge-base by SHA before diffing (shallow clone)", async () => { + const { runtime, calls } = layerRuntime({ + missingObjects: new Set([MERGE_BASE]), + fetchable: new Set([MERGE_BASE]), + }); + const diff = await runPRLayerLocalDiff(runtime, layerMetadata, "/tmp/checkout"); + + expect(diff.error).toBeUndefined(); + expect(calls.some((c) => c[0] === "fetch" && c.at(-1) === MERGE_BASE)).toBe(true); + expect(calls.at(-1)?.slice(-2)).toEqual([MERGE_BASE, HEAD]); + }); + + test("falls back to three-dot against baseSha when the merge-base is unavailable", async () => { + const { runtime, calls } = layerRuntime({ missingObjects: new Set([MERGE_BASE]) }); + const diff = await runPRLayerLocalDiff(runtime, layerMetadata, "/tmp/checkout"); + + expect(diff.error).toBeUndefined(); + expect(calls.at(-1)?.at(-1)).toBe(`${BASE}...${HEAD}`); + }); + + test("falls back to three-dot when no merge-base SHA is reported (GitLab)", async () => { + const { runtime, calls } = layerRuntime({}); + const noMergeBase: PRMetadata = { ...layerMetadata }; + delete (noMergeBase as { mergeBaseSha?: string }).mergeBaseSha; + const diff = await runPRLayerLocalDiff(runtime, noMergeBase, "/tmp/checkout"); + + expect(diff.error).toBeUndefined(); + expect(calls.at(-1)?.at(-1)).toBe(`${BASE}...${HEAD}`); + }); + + test("errors when the PR head cannot be made available", async () => { + const { runtime } = layerRuntime({ missingObjects: new Set([HEAD]) }); + const diff = await runPRLayerLocalDiff(runtime, layerMetadata, "/tmp/checkout"); + + expect(diff.patch).toBe(""); + expect(diff.error).toContain("not available in the local checkout"); + }); + + test("errors when neither merge-base nor baseSha resolve locally", async () => { + const { runtime } = layerRuntime({ missingObjects: new Set([MERGE_BASE, BASE]) }); + const diff = await runPRLayerLocalDiff(runtime, layerMetadata, "/tmp/checkout"); + + expect(diff.patch).toBe(""); + expect(diff.error).toContain("Could not resolve the PR base commit"); + }); + + test("surfaces git diff failures and never returns a partial patch", async () => { + const { runtime } = layerRuntime({ diffExitCode: 128, diffStderr: "fatal: bad object\nmore" }); + const diff = await runPRLayerLocalDiff(runtime, layerMetadata, "/tmp/checkout"); + + expect(diff.patch).toBe(""); + expect(diff.error).toBe("fatal: bad object"); + }); + + test("treats an empty recompute as an error (must not blank a working review)", async () => { + const { runtime } = layerRuntime({ diffStdout: "" }); + const diff = await runPRLayerLocalDiff(runtime, layerMetadata, "/tmp/checkout"); + + expect(diff.error).toContain("empty diff"); + }); + + test("rejects an invalid head SHA without running git", async () => { + const { runtime, calls } = layerRuntime({}); + const bad: PRMetadata = { ...layerMetadata, headSha: "HEAD; rm -rf /" }; + const diff = await runPRLayerLocalDiff(runtime, bad, "/tmp/checkout"); + + expect(diff.error).toContain("Invalid PR head SHA"); + expect(calls.length).toBe(0); + }); +}); diff --git a/packages/shared/pr-stack.ts b/packages/shared/pr-stack.ts index 0d82ea7e4..042b6ab6f 100644 --- a/packages/shared/pr-stack.ts +++ b/packages/shared/pr-stack.ts @@ -1,4 +1,5 @@ import type { DiffResult, ReviewGitRuntime } from "./review-core"; +import { ensureObjectAvailable } from "./worktree"; import type { PRDiffScopeOption, PRMetadata, @@ -135,6 +136,79 @@ export async function runPRFullStackDiff( }; } +const FULL_SHA_RE = /^[0-9a-f]{40,64}$/i; + +/** + * Recompute the PR's LAYER diff locally — the same merge-base..head diff the + * platform renders, but with no API size limits. Used to upgrade a truncated + * files-API reconstruction (platforms withhold per-file patches on very large + * PRs) once a local checkout exists. + * + * Prefers the platform-reported merge-base SHA (the exact commit the platform + * diffed against); falls back to discovering the merge base locally via a + * three-dot diff against baseSha. Diffs explicit SHAs, not HEAD — agent jobs + * may move HEAD in the checkout. + */ +export async function runPRLayerLocalDiff( + runtime: ReviewGitRuntime, + metadata: PRMetadata, + cwd: string, +): Promise { + const unavailable = (error: string): DiffResult => ({ + patch: "", + label: "PR diff unavailable", + error, + }); + + if (!FULL_SHA_RE.test(metadata.headSha)) { + return unavailable(`Invalid PR head SHA: ${metadata.headSha}`); + } + + // Shallow warmup clones may lack an object; both GitHub and GitLab allow + // fetching reachable commits by SHA (the warmup already relies on this). + const ensureObject = (sha: string): Promise => + ensureObjectAvailable(runtime, sha, { cwd }); + + if (!(await ensureObject(metadata.headSha))) { + return unavailable(`PR head ${metadata.headSha} is not available in the local checkout.`); + } + + const diffArgsFor = (range: string[]): string[] => [ + "diff", + "--no-ext-diff", + // -l0 lifts diff.renameLimit: on the multi-thousand-file PRs this path + // exists for, the default limit silently downgrades rename detection and + // renamed+edited files would render as delete+add pairs. + "--find-renames", + "-l0", + "--src-prefix=a/", + "--dst-prefix=b/", + "--end-of-options", + ...range, + ]; + + let range: string[] | null = null; + if (metadata.mergeBaseSha && FULL_SHA_RE.test(metadata.mergeBaseSha) && (await ensureObject(metadata.mergeBaseSha))) { + range = [metadata.mergeBaseSha, metadata.headSha]; + } else if (FULL_SHA_RE.test(metadata.baseSha) && (await ensureObject(metadata.baseSha))) { + range = [`${metadata.baseSha}...${metadata.headSha}`]; + } + if (!range) { + return unavailable("Could not resolve the PR base commit in the local checkout."); + } + + const diff = await runtime.runGit(diffArgsFor(range), { cwd }); + if (diff.exitCode !== 0) { + const message = diff.stderr.trim() || "git diff failed"; + return unavailable(message.split("\n").find((line) => line.trim().length > 0) ?? message); + } + if (!diff.stdout.trim()) { + return unavailable("Local recompute produced an empty diff."); + } + + return { patch: diff.stdout, label: "PR diff (recomputed locally)" }; +} + /** * Staleness fingerprint for the full-stack diff above. The diff is three-dot * (`baseRef...HEAD` = merge-base(baseRef, HEAD)..HEAD), so its content changes From c8634f6163fe41e678d0c258cbdd7854207423e6 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 21:47:39 -0700 Subject: [PATCH 04/14] fix(review): use GitLab's too_large/collapsed flags for withheld-diff detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External review caught a false negative: a too-large ADDED file comes back new_file:true with an empty diff — indistinguishable from a legitimately empty new file under the old heuristic, so the partial-diff upgrade was never offered for exactly the files that matter most on big MRs. The REST /diffs endpoint marks withheld content explicitly per entry (verified against gitlab.com): too_large/collapsed are now authoritative in both directions — withheld adds/deletes are flagged, binaries and empty files are never misflagged. Older GitLab without the fields keeps the empty-diff-on-modification heuristic. --- apps/opencode-plugin/cli-bridge.test.ts | 2 +- packages/shared/pr-gitlab.test.ts | 61 +++++++++++++++++++++++++ packages/shared/pr-gitlab.ts | 23 ++++++++-- packages/shared/prompts.test.ts | 4 +- packages/shared/prompts.ts | 4 +- 5 files changed, 85 insertions(+), 9 deletions(-) diff --git a/apps/opencode-plugin/cli-bridge.test.ts b/apps/opencode-plugin/cli-bridge.test.ts index 0f6d7d1c9..6dd196262 100644 --- a/apps/opencode-plugin/cli-bridge.test.ts +++ b/apps/opencode-plugin/cli-bridge.test.ts @@ -148,7 +148,7 @@ describe("OpenCode CLI bridge helpers", () => { }); expect(localFeedback.agent).toBeUndefined(); expect(localFeedback.message).toContain("Fix these issues."); - expect(localFeedback.message).toContain("Please address this feedback."); + expect(localFeedback.message).toContain("This feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."); const prFeedback = buildReviewPromptFromBridgeOutcome({ decision: "annotated", diff --git a/packages/shared/pr-gitlab.test.ts b/packages/shared/pr-gitlab.test.ts index de58cf9ce..9c5076e0d 100644 --- a/packages/shared/pr-gitlab.test.ts +++ b/packages/shared/pr-gitlab.test.ts @@ -227,6 +227,67 @@ describe("fetchGlMR raw_diffs fallback", () => { } }); + test("too_large/collapsed flags catch withheld ADDED files (modern GitLab)", async () => { + // A too-large added file has new_file:true and an empty diff — without + // the explicit flag it would be indistinguishable from a legitimately + // empty new file and the upgrade would never be offered. + const entries = JSON.stringify([ + { + old_path: "src/huge.ts", + new_path: "src/huge.ts", + new_file: true, + deleted_file: false, + renamed_file: false, + too_large: true, + collapsed: false, + diff: "", + }, + ]); + const { runtime } = gitlabRuntime({ + rawDiffs: { exitCode: 1, stderr: "404 Not Found" }, + diffs: { exitCode: 0, stdout: entries }, + }); + + const errSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const result = await fetchGlMR(runtime, REF); + expect(result.patchIncomplete).toBe(true); + } finally { + errSpy.mockRestore(); + } + }); + + test("explicit too_large:false exonerates empty-diff entries (binary/empty files, modern GitLab)", async () => { + const entries = JSON.stringify([ + { + old_path: "logo.png", + new_path: "logo.png", + new_file: false, + deleted_file: false, + renamed_file: false, + too_large: false, + collapsed: false, + diff: "", // binary — complete information, must not flag + }, + { + old_path: "src/ok.ts", + new_path: "src/ok.ts", + new_file: false, + deleted_file: false, + renamed_file: false, + too_large: false, + collapsed: false, + 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.patchIncomplete).toBeFalsy(); + }); + test("does not flag a fallback where every entry carries content", async () => { const { runtime } = gitlabRuntime({ rawDiffs: { exitCode: 1, stderr: "404 Not Found" }, diff --git a/packages/shared/pr-gitlab.ts b/packages/shared/pr-gitlab.ts index abe815fe6..d318df188 100644 --- a/packages/shared/pr-gitlab.ts +++ b/packages/shared/pr-gitlab.ts @@ -41,6 +41,10 @@ interface GitLabDiffEntry { new_file: boolean; deleted_file: boolean; renamed_file: boolean; + /** Content withheld because the file's diff exceeds GitLab's size limits. Absent on older GitLab. */ + too_large?: boolean | null; + /** Diff collapsed (content omitted from the response). Absent on older GitLab. */ + collapsed?: boolean | null; } export { parsePaginatedArray } from "./cli-pagination"; @@ -116,12 +120,23 @@ export async function getGlUser(runtime: PRRuntime, host: string): Promise { test("opencode and pi use softer runtime default", () => { const oc = getReviewDeniedSuffix("opencode", {}); const pi = getReviewDeniedSuffix("pi", {}); - expect(oc).toBe("\n\nPlease address this feedback."); - expect(pi).toBe("\n\nPlease address this feedback."); + expect(oc).toBe("\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."); + expect(pi).toBe("\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."); }); test("uses configured override", () => { diff --git a/packages/shared/prompts.ts b/packages/shared/prompts.ts index 1f0f3c326..aa53383de 100644 --- a/packages/shared/prompts.ts +++ b/packages/shared/prompts.ts @@ -113,8 +113,8 @@ export function getReviewApprovedPrompt( } const REVIEW_DENIED_RUNTIME_DEFAULTS: Partial> = { - opencode: "\n\nPlease address this feedback.", - pi: "\n\nPlease address this feedback.", + opencode: "\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings.", + pi: "\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings.", }; export function getReviewDeniedSuffix( From 096372b6a05071adb1ab04913a84cd42caf8c4bd Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 22:09:34 -0700 Subject: [PATCH 05/14] =?UTF-8?q?feat(prompts):=20unify=20review-denied=20?= =?UTF-8?q?suffix=20=E2=80=94=20triage=20first,=20no=20coding=20off=20raw?= =?UTF-8?q?=20feedback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-runtime defaults map (#627) gave OpenCode and Pi a different review-denied suffix than every other runtime; updating one meant the others silently kept "you must address all of them" — an instruction to start coding immediately. Claude Code, Amp, Droid, Codex, Copilot, Gemini, and Kiro were all still on it. One default for every runtime now: triage the feedback, verify it against the code, discuss before changing anything. Per-runtime customization remains available via config (prompts.review.runtimes..denied), which resolves above the built-in default as before. --- packages/shared/prompts.test.ts | 16 ++++++---------- packages/shared/prompts.ts | 12 +++++------- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/packages/shared/prompts.test.ts b/packages/shared/prompts.test.ts index 8bc207017..f39778af7 100644 --- a/packages/shared/prompts.test.ts +++ b/packages/shared/prompts.test.ts @@ -307,16 +307,12 @@ describe("getAnnotateApprovedPrompt", () => { // ─── A4b. Review denied suffix ─────────────────────────────────────────────── describe("getReviewDeniedSuffix", () => { - test("returns default suffix for claude-code", () => { - const result = getReviewDeniedSuffix("claude-code", {}); - expect(result).toContain("identified issues"); - }); - - test("opencode and pi use softer runtime default", () => { - const oc = getReviewDeniedSuffix("opencode", {}); - const pi = getReviewDeniedSuffix("pi", {}); - expect(oc).toBe("\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."); - expect(pi).toBe("\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."); + test("every runtime gets the same triage-first default — no agent starts coding off raw review feedback", () => { + const runtimes = ["claude-code", "opencode", "pi", "amp", "droid", "codex", "copilot-cli", "gemini-cli", "kiro-cli"] as const; + for (const runtime of runtimes) { + expect(getReviewDeniedSuffix(runtime, {})).toBe(DEFAULT_REVIEW_DENIED_SUFFIX); + } + expect(DEFAULT_REVIEW_DENIED_SUFFIX).toContain("Do not change any code until we've discussed"); }); test("uses configured override", () => { diff --git a/packages/shared/prompts.ts b/packages/shared/prompts.ts index aa53383de..5d9ef097e 100644 --- a/packages/shared/prompts.ts +++ b/packages/shared/prompts.ts @@ -39,7 +39,7 @@ export function buildPlanFileRule(toolName: string, planFilePath?: string): stri export const DEFAULT_REVIEW_APPROVED_PROMPT = "# Code Review\n\nCode review completed — no changes requested."; -export const DEFAULT_REVIEW_DENIED_SUFFIX = "\nThe reviewer has identified issues above. You must address all of them."; +export const DEFAULT_REVIEW_DENIED_SUFFIX = "\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."; export const DEFAULT_PLAN_DENIED_PROMPT = "YOUR PLAN WAS NOT APPROVED.\n\nYou MUST revise the plan to address ALL of the feedback below before calling {{toolName}} again.\n\nRules:\n{{planFileRule}}- Do not resubmit the same plan unchanged.\n- Do NOT change the plan title (first # heading) unless the user explicitly asks you to.\n\n{{feedback}}"; @@ -112,22 +112,20 @@ export function getReviewApprovedPrompt( }); } -const REVIEW_DENIED_RUNTIME_DEFAULTS: Partial> = { - opencode: "\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings.", - pi: "\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings.", -}; - export function getReviewDeniedSuffix( runtime?: PromptRuntime | null, config?: PlannotatorConfig, ): string { + // Intentionally no per-runtime defaults: every agent gets the same + // triage-first instruction so none of them start coding off raw review + // feedback. Per-runtime customization stays available via config + // (prompts.review.runtimes..denied). return getConfiguredPrompt({ section: "review", key: "denied", runtime, config, fallback: DEFAULT_REVIEW_DENIED_SUFFIX, - runtimeFallbacks: REVIEW_DENIED_RUNTIME_DEFAULTS, }); } From 9e5c9ec505b006c3012cfdbc18d8f8b40d148ce9 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 22:21:50 -0700 Subject: [PATCH 06/14] =?UTF-8?q?fix(prompts):=20generalize=20review-denie?= =?UTF-8?q?d=20suffix=20=E2=80=94=20'from=20review',=20not=20'external=20A?= =?UTF-8?q?I=20reviewers'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback isn't always from AI reviewers or agent jobs; often it's the human reviewer's own annotations. Neutral wording covers both. --- apps/opencode-plugin/cli-bridge.test.ts | 2 +- packages/shared/prompts.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/opencode-plugin/cli-bridge.test.ts b/apps/opencode-plugin/cli-bridge.test.ts index 6dd196262..51acc498f 100644 --- a/apps/opencode-plugin/cli-bridge.test.ts +++ b/apps/opencode-plugin/cli-bridge.test.ts @@ -148,7 +148,7 @@ describe("OpenCode CLI bridge helpers", () => { }); expect(localFeedback.agent).toBeUndefined(); expect(localFeedback.message).toContain("Fix these issues."); - expect(localFeedback.message).toContain("This feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."); + expect(localFeedback.message).toContain("This feedback came from review. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."); const prFeedback = buildReviewPromptFromBridgeOutcome({ decision: "annotated", diff --git a/packages/shared/prompts.ts b/packages/shared/prompts.ts index 5d9ef097e..3b68f774c 100644 --- a/packages/shared/prompts.ts +++ b/packages/shared/prompts.ts @@ -39,7 +39,7 @@ export function buildPlanFileRule(toolName: string, planFilePath?: string): stri export const DEFAULT_REVIEW_APPROVED_PROMPT = "# Code Review\n\nCode review completed — no changes requested."; -export const DEFAULT_REVIEW_DENIED_SUFFIX = "\n\nThis feedback came from external AI reviewers. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."; +export const DEFAULT_REVIEW_DENIED_SUFFIX = "\n\nThis feedback came from review. Please triage it and verify it against the code and then come back to me with your thoughts on the findings. Do not change any code until we've discussed the findings."; export const DEFAULT_PLAN_DENIED_PROMPT = "YOUR PLAN WAS NOT APPROVED.\n\nYou MUST revise the plan to address ALL of the feedback below before calling {{toolName}} again.\n\nRules:\n{{planFileRule}}- Do not resubmit the same plan unchanged.\n- Do NOT change the plan title (first # heading) unless the user explicitly asks you to.\n\n{{feedback}}"; From 8ec2a53d880832c184a04bad337cb48bc9edd0c8 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 22:34:12 -0700 Subject: [PATCH 07/14] fix(review): non-blocking 'Load full diff' + flag-handling hardenings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review findings: - The partial-diff upgrade reused the scope-switch handler, so clicking "Load full diff" raised the full-screen PRSwitchOverlay — blocking the entire UI, potentially for minutes behind a cold clone, with no text and no cancel. The upgrade now has its own loading state: the notice shows a spinner ("Loading full diff…") and the reviewer keeps working with the partial diff while the request parks. Server-side epoch guards already handle scope/PR changes made during the wait. - GitLab too_large/collapsed: treat explicit null like absent (flags inconclusive → legacy heuristic decides) instead of silently exonerating. - Rename-limit lift uses -l100000 instead of -l0 ("0 = unlimited" only holds on git >= 2.29; on older git it could disable detection outright). --- packages/review-editor/App.tsx | 28 +++++++++++++++------ packages/review-editor/hooks/usePRStack.ts | 29 ++++++++++++++++++++++ packages/shared/pr-gitlab.test.ts | 27 ++++++++++++++++++++ packages/shared/pr-gitlab.ts | 5 +++- packages/shared/pr-stack.test.ts | 2 +- packages/shared/pr-stack.ts | 9 ++++--- 6 files changed, 86 insertions(+), 14 deletions(-) diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 5a638c197..8879af06c 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -214,7 +214,9 @@ const ReviewApp: React.FC = () => { const prStackCallbacksRef = useRef(null); const { isSwitchingPRScope, + isLoadingFullDiff, handleScopeSelect: handlePRDiffScopeSelect, + handleLoadFullDiff, handlePRSwitch, } = usePRStack(prStackCallbacksRef); const [reviewDestination, setReviewDestination] = useState<'agent' | 'platform'>(() => { @@ -2182,20 +2184,30 @@ const ReviewApp: React.FC = () => { {/* Partial PR diff notice — the platform withheld per-file content (PR too large). "Load full diff" re-requests the layer scope; the server recomputes the exact diff from the - local checkout (waiting out the warmup if needed). */} + local checkout (waiting out the warmup if needed). The + request is non-blocking on purpose: it can park for + minutes behind a cold clone, and the reviewer keeps + working with the partial diff meanwhile. */} {prPatchIncomplete && prDiffScope === 'layer' && !isSwitchingPRScope && (
Partial diff Partial - + {isLoadingFullDiff ? ( + + + Loading full diff… + + ) : ( + + )}
)} diff --git a/packages/review-editor/hooks/usePRStack.ts b/packages/review-editor/hooks/usePRStack.ts index dc418d747..15d883983 100644 --- a/packages/review-editor/hooks/usePRStack.ts +++ b/packages/review-editor/hooks/usePRStack.ts @@ -24,6 +24,7 @@ export interface PRStackCallbacks { export function usePRStack(callbacksRef: RefObject) { const [isSwitchingPRScope, setIsSwitchingPRScope] = useState(false); + const [isLoadingFullDiff, setIsLoadingFullDiff] = useState(false); const handleScopeSelect = useCallback(async (scope: PRDiffScope) => { const cb = callbacksRef.current; @@ -47,6 +48,32 @@ export function usePRStack(callbacksRef: RefObject) { } }, [callbacksRef]); + // Partial-diff upgrade: same layer re-POST as handleScopeSelect, but with + // its own loading flag so the full-screen PRSwitchOverlay does NOT render. + // The request can park for minutes behind the checkout warmup — the user + // keeps reviewing the partial diff while the notice shows progress. + const handleLoadFullDiff = useCallback(async () => { + const cb = callbacksRef.current; + if (!cb) return; + setIsLoadingFullDiff(true); + try { + const res = await fetch('/api/pr-diff-scope', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ scope: 'layer' }), + }); + const data = await res.json(); + if (!res.ok) { + throw new Error(data.error ?? 'Failed to load the full diff'); + } + cb.applyPRResponse(data); + } catch (err) { + cb.onError(err instanceof Error ? err.message : 'Failed to load the full diff'); + } finally { + setIsLoadingFullDiff(false); + } + }, [callbacksRef]); + const handlePRSwitch = useCallback(async (prUrl: string) => { const cb = callbacksRef.current; if (!cb) return; @@ -71,7 +98,9 @@ export function usePRStack(callbacksRef: RefObject) { return { isSwitchingPRScope, + isLoadingFullDiff, handleScopeSelect, + handleLoadFullDiff, handlePRSwitch, }; } diff --git a/packages/shared/pr-gitlab.test.ts b/packages/shared/pr-gitlab.test.ts index 9c5076e0d..68ad42e86 100644 --- a/packages/shared/pr-gitlab.test.ts +++ b/packages/shared/pr-gitlab.test.ts @@ -257,6 +257,33 @@ describe("fetchGlMR raw_diffs fallback", () => { } }); + test("null too_large/collapsed are inconclusive — the legacy heuristic still decides", async () => { + const entries = JSON.stringify([ + { + old_path: "src/big.ts", + new_path: "src/big.ts", + new_file: false, + deleted_file: false, + renamed_file: false, + too_large: null, + collapsed: null, + diff: "", // modified file, no content, flags unknown → withheld + }, + ]); + const { runtime } = gitlabRuntime({ + rawDiffs: { exitCode: 1, stderr: "404 Not Found" }, + diffs: { exitCode: 0, stdout: entries }, + }); + + const errSpy = spyOn(console, "error").mockImplementation(() => {}); + try { + const result = await fetchGlMR(runtime, REF); + expect(result.patchIncomplete).toBe(true); + } finally { + errSpy.mockRestore(); + } + }); + test("explicit too_large:false exonerates empty-diff entries (binary/empty files, modern GitLab)", async () => { const entries = JSON.stringify([ { diff --git a/packages/shared/pr-gitlab.ts b/packages/shared/pr-gitlab.ts index d318df188..29c0d500e 100644 --- a/packages/shared/pr-gitlab.ts +++ b/packages/shared/pr-gitlab.ts @@ -133,7 +133,10 @@ export async function getGlUser(runtime: PRRuntime, host: string): Promise { "diff", "--no-ext-diff", "--find-renames", - "-l0", + "-l100000", "--src-prefix=a/", "--dst-prefix=b/", "--end-of-options", diff --git a/packages/shared/pr-stack.ts b/packages/shared/pr-stack.ts index 042b6ab6f..17dca3719 100644 --- a/packages/shared/pr-stack.ts +++ b/packages/shared/pr-stack.ts @@ -176,11 +176,12 @@ export async function runPRLayerLocalDiff( const diffArgsFor = (range: string[]): string[] => [ "diff", "--no-ext-diff", - // -l0 lifts diff.renameLimit: on the multi-thousand-file PRs this path - // exists for, the default limit silently downgrades rename detection and - // renamed+edited files would render as delete+add pairs. + // Lift diff.renameLimit: on the multi-thousand-file PRs this path exists + // for, the default limit (~1000) silently downgrades rename detection and + // renamed+edited files would render as delete+add pairs. A large literal + // instead of -l0 because "0 = unlimited" only holds on git >= 2.29. "--find-renames", - "-l0", + "-l100000", "--src-prefix=a/", "--dst-prefix=b/", "--end-of-options", From e2b321f5fa37c079c4438cf1075929680b3b15a1 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 23:06:12 -0700 Subject: [PATCH 08/14] fix(review): stop scroll-driven sem stampede when semantic diff is failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The badge retry change (a2d19a4e) cleared the client-side sem cache on failure so transient errors could recover. But file-header badges mount and unmount on every scroll in the virtualized all-files view, and each mount re-requests /api/semantic-diff — and the server only cached SUCCESSFUL runs. With sem erroring, scrolling spawned a continuous stream of sem processes, pegging the CPU and making scrolling severely choppy. Bound retry rate by time, not by mount events: - client: keep the failed result memoized and expire it after a 60s cooldown instead of clearing immediately - server (Bun + Pi): memoize failed sem runs for 30s in SemanticDiffResponseCache — request rate can no longer drive execution rate --- apps/pi-extension/server/serverReview.ts | 4 +++ .../hooks/useFileSemanticChanges.ts | 14 ++++++++-- packages/server/review.ts | 4 +++ packages/shared/semantic-diff.test.ts | 27 +++++++++++++++++++ packages/shared/semantic-diff.ts | 24 ++++++++++++++++- 5 files changed, 70 insertions(+), 3 deletions(-) diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index 8901964dc..b51eaffdb 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -449,6 +449,10 @@ export async function startReviewServer(options: { ); if (result.status === "ok") { semanticDiffCache.set(cacheKey, currentPatch, result); + } else if (result.status === "error") { + // Cooldown-memoized: request rate (file badges remount on scroll) must + // not drive sem execution rate when it's failing. + semanticDiffCache.setFailure(cacheKey, currentPatch, result); } return result; } diff --git a/packages/review-editor/hooks/useFileSemanticChanges.ts b/packages/review-editor/hooks/useFileSemanticChanges.ts index 627e24098..23ecb7061 100644 --- a/packages/review-editor/hooks/useFileSemanticChanges.ts +++ b/packages/review-editor/hooks/useFileSemanticChanges.ts @@ -19,6 +19,12 @@ let cacheKey: string | null = null; let cachePromise: Promise | null = null; const RETRY_DELAYS_MS = [5_000, 15_000, 30_000]; +// After the in-flight retries are exhausted, the failure stays memoized for +// this long before a fresh attempt is allowed. Badges mount/unmount on every +// scroll in the virtualized all-files view — clearing the cache immediately +// on failure turned scrolling into a refetch (and server-side sem re-run) +// stampede whenever sem was erroring. +const FAILURE_RETRY_COOLDOWN_MS = 60_000; async function fetchSemanticDiff(): Promise { const res = await fetch('/api/semantic-diff'); @@ -56,8 +62,12 @@ function loadSemanticDiff(rawPatch: string): Promise { 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; + setTimeout(() => { + if (cacheKey === rawPatch && cachePromise === promise) { + cacheKey = null; + cachePromise = null; + } + }, FAILURE_RETRY_COOLDOWN_MS); } return data; }); diff --git a/packages/server/review.ts b/packages/server/review.ts index 46c865823..8a9527a96 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -412,6 +412,10 @@ export async function startReviewServer( ); if (result.status === "ok") { semanticDiffCache.set(cacheKey, currentPatch, result); + } else if (result.status === "error") { + // Cooldown-memoized: request rate (file badges remount on scroll) must + // not drive sem execution rate when it's failing. + semanticDiffCache.setFailure(cacheKey, currentPatch, result); } return result; }; diff --git a/packages/shared/semantic-diff.test.ts b/packages/shared/semantic-diff.test.ts index b178a66b9..bb390f653 100644 --- a/packages/shared/semantic-diff.test.ts +++ b/packages/shared/semantic-diff.test.ts @@ -319,4 +319,31 @@ describe("semantic diff runner", () => { expect(cache.get("b", "patch-a")).toBe(second); expect(cache.get("b", "patch-b")).toBeUndefined(); }); + + test("failures are memoized within their TTL and retryable after it", () => { + const cache = new SemanticDiffResponseCache(); + const failure: SemanticDiffResponse = { + status: "error", + reason: "sem-exit", + message: "failed", + }; + + // Within TTL: served from memo — repeated requests must not re-run sem. + cache.setFailure("k", "patch-a", failure, 60_000); + expect(cache.get("k", "patch-a")).toBe(failure); + + // Expired TTL: gone — the next request may retry. + cache.setFailure("k2", "patch-a", failure, -1); + expect(cache.get("k2", "patch-a")).toBeUndefined(); + + // A success overwrites and outlives the failure memo. + const ok = { status: "ok", changes: [], binaryChanges: [] } as unknown as SemanticDiffResponse; + cache.setFailure("k3", "patch-a", failure, 60_000); + cache.set("k3", "patch-a", ok); + expect(cache.get("k3", "patch-a")).toBe(ok); + + // Patch change clears failure memos too. + cache.setFailure("k4", "patch-a", failure, 60_000); + expect(cache.get("k4", "patch-b")).toBeUndefined(); + }); }); diff --git a/packages/shared/semantic-diff.ts b/packages/shared/semantic-diff.ts index 80ece8b95..85e674b12 100644 --- a/packages/shared/semantic-diff.ts +++ b/packages/shared/semantic-diff.ts @@ -417,13 +417,21 @@ export function semanticDiffCacheKey(input: { export class SemanticDiffResponseCache { private readonly cache = new Map(); + private readonly failures = new Map(); private rawPatch: string | null = null; constructor(private readonly maxEntries = 8) {} get(cacheKey: string, rawPatch: string): SemanticDiffResponse | undefined { this.syncPatch(rawPatch); - return this.cache.get(cacheKey); + const ok = this.cache.get(cacheKey); + if (ok) return ok; + const failed = this.failures.get(cacheKey); + if (failed) { + if (failed.expiresAt > Date.now()) return failed.response; + this.failures.delete(cacheKey); + } + return undefined; } set(cacheKey: string, rawPatch: string, response: SemanticDiffResponse): void { @@ -437,11 +445,25 @@ export class SemanticDiffResponseCache { } this.cache.set(cacheKey, response); + this.failures.delete(cacheKey); + } + + /** + * Memoize a FAILED run for a short window. Without this, every request for + * a failing (patch, cwd) re-executes sem — and the review UI's file badges + * re-request on every scroll-driven mount, so an erroring sem turns + * scrolling into a process stampede. The TTL keeps failures retryable + * without letting request rate drive execution rate. + */ + setFailure(cacheKey: string, rawPatch: string, response: SemanticDiffResponse, ttlMs = 30_000): void { + this.syncPatch(rawPatch); + this.failures.set(cacheKey, { response, expiresAt: Date.now() + ttlMs }); } private syncPatch(rawPatch: string): void { if (this.rawPatch === rawPatch) return; this.cache.clear(); + this.failures.clear(); this.rawPatch = rawPatch; } } From 711f58aeb67d5b3c289c836bfb4543cbadd43bb8 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 11 Jun 2026 23:33:47 -0700 Subject: [PATCH 09/14] fix(review): eliminate all-files scroll jank (pre-existing on main, from #885) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CodeView migration introduced severe scroll chop; scrolling UP could freeze the viewport entirely ("scrolling but nothing changes"). Three compounding causes, diagnosed against Pierre 1.2.8 source: 1. Lazy full-content augmentation landed updateItem() mid-scroll-gesture: the full-content parse counts collapsed-context regions the raw-patch parse doesn't, so the item GROWS — re-render + re-tokenize hitches both directions, and when the grown item sat above CodeView's scroll anchor, its corrective scrollTo() killed wheel momentum (the up-scroll freeze). Fetches still start as items enter the window; the item mutation now waits for 150ms of scroll quiet (staleness re-checked at apply time). 2. reportVisibleFile read container.scrollTop/clientHeight/scrollHeight on EVERY scroll event — a forced synchronous layout right after each frame's DOM writes. Replaced with CodeView's cached accessors and coalesced the handler to once per animation frame. 3. Missing containment CSS: Pierre's own production wrapper uses contain:strict + will-change:scroll-position so forced layouts stay scoped to the scroller instead of the whole document. Adopted. Also: __devOnlyValidateItemHeights now requires explicit opt-in (VITE_PIERRE_VALIDATE_HEIGHTS=1) — it runs getBoundingClientRect() per rendered item per frame and made dev-server scrolling choppy by itself. --- .../components/AllFilesCodeView.tsx | 144 +++++++++++++----- 1 file changed, 108 insertions(+), 36 deletions(-) diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index 7cf38796f..1eb65cd81 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -365,6 +365,11 @@ const PANEL_HEADER_HEIGHT = 33; // --panel-header-h // OUR unsafeCSS rule rather than silently tracking a library default. const HUNK_SEPARATOR_HEIGHT = 32; +// How long the scroller must be quiet before queued augmentation applies +// (item growth + re-render) are allowed to land. Slightly above Pierre's own +// post-interaction restore delay (120ms). +const AUGMENT_APPLY_IDLE_MS = 150; + export const AllFilesCodeView: React.FC = ({ files, diffStyle, @@ -807,6 +812,45 @@ export const AllFilesCodeView: React.FC = ({ const fileSetKeyRef = useRef(fileSetKey); fileSetKeyRef.current = fileSetKey; + // Augmentation APPLIES are deferred to scroll-idle. updateItem() mutates + // item layout — the full-content parse counts collapsed-context regions the + // raw-patch parse doesn't, so the item GROWS — and forces a re-render + + // re-tokenize. Landing that mid-gesture causes visible chop; worse, when + // the grown item sits ABOVE CodeView's scroll anchor, its corrective + // scrollTo() kills wheel momentum and pins the viewport ("scrolling but + // nothing changes"). Fetches still start as items enter the window — only + // the item mutation waits for the scroll to settle. Staleness is re-checked + // at apply time; the per-item map keeps only the newest apply per item. + const pendingAugmentAppliesRef = useRef(new Map void>()); + const augmentFlushTimerRef = useRef | null>(null); + const lastScrollTsRef = useRef(0); + + const flushAugmentApplies = useCallback(() => { + augmentFlushTimerRef.current = null; + const idleFor = Date.now() - lastScrollTsRef.current; + if (idleFor < AUGMENT_APPLY_IDLE_MS) { + augmentFlushTimerRef.current = setTimeout( + flushAugmentApplies, + AUGMENT_APPLY_IDLE_MS - idleFor + 10, + ); + return; + } + const applies = [...pendingAugmentAppliesRef.current.values()]; + pendingAugmentAppliesRef.current.clear(); + for (const apply of applies) apply(); + }, []); + + const queueAugmentApply = useCallback((itemId: string, apply: () => void) => { + pendingAugmentAppliesRef.current.set(itemId, apply); + if (augmentFlushTimerRef.current == null) { + augmentFlushTimerRef.current = setTimeout(flushAugmentApplies, AUGMENT_APPLY_IDLE_MS); + } + }, [flushAugmentApplies]); + + useEffect(() => () => { + if (augmentFlushTimerRef.current != null) clearTimeout(augmentFlushTimerRef.current); + }, []); + // Fetch full file contents for one item, reparse with processFile, and swap // the item's fileDiff in place so hunk expansion (expand-unchanged gutter // controls) works against the COMPLETE file. Mirrors LazyFileDiff's per-mount @@ -907,31 +951,38 @@ export const AllFilesCodeView: React.FC = ({ return; } - // Re-check right before the write: processFile above is synchronous but - // an abort / diff switch could have landed since the first check. if (isStale()) return; - const liveHandle = viewerRef.current; - const item = liveHandle?.getItem(itemId); - // The item may have been torn down between fetch start and resolution; - // belt-and-suspenders on top of the staleness check above. - if (liveHandle == null || item == null || item.type !== 'diff') { - augmentState.set(itemId, { status: 'done', controller, generation }); - return; - } - // cacheKey MUST change when fileDiff contents change (types.ts warning): - // otherwise the worker / highlight caches would serve the stale partial - // AST. Derive a fresh key from the augmented (now full-content) diff, - // scoped by generation so the same item id across diff switches never - // collides in a (future) cross-mount worker cache. - augmented.cacheKey = `${generation}::${itemId}#full`; - item.fileDiff = augmented; - item.version = (item.version ?? 0) + 1; - // updateItem re-measures the (now taller) item and resolves the captured - // scroll anchor, so the viewport stays put whether this item is above or - // below the fold — no manual scroll correction needed. - liveHandle.updateItem(item); - augmentState.set(itemId, { status: 'done', controller, generation }); + // Defer the item mutation to scroll-idle (see queueAugmentApply) — + // landing it mid-gesture chops scrolling and can kill momentum via + // CodeView's anchor-correcting scrollTo. All staleness checks re-run + // at apply time: the queue can hold entries across aborts and diff + // switches. + queueAugmentApply(itemId, () => { + if (isStale()) return; + const liveHandle = viewerRef.current; + const item = liveHandle?.getItem(itemId); + // The item may have been torn down between fetch start and apply; + // belt-and-suspenders on top of the staleness check above. + if (liveHandle == null || item == null || item.type !== 'diff') { + augmentState.set(itemId, { status: 'done', controller, generation }); + return; + } + + // cacheKey MUST change when fileDiff contents change (types.ts warning): + // otherwise the worker / highlight caches would serve the stale partial + // AST. Derive a fresh key from the augmented (now full-content) diff, + // scoped by generation so the same item id across diff switches never + // collides in a (future) cross-mount worker cache. + augmented.cacheKey = `${generation}::${itemId}#full`; + item.fileDiff = augmented; + item.version = (item.version ?? 0) + 1; + // updateItem re-measures the (now taller) item and resolves the captured + // scroll anchor, so the viewport stays put whether this item is above or + // below the fold — no manual scroll correction needed. + liveHandle.updateItem(item); + augmentState.set(itemId, { status: 'done', controller, generation }); + }); }) .catch((err) => { if (isStale()) { @@ -945,7 +996,7 @@ export const AllFilesCodeView: React.FC = ({ augmentState.set(itemId, { status: 'error', controller, generation }); void err; }); - }, []); + }, [queueAugmentApply]); // (Re)apply search marks for ONE item's node. Called on every render of that // item (onPostRender mount/update) so marks survive CodeView's element @@ -1399,12 +1450,11 @@ export const AllFilesCodeView: React.FC = ({ // At-bottom override (legacy parity): a short final file pinned at the // container bottom never gets its top above scrollTop+threshold, so the // loop would leave an earlier file active while the user reads the last - // one. DOM scrollTop/scrollHeight are consistent with each other even - // under CodeView's paged scroll rebasing, so this check is safe. - const container = scrollRef.current; + // one. Uses CodeView's cached accessors — raw container.scrollHeight / + // clientHeight reads here forced a synchronous layout on EVERY scroll + // event, right after the frame's DOM writes (measurable jank). if ( - container != null && - container.scrollTop + container.clientHeight >= container.scrollHeight - 2 + viewer.getScrollTop() + viewer.getHeight() >= viewer.getScrollHeight() - 2 ) { bestId = rendered[rendered.length - 1].id; } @@ -1415,10 +1465,23 @@ export const AllFilesCodeView: React.FC = ({ } }); + // Coalesced to one run per animation frame — CodeView fires onScroll per + // scroll EVENT, which can outpace frames during momentum scrolling. Also + // stamps scroll activity for the augmentation idle-flush. + const scrollReportRafRef = useRef(null); const handleScroll = useStableCallback(() => { - reportVisibleFile(); + lastScrollTsRef.current = Date.now(); + if (scrollReportRafRef.current != null) return; + scrollReportRafRef.current = requestAnimationFrame(() => { + scrollReportRafRef.current = null; + reportVisibleFile(); + }); }); + useEffect(() => () => { + if (scrollReportRafRef.current != null) cancelAnimationFrame(scrollReportRafRef.current); + }, []); + // CodeView's onScroll only fires on actual scroll, so seed the initial // active-file highlight once the viewer has rendered its first window. rAF // gives CodeView a frame to mount + measure before we read rendered items. @@ -1681,12 +1744,18 @@ export const AllFilesCodeView: React.FC = ({ hunkSeparatorHeight: HUNK_SEPARATOR_HEIGHT, ...(customLineHeight != null && { lineHeight: customLineHeight }), }, - // Dev-only safety net for the hand-maintained itemMetrics above: Pierre + // Opt-in safety net for the hand-maintained itemMetrics above: Pierre // compares its virtualization estimates against measured DOM heights and - // warns on drift. Doubly gated — the option only takes effect when the - // library itself runs a development build (NODE_ENV), so it is inert in - // production even if the flag leaks through. - ...(import.meta.env.DEV && { __devOnlyValidateItemHeights: true }), + // warns on drift. Explicit env opt-in (VITE_PIERRE_VALIDATE_HEIGHTS=1) + // rather than blanket DEV: validation runs getBoundingClientRect() per + // rendered item per frame inside the scroll loop — it made the dev + // server's scrolling visibly choppy on its own. Still doubly gated: the + // option only takes effect when the library itself runs a development + // build (NODE_ENV), so it is inert in production even if it leaks. + ...(import.meta.env.DEV && + import.meta.env.VITE_PIERRE_VALIDATE_HEIGHTS === '1' && { + __devOnlyValidateItemHeights: true, + }), onLineSelectionEnd(range, context) { handleLineSelectionEnd(range, context.item); }, @@ -1747,7 +1816,10 @@ export const AllFilesCodeView: React.FC = ({ key={fileSetKey} ref={viewerRef} containerRef={scrollRef} - className="h-full overflow-auto" + // Containment mirrors Pierre's own production wrapper (diffshub + // CodeViewWrapper): without it, every forced layout during scrolling + // recomputes the whole document instead of the clipped subtree. + className="h-full overflow-auto overscroll-contain [contain:strict] [will-change:scroll-position] [&_diffs-container]:[contain:layout_paint_style]" initialItems={identity.items} options={options} selectedLines={selectedLines} From 9e959fe55d7d0f479140139c20987c6f3f0f57c3 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Fri, 12 Jun 2026 08:08:49 -0700 Subject: [PATCH 10/14] feat(review): change-type status in headers + tree, diffshub CSS parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adopts two diffshub practices identified in the architecture comparison: - DiffFile now carries a derived status (added/deleted/renamed/modified) from the chunk's git metadata lines. FileHeader shows a status icon and renders renames as "old/path → new/path" (dimmed old, arrow — diffshub's treatment, including its rename blue); the file tree shows A/D/R markers. 'modified' is deliberately undecorated so the others pop. Works in both the all-files surface and the single-file panel, including header-only pure renames from the large-PR reconstruction. - CodeView container gains diffshub's remaining perf CSS: overflow-anchor: none (native scroll anchoring fights CodeView's own anchor resolution whenever item heights change — exactly our augmentation applies), overflow-x-clip, and overflow-clip containment on item elements. --- .../components/AllFilesCodeView.tsx | 7 +- .../review-editor/components/DiffViewer.tsx | 5 + .../review-editor/components/FileHeader.tsx | 67 ++++++++++++- .../review-editor/components/FileTreeNode.tsx | 17 ++++ .../dock/panels/ReviewDiffPanel.tsx | 1 + packages/review-editor/types.ts | 8 ++ .../utils/buildFileTree.workspace.test.ts | 1 + .../review-editor/utils/diffParser.test.ts | 98 +++++++++++++++++++ packages/review-editor/utils/diffParser.ts | 19 +++- 9 files changed, 220 insertions(+), 3 deletions(-) diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index 1eb65cd81..c0c8450ea 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -1661,6 +1661,8 @@ export const AllFilesCodeView: React.FC = ({ handleToggleViewedAndCollapse(filePath, item.id) : undefined} isStaged={stagedFiles?.has(filePath)} @@ -1819,7 +1821,10 @@ export const AllFilesCodeView: React.FC = ({ // Containment mirrors Pierre's own production wrapper (diffshub // CodeViewWrapper): without it, every forced layout during scrolling // recomputes the whole document instead of the clipped subtree. - className="h-full overflow-auto overscroll-contain [contain:strict] [will-change:scroll-position] [&_diffs-container]:[contain:layout_paint_style]" + // overflow-anchor:none disables the BROWSER's scroll anchoring, which + // otherwise fights CodeView's own anchor resolution whenever item + // heights change (our augmentation applies). + className="h-full overflow-y-auto overflow-x-clip overscroll-contain [contain:strict] [overflow-anchor:none] [will-change:scroll-position] [&_diffs-container]:overflow-clip [&_diffs-container]:[contain:layout_paint_style]" initialItems={identity.items} options={options} selectedLines={selectedLines} diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index 2c787dc28..e24ce267e 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -124,6 +124,8 @@ interface DiffViewerProps { patch: string; filePath: string; oldPath?: string; + /** Change type for the header icon + rename display. */ + status?: import('../types').DiffFileStatus; /** Base branch override used for file-content lookups (branch / merge-base modes only). */ reviewBase?: string; /** Current PR url + diff scope — used to namespace file-comment drafts so they don't leak across in-place PR switches. */ @@ -176,6 +178,7 @@ export const DiffViewer: React.FC = ({ patch, filePath, oldPath, + status, reviewBase, prUrl, prDiffScope, @@ -627,6 +630,8 @@ export const DiffViewer: React.FC = ({ void; isStaged?: boolean; @@ -40,10 +45,48 @@ function frontEllipsize(text: string, visibleChars: number): string { return `...${text.slice(-visibleChars)}`; } +/** + * Change-type icon (added/deleted/renamed). 'modified' deliberately renders + * nothing — most files are modifications, so only the others stand out + * (mirrors Pierre's diffshub). Renamed uses diffshub's blue (#007aff). + */ +const FileStatusIcon: React.FC<{ status: DiffFileStatus; oldPath?: string }> = ({ status, oldPath }) => { + if (status === 'modified') return null; + const meta = + status === 'added' + ? { className: 'text-success', title: 'Added file' } + : status === 'deleted' + ? { className: 'text-destructive', title: 'Deleted file' } + : { className: 'text-[#007aff]', title: oldPath ? `Renamed from ${oldPath}` : 'Renamed file' }; + return ( + + {status === 'added' && ( + + + + + )} + {status === 'deleted' && ( + + + + + )} + {status === 'renamed' && ( + + + + )} + + ); +}; + /** Sticky file header with file path, Viewed toggle, Git Add, and Copy Diff button */ export const FileHeader: React.FC = ({ filePath, patch, + status, + oldPath, isViewed = false, onToggleViewed, isStaged = false, @@ -97,7 +140,29 @@ export const FileHeader: React.FC = ({ >
{collapseToggle} - + {status && } + + {/* Rename: dimmed old path → new path (diffshub treatment). Dropped + under tight widths — the icon + tooltip still carry it. */} + {status === 'renamed' && oldPath && !showFilenameOnly && ( + <> + + {oldPath} + + + + + + )} {!showFilenameOnly && directory && ( {directory} diff --git a/packages/review-editor/components/FileTreeNode.tsx b/packages/review-editor/components/FileTreeNode.tsx index c6c12d040..8aba2d998 100644 --- a/packages/review-editor/components/FileTreeNode.tsx +++ b/packages/review-editor/components/FileTreeNode.tsx @@ -165,6 +165,23 @@ export const FileTreeNodeItem: React.FC = ({ {node.file!.deletions > 0 && ( -{node.file!.deletions} )} + {/* Change-type marker — modified is deliberately undecorated so + added/deleted/renamed pop (diffshub treatment; renamed uses + its blue). */} + {node.file!.status === 'added' && ( + A + )} + {node.file!.status === 'deleted' && ( + D + )} + {node.file!.status === 'renamed' && ( + + R + + )}
diff --git a/packages/review-editor/dock/panels/ReviewDiffPanel.tsx b/packages/review-editor/dock/panels/ReviewDiffPanel.tsx index 60787ce57..25c3d5211 100644 --- a/packages/review-editor/dock/panels/ReviewDiffPanel.tsx +++ b/packages/review-editor/dock/panels/ReviewDiffPanel.tsx @@ -70,6 +70,7 @@ export const ReviewDiffPanel: React.FC = (props) => { patch={file.patch} filePath={file.path} oldPath={file.oldPath} + status={file.status} reviewBase={state.reviewBase} prUrl={state.prMetadata?.url} prDiffScope={state.prDiffScope} diff --git a/packages/review-editor/types.ts b/packages/review-editor/types.ts index 5e44f14f3..5a19d5580 100644 --- a/packages/review-editor/types.ts +++ b/packages/review-editor/types.ts @@ -1,7 +1,15 @@ +/** + * Change type derived from the diff's metadata lines. 'modified' is the + * default and is deliberately NOT decorated in the UI (mirroring Pierre's + * diffshub: most files are modifications, so only A/D/R stand out). + */ +export type DiffFileStatus = 'added' | 'deleted' | 'renamed' | 'modified'; + export interface DiffFile { path: string; oldPath?: string; patch: string; additions: number; deletions: number; + status: DiffFileStatus; } diff --git a/packages/review-editor/utils/buildFileTree.workspace.test.ts b/packages/review-editor/utils/buildFileTree.workspace.test.ts index c3d22c3e1..980f32a32 100644 --- a/packages/review-editor/utils/buildFileTree.workspace.test.ts +++ b/packages/review-editor/utils/buildFileTree.workspace.test.ts @@ -7,6 +7,7 @@ const diffFile = (path: string, overrides: Partial = {}): DiffFile => patch: "", additions: 0, deletions: 0, + status: "modified", ...overrides, }); diff --git a/packages/review-editor/utils/diffParser.test.ts b/packages/review-editor/utils/diffParser.test.ts index 28e9c1664..307c99e86 100644 --- a/packages/review-editor/utils/diffParser.test.ts +++ b/packages/review-editor/utils/diffParser.test.ts @@ -63,3 +63,101 @@ describe("parseDiffToFiles", () => { expect(files[0].oldPath).toBeUndefined(); }); }); + +describe("parseDiffToFiles — change-type status", () => { + const parse = (...lines: string[]) => parseDiffToFiles(lines.join("\n")); + + it("classifies modified files", () => { + const [file] = parse( + "diff --git a/src/app.ts b/src/app.ts", + "index 1111111..2222222 100644", + "--- a/src/app.ts", + "+++ b/src/app.ts", + "@@ -1 +1 @@", + "-old", + "+new", + ); + expect(file.status).toBe("modified"); + }); + + it("classifies added files", () => { + const [file] = parse( + "diff --git a/src/new.ts b/src/new.ts", + "new file mode 100644", + "index 0000000..2222222", + "--- /dev/null", + "+++ b/src/new.ts", + "@@ -0,0 +1 @@", + "+hello", + ); + expect(file.status).toBe("added"); + }); + + it("classifies deleted files", () => { + const [file] = parse( + "diff --git a/src/gone.ts b/src/gone.ts", + "deleted file mode 100644", + "index 1111111..0000000", + "--- a/src/gone.ts", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-bye", + ); + expect(file.status).toBe("deleted"); + }); + + it("classifies renames with edits and keeps the old path", () => { + const [file] = parse( + "diff --git a/src/before.ts b/src/after.ts", + "similarity index 90%", + "rename from src/before.ts", + "rename to src/after.ts", + "--- a/src/before.ts", + "+++ b/src/after.ts", + "@@ -1 +1 @@", + "-a", + "+b", + ); + expect(file.status).toBe("renamed"); + expect(file.path).toBe("src/after.ts"); + expect(file.oldPath).toBe("src/before.ts"); + }); + + it("classifies header-only pure renames (no hunks at all)", () => { + const [file] = parse( + "diff --git a/src/old.ts b/src/new.ts", + "similarity index 100%", + "rename from src/old.ts", + "rename to src/new.ts", + "", + ); + expect(file.status).toBe("renamed"); + expect(file.oldPath).toBe("src/old.ts"); + }); + + it("falls back to renamed when paths differ without rename metadata", () => { + const [file] = parse( + "diff --git a/src/old.ts b/src/new.ts", + "--- a/src/old.ts", + "+++ b/src/new.ts", + "@@ -1 +1 @@", + "-a", + "+b", + ); + expect(file.status).toBe("renamed"); + }); + + it("is not fooled by diff content containing metadata-looking lines", () => { + const [file] = parse( + "diff --git a/docs/git.md b/docs/git.md", + "index 1111111..2222222 100644", + "--- a/docs/git.md", + "+++ b/docs/git.md", + "@@ -1,2 +1,3 @@", + " how git renames work:", + "+rename from and rename to lines appear in headers", + " new file mode is another header", + ); + expect(file.status).toBe("modified"); + }); +}); diff --git a/packages/review-editor/utils/diffParser.ts b/packages/review-editor/utils/diffParser.ts index 9ceba9138..04f6ef12b 100644 --- a/packages/review-editor/utils/diffParser.ts +++ b/packages/review-editor/utils/diffParser.ts @@ -1,5 +1,5 @@ import { parseDiffFilePathLines, parseDiffGitHeader } from '@plannotator/shared/diff-paths'; -import type { DiffFile } from '../types'; +import type { DiffFile, DiffFileStatus } from '../types'; function splitDiffChunks(rawPatch: string): string[] { const matches = [...rawPatch.matchAll(/^diff --git /gm)]; @@ -12,6 +12,22 @@ function splitDiffChunks(rawPatch: string): string[] { }); } +/** + * Change type from the chunk's git metadata lines. Scans only the extended + * header (everything before the first hunk/--- line) so file content that + * happens to contain "rename from" etc. can't misclassify. + */ +function deriveStatus(lines: string[], oldPath: string, newPath: string): DiffFileStatus { + for (const line of lines) { + if (line.startsWith('@@') || line.startsWith('--- ') || line.startsWith('+++ ')) break; + if (line.startsWith('new file mode')) return 'added'; + if (line.startsWith('deleted file mode')) return 'deleted'; + if (line.startsWith('rename from ') || line.startsWith('copy from ')) return 'renamed'; + } + // Reconstructed/odd patches may carry distinct paths without rename lines. + return oldPath !== newPath ? 'renamed' : 'modified'; +} + export function parseDiffToFiles(rawPatch: string): DiffFile[] { const files: DiffFile[] = []; @@ -37,6 +53,7 @@ export function parseDiffToFiles(rawPatch: string): DiffFile[] { patch: chunk, additions, deletions, + status: deriveStatus(lines, oldPath, newPath), }); } From 180042b3dbb109344787eca6ad5ced5964bce882 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Fri, 12 Jun 2026 08:54:13 -0700 Subject: [PATCH 11/14] feat(review): worker-pool syntax highlighting (diffshub parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A performance trace of scrolling a small local diff attributed 2.2s of 2.6s main-thread CPU to findNextMatchSync — shiki's TextMate regex scanner tokenizing on the main thread. diffshub avoids this entirely by running tokenization in Pierre's worker pool; we never opted in. Wires WorkerPoolContextProvider around the review app (pool size min(cores-1, 3), 100-entry AST LRU, common languages preloaded), gates the all-files surface on pool readiness with a 5s escape hatch (a dead pool degrades to plaintext-then-highlight, never a blank view), and syncs the UI theme pair into the long-lived pool. Single-file build constraint solved with Vite's ?worker&inline (base64 blob worker) + worker.format 'es' with inlineDynamicImports — the worker's lazy import("shiki/wasm") branch collapses into the bundle and is never taken (shiki-js engine: the win is moving work off the main thread, with no .wasm asset to smuggle into one HTML file). Bundle +850KB. --- apps/review/index.tsx | 8 +- apps/review/vite.config.ts | 13 +++ .../components/AllFilesCodeView.tsx | 12 +++ .../review-editor/components/DiffViewer.tsx | 5 + packages/review-editor/package.json | 3 +- packages/review-editor/workerPool.tsx | 98 +++++++++++++++++++ 6 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 packages/review-editor/workerPool.tsx diff --git a/apps/review/index.tsx b/apps/review/index.tsx index ccd0f6d19..d1e00590d 100644 --- a/apps/review/index.tsx +++ b/apps/review/index.tsx @@ -1,6 +1,7 @@ import React from 'react'; import ReactDOM from 'react-dom/client'; import App from '@plannotator/review-editor'; +import { ReviewWorkerPoolProvider } from '@plannotator/review-editor/worker-pool'; import '@plannotator/review-editor/styles'; const rootElement = document.getElementById('root'); @@ -11,6 +12,11 @@ if (!rootElement) { const root = ReactDOM.createRoot(rootElement); root.render( - + {/* Worker-pool syntax highlighting — tokenization off the main thread + (diffshub parity). Pierre's CodeView/FileDiff pick the pool up from + context automatically. */} + + + ); diff --git a/apps/review/vite.config.ts b/apps/review/vite.config.ts index e0b185575..440f44eba 100644 --- a/apps/review/vite.config.ts +++ b/apps/review/vite.config.ts @@ -37,9 +37,22 @@ export default defineConfig({ '@plannotator/shared': path.resolve(__dirname, '../../packages/shared'), '@plannotator/ui': path.resolve(__dirname, '../../packages/ui'), '@plannotator/review-editor/styles': path.resolve(__dirname, '../../packages/review-editor/index.css'), + '@plannotator/review-editor/worker-pool': path.resolve(__dirname, '../../packages/review-editor/workerPool.tsx'), '@plannotator/review-editor': path.resolve(__dirname, '../../packages/review-editor/App.tsx'), } }, + // The Pierre highlight worker (?worker&inline) contains a dynamic + // import("shiki/wasm") branch; iife (Vite's default worker format) can't + // code-split, so emit the worker as ES with dynamic imports collapsed into + // the single inlined bundle. + worker: { + format: 'es', + rollupOptions: { + output: { + inlineDynamicImports: true, + }, + }, + }, build: { target: 'esnext', assetsInlineLimit: 100000000, diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index c0c8450ea..1b421eebf 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -22,6 +22,7 @@ import type { } from '@plannotator/ui/types'; import { CommentPopover } from '@plannotator/ui/components/CommentPopover'; import { usePierreTheme } from '../hooks/usePierreTheme'; +import { useIsWorkerPoolReadyOrDisabled, useWorkerPoolThemeSync } from '../workerPool'; import type { DiffFile } from '../types'; import { buildFileTree, getVisualFileOrder } from '../utils/buildFileTree'; import { buildCodeNavRequest } from '../utils/buildCodeNavRequest'; @@ -419,6 +420,11 @@ export const AllFilesCodeView: React.FC = ({ // way — we keep `true` to be explicit that the built-in title is irrelevant // here (our FileHeader owns all header chrome). const pierreTheme = usePierreTheme({ fontFamily, fontSize, showFileHeader: true }); + // Worker-pool highlighting: wait for the pool so the first tokenization + // wave runs in workers (not a main-thread fallback), and keep the pool's + // theme pair in step with the UI theme. + const workerPoolReady = useIsWorkerPoolReadyOrDisabled(); + useWorkerPoolThemeSync(pierreTheme.syntaxTheme); const viewerRef = useRef | null>(null); const scrollRef = useRef(null); const toolbarHostRef = useRef(null); @@ -1808,6 +1814,12 @@ export const AllFilesCodeView: React.FC = ({ ], ); + // After all hooks: hold the surface until the worker pool can take the + // first tokenization wave (≈100-300ms once per session; instant after). + if (!workerPoolReady) { + return
; + } + return (
diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index e24ce267e..3696f73ab 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -4,6 +4,7 @@ import { getSingularPatch, processFile } from '@pierre/diffs'; import { CodeAnnotation, CodeAnnotationType, SelectedLineRange, DiffAnnotationMetadata, TokenAnnotationMeta, ConventionalLabel, ConventionalDecoration } from '@plannotator/ui/types'; import type { DiffTokenEventBaseProps } from '@pierre/diffs'; import { usePierreTheme } from '../hooks/usePierreTheme'; +import { useWorkerPoolThemeSync } from '../workerPool'; import { CommentPopover } from '@plannotator/ui/components/CommentPopover'; import { storage } from '@plannotator/ui/utils/storage'; import { detectLanguage } from '../utils/detectLanguage'; @@ -222,6 +223,10 @@ export const DiffViewer: React.FC = ({ onCodeNavRequest, }) => { const pierreTheme = usePierreTheme({ fontFamily, fontSize }); + // Worker-pool highlighting: keep the pool's theme pair in step with the UI + // theme. (No mount gating here — the single-file panel renders one diff; + // a main-thread fallback frame at startup is invisible.) + useWorkerPoolThemeSync(pierreTheme.syntaxTheme); // containerRef must point at the actual scrolling element (the // OverlayScrollbars viewport), not the OverlayScrollArea host. `viewport` // is state so effects re-run once the library has mounted the viewport. diff --git a/packages/review-editor/package.json b/packages/review-editor/package.json index dab1cfeb1..a1685d72d 100644 --- a/packages/review-editor/package.json +++ b/packages/review-editor/package.json @@ -5,7 +5,8 @@ "exports": { ".": "./App.tsx", "./styles": "./index.css", - "./shortcuts": "./shortcuts.ts" + "./shortcuts": "./shortcuts.ts", + "./worker-pool": "./workerPool.tsx" }, "dependencies": { "@fontsource-variable/geist-mono": "5.2.7", diff --git a/packages/review-editor/workerPool.tsx b/packages/review-editor/workerPool.tsx new file mode 100644 index 000000000..7632ad6e5 --- /dev/null +++ b/packages/review-editor/workerPool.tsx @@ -0,0 +1,98 @@ +import React, { useEffect, useRef, useState, type ReactNode } from 'react'; +import { WorkerPoolContextProvider, useWorkerPool } from '@pierre/diffs/react'; +import type { WorkerInitializationRenderOptions, WorkerPoolOptions } from '@pierre/diffs/react'; +// Vite-inlined worker (base64 blob) — required by the single-file HTML build: +// the review UI ships as one self-contained file, so there is no separate +// asset URL to load a worker script from. +// @ts-expect-error vite ?worker&inline virtual module (no ambient types here) +import DiffsWorker from '@pierre/diffs/worker/worker.js?worker&inline'; + +/** + * Worker-pool syntax highlighting (diffshub parity). Without a pool, Pierre + * tokenizes on the main thread — profiled at >2s of `findNextMatchSync` + * during a few seconds of scrolling, the dominant cause of scroll chop. + * + * Pool sizing mirrors diffshub: min(cores - 1, 3), never 0. + * `shiki-js` (pure JS regex engine) instead of `shiki-wasm`: the win is + * moving tokenization OFF the main thread, and the JS engine avoids having + * to smuggle a .wasm asset into the single-file bundle. + */ +const poolOptions: WorkerPoolOptions = { + poolSize: Math.min(Math.max(1, (globalThis.navigator?.hardwareConcurrency ?? 2) - 1), 3), + totalASTLRUCacheSize: 100, + workerFactory: () => new DiffsWorker() as Worker, +}; + +const highlighterOptions: WorkerInitializationRenderOptions = { + preferredHighlighter: 'shiki-js', + // Preload the common languages; anything else resolves on demand. + langs: ['typescript', 'tsx', 'javascript', 'json', 'css', 'html', 'python', 'go', 'rust', 'sh', 'yaml', 'markdown'], +}; + +export function ReviewWorkerPoolProvider({ children }: { children: ReactNode }) { + return ( + + {children} + + ); +} + +// If the pool never reaches 'initialized' (e.g. worker spawn blocked), stop +// waiting and render anyway: Pierre paints code as plaintext immediately and +// applies highlights asynchronously, so a dead pool degrades to unhighlighted +// content — never a blank view. +const POOL_READY_TIMEOUT_MS = 5_000; + +/** + * True once the pool finished initializing (or when no pool exists — callers + * fall back to main-thread rendering). diffshub gates its viewer mount on + * this so the first tokenization wave never races pool startup. + */ +export function useIsWorkerPoolReadyOrDisabled(): boolean { + const workerPool = useWorkerPool(); + const [isReady, setIsReady] = useState(() => workerPool?.isInitialized() ?? true); + const isReadyRef = useRef(isReady); + useEffect(() => { + if (workerPool == null) return; + const timeout = setTimeout(() => { + if (!isReadyRef.current) { + console.warn('Plannotator: highlight worker pool not ready after 5s — rendering without waiting.'); + isReadyRef.current = true; + setIsReady(true); + } + }, POOL_READY_TIMEOUT_MS); + // The callback fires immediately with the current state. + const unsubscribe = workerPool.subscribeToStatChanges((stats) => { + const ready = stats.managerState === 'initialized'; + if (ready && !isReadyRef.current) { + isReadyRef.current = ready; + setIsReady(ready); + } + }); + return () => { + clearTimeout(timeout); + unsubscribe(); + }; + }, [workerPool]); + return workerPool == null ? true : isReady; +} + +// The pool is long-lived and shared; multiple surfaces (all-files view, +// single-file panels) sync the same theme pair. Dedup so each render pass +// issues at most one setRenderOptions round-trip. +let lastSyncedTheme = ''; + +/** + * Keeps the worker pool's theme pair in step with the UI theme (diffshub's + * useWorkerDiffTheme). Component options alone don't reach the workers. + */ +export function useWorkerPoolThemeSync(theme: { dark: string; light: string } | undefined): void { + const workerPool = useWorkerPool(); + useEffect(() => { + if (workerPool == null || theme == null) return; + const key = `${theme.dark}\0${theme.light}`; + if (key === lastSyncedTheme) return; + lastSyncedTheme = key; + void workerPool.setRenderOptions({ theme }); + }, [workerPool, theme?.dark, theme?.light]); // eslint-disable-line react-hooks/exhaustive-deps +} From d4120189a02ffae95e1e39182a316a1243eb03fd Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Fri, 12 Jun 2026 09:00:47 -0700 Subject: [PATCH 12/14] fix(review): un-poison worker-pool theme dedup on failed setRenderOptions A failed round-trip recorded the theme as synced and never retried, pinning the pool to the wrong palette for the session. --- packages/review-editor/workerPool.tsx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/review-editor/workerPool.tsx b/packages/review-editor/workerPool.tsx index 7632ad6e5..b103c5234 100644 --- a/packages/review-editor/workerPool.tsx +++ b/packages/review-editor/workerPool.tsx @@ -93,6 +93,11 @@ export function useWorkerPoolThemeSync(theme: { dark: string; light: string } | const key = `${theme.dark}\0${theme.light}`; if (key === lastSyncedTheme) return; lastSyncedTheme = key; - void workerPool.setRenderOptions({ theme }); + workerPool.setRenderOptions({ theme }).catch((err) => { + // Un-poison the dedup so a later render retries — otherwise one failed + // round-trip would pin the pool to the wrong theme for the session. + if (lastSyncedTheme === key) lastSyncedTheme = ''; + console.warn('Plannotator: failed to sync highlight theme to worker pool', err); + }); }, [workerPool, theme?.dark, theme?.light]); // eslint-disable-line react-hooks/exhaustive-deps } From fd3518e76d26c76cb929e5620b7142ac3b71758f Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Fri, 12 Jun 2026 13:26:11 -0700 Subject: [PATCH 13/14] fix(review): report partial diffs without a checkout; fail fast on missing checkout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dogfood review of this PR (via plannotator itself) caught two valid issues: - prPatchIncomplete was gated on the worktree pool, so a --no-local session showed a truncated diff with no indication at all. Partiality is information; upgradability is a capability. The flag is now always reported, with a separate prPatchUpgradeAvailable — the UI shows the amber notice either way, with the "Load full diff" button only when a checkout can exist (otherwise a "re-run with --local" hint). - After a FAILED checkout warmup, Ask AI sessions and agent jobs fell back to process.cwd() (or a wrong revision on Pi) — running in the wrong tree instead of failing. Both launch points now refuse with a clear "Local PR checkout unavailable — retry shortly" error (503); the job handlers surface buildCommand refusals instead of mislabeling them "Invalid JSON". Bun and Pi mirrored. A third finding (sem availability stuck after warmup) was triaged invalid: the availability probe detects the sem binary, which is cwd-independent. --- apps/pi-extension/server/agent-jobs.ts | 10 ++++- apps/pi-extension/server/serverReview.ts | 35 +++++++++++---- packages/review-editor/App.tsx | 20 +++++++-- packages/review-editor/hooks/usePRSession.ts | 14 ++++-- packages/review-editor/hooks/usePRStack.ts | 1 + packages/server/agent-jobs.ts | 10 ++++- packages/server/review.ts | 45 ++++++++++++++------ 7 files changed, 104 insertions(+), 31 deletions(-) diff --git a/apps/pi-extension/server/agent-jobs.ts b/apps/pi-extension/server/agent-jobs.ts index 8711189a2..d4a492402 100644 --- a/apps/pi-extension/server/agent-jobs.ts +++ b/apps/pi-extension/server/agent-jobs.ts @@ -479,8 +479,14 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) { diffContext: jobDiffContext, }); json(res, { job }, 201); - } catch { - json(res, { error: "Invalid JSON" }, 400); + } catch (err) { + // buildCommand can refuse a launch (e.g. PR checkout unavailable) — + // surface its message instead of mislabeling it a JSON error. + if (err instanceof SyntaxError) { + json(res, { error: "Invalid JSON" }, 400); + } else { + json(res, { error: err instanceof Error ? err.message : "Failed to launch agent" }, 503); + } } return true; } diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index b51eaffdb..59f54d885 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -240,9 +240,11 @@ export async function startReviewServer(options: { // limits) becomes available once a pool checkout exists — the layer // fingerprint flips to drive the refresh notice, and the pr-diff-scope // "layer" branch performs the upgrade. Tracked per-PR across pr-switch. - // Gated on the pool: without a local checkout the upgrade can never run, - // so the client must not be offered one. - let layerPatchIncomplete = (options.prPatchIncomplete ?? false) && isPRMode && !!options.worktreePool; + // Partiality is INFORMATION (the platform withheld content) and is always + // reported; whether a local recompute can be OFFERED is a separate + // capability, gated on the pool below (layerUpgradeAvailable). + let layerPatchIncomplete = (options.prPatchIncomplete ?? false) && isPRMode; + const layerUpgradeAvailable = !!options.worktreePool; const prSwitchCache = new Map(); if (isPRMode && prMeta) { prSwitchCache.set(prMeta.url, { @@ -463,6 +465,14 @@ export async function startReviewServer(options: { getCwd: resolveAgentCwd, async buildCommand(provider, config) { + // Fail fast in PR-pool mode when this PR's checkout doesn't exist + // (e.g. a pr-switch whose worktree creation failed): falling back + // would run the agent against the wrong revision or directory. + if (options.worktreePool && prMeta && !options.worktreePool.resolve(prMeta.url)) { + throw new Error( + "Local PR checkout unavailable — the agent can't run against the PR files. Retry shortly (the checkout may still be recovering).", + ); + } const cwd = resolveAgentCwd(); const workspacePrompt = getWorkspacePromptContext(); const hasAgentLocalAccess = !!workspacePrompt || !!options.worktreePool || !!options.agentCwd || !!options.gitContext; @@ -696,7 +706,7 @@ export async function startReviewServer(options: { prDiffScope: currentPRDiffScope, prDiffScopeOptions, }), - ...(isPRMode && layerPatchIncomplete && { prPatchIncomplete: true }), + ...(isPRMode && layerPatchIncomplete && { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable }), ...(isPRMode && initialViewedFiles.length > 0 && { viewedFiles: initialViewedFiles }), ...(currentError && { error: currentError }), semanticDiff: await getSemanticDiffAdvert(), @@ -833,7 +843,7 @@ export async function startReviewServer(options: { rawPatch: currentPatch, gitRef: currentGitRef, prDiffScope: currentPRDiffScope, - ...(layerPatchIncomplete ? { prPatchIncomplete: true } : {}), + ...(layerPatchIncomplete ? { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable } : {}), ...(currentError ? { error: currentError } : {}), semanticDiff, }); @@ -892,7 +902,7 @@ export async function startReviewServer(options: { rawPatch: currentPatch, gitRef: currentGitRef, prDiffScope: currentPRDiffScope, - ...(layerPatchIncomplete ? { prPatchIncomplete: true } : {}), + ...(layerPatchIncomplete ? { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable } : {}), ...((currentError ?? upgradeError) ? { error: currentError ?? upgradeError } : {}), semanticDiff: await getSemanticDiffAdvert(), }); @@ -955,7 +965,7 @@ export async function startReviewServer(options: { originalPRGitRef = currentGitRef; originalPRError = undefined; currentPRDiffScope = "layer"; - layerPatchIncomplete = (pr.patchIncomplete ?? false) && !!options.worktreePool; + layerPatchIncomplete = pr.patchIncomplete ?? false; draftKey = contentHash(pr.rawPatch); prListCache = null; captureDiffFingerprint(); @@ -1007,7 +1017,7 @@ export async function startReviewServer(options: { prStackTree, prDiffScope: currentPRDiffScope, prDiffScopeOptions, - ...(layerPatchIncomplete ? { prPatchIncomplete: true } : {}), + ...(layerPatchIncomplete ? { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable } : {}), repoInfo, ...(switchedViewedFiles.length > 0 && { viewedFiles: switchedViewedFiles }), ...(currentError ? { error: currentError } : {}), @@ -1351,7 +1361,14 @@ export async function startReviewServer(options: { // exists first so sessions never root in a transient fallback // (mirrors the Bun server; no-op while the pool entry is ready). if (req.method === "POST" && url.pathname === "/api/ai/session" && options.worktreePool && prMeta) { - try { await options.worktreePool.ensure(reviewRuntime, prMeta); } catch { /* capability degrades below */ } + // If the checkout can't be produced, refuse instead of starting a + // session rooted in the wrong directory. + try { + await options.worktreePool.ensure(reviewRuntime, prMeta); + } catch { + json(res, { error: "Local PR checkout unavailable — Ask AI can't read the PR files right now. Retry shortly." }, 503); + return; + } } if (await handlePiAIRequest(req, res, url, aiRuntime)) return; // Unmatched /api/ai/* paths fall through to the app shell, same as diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 8879af06c..3c55e67c9 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -208,7 +208,7 @@ const ReviewApp: React.FC = () => { document.title = repoInfo ? `${repoInfo.display} · Code Review` : "Code Review"; }, [repoInfo]); - const { prMetadata, prStackInfo, prStackTree, prDiffScope, prDiffScopeOptions, prPatchIncomplete, updatePRSession } = usePRSession(); + const { prMetadata, prStackInfo, prStackTree, prDiffScope, prDiffScopeOptions, prPatchIncomplete, prPatchUpgradeAvailable, updatePRSession } = usePRSession(); const { withPRContext } = useAnnotationFactory(prMetadata, prStackInfo ? prDiffScope : undefined); const prStackCallbacksRef = useRef(null); @@ -924,6 +924,7 @@ const ReviewApp: React.FC = () => { prDiffScope?: PRDiffScope; prDiffScopeOptions?: PRDiffScopeOption[]; prPatchIncomplete?: boolean; + prPatchUpgradeAvailable?: boolean; platformUser?: string; viewedFiles?: string[]; error?: string; @@ -969,7 +970,10 @@ const ReviewApp: React.FC = () => { ...(data.prStackTree !== undefined && { prStackTree: data.prStackTree }), ...(data.prDiffScope && { prDiffScope: data.prDiffScope }), ...(data.prDiffScopeOptions && { prDiffScopeOptions: data.prDiffScopeOptions }), - ...(data.prMetadata && { prPatchIncomplete: data.prPatchIncomplete === true }), + ...(data.prMetadata && { + prPatchIncomplete: data.prPatchIncomplete === true, + prPatchUpgradeAvailable: data.prPatchUpgradeAvailable === true, + }), }); if (data.platformUser) setPlatformUser(data.platformUser); // Initialize viewed files from GitHub's state (set before draft restore so draft takes precedence) @@ -1282,6 +1286,7 @@ const ReviewApp: React.FC = () => { // Scope/switch responses authoritatively report partiality; absence // means the patch is complete (e.g. after the local recompute upgrade). prPatchIncomplete: data.prPatchIncomplete === true, + prPatchUpgradeAvailable: data.prPatchUpgradeAvailable === true, }); if (data.repoInfo) setRepoInfo(data.repoInfo); if (data.prMetadata) { @@ -2194,7 +2199,16 @@ const ReviewApp: React.FC = () => { Partial diff Partial - {isLoadingFullDiff ? ( + {!prPatchUpgradeAvailable ? ( + // Partiality without a local checkout: informational + // only — never offer a button that cannot work. + + (re-run with --local for the full diff) + + ) : isLoadingFullDiff ? ( Loading full diff… diff --git a/packages/review-editor/hooks/usePRSession.ts b/packages/review-editor/hooks/usePRSession.ts index 866394a7f..2630f8cb5 100644 --- a/packages/review-editor/hooks/usePRSession.ts +++ b/packages/review-editor/hooks/usePRSession.ts @@ -9,11 +9,16 @@ export interface PRSessionState { prDiffScope: PRDiffScope; prDiffScopeOptions: PRDiffScopeOption[]; /** - * The platform withheld per-file content for this PR (too large). A local - * recompute is offered via the partial-diff notice; cleared when a server - * response stops reporting the flag. + * The platform withheld per-file content for this PR (too large). Always + * reported; cleared when a server response stops reporting the flag. */ prPatchIncomplete: boolean; + /** + * A local checkout (worktree pool) exists, so the partial-diff notice can + * offer the "Load full diff" recompute. Without it the notice is + * informational only. + */ + prPatchUpgradeAvailable: boolean; } export interface PRSessionUpdate { @@ -23,6 +28,7 @@ export interface PRSessionUpdate { prDiffScope?: PRDiffScope; prDiffScopeOptions?: PRDiffScopeOption[]; prPatchIncomplete?: boolean; + prPatchUpgradeAvailable?: boolean; } export function usePRSession() { @@ -33,6 +39,7 @@ export function usePRSession() { prDiffScope: 'layer', prDiffScopeOptions: [], prPatchIncomplete: false, + prPatchUpgradeAvailable: false, }); const updatePRSession = useCallback((update: PRSessionUpdate) => { @@ -44,6 +51,7 @@ export function usePRSession() { if (update.prDiffScope !== undefined) next.prDiffScope = update.prDiffScope; if (update.prDiffScopeOptions !== undefined) next.prDiffScopeOptions = update.prDiffScopeOptions; if (update.prPatchIncomplete !== undefined) next.prPatchIncomplete = update.prPatchIncomplete; + if (update.prPatchUpgradeAvailable !== undefined) next.prPatchUpgradeAvailable = update.prPatchUpgradeAvailable; return next; }); }, []); diff --git a/packages/review-editor/hooks/usePRStack.ts b/packages/review-editor/hooks/usePRStack.ts index 15d883983..67713df10 100644 --- a/packages/review-editor/hooks/usePRStack.ts +++ b/packages/review-editor/hooks/usePRStack.ts @@ -11,6 +11,7 @@ export interface PRSwitchResponse { prDiffScope?: PRDiffScope; prDiffScopeOptions?: unknown[]; prPatchIncomplete?: boolean; + prPatchUpgradeAvailable?: boolean; repoInfo?: unknown; viewedFiles?: string[]; error?: string; diff --git a/packages/server/agent-jobs.ts b/packages/server/agent-jobs.ts index 075f26584..9a1424415 100644 --- a/packages/server/agent-jobs.ts +++ b/packages/server/agent-jobs.ts @@ -507,8 +507,14 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob diffContext: jobDiffContext, }); return Response.json({ job }, { status: 201 }); - } catch { - return Response.json({ error: "Invalid JSON" }, { status: 400 }); + } catch (err) { + // buildCommand can refuse a launch (e.g. PR checkout unavailable) — + // surface its message instead of mislabeling it a JSON error. + if (err instanceof SyntaxError) { + return Response.json({ error: "Invalid JSON" }, { status: 400 }); + } + const message = err instanceof Error ? err.message : "Failed to launch agent"; + return Response.json({ error: message }, { status: 503 }); } } diff --git a/packages/server/review.ts b/packages/server/review.ts index 8a9527a96..214213932 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -192,9 +192,11 @@ export async function startReviewServer( // limits) becomes available once the checkout warmup finishes — the layer // fingerprint flips to drive the refresh notice, and the pr-diff-scope // "layer" branch performs the upgrade. Tracked per-PR across pr-switch. - // Gated on the pool: without a local checkout the upgrade can never run, - // so the client must not be offered one. - let layerPatchIncomplete = (options.prPatchIncomplete ?? false) && isPRMode && !!options.worktreePool; + // Partiality is INFORMATION (the platform withheld content) and is always + // reported; whether a local recompute can be OFFERED is a separate + // capability, gated on the pool below (layerUpgradeAvailable). + let layerPatchIncomplete = (options.prPatchIncomplete ?? false) && isPRMode; + const layerUpgradeAvailable = !!options.worktreePool; let prListCache: PRListItem[] | null = null; let prListCacheTime = 0; const prSwitchCache = new Map(); @@ -437,9 +439,20 @@ export async function startReviewServer( // Agents run inside the PR checkout — wait out the background warmup so // the spawn-time getCwd() below resolves to a path that exists. - const cwd = options.worktreePool && launchMetadata - ? (await ensurePRLocalCwd(launchMetadata)) ?? resolveAgentCwd() - : await resolveAgentCwdReady(); + let cwd: string; + if (options.worktreePool && launchMetadata) { + const checkout = await ensurePRLocalCwd(launchMetadata); + if (!checkout) { + // Fail fast: without the checkout the job would run in whatever + // directory the CLI was launched from — possibly an unrelated repo. + throw new Error( + "Local PR checkout unavailable — the agent can't run against the PR files. Retry shortly (the checkout may still be recovering).", + ); + } + cwd = checkout; + } else { + cwd = await resolveAgentCwdReady(); + } const workspacePrompt = getWorkspacePromptContext(); // Honest local-access claim: in PR mode the checkout must actually be // available (warmup done, not failed) — the prompt tells the agent it @@ -748,7 +761,7 @@ export async function startReviewServer( prDiffScope: currentPRDiffScope, prDiffScopeOptions, }), - ...(isPRMode && layerPatchIncomplete && { prPatchIncomplete: true }), + ...(isPRMode && layerPatchIncomplete && { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable }), ...(isPRMode && initialViewedFiles.length > 0 && { viewedFiles: initialViewedFiles }), ...(currentError && { error: currentError }), semanticDiff: await getSemanticDiffAdvert(), @@ -903,7 +916,7 @@ export async function startReviewServer( rawPatch: currentPatch, gitRef: currentGitRef, prDiffScope: currentPRDiffScope, - ...(layerPatchIncomplete && { prPatchIncomplete: true }), + ...(layerPatchIncomplete && { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable }), ...(currentError && { error: currentError }), semanticDiff, }); @@ -953,7 +966,7 @@ export async function startReviewServer( rawPatch: currentPatch, gitRef: currentGitRef, prDiffScope: currentPRDiffScope, - ...(layerPatchIncomplete && { prPatchIncomplete: true }), + ...(layerPatchIncomplete && { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable }), ...((currentError ?? upgradeError) && { error: currentError ?? upgradeError }), semanticDiff: await getSemanticDiffAdvert(), }); @@ -1056,7 +1069,7 @@ export async function startReviewServer( originalPRGitRef = currentGitRef; originalPRError = undefined; currentPRDiffScope = "layer"; - layerPatchIncomplete = (pr.patchIncomplete ?? false) && !!options.worktreePool; + layerPatchIncomplete = pr.patchIncomplete ?? false; draftKey = contentHash(pr.rawPatch); prListCache = null; captureDiffFingerprint(); @@ -1120,7 +1133,7 @@ export async function startReviewServer( prStackTree, prDiffScope: currentPRDiffScope, prDiffScopeOptions, - ...(layerPatchIncomplete && { prPatchIncomplete: true }), + ...(layerPatchIncomplete && { prPatchIncomplete: true, prPatchUpgradeAvailable: layerUpgradeAvailable }), repoInfo, ...(switchedViewedFiles.length > 0 && { viewedFiles: switchedViewedFiles }), ...(currentError ? { error: currentError } : {}), @@ -1505,8 +1518,16 @@ export async function startReviewServer( // AI sessions pin their cwd at creation — wait out the PR // checkout warmup so a session opened in the first seconds // isn't rooted in a transient fallback directory for life. + // If the checkout can't be produced (warmup failed), refuse + // instead of starting a session in the wrong directory. if (req.method === "POST" && url.pathname === "/api/ai/session" && options.worktreePool && prMetadata) { - await ensurePRLocalCwd(); + const checkout = await ensurePRLocalCwd(); + if (!checkout) { + return Response.json( + { error: "Local PR checkout unavailable — Ask AI can't read the PR files right now. Retry shortly." }, + { status: 503 }, + ); + } } if (url.pathname === AI_QUERY_ENDPOINT) { server.timeout(req, 0); From cb805ec0651da3452d18c67f83679f6efa5da5ca Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Fri, 12 Jun 2026 13:33:11 -0700 Subject: [PATCH 14/14] fix(review): runtime-neutral copy for the no-checkout partial-diff hint --local is a CLI remedy; OpenCode sessions have no such flag. Visible text states the fact, the tooltip carries the CLI guidance. --- packages/review-editor/App.tsx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 3c55e67c9..d59319669 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -2201,12 +2201,14 @@ const ReviewApp: React.FC = () => { Partial {!prPatchUpgradeAvailable ? ( // Partiality without a local checkout: informational - // only — never offer a button that cannot work. + // only — never offer a button that cannot work. The + // visible text stays runtime-neutral (--local is a CLI + // remedy that not every runtime supports). - (re-run with --local for the full diff) + (no local checkout — full diff unavailable) ) : isLoadingFullDiff ? (