-
Notifications
You must be signed in to change notification settings - Fork 274
feat(review): AI review agents, local worktree, and UI polish #491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
dd23155
9efc16a
d558229
0c41712
2725739
81d9332
b5e27d5
abaab38
5ab5bb6
aea8de5
3f393ae
f4183bb
6b510ac
bb26579
ee3b2b4
391ceaa
5808b3c
078f3d3
2b35d5b
6e07d0c
ff11419
4b00045
5059829
dc05bc8
4124841
cda3245
5bad2e9
ee6ede3
2da62a4
82dc068
49347b8
0e8e0a8
f8b5499
7003465
8ce8253
fa53662
6f44439
4a2a919
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,12 +63,14 @@ import { | |
| startAnnotateServer, | ||
| handleAnnotateServerReady, | ||
| } from "@plannotator/server/annotate"; | ||
| import { type DiffType, getVcsContext, runVcsDiff } from "@plannotator/server/vcs"; | ||
| import { type DiffType, getVcsContext, runVcsDiff, gitRuntime } from "@plannotator/server/vcs"; | ||
| import { fetchRef, createWorktree, removeWorktree, ensureObjectAvailable } from "@plannotator/shared/worktree"; | ||
| import { parsePRUrl, checkPRAuth, fetchPR, getCliName, getCliInstallUrl, getMRLabel, getMRNumberLabel, getDisplayRepo } from "@plannotator/server/pr"; | ||
| import { writeRemoteShareLink } from "@plannotator/server/share-url"; | ||
| import { resolveMarkdownFile, hasMarkdownFiles } from "@plannotator/shared/resolve-file"; | ||
| import { FILE_BROWSER_EXCLUDED } from "@plannotator/shared/reference-common"; | ||
| import { statSync } from "fs"; | ||
| import { statSync, rmSync, realpathSync } from "fs"; | ||
| import { parseRemoteUrl } from "@plannotator/shared/repo"; | ||
| import { registerSession, unregisterSession, listSessions } from "@plannotator/server/sessions"; | ||
| import { openBrowser } from "@plannotator/server/browser"; | ||
| import { detectProjectName } from "@plannotator/server/project"; | ||
|
|
@@ -85,6 +87,7 @@ import { | |
| isTopLevelHelpInvocation, | ||
| } from "./cli"; | ||
| import path from "path"; | ||
| import { tmpdir } from "os"; | ||
|
|
||
| // Embed the built HTML at compile time | ||
| // @ts-ignore - Bun import attribute for text | ||
|
|
@@ -184,15 +187,26 @@ if (args[0] === "sessions") { | |
| // CODE REVIEW MODE | ||
| // ============================================ | ||
|
|
||
| // Parse local flags (strip before URL detection) | ||
| // --local is now the default for PR/MR reviews; --no-local opts out. | ||
| // --local kept for backwards compat (no-op). | ||
| const localIdx = args.indexOf("--local"); | ||
| if (localIdx !== -1) args.splice(localIdx, 1); | ||
| const noLocalIdx = args.indexOf("--no-local"); | ||
| if (noLocalIdx !== -1) args.splice(noLocalIdx, 1); | ||
|
|
||
| const urlArg = args[1]; | ||
| const isPRMode = urlArg?.startsWith("http://") || urlArg?.startsWith("https://"); | ||
| const useLocal = isPRMode && noLocalIdx === -1; | ||
|
|
||
| let rawPatch: string; | ||
| let gitRef: string; | ||
| let diffError: string | undefined; | ||
| let gitContext: Awaited<ReturnType<typeof getVcsContext>> | undefined; | ||
| let prMetadata: Awaited<ReturnType<typeof fetchPR>>["metadata"] | undefined; | ||
| let initialDiffType: DiffType | undefined; | ||
| let agentCwd: string | undefined; | ||
| let worktreeCleanup: (() => void | Promise<void>) | undefined; | ||
|
|
||
| if (isPRMode) { | ||
| // --- PR Review Mode --- | ||
|
|
@@ -231,6 +245,132 @@ if (args[0] === "sessions") { | |
| console.error(err instanceof Error ? err.message : "Failed to fetch PR"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // --local: create a local checkout with the PR head for full file access | ||
| if (useLocal && prMetadata) { | ||
| // Hoisted so catch block can clean up partially-created directories | ||
| let localPath: string | undefined; | ||
| try { | ||
| const repoDir = process.cwd(); | ||
| const identifier = prMetadata.platform === "github" | ||
| ? `${prMetadata.owner}-${prMetadata.repo}-${prMetadata.number}` | ||
| : `${prMetadata.projectPath.replace(/\//g, "-")}-${prMetadata.iid}`; | ||
| const suffix = Math.random().toString(36).slice(2, 8); | ||
| // Resolve tmpdir to its real path — on macOS, tmpdir() returns /var/folders/... | ||
| // but processes report /private/var/folders/... which breaks path stripping. | ||
| localPath = path.join(realpathSync(tmpdir()), `plannotator-pr-${identifier}-${suffix}`); | ||
| const fetchRefStr = prMetadata.platform === "github" | ||
| ? `refs/pull/${prMetadata.number}/head` | ||
| : `refs/merge-requests/${prMetadata.iid}/head`; | ||
|
|
||
| // Validate inputs from platform API to prevent git flag/path injection | ||
| if (prMetadata.baseBranch.includes('..') || prMetadata.baseBranch.startsWith('-')) throw new Error(`Invalid base branch: ${prMetadata.baseBranch}`); | ||
| if (!/^[0-9a-f]{40,64}$/i.test(prMetadata.baseSha)) throw new Error(`Invalid base SHA: ${prMetadata.baseSha}`); | ||
|
|
||
| // Detect same-repo vs cross-repo (must match both owner/repo AND host) | ||
| let isSameRepo = false; | ||
| try { | ||
| const remoteResult = await gitRuntime.runGit(["remote", "get-url", "origin"]); | ||
| if (remoteResult.exitCode === 0) { | ||
| const remoteUrl = remoteResult.stdout.trim(); | ||
| const currentRepo = parseRemoteUrl(remoteUrl); | ||
| const prRepo = prMetadata.platform === "github" | ||
| ? `${prMetadata.owner}/${prMetadata.repo}` | ||
| : prMetadata.projectPath; | ||
| const repoMatches = !!currentRepo && currentRepo.toLowerCase() === prRepo.toLowerCase(); | ||
| // Extract host from remote URL to avoid cross-instance false positives (GHE) | ||
| const sshHost = remoteUrl.match(/^[^@]+@([^:]+):/)?.[1]; | ||
| const httpsHost = (() => { try { return new URL(remoteUrl).hostname; } catch { return null; } })(); | ||
| const remoteHost = (sshHost || httpsHost || "").toLowerCase(); | ||
| const prHost = prMetadata.host.toLowerCase(); | ||
| isSameRepo = repoMatches && remoteHost === prHost; | ||
| } | ||
| } 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/<baseBranch> 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, | ||
| }); | ||
|
|
||
| worktreeCleanup = () => removeWorktree(gitRuntime, localPath, { force: true, cwd: repoDir }); | ||
| process.once("exit", () => { | ||
| try { Bun.spawnSync(["git", "worktree", "remove", "--force", localPath]); } 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; | ||
| const hostnameArgs = (host === "github.com" || host === "gitlab.com") ? [] : ["--hostname", 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, ...hostnameArgs, "--", "--depth=1", "--no-checkout"], | ||
| { stderr: "pipe" }, | ||
| ); | ||
|
Comment on lines
+326
to
+329
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] The [cli, "repo", "clone", prRepo, localPath, ...hostnameArgs, "--", "--depth=1", "--no-checkout"]A URL like Fix: move [cli, "repo", "clone", "--", prRepo, localPath, ...hostnameArgs, "--depth=1", "--no-checkout"]Or validate |
||
| if (cloneResult.exitCode !== 0) { | ||
| throw new Error(`${cli} repo clone failed: ${new TextDecoder().decode(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=50", "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()}`); | ||
| } | ||
|
|
||
| // Best-effort: create base refs so `git diff main...HEAD` and `git diff origin/main...HEAD` work | ||
| const baseFetch = Bun.spawnSync(["git", "fetch", "--depth=50", "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" }); | ||
|
|
||
| worktreeCleanup = () => { try { rmSync(localPath, { recursive: true, force: true }); } catch {} }; | ||
| process.once("exit", () => { | ||
| try { Bun.spawnSync(["rm", "-rf", localPath]); } catch {} | ||
| }); | ||
| } | ||
|
|
||
| // --local only provides a sandbox path for agent processes. | ||
| // Do NOT set gitContext — that would contaminate the diff pipeline. | ||
| agentCwd = localPath; | ||
|
|
||
| console.error(`Local checkout ready at ${localPath}`); | ||
| } catch (err) { | ||
| console.error(`Warning: --local failed, falling back to remote diff`); | ||
| console.error(err instanceof Error ? err.message : String(err)); | ||
| // Clean up partially-created directory (clone may have succeeded before fetch/checkout failed) | ||
| if (localPath) try { rmSync(localPath, { recursive: true, force: true }); } catch {} | ||
| agentCwd = undefined; | ||
| worktreeCleanup = undefined; | ||
| } | ||
| } | ||
| } else { | ||
| // --- Local Review Mode --- | ||
| gitContext = await getVcsContext(); | ||
|
|
@@ -249,12 +389,14 @@ if (args[0] === "sessions") { | |
| gitRef, | ||
| error: diffError, | ||
| origin: detectedOrigin, | ||
| diffType: isPRMode ? undefined : (initialDiffType ?? "uncommitted"), | ||
| diffType: gitContext ? (initialDiffType ?? "uncommitted") : undefined, | ||
| gitContext, | ||
| prMetadata, | ||
| agentCwd, | ||
| sharingEnabled, | ||
| shareBaseUrl, | ||
| htmlContent: reviewHtmlContent, | ||
| onCleanup: worktreeCleanup, | ||
| onReady: async (url, isRemote, port) => { | ||
| handleReviewServerReady(url, isRemote, port); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The explicit
process.on("exit")cleanup runsgit worktree removewithout settingcwdto the source repo. If the process cwd changes during runtime, cleanup may fail. Prefer reusingworktreeCleanup(which already suppliescwd) or pass{ cwd: repoDir }to the spawn call.