Skip to content

Commit 181d2a1

Browse files
authored
fix(pi): full PR review parity with Bun server (#503)
## Summary Full Pi PR review parity with Bun server, plus shared fixes and UI improvements. ### Pi PR Review (new) - Parse PR/MR URLs, authenticate, fetch metadata and diff - Create local worktree (same-repo) or shallow clone (cross-repo) for agent file access - Full server parity: all 27 feature areas audited and aligned ### Shared Fixes (Bun + Pi) - parseRemoteUrl: GitLab subgroup support, SSH+port, HTTPS non-standard port - Cross-repo clone: --hostname replaced with GH_HOST/GITLAB_HOST env vars - Cross-repo clone: shallow fetch depth 50 → 200 - Git Add button disabled in PR review mode (client-side) - DiffViewer: eliminate dark flash on file switch (sync theme init) ### UI - Resolved/Outdated toggle filters in PR comments tab For provenance purposes, this commit was AI assisted.
1 parent e3f6fc8 commit 181d2a1

10 files changed

Lines changed: 364 additions & 76 deletions

File tree

apps/hook/server/index.ts

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -319,13 +319,18 @@ if (args[0] === "sessions") {
319319
if (/^-/.test(prRepo)) throw new Error(`Invalid repository identifier: ${prRepo}`);
320320
const cli = prMetadata.platform === "github" ? "gh" : "glab";
321321
const host = prMetadata.host;
322-
const hostnameArgs = (host === "github.com" || host === "gitlab.com") ? [] : ["--hostname", host];
322+
// gh/glab repo clone doesn't accept --hostname; set GH_HOST/GITLAB_HOST env instead
323+
const isDefaultHost = host === "github.com" || host === "gitlab.com";
324+
const cloneEnv = isDefaultHost ? undefined : {
325+
...process.env,
326+
...(prMetadata.platform === "github" ? { GH_HOST: host } : { GITLAB_HOST: host }),
327+
};
323328

324329
// Step 1: Fast skeleton clone (no checkout, depth 1 — minimal data transfer)
325330
console.error(`Cloning ${prRepo} (shallow)...`);
326331
const cloneResult = Bun.spawnSync(
327-
[cli, "repo", "clone", prRepo, localPath, ...hostnameArgs, "--", "--depth=1", "--no-checkout"],
328-
{ stderr: "pipe" },
332+
[cli, "repo", "clone", prRepo, localPath, "--", "--depth=1", "--no-checkout"],
333+
{ stderr: "pipe", env: cloneEnv },
329334
);
330335
if (cloneResult.exitCode !== 0) {
331336
throw new Error(`${cli} repo clone failed: ${new TextDecoder().decode(cloneResult.stderr).trim()}`);
@@ -334,7 +339,7 @@ if (args[0] === "sessions") {
334339
// Step 2: Fetch only the PR head ref (targeted, much faster than full fetch)
335340
console.error("Fetching PR branch...");
336341
const fetchResult = Bun.spawnSync(
337-
["git", "fetch", "--depth=50", "origin", fetchRefStr],
342+
["git", "fetch", "--depth=200", "origin", fetchRefStr],
338343
{ cwd: localPath, stderr: "pipe" },
339344
);
340345
if (fetchResult.exitCode !== 0) throw new Error(`Failed to fetch PR head ref: ${new TextDecoder().decode(fetchResult.stderr).trim()}`);
@@ -346,7 +351,7 @@ if (args[0] === "sessions") {
346351
}
347352

348353
// Best-effort: create base refs so `git diff main...HEAD` and `git diff origin/main...HEAD` work
349-
const baseFetch = Bun.spawnSync(["git", "fetch", "--depth=50", "origin", prMetadata.baseSha], { cwd: localPath, stderr: "pipe" });
354+
const baseFetch = Bun.spawnSync(["git", "fetch", "--depth=200", "origin", prMetadata.baseSha], { cwd: localPath, stderr: "pipe" });
350355
if (baseFetch.exitCode !== 0) console.error("Warning: failed to fetch baseSha, agent diffs may be inaccurate");
351356
Bun.spawnSync(["git", "branch", "--", prMetadata.baseBranch, prMetadata.baseSha], { cwd: localPath, stderr: "pipe" });
352357
Bun.spawnSync(["git", "update-ref", `refs/remotes/origin/${prMetadata.baseBranch}`, prMetadata.baseSha], { cwd: localPath, stderr: "pipe" });

apps/pi-extension/index.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -333,8 +333,8 @@ export default function plannotator(pi: ExtensionAPI): void {
333333
});
334334

335335
pi.registerCommand("plannotator-review", {
336-
description: "Open interactive code review for current changes",
337-
handler: async (_args, ctx) => {
336+
description: "Open interactive code review for current changes or a PR URL",
337+
handler: async (args, ctx) => {
338338
if (!hasReviewBrowserHtml()) {
339339
ctx.ui.notify(
340340
"Code review UI not available. Run 'bun run build' in the pi-extension directory.",
@@ -344,12 +344,18 @@ export default function plannotator(pi: ExtensionAPI): void {
344344
}
345345

346346
try {
347-
const result = await openCodeReview(ctx);
347+
const prUrl = args?.trim() || undefined;
348+
const isPRReview = prUrl?.startsWith("http://") || prUrl?.startsWith("https://");
349+
const result = await openCodeReview(ctx, { prUrl });
348350
if (result.feedback) {
349351
if (result.approved) {
350352
pi.sendUserMessage(
351353
`# Code Review\n\nCode review completed — no changes requested.`,
352354
);
355+
} else if (isPRReview) {
356+
// Platform PR actions (approve/comment) return approved:false with a
357+
// status message — don't tell the agent to "address" a platform action.
358+
pi.sendUserMessage(result.feedback);
353359
} else {
354360
pi.sendUserMessage(
355361
`${result.feedback}\n\nPlease address this feedback.`,

apps/pi-extension/plannotator-browser.ts

Lines changed: 193 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,29 @@
1-
import { existsSync, readFileSync, statSync } from "node:fs";
2-
import { dirname, resolve } from "node:path";
1+
import { existsSync, readFileSync, realpathSync, rmSync, statSync } from "node:fs";
2+
import { dirname, join, resolve } from "node:path";
3+
import { tmpdir } from "node:os";
4+
import { spawnSync } from "node:child_process";
35
import { fileURLToPath } from "node:url";
46
import type { ExtensionContext } from "@mariozechner/pi-coding-agent";
57
import {
68
getGitContext,
9+
reviewRuntime,
710
runGitDiff,
811
startAnnotateServer,
912
startPlanReviewServer,
1013
startReviewServer,
1114
type DiffType,
1215
} from "./server.js";
1316
import { openBrowser } from "./server/network.js";
17+
import { parsePRUrl, checkPRAuth, fetchPR } from "./server/pr.js";
18+
import {
19+
getMRLabel,
20+
getMRNumberLabel,
21+
getDisplayRepo,
22+
getCliName,
23+
getCliInstallUrl,
24+
} from "./generated/pr-provider.js";
25+
import { parseRemoteUrl } from "./generated/repo.js";
26+
import { fetchRef, createWorktree, removeWorktree, ensureObjectAvailable } from "./generated/worktree.js";
1427

1528
export type AnnotateMode = "annotate" | "annotate-folder" | "annotate-last";
1629
export interface PlanReviewDecision {
@@ -151,27 +164,198 @@ export async function openPlanReviewBrowser(
151164

152165
export async function openCodeReview(
153166
ctx: ExtensionContext,
154-
options: { cwd?: string; defaultBranch?: string; diffType?: DiffType } = {},
167+
options: { cwd?: string; defaultBranch?: string; diffType?: DiffType; prUrl?: string } = {},
155168
): Promise<{ approved: boolean; feedback?: string; annotations?: unknown[]; agentSwitch?: string }> {
156169
if (!ctx.hasUI || !reviewHtmlContent) {
157170
throw new Error("Plannotator code review browser is unavailable in this session.");
158171
}
159172

160-
const cwd = options.cwd ?? ctx.cwd;
161-
const gitCtx = await getGitContext(cwd);
162-
const defaultBranch = options.defaultBranch ?? gitCtx.defaultBranch;
163-
const diffType: DiffType = options.diffType ?? "uncommitted";
164-
const { patch: rawPatch, label: gitRef, error } = await runGitDiff(diffType, defaultBranch, cwd);
173+
const urlArg = options.prUrl;
174+
const isPRMode = urlArg?.startsWith("http://") || urlArg?.startsWith("https://");
175+
176+
let rawPatch: string;
177+
let gitRef: string;
178+
let diffError: string | undefined;
179+
let gitCtx: Awaited<ReturnType<typeof getGitContext>> | undefined;
180+
let prMetadata: Awaited<ReturnType<typeof fetchPR>>["metadata"] | undefined;
181+
let diffType: DiffType | undefined;
182+
let agentCwd: string | undefined;
183+
let worktreeCleanup: (() => void | Promise<void>) | undefined;
184+
let exitHandler: (() => void) | undefined;
185+
186+
if (isPRMode && urlArg) {
187+
// --- PR Review Mode ---
188+
const prRef = parsePRUrl(urlArg);
189+
if (!prRef) {
190+
throw new Error(
191+
`Invalid PR/MR URL: ${urlArg}\n` +
192+
"Supported formats:\n" +
193+
" GitHub: https://github.com/owner/repo/pull/123\n" +
194+
" GitLab: https://gitlab.com/group/project/-/merge_requests/42",
195+
);
196+
}
197+
198+
const cliName = getCliName(prRef);
199+
const cliUrl = getCliInstallUrl(prRef);
200+
201+
try {
202+
await checkPRAuth(prRef);
203+
} catch (err) {
204+
const msg = err instanceof Error ? err.message : String(err);
205+
if (msg.includes("not found") || msg.includes("ENOENT")) {
206+
throw new Error(`${cliName === "gh" ? "GitHub" : "GitLab"} CLI (${cliName}) is not installed. Install it from ${cliUrl}`);
207+
}
208+
throw err;
209+
}
210+
211+
console.error(`Fetching ${getMRLabel(prRef)} ${getMRNumberLabel(prRef)} from ${getDisplayRepo(prRef)}...`);
212+
const pr = await fetchPR(prRef);
213+
rawPatch = pr.rawPatch;
214+
gitRef = `${getMRLabel(prRef)} ${getMRNumberLabel(prRef)}`;
215+
prMetadata = pr.metadata;
216+
217+
// Create local worktree for agent file access (--local is the default for PR reviews)
218+
let localPath: string | undefined;
219+
try {
220+
const repoDir = options.cwd ?? ctx.cwd;
221+
const identifier = prMetadata.platform === "github"
222+
? `${prMetadata.owner}-${prMetadata.repo}-${prMetadata.number}`
223+
: `${prMetadata.projectPath.replace(/\//g, "-")}-${prMetadata.iid}`;
224+
const suffix = Math.random().toString(36).slice(2, 8);
225+
localPath = join(realpathSync(tmpdir()), `plannotator-pr-${identifier}-${suffix}`);
226+
const fetchRefStr = prMetadata.platform === "github"
227+
? `refs/pull/${prMetadata.number}/head`
228+
: `refs/merge-requests/${prMetadata.iid}/head`;
229+
230+
// Validate inputs from platform API to prevent git flag/path injection
231+
if (prMetadata.baseBranch.includes('..') || prMetadata.baseBranch.startsWith('-')) throw new Error(`Invalid base branch: ${prMetadata.baseBranch}`);
232+
if (!/^[0-9a-f]{40,64}$/i.test(prMetadata.baseSha)) throw new Error(`Invalid base SHA: ${prMetadata.baseSha}`);
233+
234+
// Detect same-repo vs cross-repo (must match both owner/repo AND host)
235+
let isSameRepo = false;
236+
try {
237+
const remoteResult = await reviewRuntime.runGit(["remote", "get-url", "origin"], { cwd: repoDir });
238+
if (remoteResult.exitCode === 0) {
239+
const remoteUrl = remoteResult.stdout.trim();
240+
const currentRepo = parseRemoteUrl(remoteUrl);
241+
const prRepo = prMetadata.platform === "github"
242+
? `${prMetadata.owner}/${prMetadata.repo}`
243+
: prMetadata.projectPath;
244+
const repoMatches = !!currentRepo && currentRepo.toLowerCase() === prRepo.toLowerCase();
245+
const sshHost = remoteUrl.match(/^[^@]+@([^:]+):/)?.[1];
246+
const httpsHost = (() => { try { return new URL(remoteUrl).hostname; } catch { return null; } })();
247+
const remoteHost = (sshHost || httpsHost || "").toLowerCase();
248+
const prHost = prMetadata.host.toLowerCase();
249+
isSameRepo = repoMatches && remoteHost === prHost;
250+
}
251+
} catch { /* not in a git repo — cross-repo path */ }
252+
253+
if (isSameRepo) {
254+
// ── Same-repo: fast worktree path ──
255+
console.error("Fetching PR branch and creating local worktree...");
256+
await fetchRef(reviewRuntime, prMetadata.baseBranch, { cwd: repoDir });
257+
await ensureObjectAvailable(reviewRuntime, prMetadata.baseSha, { cwd: repoDir });
258+
await fetchRef(reviewRuntime, fetchRefStr, { cwd: repoDir });
259+
260+
await createWorktree(reviewRuntime, {
261+
ref: "FETCH_HEAD",
262+
path: localPath,
263+
detach: true,
264+
cwd: repoDir,
265+
});
266+
267+
const worktreePath = localPath;
268+
const wtRepoDir = repoDir;
269+
exitHandler = () => {
270+
try { spawnSync("git", ["worktree", "remove", "--force", worktreePath], { cwd: wtRepoDir }); } catch {}
271+
};
272+
worktreeCleanup = () => {
273+
if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; }
274+
return removeWorktree(reviewRuntime, worktreePath, { force: true, cwd: wtRepoDir });
275+
};
276+
process.once("exit", exitHandler);
277+
} else {
278+
// ── Cross-repo: shallow clone + fetch PR head ──
279+
const prRepo = prMetadata.platform === "github"
280+
? `${prMetadata.owner}/${prMetadata.repo}`
281+
: prMetadata.projectPath;
282+
if (/^-/.test(prRepo)) throw new Error(`Invalid repository identifier: ${prRepo}`);
283+
const cli = prMetadata.platform === "github" ? "gh" : "glab";
284+
const host = prMetadata.host;
285+
// gh/glab repo clone doesn't accept --hostname; set GH_HOST/GITLAB_HOST env instead
286+
const isDefaultHost = host === "github.com" || host === "gitlab.com";
287+
const cloneEnv = isDefaultHost ? undefined : {
288+
...process.env,
289+
...(prMetadata.platform === "github" ? { GH_HOST: host } : { GITLAB_HOST: host }),
290+
};
291+
292+
console.error(`Cloning ${prRepo} (shallow)...`);
293+
const cloneResult = spawnSync(cli, ["repo", "clone", prRepo, localPath, "--", "--depth=1", "--no-checkout"], { encoding: "utf-8", env: cloneEnv });
294+
if ((cloneResult.status ?? 1) !== 0) {
295+
throw new Error(`${cli} repo clone failed: ${(cloneResult.stderr ?? "").trim()}`);
296+
}
297+
298+
console.error("Fetching PR branch...");
299+
const fetchResult = await reviewRuntime.runGit(["fetch", "--depth=200", "origin", fetchRefStr], { cwd: localPath });
300+
if (fetchResult.exitCode !== 0) throw new Error(`Failed to fetch PR head ref: ${fetchResult.stderr.trim()}`);
301+
302+
const checkoutResult = await reviewRuntime.runGit(["checkout", "FETCH_HEAD"], { cwd: localPath });
303+
if (checkoutResult.exitCode !== 0) {
304+
throw new Error(`git checkout FETCH_HEAD failed: ${checkoutResult.stderr.trim()}`);
305+
}
306+
307+
// Best-effort: create base refs so agent diffs work
308+
const baseFetch = await reviewRuntime.runGit(["fetch", "--depth=200", "origin", prMetadata.baseSha], { cwd: localPath });
309+
if (baseFetch.exitCode !== 0) console.error("Warning: failed to fetch baseSha, agent diffs may be inaccurate");
310+
await reviewRuntime.runGit(["branch", "--", prMetadata.baseBranch, prMetadata.baseSha], { cwd: localPath });
311+
await reviewRuntime.runGit(["update-ref", `refs/remotes/origin/${prMetadata.baseBranch}`, prMetadata.baseSha], { cwd: localPath });
312+
313+
const clonePath = localPath;
314+
exitHandler = () => {
315+
try { rmSync(clonePath, { recursive: true, force: true }); } catch {}
316+
};
317+
worktreeCleanup = () => {
318+
if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; }
319+
try { rmSync(clonePath, { recursive: true, force: true }); } catch {}
320+
};
321+
process.once("exit", exitHandler);
322+
}
323+
324+
agentCwd = localPath;
325+
console.error(`Local checkout ready at ${localPath}`);
326+
} catch (err) {
327+
console.error("Warning: local worktree creation failed, falling back to remote diff");
328+
console.error(err instanceof Error ? err.message : String(err));
329+
if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; }
330+
if (localPath) try { rmSync(localPath, { recursive: true, force: true }); } catch {}
331+
agentCwd = undefined;
332+
worktreeCleanup = undefined;
333+
}
334+
} else {
335+
// --- Local Review Mode ---
336+
const cwd = options.cwd ?? ctx.cwd;
337+
gitCtx = await getGitContext(cwd);
338+
const defaultBranch = options.defaultBranch ?? gitCtx.defaultBranch;
339+
diffType = options.diffType ?? "uncommitted";
340+
const result = await runGitDiff(diffType, defaultBranch, cwd);
341+
rawPatch = result.patch;
342+
gitRef = result.label;
343+
diffError = result.error;
344+
}
345+
165346
const server = await startReviewServer({
166347
rawPatch,
167348
gitRef,
168-
error,
349+
error: diffError,
169350
origin: "pi",
170351
diffType,
171352
gitContext: gitCtx,
353+
prMetadata,
354+
agentCwd,
172355
htmlContent: reviewHtmlContent,
173356
sharingEnabled: process.env.PLANNOTATOR_SHARE !== "disabled",
174357
shareBaseUrl: process.env.PLANNOTATOR_SHARE_URL || undefined,
358+
onCleanup: worktreeCleanup,
175359
});
176360

177361
return openBrowserAndWait(server, ctx, server.waitForDecision);

apps/pi-extension/plannotator-events.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ export interface PlannotatorCodeReviewPayload {
8585
diffType?: DiffType;
8686
defaultBranch?: string;
8787
cwd?: string;
88+
prUrl?: string;
8889
}
8990

9091
export interface PlannotatorCodeReviewResult {
@@ -254,6 +255,7 @@ export function registerPlannotatorEventListeners(pi: ExtensionAPI): void {
254255
cwd: request.payload?.cwd,
255256
defaultBranch: request.payload?.defaultBranch,
256257
diffType: request.payload?.diffType,
258+
prUrl: request.payload?.prUrl,
257259
});
258260
request.respond({ status: "handled", result });
259261
return;

apps/pi-extension/server.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ export {
2121
} from "./server/serverPlan.js";
2222
export {
2323
getGitContext,
24+
reviewRuntime,
2425
type ReviewServerResult,
2526
runGitDiff,
2627
startReviewServer,

0 commit comments

Comments
 (0)