diff --git a/AGENTS.md b/AGENTS.md index 03307e1c..d33b9fc0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -152,7 +152,7 @@ User runs /plannotator-review command Claude Code: plannotator review subcommand runs OpenCode: event handler intercepts command ↓ -git diff captures unstaged changes +VCS diff captures local changes (git diff or jj diff) ↓ Review server starts, opens browser with diff viewer ↓ diff --git a/apps/hook/README.md b/apps/hook/README.md index 6f154ec1..1606da5d 100644 --- a/apps/hook/README.md +++ b/apps/hook/README.md @@ -110,7 +110,7 @@ The plugin registers three slash commands: | Command | Description | |---------|-------------| -| `/plannotator-review` | Open code review UI for uncommitted changes or a GitHub PR | +| `/plannotator-review [--git]` | Open code review UI for current changes or a GitHub PR; `--git` forces Git in JJ workspaces | | `/plannotator-annotate ` | Annotate any markdown file | | `/plannotator-last` | Annotate the agent's last message | diff --git a/apps/hook/commands/plannotator-review.md b/apps/hook/commands/plannotator-review.md index 6f3d544a..08709821 100644 --- a/apps/hook/commands/plannotator-review.md +++ b/apps/hook/commands/plannotator-review.md @@ -1,5 +1,5 @@ --- -description: Open interactive code review for current changes or a PR URL +description: Open interactive code review for current changes or a PR URL; pass --git to force Git in JJ workspaces allowed-tools: Bash(plannotator:*) disable-model-invocation: true --- diff --git a/apps/hook/server/cli.test.ts b/apps/hook/server/cli.test.ts index 3a5ad440..9da4a9e3 100644 --- a/apps/hook/server/cli.test.ts +++ b/apps/hook/server/cli.test.ts @@ -18,7 +18,7 @@ describe("CLI top-level help", () => { expect(output).toContain("plannotator --help"); expect(output).toContain("plannotator [--browser ]"); - expect(output).toContain("plannotator review [PR_URL]"); + expect(output).toContain("plannotator review [--git] [PR_URL]"); expect(output).toContain("plannotator annotate "); expect(output).toContain("running 'plannotator' without arguments is for hook integration"); }); diff --git a/apps/hook/server/cli.ts b/apps/hook/server/cli.ts index 06d8f5ac..67b06d86 100644 --- a/apps/hook/server/cli.ts +++ b/apps/hook/server/cli.ts @@ -14,7 +14,7 @@ export function formatTopLevelHelp(): string { "Usage:", " plannotator --help", " plannotator [--browser ]", - " plannotator review [PR_URL]", + " plannotator review [--git] [PR_URL]", " plannotator annotate [--no-jina] [--gate] [--json] [--hook]", " plannotator last", " plannotator archive", diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 5afcc0c6..8c184999 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -8,7 +8,7 @@ * - Reads hook event from stdin, extracts plan content * - Serves UI, returns approve/deny decision to stdout * - * 2. Code Review (`plannotator review`): + * 2. Code Review (`plannotator review`, `plannotator review --git`): * - Triggered by /review slash command * - Runs git diff, opens review UI * - Outputs feedback to stdout (captured by slash command) @@ -63,8 +63,9 @@ import { startAnnotateServer, handleAnnotateServerReady, } from "@plannotator/server/annotate"; -import { type DiffType, getVcsContext, runVcsDiff, gitRuntime } from "@plannotator/server/vcs"; +import { type DiffType, prepareLocalReviewDiff, gitRuntime } from "@plannotator/server/vcs"; import { loadConfig, resolveDefaultDiffType, resolveUseJina } from "@plannotator/shared/config"; +import { parseReviewArgs } from "@plannotator/shared/review-args"; 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"; @@ -289,22 +290,15 @@ 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; + const reviewArgs = parseReviewArgs(args.slice(1)); + const urlArg = reviewArgs.prUrl; + const isPRMode = urlArg !== undefined; + const useLocal = isPRMode && reviewArgs.useLocal; let rawPatch: string; let gitRef: string; let diffError: string | undefined; - let gitContext: Awaited> | undefined; + let gitContext: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; let initialDiffType: DiffType | undefined; let agentCwd: string | undefined; @@ -499,14 +493,16 @@ if (args[0] === "sessions") { } } else { // --- Local Review Mode --- - gitContext = await getVcsContext(); const config = loadConfig(); - initialDiffType = gitContext.vcsType === "p4" ? "p4-default" : resolveDefaultDiffType(config); - const diffResult = await runVcsDiff(initialDiffType, gitContext.defaultBranch, undefined, { + const diffResult = await prepareLocalReviewDiff({ + vcsType: reviewArgs.vcsType, + configuredDiffType: resolveDefaultDiffType(config), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); - rawPatch = diffResult.patch; - gitRef = diffResult.label; + gitContext = diffResult.gitContext; + initialDiffType = diffResult.diffType; + rawPatch = diffResult.rawPatch; + gitRef = diffResult.gitRef; diffError = diffResult.error; } diff --git a/apps/marketing/src/content/docs/guides/ai-code-review.md b/apps/marketing/src/content/docs/guides/ai-code-review.md index cdba907e..3ae20b96 100644 --- a/apps/marketing/src/content/docs/guides/ai-code-review.md +++ b/apps/marketing/src/content/docs/guides/ai-code-review.md @@ -189,7 +189,7 @@ claude -p \ --no-session-persistence \ --model sonnet \ --tools Agent,Bash,Read,Glob,Grep \ - --allowedTools Agent,Read,Glob,Grep,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh api repos/*/*/pulls/*),Bash(gh api repos/*/*/pulls/*/files*),Bash(gh api repos/*/*/pulls/*/comments*),Bash(gh api repos/*/*/issues/*/comments*),Bash(glab mr view:*),Bash(glab mr diff:*),Bash(glab mr list:*),Bash(glab api:*),Bash(git status:*),Bash(git diff:*),Bash(git log:*),Bash(git show:*),Bash(git blame:*),Bash(git branch:*),Bash(git grep:*),Bash(git ls-remote:*),Bash(git ls-tree:*),Bash(git merge-base:*),Bash(git remote:*),Bash(git rev-parse:*),Bash(git show-ref:*),Bash(wc:*) \ + --allowedTools Agent,Read,Glob,Grep,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr list:*),Bash(gh issue view:*),Bash(gh issue list:*),Bash(gh api repos/*/*/pulls/*),Bash(gh api repos/*/*/pulls/*/files*),Bash(gh api repos/*/*/pulls/*/comments*),Bash(gh api repos/*/*/issues/*/comments*),Bash(glab mr view:*),Bash(glab mr diff:*),Bash(glab mr list:*),Bash(glab api:*),Bash(git status:*),Bash(git diff:*),Bash(git log:*),Bash(git show:*),Bash(git blame:*),Bash(git branch:*),Bash(git grep:*),Bash(git ls-remote:*),Bash(git ls-tree:*),Bash(git merge-base:*),Bash(git remote:*),Bash(git rev-parse:*),Bash(git show-ref:*),Bash(jj status:*),Bash(jj diff:*),Bash(jj log:*),Bash(jj show:*),Bash(jj file show:*),Bash(jj cat:*),Bash(jj bookmark list:*),Bash(wc:*) \ --disallowedTools Edit,Write,NotebookEdit,WebFetch,WebSearch,Bash(python:*),Bash(python3:*),Bash(node:*),Bash(npx:*),Bash(bun:*),Bash(bunx:*),Bash(sh:*),Bash(bash:*),Bash(zsh:*),Bash(curl:*),Bash(wget:*) ``` diff --git a/apps/marketing/src/content/docs/reference/prompts.md b/apps/marketing/src/content/docs/reference/prompts.md index b5d479d2..fcebc7be 100644 --- a/apps/marketing/src/content/docs/reference/prompts.md +++ b/apps/marketing/src/content/docs/reference/prompts.md @@ -32,15 +32,15 @@ The user message Plannotator sends is always: **Review prompt** is a long, static review instruction that lives in the repo as a TypeScript constant. It's distinct per provider. -**User prompt** is a short, dynamic line built per call from the diff type (`uncommitted`, `staged`, `last-commit`, `branch`, PR URL, and so on). The same builder is used for all providers. +**User prompt** is a short, dynamic line built per call from the diff type (`uncommitted`, `staged`, `last-commit`, `branch`, `jj-current`, PR URL, and so on). Review agents share one builder; Code Tour uses a tour-specific builder with the same diff instructions. ## Matrix | | Claude review | Codex review | Code Tour (Claude or Codex) | |---|---|---|---| | **System prompt** | Owned by `claude` CLI. We don't touch it. | Owned by `codex` CLI. We don't touch it. | Same as whichever engine runs. | -| **Review prompt (static, ours)** | `CLAUDE_REVIEW_PROMPT` in `packages/server/claude-review.ts` | `CODEX_REVIEW_SYSTEM_PROMPT` in `packages/server/codex-review.ts` (misnamed; it's user content) | `TOUR_REVIEW_PROMPT` in `packages/server/tour-review.ts` | -| **User prompt (dynamic, ours)** | `buildCodexReviewUserMessage(patch, diffType, …)` | same function | same function | +| **Review prompt (static, ours)** | `CLAUDE_REVIEW_PROMPT` in `packages/server/claude-review.ts` | `CODEX_REVIEW_SYSTEM_PROMPT` in `packages/server/codex-review.ts` (misnamed; it's user content) | `TOUR_REVIEW_PROMPT` in `packages/server/tour/tour-review.ts` | +| **User prompt (dynamic, ours)** | `buildAgentReviewUserMessage(patch, diffType, …)` | same function | `buildTourUserMessage(patch, diffType, …)` | | **Full user message** | `review prompt + "\n\n---\n\n" + user prompt` | same | same | | **Delivered via** | stdin | last positional argv | stdin (Claude engine) or positional argv (Codex engine) | | **Output schema flag** | `--json-schema ` | `--output-schema ` | same as engine | diff --git a/apps/opencode-plugin/commands.ts b/apps/opencode-plugin/commands.ts index ae5a1963..1900885e 100644 --- a/apps/opencode-plugin/commands.ts +++ b/apps/opencode-plugin/commands.ts @@ -18,7 +18,7 @@ import { startAnnotateServer, handleAnnotateServerReady, } from "@plannotator/server/annotate"; -import { getGitContext, runGitDiffWithContext } from "@plannotator/server/git"; +import { type DiffType, prepareLocalReviewDiff } from "@plannotator/server/vcs"; import { parsePRUrl, checkPRAuth, fetchPR, getCliName, getMRLabel, getMRNumberLabel, getDisplayRepo } from "@plannotator/server/pr"; import { loadConfig, resolveDefaultDiffType, resolveUseJina } from "@plannotator/shared/config"; import { @@ -30,6 +30,7 @@ import { resolveMarkdownFile, resolveUserPath, hasMarkdownFiles } from "@plannot import { FILE_BROWSER_EXCLUDED } from "@plannotator/shared/reference-common"; import { htmlToMarkdown } from "@plannotator/shared/html-to-markdown"; import { parseAnnotateArgs } from "@plannotator/shared/annotate-args"; +import { parseReviewArgs } from "@plannotator/shared/review-args"; import { urlToMarkdown, isConvertedSource } from "@plannotator/shared/url-to-markdown"; import { statSync } from "fs"; import path from "path"; @@ -52,14 +53,15 @@ export async function handleReviewCommand( const { client, reviewHtmlContent, getSharingEnabled, getShareBaseUrl, directory } = deps; // @ts-ignore - Event properties contain arguments - const urlArg: string = event.properties?.arguments || ""; - const isPRMode = urlArg?.startsWith("http://") || urlArg?.startsWith("https://"); + const reviewArgs = parseReviewArgs(event.properties?.arguments || ""); + const urlArg = reviewArgs.prUrl; + const isPRMode = urlArg !== undefined; let rawPatch: string; let gitRef: string; let diffError: string | undefined; - let userDiffType: import("@plannotator/shared/config").DefaultDiffType | undefined; - let gitContext: Awaited> | undefined; + let userDiffType: DiffType | undefined; + let gitContext: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; if (isPRMode) { @@ -91,14 +93,17 @@ export async function handleReviewCommand( } else { client.app.log({ level: "info", message: "Opening code review UI..." }); - gitContext = await getGitContext(directory); const config = loadConfig(); - userDiffType = resolveDefaultDiffType(config); - const diffResult = await runGitDiffWithContext(userDiffType, gitContext, { + const diffResult = await prepareLocalReviewDiff({ + cwd: directory, + vcsType: reviewArgs.vcsType, + configuredDiffType: resolveDefaultDiffType(config), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); - rawPatch = diffResult.patch; - gitRef = diffResult.label; + gitContext = diffResult.gitContext; + userDiffType = diffResult.diffType; + rawPatch = diffResult.rawPatch; + gitRef = diffResult.gitRef; diffError = diffResult.error; } diff --git a/apps/opencode-plugin/commands/plannotator-review.md b/apps/opencode-plugin/commands/plannotator-review.md index bcbb55fc..172904be 100644 --- a/apps/opencode-plugin/commands/plannotator-review.md +++ b/apps/opencode-plugin/commands/plannotator-review.md @@ -1,5 +1,5 @@ --- -description: Open interactive code review for current changes or a PR URL +description: Open interactive code review for current changes or a PR URL; pass --git to force Git in JJ workspaces --- The Plannotator Code Review has been triggered. Opening the review UI... diff --git a/apps/pi-extension/index.ts b/apps/pi-extension/index.ts index 38a2f78c..952f505b 100644 --- a/apps/pi-extension/index.ts +++ b/apps/pi-extension/index.ts @@ -49,6 +49,7 @@ import { getAnnotateMessageFeedbackPrompt, } from "./generated/prompts.js"; import { parseAnnotateArgs } from "./generated/annotate-args.js"; +import { parseReviewArgs } from "./generated/review-args.js"; import { resolveAtReference } from "./generated/at-reference.js"; import { hasPlanBrowserHtml, @@ -404,7 +405,7 @@ export default function plannotator(pi: ExtensionAPI): void { }); pi.registerCommand("plannotator-review", { - description: "Open interactive code review for current changes or a PR URL", + description: "Open interactive code review for current changes or a PR URL; pass --git to force Git in JJ workspaces", handler: async (args, ctx) => { if (!hasReviewBrowserHtml()) { ctx.ui.notify( @@ -418,9 +419,13 @@ export default function plannotator(pi: ExtensionAPI): void { const origin = getPiSessionIdentity(ctx); try { - const prUrl = args?.trim() || undefined; - const isPRReview = prUrl?.startsWith("http://") || prUrl?.startsWith("https://"); - const session = await startCodeReviewBrowserSession(ctx, { prUrl }); + const reviewArgs = parseReviewArgs(args ?? ""); + const isPRReview = reviewArgs.prUrl !== undefined; + const session = await startCodeReviewBrowserSession(ctx, { + prUrl: reviewArgs.prUrl, + vcsType: reviewArgs.vcsType, + useLocal: reviewArgs.useLocal, + }); ctx.ui.notify("Code review opened. You can keep chatting while it runs.", "info"); void session .waitForDecision() diff --git a/apps/pi-extension/plannotator-browser.test.ts b/apps/pi-extension/plannotator-browser.test.ts new file mode 100644 index 00000000..25450df7 --- /dev/null +++ b/apps/pi-extension/plannotator-browser.test.ts @@ -0,0 +1,13 @@ +import { describe, expect, test } from "bun:test"; +import { shouldUseLocalPrCheckout } from "./plannotator-browser"; + +describe("shouldUseLocalPrCheckout", () => { + test("uses local PR checkout by default", () => { + expect(shouldUseLocalPrCheckout({})).toBe(true); + expect(shouldUseLocalPrCheckout({ useLocal: true })).toBe(true); + }); + + test("honors the Pi --no-local opt-out", () => { + expect(shouldUseLocalPrCheckout({ useLocal: false })).toBe(false); + }); +}); diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index 984f855d..2235bca6 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -6,13 +6,13 @@ import { createWorktreePool, type WorktreePool } from "./generated/worktree-pool import { fileURLToPath } from "node:url"; import type { ExtensionContext } from "@mariozechner/pi-coding-agent"; import { - getGitContext, + prepareLocalReviewDiff, reviewRuntime, - runGitDiff, startAnnotateServer, startPlanReviewServer, startReviewServer, type DiffType, + type VcsSelection, } from "./server.js"; import { openBrowser, isRemoteSession } from "./server/network.js"; import { parsePRUrl, checkPRAuth, fetchPR } from "./server/pr.js"; @@ -190,9 +190,13 @@ export async function openPlanReviewBrowser( return session.waitForDecision(); } +export function shouldUseLocalPrCheckout(options: { useLocal?: boolean }): boolean { + return options.useLocal !== false; +} + export async function openCodeReview( ctx: ExtensionContext, - options: { cwd?: string; defaultBranch?: string; diffType?: DiffType; prUrl?: string } = {}, + options: { cwd?: string; defaultBranch?: string; diffType?: DiffType; prUrl?: string; vcsType?: VcsSelection; useLocal?: boolean } = {}, ): Promise<{ approved: boolean; feedback?: string; annotations?: unknown[]; agentSwitch?: string; exit?: boolean }> { const session = await startCodeReviewBrowserSession(ctx, options); return session.waitForDecision(); @@ -200,7 +204,7 @@ export async function openCodeReview( export async function startCodeReviewBrowserSession( ctx: ExtensionContext, - options: { cwd?: string; defaultBranch?: string; diffType?: DiffType; prUrl?: string } = {}, + options: { cwd?: string; defaultBranch?: string; diffType?: DiffType; prUrl?: string; vcsType?: VcsSelection; useLocal?: boolean } = {}, ): Promise< BrowserDecisionSession<{ approved: boolean; @@ -220,7 +224,7 @@ export async function startCodeReviewBrowserSession( let rawPatch: string; let gitRef: string; let diffError: string | undefined; - let gitCtx: Awaited> | undefined; + let gitCtx: Awaited>["gitContext"] | undefined; let prMetadata: Awaited>["metadata"] | undefined; let diffType: DiffType | undefined; let agentCwd: string | undefined; @@ -260,152 +264,158 @@ export async function startCodeReviewBrowserSession( gitRef = `${getMRLabel(prRef)} ${getMRNumberLabel(prRef)}`; prMetadata = pr.metadata; - // Create local worktree for agent file access (--local is the default for PR reviews) - let localPath: string | undefined; - let sessionDir: string | undefined; - try { - const repoDir = options.cwd ?? ctx.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); - const prNumber = prMetadata.platform === "github" ? prMetadata.number : prMetadata.iid; - sessionDir = join(realpathSync(tmpdir()), `plannotator-pr-${identifier}-${suffix}`); - localPath = join(sessionDir, "pool", `pr-${prNumber}`); - 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; + if (shouldUseLocalPrCheckout(options)) { + // Create local worktree for agent file access (--local is the default for PR reviews) + let localPath: string | undefined; + let sessionDir: string | undefined; try { - const remoteResult = await reviewRuntime.runGit(["remote", "get-url", "origin"], { cwd: repoDir }); - if (remoteResult.exitCode === 0) { - const remoteUrl = remoteResult.stdout.trim(); - const currentRepo = parseRemoteUrl(remoteUrl); + const repoDir = options.cwd ?? ctx.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); + const prNumber = prMetadata.platform === "github" ? prMetadata.number : prMetadata.iid; + sessionDir = join(realpathSync(tmpdir()), `plannotator-pr-${identifier}-${suffix}`); + localPath = join(sessionDir, "pool", `pr-${prNumber}`); + 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 reviewRuntime.runGit(["remote", "get-url", "origin"], { cwd: repoDir }); + 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(); + 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..."); + await fetchRef(reviewRuntime, prMetadata.baseBranch, { cwd: repoDir }); + await ensureObjectAvailable(reviewRuntime, prMetadata.baseSha, { cwd: repoDir }); + await fetchRef(reviewRuntime, fetchRefStr, { cwd: repoDir }); + + await createWorktree(reviewRuntime, { + ref: "FETCH_HEAD", + path: localPath, + detach: true, + cwd: repoDir, + }); + + const wtRepoDir = repoDir; + exitHandler = () => { + try { + for (const entry of worktreePool?.entries() ?? []) { + spawnSync("git", ["worktree", "remove", "--force", entry.path], { cwd: wtRepoDir }); + } + } catch {} + if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} + }; + worktreeCleanup = async () => { + if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; } + if (worktreePool) await worktreePool.cleanup(reviewRuntime); + if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} + }; + process.once("exit", exitHandler); + } else { + // ── Cross-repo: shallow clone + fetch PR head ── const prRepo = prMetadata.platform === "github" ? `${prMetadata.owner}/${prMetadata.repo}` : prMetadata.projectPath; - const repoMatches = !!currentRepo && currentRepo.toLowerCase() === prRepo.toLowerCase(); - 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 (/^-/.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 }), + }; - if (isSameRepo) { - // ── Same-repo: fast worktree path ── - console.error("Fetching PR branch and creating local worktree..."); - await fetchRef(reviewRuntime, prMetadata.baseBranch, { cwd: repoDir }); - await ensureObjectAvailable(reviewRuntime, prMetadata.baseSha, { cwd: repoDir }); - await fetchRef(reviewRuntime, fetchRefStr, { cwd: repoDir }); + console.error(`Cloning ${prRepo} (shallow)...`); + const cloneResult = spawnSync(cli, ["repo", "clone", prRepo, localPath, "--", "--depth=1", "--no-checkout"], { encoding: "utf-8", env: cloneEnv }); + if ((cloneResult.status ?? 1) !== 0) { + throw new Error(`${cli} repo clone failed: ${(cloneResult.stderr ?? "").trim()}`); + } - await createWorktree(reviewRuntime, { - ref: "FETCH_HEAD", - path: localPath, - detach: true, - cwd: repoDir, - }); + console.error("Fetching PR branch..."); + const fetchResult = await reviewRuntime.runGit(["fetch", "--depth=200", "origin", fetchRefStr], { cwd: localPath }); + if (fetchResult.exitCode !== 0) throw new Error(`Failed to fetch PR head ref: ${fetchResult.stderr.trim()}`); - const wtRepoDir = repoDir; - exitHandler = () => { - try { - for (const entry of worktreePool?.entries() ?? []) { - spawnSync("git", ["worktree", "remove", "--force", entry.path], { cwd: wtRepoDir }); - } - } catch {} - if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} - }; - worktreeCleanup = async () => { - if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; } - if (worktreePool) await worktreePool.cleanup(reviewRuntime); - if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} - }; - process.once("exit", exitHandler); - } else { - // ── Cross-repo: shallow clone + fetch PR head ── - const prRepo = prMetadata.platform === "github" - ? `${prMetadata.owner}/${prMetadata.repo}` - : prMetadata.projectPath; - 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 }), - }; + const checkoutResult = await reviewRuntime.runGit(["checkout", "FETCH_HEAD"], { cwd: localPath }); + if (checkoutResult.exitCode !== 0) { + throw new Error(`git checkout FETCH_HEAD failed: ${checkoutResult.stderr.trim()}`); + } - console.error(`Cloning ${prRepo} (shallow)...`); - const cloneResult = spawnSync(cli, ["repo", "clone", prRepo, localPath, "--", "--depth=1", "--no-checkout"], { encoding: "utf-8", env: cloneEnv }); - if ((cloneResult.status ?? 1) !== 0) { - throw new Error(`${cli} repo clone failed: ${(cloneResult.stderr ?? "").trim()}`); + // Best-effort: create base refs so agent diffs work + const baseFetch = await reviewRuntime.runGit(["fetch", "--depth=200", "origin", prMetadata.baseSha], { cwd: localPath }); + if (baseFetch.exitCode !== 0) console.error("Warning: failed to fetch baseSha, agent diffs may be inaccurate"); + await reviewRuntime.runGit(["branch", "--", prMetadata.baseBranch, prMetadata.baseSha], { cwd: localPath }); + await reviewRuntime.runGit(["update-ref", `refs/remotes/origin/${prMetadata.baseBranch}`, prMetadata.baseSha], { cwd: localPath }); + + exitHandler = () => { + if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} + }; + worktreeCleanup = () => { + if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; } + if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} + }; + process.once("exit", exitHandler); } - console.error("Fetching PR branch..."); - const fetchResult = await reviewRuntime.runGit(["fetch", "--depth=200", "origin", fetchRefStr], { cwd: localPath }); - if (fetchResult.exitCode !== 0) throw new Error(`Failed to fetch PR head ref: ${fetchResult.stderr.trim()}`); - - const checkoutResult = await reviewRuntime.runGit(["checkout", "FETCH_HEAD"], { cwd: localPath }); - if (checkoutResult.exitCode !== 0) { - throw new Error(`git checkout FETCH_HEAD failed: ${checkoutResult.stderr.trim()}`); - } - - // Best-effort: create base refs so agent diffs work - const baseFetch = await reviewRuntime.runGit(["fetch", "--depth=200", "origin", prMetadata.baseSha], { cwd: localPath }); - if (baseFetch.exitCode !== 0) console.error("Warning: failed to fetch baseSha, agent diffs may be inaccurate"); - await reviewRuntime.runGit(["branch", "--", prMetadata.baseBranch, prMetadata.baseSha], { cwd: localPath }); - await reviewRuntime.runGit(["update-ref", `refs/remotes/origin/${prMetadata.baseBranch}`, prMetadata.baseSha], { cwd: localPath }); - - exitHandler = () => { - if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} - }; - worktreeCleanup = () => { - if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; } - if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} - }; - process.once("exit", exitHandler); + agentCwd = localPath; + worktreePool = createWorktreePool( + { sessionDir: sessionDir!, repoDir, isSameRepo }, + { path: localPath, prUrl: prMetadata.url, number: prNumber, ready: true }, + ); + console.error(`Local checkout ready at ${localPath}`); + } catch (err) { + console.error("Warning: local worktree creation failed, falling back to remote diff"); + console.error(err instanceof Error ? err.message : String(err)); + if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; } + if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} + agentCwd = undefined; + worktreePool = undefined; + worktreeCleanup = undefined; } - - agentCwd = localPath; - worktreePool = createWorktreePool( - { sessionDir: sessionDir!, repoDir, isSameRepo }, - { path: localPath, prUrl: prMetadata.url, number: prNumber, ready: true }, - ); - console.error(`Local checkout ready at ${localPath}`); - } catch (err) { - console.error("Warning: local worktree creation failed, falling back to remote diff"); - console.error(err instanceof Error ? err.message : String(err)); - if (exitHandler) { process.removeListener("exit", exitHandler); exitHandler = undefined; } - if (sessionDir) try { rmSync(sessionDir, { recursive: true, force: true }); } catch {} - agentCwd = undefined; - worktreePool = undefined; - worktreeCleanup = undefined; } } else { // --- Local Review Mode --- const cwd = options.cwd ?? ctx.cwd; - gitCtx = await getGitContext(cwd); - const defaultBranch = options.defaultBranch ?? gitCtx.defaultBranch; const config = loadConfig(); - diffType = options.diffType ?? resolveDefaultDiffType(config); - const result = await runGitDiff(diffType, defaultBranch, cwd, { + const result = await prepareLocalReviewDiff({ + cwd, + vcsType: options.vcsType, + requestedDiffType: options.diffType, + requestedBase: options.defaultBranch, + configuredDiffType: resolveDefaultDiffType(config), hideWhitespace: config.diffOptions?.hideWhitespace ?? false, }); - rawPatch = result.patch; - gitRef = result.label; + gitCtx = result.gitContext; + diffType = result.diffType; + rawPatch = result.rawPatch; + gitRef = result.gitRef; diffError = result.error; // Remember which base the initial diff was computed against so it can // be forwarded to the server below. Only matters when the caller // overrode the detected default; otherwise it matches gitCtx already. - initialBase = defaultBranch; + initialBase = result.base; } const server = await startReviewServer({ diff --git a/apps/pi-extension/plannotator-events.ts b/apps/pi-extension/plannotator-events.ts index cfa6ddf6..12845ae7 100644 --- a/apps/pi-extension/plannotator-events.ts +++ b/apps/pi-extension/plannotator-events.ts @@ -2,7 +2,7 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; import type { ExtensionAPI, ExtensionContext } from "@mariozechner/pi-coding-agent"; -import type { DiffType } from "./server.js"; +import type { DiffType, VcsSelection } from "./server.js"; import { getLastAssistantMessageText, getStartupErrorMessage, @@ -87,6 +87,8 @@ export type PlannotatorReviewStatusResult = export interface PlannotatorCodeReviewPayload { diffType?: DiffType; defaultBranch?: string; + vcsType?: VcsSelection; + useLocal?: boolean; cwd?: string; prUrl?: string; } @@ -264,6 +266,8 @@ export function registerPlannotatorEventListeners(pi: ExtensionAPI): void { cwd: request.payload?.cwd, defaultBranch: request.payload?.defaultBranch, diffType: request.payload?.diffType, + vcsType: request.payload?.vcsType, + useLocal: request.payload?.useLocal, prUrl: request.payload?.prUrl, }); request.respond({ status: "handled", result }); diff --git a/apps/pi-extension/server.test.ts b/apps/pi-extension/server.test.ts index 34eb7949..bbfafaff 100644 --- a/apps/pi-extension/server.test.ts +++ b/apps/pi-extension/server.test.ts @@ -4,11 +4,18 @@ import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { createServer as createNetServer } from "node:net"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { getGitContext, runGitDiff, startReviewServer } from "./server"; +import { + getGitContext, + getVcsContext, + prepareLocalReviewDiff, + runGitDiff, + startReviewServer, +} from "./server"; const tempDirs: string[] = []; const originalCwd = process.cwd(); const originalHome = process.env.HOME; +const originalXdgConfigHome = process.env.XDG_CONFIG_HOME; const originalPort = process.env.PLANNOTATOR_PORT; function makeTempDir(prefix: string): string { @@ -17,14 +24,30 @@ function makeTempDir(prefix: string): string { return dir; } +function childEnv(): NodeJS.ProcessEnv { + return { ...process.env }; +} + function git(cwd: string, args: string[]): string { - const result = spawnSync("git", args, { cwd, encoding: "utf-8" }); + const result = spawnSync("git", args, { cwd, encoding: "utf-8", env: childEnv() }); if (result.status !== 0) { throw new Error(result.stderr || `git ${args.join(" ")} failed`); } return result.stdout.trim(); } +function hasJj(): boolean { + return spawnSync("jj", ["--version"], { encoding: "utf-8", env: childEnv() }).status === 0; +} + +function jj(cwd: string, args: string[]): string { + const result = spawnSync("jj", ["-R", cwd, ...args], { encoding: "utf-8", env: childEnv() }); + if (result.status !== 0) { + throw new Error(result.stderr || `jj ${args.join(" ")} failed`); + } + return result.stdout.trim(); +} + function initRepo(): string { const repoDir = makeTempDir("plannotator-pi-review-"); git(repoDir, ["init"]); @@ -39,6 +62,28 @@ function initRepo(): string { return repoDir; } +function initJjRepo(): string { + const repoDir = initRepo(); + writeFileSync(join(repoDir, "spacey.ts"), "const x = 1;\n", "utf-8"); + git(repoDir, ["add", "spacey.ts"]); + git(repoDir, ["commit", "-m", "add spacey file"]); + + const init = spawnSync("jj", ["git", "init", "--colocate", repoDir], { encoding: "utf-8", env: childEnv() }); + if (init.status !== 0) { + throw new Error(init.stderr || "jj git init --colocate failed"); + } + jj(repoDir, ["config", "set", "--repo", "user.name", "Pi Review"]); + jj(repoDir, ["config", "set", "--repo", "user.email", "pi-review@example.com"]); + + writeFileSync(join(repoDir, "last.txt"), "last\n", "utf-8"); + jj(repoDir, ["commit", "-m", "add last change"]); + + writeFileSync(join(repoDir, "tracked.txt"), "after\n", "utf-8"); + writeFileSync(join(repoDir, "spacey.ts"), "const x = 1;\n", "utf-8"); + + return repoDir; +} + function reservePort(): Promise { return new Promise((resolve, reject) => { const server = createNetServer(); @@ -70,6 +115,11 @@ afterEach(() => { } else { process.env.HOME = originalHome; } + if (originalXdgConfigHome === undefined) { + delete process.env.XDG_CONFIG_HOME; + } else { + process.env.XDG_CONFIG_HOME = originalXdgConfigHome; + } if (originalPort === undefined) { delete process.env.PLANNOTATOR_PORT; } else { @@ -82,6 +132,8 @@ afterEach(() => { }); describe("pi review server", () => { + const testIfJj = hasJj() ? test : test.skip; + test("serves review diff parity endpoints including drafts, uploads, and editor annotations", async () => { const homeDir = makeTempDir("plannotator-pi-home-"); const repoDir = initRepo(); @@ -357,6 +409,14 @@ describe("pi review server", () => { expect(switched.base).toBe("develop"); expect(switched.diffType).toBe("branch"); + const stageWhileOnBranch = await fetch(`${server.url}/api/git-add`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath: "feature-file.txt" }), + }); + expect(stageWhileOnBranch.status).toBe(400); + expect(await stageWhileOnBranch.json()).toEqual({ error: "Staging not available" }); + // Subsequent /api/diff load reflects the switched base — this is what // survives a page refresh / reconnect. const rehydrate = await fetch(`${server.url}/api/diff`).then((r) => r.json()) as { @@ -440,4 +500,152 @@ describe("pi review server", () => { server.stop(); } }, 15_000); + + testIfJj("supports JJ local review modes through the Pi server", async () => { + const homeDir = makeTempDir("plannotator-pi-home-"); + process.env.HOME = homeDir; + process.env.XDG_CONFIG_HOME = join(homeDir, ".config"); + const repoDir = initJjRepo(); + process.chdir(repoDir); + process.env.PLANNOTATOR_PORT = String(await reservePort()); + + const vcsContext = await getVcsContext(repoDir); + expect(vcsContext.vcsType).toBe("jj"); + const prepared = await prepareLocalReviewDiff({ + cwd: repoDir, + requestedDiffType: "merge-base", + requestedBase: "main", + configuredDiffType: "unstaged", + }); + expect(prepared.gitContext.vcsType).toBe("jj"); + expect(prepared.diffType).toBe("jj-current"); + expect(prepared.base).toBe("main@git"); + + const forcedGit = await prepareLocalReviewDiff({ + cwd: repoDir, + vcsType: "git", + requestedDiffType: "unstaged", + configuredDiffType: "unstaged", + }); + expect(forcedGit.gitContext.vcsType).toBe("git"); + expect(forcedGit.diffType).toBe("unstaged"); + expect(forcedGit.rawPatch).toContain("tracked.txt"); + + const forcedGitServer = await startReviewServer({ + rawPatch: forcedGit.rawPatch, + gitRef: forcedGit.gitRef, + error: forcedGit.error, + diffType: forcedGit.diffType, + gitContext: forcedGit.gitContext, + initialBase: forcedGit.base, + origin: "pi", + htmlContent: "review", + }); + try { + const switchResponse = await fetch(`${forcedGitServer.url}/api/diff/switch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ diffType: "merge-base", base: "main" }), + }); + expect(switchResponse.status).toBe(200); + const switched = await switchResponse.json() as { + gitContext?: { vcsType?: string; diffOptions: Array<{ id: string }> }; + }; + expect(switched.gitContext?.vcsType).toBe("git"); + expect(switched.gitContext?.diffOptions.map((option) => option.id)).toContain("merge-base"); + expect(switched.gitContext?.diffOptions.map((option) => option.id)).not.toContain("jj-current"); + } finally { + forcedGitServer.stop(); + } + + process.env.PLANNOTATOR_PORT = String(await reservePort()); + const server = await startReviewServer({ + rawPatch: prepared.rawPatch, + gitRef: prepared.gitRef, + error: prepared.error, + diffType: prepared.diffType, + gitContext: prepared.gitContext, + initialBase: prepared.base, + origin: "pi", + htmlContent: "review", + }); + + try { + const initial = await fetch(`${server.url}/api/diff`).then((r) => r.json()) as { + diffType: string; + rawPatch: string; + base?: string; + gitContext?: { vcsType?: string; diffOptions: Array<{ id: string }> }; + }; + expect(initial.diffType).toBe("jj-current"); + expect(initial.base).toBe("main@git"); + expect(initial.gitContext?.vcsType).toBe("jj"); + expect(initial.gitContext?.diffOptions.map((option) => option.id)).toEqual([ + "jj-current", + "jj-last", + "jj-line", + "jj-all", + ]); + expect(initial.rawPatch).toContain("tracked.txt"); + expect(initial.rawPatch).toContain("+after"); + + const lastResponse = await fetch(`${server.url}/api/diff/switch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ diffType: "jj-last" }), + }); + expect(lastResponse.status).toBe(200); + const last = await lastResponse.json() as { rawPatch: string; diffType: string }; + expect(last.diffType).toBe("jj-last"); + expect(last.rawPatch).toContain("last.txt"); + + for (const nextType of ["jj-line", "jj-all"] as const) { + const response = await fetch(`${server.url}/api/diff/switch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ diffType: nextType }), + }); + expect(response.status).toBe(200); + const payload = await response.json() as { diffType: string; rawPatch: string }; + expect(payload.diffType).toBe(nextType); + expect(payload.rawPatch).toContain("tracked.txt"); + } + + const hideWhitespaceResponse = await fetch(`${server.url}/api/diff/switch`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ diffType: "jj-current", hideWhitespace: true }), + }); + expect(hideWhitespaceResponse.status).toBe(200); + const hidden = await hideWhitespaceResponse.json() as { rawPatch: string }; + expect(hidden.rawPatch).toContain("+after"); + expect(hidden.rawPatch).not.toContain("+const x = 1;"); + + const fileContentResponse = await fetch(`${server.url}/api/file-content?path=tracked.txt`); + expect(fileContentResponse.status).toBe(200); + const fileContent = await fileContentResponse.json() as { + oldContent: string | null; + newContent: string | null; + }; + expect(fileContent.oldContent).toBe("before\n"); + expect(fileContent.newContent).toBe("after\n"); + + const stageResponse = await fetch(`${server.url}/api/git-add`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ filePath: "tracked.txt" }), + }); + expect(stageResponse.status).toBe(400); + expect(await stageResponse.json()).toEqual({ error: "Staging not available" }); + + await fetch(`${server.url}/api/feedback`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ approved: true, feedback: "LGTM", annotations: [] }), + }); + await server.waitForDecision(); + } finally { + server.stop(); + } + }, 20_000); }); diff --git a/apps/pi-extension/server.ts b/apps/pi-extension/server.ts index 6682ace7..bbdc3c83 100644 --- a/apps/pi-extension/server.ts +++ b/apps/pi-extension/server.ts @@ -11,6 +11,7 @@ export type { DiffType, GitContext, } from "./generated/review-core.js"; +export type { VcsSelection } from "./server/vcs.js"; export { type AnnotateServerResult, startAnnotateServer, @@ -20,9 +21,22 @@ export { startPlanReviewServer, } from "./server/serverPlan.js"; export { - getGitContext, - reviewRuntime, type ReviewServerResult, - runGitDiff, startReviewServer, } from "./server/serverReview.js"; +export { + canStageFiles, + detectRemoteDefaultCompareTarget, + detectVcs, + getGitContext, + getVcsContext, + getVcsFileContentsForDiff, + prepareLocalReviewDiff, + resolveInitialDiffType, + resolveVcsCwd, + reviewRuntime, + runGitDiff, + runVcsDiff, + stageFile, + unstageFile, +} from "./server/vcs.js"; diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index 8a6391b0..85cda417 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -1,4 +1,4 @@ -import { execSync, spawn } from "node:child_process"; +import { execSync } from "node:child_process"; import { readFileSync, existsSync } from "node:fs"; import { createServer } from "node:http"; import os from "node:os"; @@ -25,18 +25,10 @@ import { } from "../generated/pr-provider.js"; import { type DiffType, - type GitCommandResult, type GitContext, - type GitDiffOptions, - detectRemoteDefaultBranch, getFileContentsForDiff as getFileContentsForDiffCore, - getGitContext as getGitContextCore, - gitAddFile as gitAddFileCore, - gitResetFile as gitResetFileCore, parseWorktreeDiffType, resolveBaseBranch, - type ReviewGitRuntime, - runGitDiff as runGitDiffCore, validateFilePath, } from "../generated/review-core.js"; import { @@ -79,12 +71,12 @@ import { import { getRepoInfo } from "./project.js"; import { CODEX_REVIEW_SYSTEM_PROMPT, - buildCodexReviewUserMessage, buildCodexCommand, generateOutputPath, parseCodexOutput, transformReviewFindings, } from "../generated/codex-review.js"; +import { buildAgentReviewUserMessage } from "../generated/agent-review-message.js"; import { CLAUDE_REVIEW_PROMPT, buildClaudeCommand, @@ -92,6 +84,17 @@ import { transformClaudeFindings, } from "../generated/claude-review.js"; import { createTourSession, TOUR_EMPTY_OUTPUT_ERROR } from "../generated/tour-review.js"; +import { + canStageFiles, + detectRemoteDefaultCompareTarget, + getVcsContext, + getVcsFileContentsForDiff, + resolveVcsCwd, + reviewRuntime, + runVcsDiff, + stageFile, + unstageFile, +} from "./vcs.js"; /** Detect if running inside WSL (Windows Subsystem for Linux) */ function detectWSL(): boolean { @@ -121,65 +124,6 @@ export interface ReviewServerResult { stop: () => void; } -export const reviewRuntime: ReviewGitRuntime = { - runGit( - args: string[], - options?: { cwd?: string; timeoutMs?: number }, - ): Promise { - return new Promise((resolve) => { - const proc = spawn("git", ["-c", "core.quotePath=false", ...args], { - cwd: options?.cwd, - stdio: ["ignore", "pipe", "pipe"], - }); - - let timer: ReturnType | undefined; - if (options?.timeoutMs) { - timer = setTimeout(() => proc.kill(), options.timeoutMs); - } - - const stdoutChunks: Buffer[] = []; - const stderrChunks: Buffer[] = []; - proc.stdout!.on("data", (chunk: Buffer) => stdoutChunks.push(chunk)); - proc.stderr!.on("data", (chunk: Buffer) => stderrChunks.push(chunk)); - - proc.on("close", (code) => { - if (timer) clearTimeout(timer); - resolve({ - stdout: Buffer.concat(stdoutChunks).toString("utf-8"), - stderr: Buffer.concat(stderrChunks).toString("utf-8"), - exitCode: code ?? 1, - }); - }); - - proc.on("error", () => { - if (timer) clearTimeout(timer); - resolve({ stdout: "", stderr: "git not found", exitCode: 1 }); - }); - }); - }, - - async readTextFile(path: string): Promise { - try { - return readFileSync(path, "utf-8"); - } catch { - return null; - } - }, -}; - -export function getGitContext(cwd?: string): Promise { - return getGitContextCore(reviewRuntime, cwd); -} - -export function runGitDiff( - diffType: DiffType, - defaultBranch = "main", - cwd?: string, - options?: GitDiffOptions, -): Promise<{ patch: string; label: string; error?: string }> { - return runGitDiffCore(reviewRuntime, diffType, defaultBranch, cwd, options); -} - export async function startReviewServer(options: { rawPatch: string; gitRef: string; @@ -214,6 +158,7 @@ export async function startReviewServer(options: { let prMeta = options.prMetadata; const isPRMode = !!prMeta; const hasLocalAccess = !!options.gitContext; + const sessionVcsType = options.gitContext?.vcsType; const isRemote = isRemoteSession(); const wslFlag = detectWSL(); let prRef = prMeta ? prRefFromMetadata(prMeta) : null; @@ -280,12 +225,14 @@ export async function startReviewServer(options: { // read this (not gitContext.defaultBranch) so they analyze the same diff // the reviewer is currently looking at. Honors an explicit initialBase from // the caller — e.g. programmatic Pi callers can request a non-detected base. - let currentBase = options.initialBase || options.gitContext?.defaultBranch || "main"; + const detectedCompareTarget = (): string => + options.gitContext?.defaultBranch || options.gitContext?.compareTarget?.fallback || "main"; + let currentBase = options.initialBase || detectedCompareTarget(); let baseEverSwitched = false; // Fire-and-forget: query the remote for its actual default branch. if (options.gitContext && !options.initialBase && !isPRMode) { - detectRemoteDefaultBranch(reviewRuntime, options.gitContext.cwd).then((remote) => { + detectRemoteDefaultCompareTarget(options.gitContext.cwd, sessionVcsType).then((remote) => { if (remote && !baseEverSwitched) currentBase = remote; }); } @@ -298,11 +245,7 @@ export async function startReviewServer(options: { if (poolPath) return poolPath; } if (options.agentCwd) return options.agentCwd; - if (currentDiffType.startsWith("worktree:")) { - const parsed = parseWorktreeDiffType(currentDiffType); - if (parsed) return parsed.path; - } - return options.gitContext?.cwd ?? process.cwd(); + return resolveVcsCwd(currentDiffType, options.gitContext?.cwd) ?? process.cwd(); } const tour = createTourSession(); @@ -344,7 +287,7 @@ export async function startReviewServer(options: { return built ? { ...built, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext } : built; } - const userMessage = buildCodexReviewUserMessage(currentPatch, currentDiffType, userMessageOptions, prMeta); + const userMessage = buildAgentReviewUserMessage(currentPatch, currentDiffType, userMessageOptions, prMeta); if (provider === "codex") { const model = typeof config?.model === "string" && config.model ? config.model : undefined; @@ -644,13 +587,13 @@ export async function startReviewServer(options: { if (typeof body.hideWhitespace === "boolean") { currentHideWhitespace = body.hideWhitespace; } - const detectedBase = options.gitContext?.defaultBranch || "main"; + const detectedBase = detectedCompareTarget(); const base = resolveBaseBranch( typeof body.base === "string" ? body.base : undefined, detectedBase, ); const defaultCwd = options.gitContext?.cwd; - const result = await runGitDiff(newType, base, defaultCwd, { + const result = await runVcsDiff(newType, base, defaultCwd, { hideWhitespace: currentHideWhitespace, }); currentPatch = result.patch; @@ -666,9 +609,8 @@ export async function startReviewServer(options: { let updatedContext: GitContext | undefined; if (options.gitContext) { try { - const worktreeParsed = parseWorktreeDiffType(newType); - const effectiveCwd = worktreeParsed?.path ?? options.gitContext.cwd; - updatedContext = await getGitContextCore(reviewRuntime, effectiveCwd); + const effectiveCwd = resolveVcsCwd(newType, options.gitContext.cwd); + updatedContext = await getVcsContext(effectiveCwd, sessionVcsType); } catch { /* best-effort */ } @@ -981,14 +923,13 @@ export async function startReviewServer(options: { // Local mode first (matches Bun server priority) if (hasLocalAccess && !isPRMode) { - const detectedBase = options.gitContext?.defaultBranch || "main"; + const detectedBase = detectedCompareTarget(); const base = resolveBaseBranch( url.searchParams.get("base") ?? undefined, detectedBase, ); const defaultCwd = options.gitContext?.cwd; - const result = await getFileContentsForDiffCore( - reviewRuntime, + const result = await getVcsFileContentsForDiff( currentDiffType, base, filePath, @@ -1043,13 +984,8 @@ export async function startReviewServer(options: { } else if (url.pathname === "/api/agents" && req.method === "GET") { json(res, { agents: [] }); } else if (url.pathname === "/api/git-add" && req.method === "POST") { - // Staging only available for local diff types that support it (not PR mode, not branch diffs). - // Worktree diff types use composite format "worktree:/path:uncommitted" — extract the base type. - const baseDiffType = currentDiffType.startsWith("worktree:") - ? (parseWorktreeDiffType(currentDiffType)?.subType ?? currentDiffType) - : currentDiffType; - const canStage = baseDiffType === "uncommitted" || baseDiffType === "unstaged"; - if (isPRMode || !canStage) { + const stageCwd = resolveVcsCwd(currentDiffType, options.gitContext?.cwd); + if (isPRMode || !(await canStageFiles(currentDiffType, stageCwd))) { json(res, { error: "Staging not available" }, 400); return; } @@ -1060,18 +996,10 @@ export async function startReviewServer(options: { json(res, { error: "Missing filePath" }, 400); return; } - let cwd: string | undefined; - if (currentDiffType.startsWith("worktree:")) { - const parsed = parseWorktreeDiffType(currentDiffType); - if (parsed) cwd = parsed.path; - } - if (!cwd) { - cwd = options.gitContext?.cwd; - } if (body.undo) { - await gitResetFileCore(reviewRuntime, filePath, cwd); + await unstageFile(currentDiffType, filePath, stageCwd); } else { - await gitAddFileCore(reviewRuntime, filePath, cwd); + await stageFile(currentDiffType, filePath, stageCwd); } json(res, { ok: true }); } catch (err) { diff --git a/apps/pi-extension/server/vcs.ts b/apps/pi-extension/server/vcs.ts new file mode 100644 index 00000000..628b72e3 --- /dev/null +++ b/apps/pi-extension/server/vcs.ts @@ -0,0 +1,120 @@ +import { spawn } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { + type DiffResult, + type DiffType, + type GitCommandResult, + type GitContext, + type GitDiffOptions, + type ReviewGitRuntime, + getGitContext as getGitContextCore, + runGitDiff as runGitDiffCore, +} from "../generated/review-core.js"; +import { + type ReviewJjRuntime, +} from "../generated/jj-core.js"; +import { + type VcsSelection, + createGitProvider, + createJjProvider, + createVcsApi, + resolveInitialDiffType, +} from "../generated/vcs-core.js"; + +function runCommand( + command: string, + args: string[], + notFoundMessage: string, + options?: { cwd?: string; timeoutMs?: number }, +): Promise { + return new Promise((resolve) => { + const proc = spawn(command, args, { + cwd: options?.cwd, + stdio: ["ignore", "pipe", "pipe"], + }); + + let timer: ReturnType | undefined; + if (options?.timeoutMs) { + timer = setTimeout(() => proc.kill(), options.timeoutMs); + } + + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + proc.stdout!.on("data", (chunk: Buffer) => stdoutChunks.push(chunk)); + proc.stderr!.on("data", (chunk: Buffer) => stderrChunks.push(chunk)); + + proc.on("close", (code) => { + if (timer) clearTimeout(timer); + resolve({ + stdout: Buffer.concat(stdoutChunks).toString("utf-8"), + stderr: Buffer.concat(stderrChunks).toString("utf-8"), + exitCode: code ?? 1, + }); + }); + + proc.on("error", () => { + if (timer) clearTimeout(timer); + resolve({ stdout: "", stderr: notFoundMessage, exitCode: 1 }); + }); + }); +} + +export const reviewRuntime: ReviewGitRuntime = { + runGit( + args: string[], + options?: { cwd?: string; timeoutMs?: number }, + ): Promise { + return runCommand("git", ["-c", "core.quotePath=false", ...args], "git not found", options); + }, + + async readTextFile(path: string): Promise { + try { + return readFileSync(path, "utf-8"); + } catch { + return null; + } + }, +}; + +export const jjRuntime: ReviewJjRuntime = { + runJj( + args: string[], + options?: { cwd?: string; timeoutMs?: number }, + ): Promise { + return runCommand("jj", args, "jj not found", options); + }, +}; + +const api = createVcsApi([ + createJjProvider(jjRuntime), + createGitProvider(reviewRuntime), +]); + +export const { + detectVcs, + getVcsContext, + detectRemoteDefaultCompareTarget, + prepareLocalReviewDiff, + runVcsDiff, + getVcsFileContentsForDiff, + canStageFiles, + stageFile, + unstageFile, + resolveVcsCwd, +} = api; + +export { resolveInitialDiffType }; +export type { VcsSelection }; + +export function getGitContext(cwd?: string): Promise { + return getGitContextCore(reviewRuntime, cwd); +} + +export function runGitDiff( + diffType: DiffType, + defaultBranch = "main", + cwd?: string, + options?: GitDiffOptions, +): Promise { + return runGitDiffCore(reviewRuntime, diffType, defaultBranch, cwd, options); +} diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index c5f3f8cd..bd31dc76 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -6,13 +6,13 @@ cd "$(dirname "$0")" mkdir -p generated generated/ai/providers -for f in feedback-templates prompts review-core storage draft project 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; do +for f in feedback-templates prompts review-core jj-core vcs-core review-args storage draft project 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; do src="../../packages/shared/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/shared/%s.ts\n' "$f" | cat - "$src" > "generated/$f.ts" done # Vendor review agent modules from packages/server/ — rewrite imports for generated/ layout -for f in codex-review claude-review path-utils; do +for f in agent-review-message codex-review claude-review path-utils; do src="../../packages/server/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/server/%s.ts\n' "$f" | cat - "$src" \ | sed 's|from "./vcs"|from "./review-core.js"|' \ @@ -28,6 +28,7 @@ for f in tour-review; do printf '// @generated — DO NOT EDIT. Source: packages/server/tour/%s.ts\n' "$f" | cat - "$src" \ | sed 's|from "\.\./vcs"|from "./review-core.js"|' \ | sed 's|from "\.\./pr"|from "./pr-provider.js"|' \ + | sed 's|from "\.\./agent-review-message"|from "./agent-review-message.js"|' \ | sed 's|from "@plannotator/shared/tour"|from "./tour.js"|' \ > "generated/$f.ts" done diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index dbb85ebd..48f3c2c4 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -760,7 +760,7 @@ const ReviewApp: React.FC = () => { // Prefer the server's active base (survives page refresh / reconnect) // over the detected default, so the picker rehydrates to what the // server is actually using. - const initial = data.base || data.gitContext.defaultBranch || null; + const initial = data.base || data.gitContext.defaultBranch || data.gitContext.compareTarget?.fallback || null; setSelectedBase(initial); setCommittedBase(initial); } @@ -782,7 +782,7 @@ const ReviewApp: React.FC = () => { if (data.error) setDiffError(data.error); if (data.isWSL) setIsWSL(true); // Mark diff type setup as pending on first run (local mode only) - if (data.diffType && !data.prMetadata && data.gitContext?.vcsType !== 'p4' && needsDiffTypeSetup()) { + if (data.diffType && !data.prMetadata && data.gitContext?.vcsType !== 'p4' && data.gitContext?.vcsType !== 'jj' && needsDiffTypeSetup()) { setDiffTypeSetupPending(true); } }) @@ -1163,6 +1163,7 @@ const ReviewApp: React.FC = () => { ...prev, defaultBranch: data.gitContext!.defaultBranch, diffOptions: data.gitContext!.diffOptions, + compareTarget: data.gitContext!.compareTarget, }; }); } @@ -1190,7 +1191,7 @@ const ReviewApp: React.FC = () => { if (branch === selectedBase) return; const previous = selectedBase; setSelectedBase(branch); - if (activeDiffBase === 'branch' || activeDiffBase === 'merge-base') { + if (activeDiffBase === 'branch' || activeDiffBase === 'merge-base' || activeDiffBase === 'jj-line') { const ok = await fetchDiffSwitch(diffType, branch); if (!ok) setSelectedBase(previous); } @@ -1304,7 +1305,7 @@ const ReviewApp: React.FC = () => { // the new patch to arrive before refetching — otherwise the viewer can // briefly pair an old patch with the new base's content. reviewBase: - (activeDiffBase === 'branch' || activeDiffBase === 'merge-base') + (activeDiffBase === 'branch' || activeDiffBase === 'merge-base' || activeDiffBase === 'jj-line') ? committedBase ?? undefined : undefined, activeDiffBase, @@ -2073,8 +2074,9 @@ const ReviewApp: React.FC = () => { currentBranch={gitContext?.currentBranch} availableBranches={prMetadata ? undefined : gitContext?.availableBranches} selectedBase={prMetadata ? undefined : selectedBase ?? undefined} - detectedBase={prMetadata ? undefined : gitContext?.defaultBranch} + detectedBase={prMetadata ? undefined : gitContext?.defaultBranch || gitContext?.compareTarget?.fallback} onSelectBase={prMetadata ? undefined : handleBaseSelect} + compareTarget={gitContext?.compareTarget} stagedFiles={stagedFiles} onCopyRawDiff={handleCopyDiff} canCopyRawDiff={!!diffData?.rawPatch} @@ -2151,6 +2153,10 @@ const ReviewApp: React.FC = () => { {activeDiffBase === 'staged' && "No staged changes. Stage some files with git add."} {activeDiffBase === 'unstaged' && "No unstaged changes. All changes are staged."} {activeDiffBase === 'last-commit' && `No changes in the last commit${activeWorktreePath ? ' in this worktree' : ''}.`} + {activeDiffBase === 'jj-current' && "No changes in the current jj change."} + {activeDiffBase === 'jj-last' && "No changes in the last jj change."} + {activeDiffBase === 'jj-line' && `No changes in your line of work vs ${selectedBase || gitContext?.defaultBranch || '@-'}.`} + {activeDiffBase === 'jj-all' && "No files at the current jj change."} {activeDiffBase === 'branch' && `No changes vs ${selectedBase || gitContext?.defaultBranch || 'main'}${activeWorktreePath ? ' in this worktree' : ''}.`} {activeDiffBase === 'merge-base' && `No changes vs ${selectedBase || gitContext?.defaultBranch || 'main'}${activeWorktreePath ? ' in this worktree' : ''}.`} {activeDiffBase === 'all' && `No tracked files${activeWorktreePath ? ' in this worktree' : ' in this repository'}.`} diff --git a/packages/review-editor/components/BaseBranchPicker.tsx b/packages/review-editor/components/BaseBranchPicker.tsx index 5810d3f8..384a216d 100644 --- a/packages/review-editor/components/BaseBranchPicker.tsx +++ b/packages/review-editor/components/BaseBranchPicker.tsx @@ -1,6 +1,6 @@ import React, { useMemo, useRef, useState } from 'react'; import * as Popover from '@radix-ui/react-popover'; -import type { AvailableBranches } from '@plannotator/shared/types'; +import type { AvailableBranches, CompareTargetPickerCopy } from '@plannotator/shared/types'; interface BaseBranchPickerProps { availableBranches: AvailableBranches; @@ -8,6 +8,7 @@ interface BaseBranchPickerProps { detectedBase: string; onSelectBase: (branch: string) => void; disabled?: boolean; + copy: CompareTargetPickerCopy; } export const BaseBranchPicker: React.FC = ({ @@ -16,6 +17,7 @@ export const BaseBranchPicker: React.FC = ({ detectedBase, onSelectBase, disabled, + copy, }) => { const [open, setOpen] = useState(false); const [query, setQuery] = useState(''); @@ -57,7 +59,7 @@ export const BaseBranchPicker: React.FC = ({