From 6c816ae5abf7a7c856c5f2f2119208497376e120 Mon Sep 17 00:00:00 2001 From: Christoph Schmatzler Date: Sun, 22 Mar 2026 18:17:09 +0000 Subject: [PATCH] review -> jj --- modules/_ai-tools/review.ts | 743 ++++++++++++++++++++++++------------ 1 file changed, 508 insertions(+), 235 deletions(-) diff --git a/modules/_ai-tools/review.ts b/modules/_ai-tools/review.ts index 103d221..9be81ee 100644 --- a/modules/_ai-tools/review.ts +++ b/modules/_ai-tools/review.ts @@ -3,19 +3,19 @@ * * Provides a `/review` command that prompts the agent to review code changes. * Supports multiple review modes: - * - Review a GitHub pull request (checks out the PR locally) - * - Review against a base branch (PR style) - * - Review uncommitted changes - * - Review a specific commit + * - Review a GitHub pull request (materializes the PR locally with jj) + * - Review against a base bookmark (PR style) + * - Review working-copy changes + * - Review a specific change * - Shared custom review instructions (applied to all review modes when configured) * * Usage: * - `/review` - show interactive selector - * - `/review pr 123` - review PR #123 (checks out locally) + * - `/review pr 123` - review PR #123 (materializes it locally with jj) * - `/review pr https://github.com/owner/repo/pull/123` - review PR from URL - * - `/review uncommitted` - review uncommitted changes directly - * - `/review branch main` - review against main branch - * - `/review commit abc123` - review specific commit + * - `/review working-copy` - review working-copy changes directly + * - `/review bookmark main` - review against the main bookmark + * - `/review change abc123` - review a specific change * - `/review folder src docs` - review specific folders/files (snapshot, not diff) * - `/review` selector includes Add/Remove custom review instructions (applies to all modes) * - `/review --extra "focus on performance regressions"` - add extra review instruction (works with any mode) @@ -24,7 +24,7 @@ * - If a REVIEW_GUIDELINES.md file exists in the same directory as .pi, * its contents are appended to the review prompt. * - * Note: PR review requires a clean working tree (no uncommitted changes to tracked files). + * Note: PR review requires a clean working copy (no local jj changes). */ import type { ExtensionAPI, ExtensionContext, ExtensionCommandContext } from "@mariozechner/pi-coding-agent"; @@ -32,7 +32,6 @@ import { DynamicBorder, BorderedLoader } from "@mariozechner/pi-coding-agent"; import { Container, fuzzyFilter, - getEditorKeybindings, Input, type SelectItem, SelectList, @@ -352,36 +351,41 @@ function hasBlockingReviewFindings(messageText: string): boolean { } // Review target types (matching Codex's approach) +type BookmarkRef = { + name: string; + remote?: string; +}; + type ReviewTarget = - | { type: "uncommitted" } - | { type: "baseBranch"; branch: string } - | { type: "commit"; sha: string; title?: string } - | { type: "pullRequest"; prNumber: number; baseBranch: string; title: string } + | { type: "workingCopy" } + | { type: "baseBookmark"; bookmark: string; remote?: string } + | { type: "change"; sha: string; title?: string } + | { type: "pullRequest"; prNumber: number; baseBookmark: string; baseRemote?: string; title: string } | { type: "folder"; paths: string[] }; // Prompts (adapted from Codex) -const UNCOMMITTED_PROMPT = - "Review the current code changes (staged, unstaged, and untracked files) and provide prioritized findings."; +const WORKING_COPY_PROMPT = + "Review the current working-copy changes (including new files) and provide prioritized findings."; const LOCAL_CHANGES_REVIEW_INSTRUCTIONS = - "Also include local working-tree changes (staged, unstaged, and untracked files) from this branch. Use `jj status` and `jj diff -r @` so local fixes are part of this review cycle."; + "Also include local working-copy changes (including new files) on top of this bookmark. Use `jj status`, `jj diff --summary`, and `jj diff` so local fixes are part of this review cycle."; -const BASE_BRANCH_PROMPT_WITH_MERGE_BASE = - "Review the code changes against the base branch '{baseBranch}'. The merge base commit for this comparison is {mergeBaseSha}. Run `jj diff --from {mergeBaseSha}` to inspect the changes relative to {baseBranch}. Provide prioritized, actionable findings."; +const BASE_BOOKMARK_PROMPT_WITH_MERGE_BASE = + "Review the code changes against the base bookmark '{baseBookmark}'. The merge-base revision for this comparison is {mergeBaseSha}. Run `jj diff --from {mergeBaseSha} --to @` to inspect the changes relative to {baseBookmark}. Provide prioritized, actionable findings."; -const BASE_BRANCH_PROMPT_FALLBACK = - "Review the code changes against the base branch '{branch}'. Start by finding the merge diff between the current branch and {branch}'s upstream e.g. (`jj log -r 'fork_point(@ | {branch})' --no-graph -T 'commit_id.short() ++ \"\\n\"'`), then run `jj diff --from ` to see what changes we would merge into the {branch} branch. Provide prioritized, actionable findings."; +const BASE_BOOKMARK_PROMPT_FALLBACK = + "Review the code changes against the base bookmark '{bookmark}'. Start by finding the merge-base revision between the working copy and {bookmark}, then run `jj diff --from --to @` to see what changes would land on the {bookmark} bookmark. Provide prioritized, actionable findings."; -const COMMIT_PROMPT_WITH_TITLE = - 'Review the code changes introduced by commit {sha} ("{title}"). Provide prioritized, actionable findings.'; +const CHANGE_PROMPT_WITH_TITLE = + 'Review the code changes introduced by change {sha} ("{title}"). Provide prioritized, actionable findings.'; -const COMMIT_PROMPT = "Review the code changes introduced by commit {sha}. Provide prioritized, actionable findings."; +const CHANGE_PROMPT = "Review the code changes introduced by change {sha}. Provide prioritized, actionable findings."; const PULL_REQUEST_PROMPT = - 'Review pull request #{prNumber} ("{title}") against the base branch \'{baseBranch}\'. The merge base commit for this comparison is {mergeBaseSha}. Run `jj diff --from {mergeBaseSha}` to inspect the changes that would be merged. Provide prioritized, actionable findings.'; + 'Review pull request #{prNumber} ("{title}") against the base bookmark \'{baseBookmark}\'. The merge-base revision for this comparison is {mergeBaseSha}. Run `jj diff --from {mergeBaseSha} --to @` to inspect the changes that would be merged. Provide prioritized, actionable findings.'; const PULL_REQUEST_PROMPT_FALLBACK = - 'Review pull request #{prNumber} ("{title}") against the base branch \'{baseBranch}\'. Start by finding the merge base between the current branch and {baseBranch} (e.g., `jj log -r \'fork_point(@ | {baseBranch})\' --no-graph -T \'commit_id.short() ++ "\\n"\'`), then run `jj diff --from ` to see the changes that would be merged. Provide prioritized, actionable findings.'; + 'Review pull request #{prNumber} ("{title}") against the base bookmark \'{baseBookmark}\'. Start by finding the merge-base revision between the working copy and {baseBookmark}, then run `jj diff --from --to @` to see the changes that would be merged. Provide prioritized, actionable findings.'; const FOLDER_REVIEW_PROMPT = "Review the code in the following paths: {paths}. This is a snapshot review (not a diff). Read the files directly in these paths and provide prioritized, actionable findings."; @@ -520,82 +524,262 @@ async function loadProjectReviewGuidelines(cwd: string): Promise } } +function parseNonEmptyLines(stdout: string): string[] { + return stdout + .trim() + .split("\n") + .map((line) => line.trim()) + .filter(Boolean); +} + +function quoteRevsetString(value: string): string { + return JSON.stringify(value); +} + +function localBookmarkRevset(bookmark: string): string { + return `bookmarks(exact:${quoteRevsetString(bookmark)})`; +} + +function remoteBookmarkRevset(bookmark: string, remote: string): string { + return `remote_bookmarks(exact:${quoteRevsetString(bookmark)}, exact:${quoteRevsetString(remote)})`; +} + +function bookmarkRefToRevset(bookmark: BookmarkRef): string { + return bookmark.remote ? remoteBookmarkRevset(bookmark.name, bookmark.remote) : localBookmarkRevset(bookmark.name); +} + +function bookmarkRefToLabel(bookmark: BookmarkRef): string { + return bookmark.remote ? `${bookmark.name}@${bookmark.remote}` : bookmark.name; +} + +function bookmarkRefsEqual(left: BookmarkRef, right: BookmarkRef): boolean { + return left.name === right.name && left.remote === right.remote; +} + +function parseBookmarkReference(value: string): BookmarkRef { + const trimmed = value.trim(); + const separatorIndex = trimmed.lastIndexOf("@"); + if (separatorIndex <= 0 || separatorIndex === trimmed.length - 1) { + return { name: trimmed }; + } + + return { + name: trimmed.slice(0, separatorIndex), + remote: trimmed.slice(separatorIndex + 1), + }; +} + +function parseBookmarkRefs(stdout: string): BookmarkRef[] { + return parseNonEmptyLines(stdout) + .map((line) => { + const [name, remote = ""] = line.split("\t"); + return { + name: name.trim(), + remote: remote.trim() || undefined, + }; + }) + .filter((bookmark) => bookmark.name && bookmark.remote !== "git"); +} + +function dedupeBookmarkRefs(bookmarks: BookmarkRef[]): BookmarkRef[] { + const seen = new Set(); + const result: BookmarkRef[] = []; + + for (const bookmark of bookmarks) { + const key = `${bookmark.name}@${bookmark.remote ?? ""}`; + if (seen.has(key)) { + continue; + } + + seen.add(key); + result.push(bookmark); + } + + return result; +} + +async function getBookmarkRefs( + pi: ExtensionAPI, + options?: { revset?: string; includeRemotes?: boolean }, +): Promise { + const args = ["bookmark", "list"]; + if (options?.includeRemotes) { + args.push("--all-remotes"); + } + if (options?.revset) { + args.push("-r", options.revset); + } + args.push("-T", 'name ++ "\\t" ++ remote ++ "\\n"'); + + const { stdout, code } = await pi.exec("jj", args); + if (code !== 0) return []; + return dedupeBookmarkRefs(parseBookmarkRefs(stdout)); +} + +async function getSingleRevisionId(pi: ExtensionAPI, revset: string): Promise { + const { stdout, code } = await pi.exec("jj", ["log", "-r", revset, "--no-graph", "-T", 'commit_id ++ "\\n"']); + if (code !== 0) { + return null; + } + + const revisions = parseNonEmptyLines(stdout); + if (revisions.length !== 1) { + return null; + } + + return revisions[0]; +} + +async function getDefaultRemoteName(pi: ExtensionAPI): Promise { + const remotes = await getJjRemotes(pi); + if (remotes.length === 0) { + return null; + } + + return remotes.find((remote) => remote.name === "origin")?.name ?? remotes[0].name; +} + +function preferBookmarkRef(bookmarks: BookmarkRef[], preferredRemote?: string | null): BookmarkRef | null { + if (bookmarks.length === 0) { + return null; + } + + return ( + bookmarks.find((bookmark) => !bookmark.remote) ?? + (preferredRemote ? bookmarks.find((bookmark) => bookmark.remote === preferredRemote) : undefined) ?? + bookmarks[0] + ); +} + +async function resolveBookmarkRef( + pi: ExtensionAPI, + bookmark: string, + remote?: string, +): Promise { + if (remote) { + return { name: bookmark, remote }; + } + + const localBookmark = (await getBookmarkRefs(pi)).find((entry) => entry.name === bookmark); + if (localBookmark) { + return localBookmark; + } + + const matchingRemoteBookmarks = (await getBookmarkRefs(pi, { includeRemotes: true })).filter( + (entry) => entry.remote && entry.name === bookmark, + ); + if (matchingRemoteBookmarks.length === 0) { + return null; + } + + return preferBookmarkRef(matchingRemoteBookmarks, await getDefaultRemoteName(pi)); +} + +async function getReviewBookmarks(pi: ExtensionAPI): Promise { + const localBookmarks = await getBookmarkRefs(pi); + const localNames = new Set(localBookmarks.map((bookmark) => bookmark.name)); + const defaultRemoteName = await getDefaultRemoteName(pi); + const remoteOnlyBookmarks = (await getBookmarkRefs(pi, { includeRemotes: true })) + .filter((bookmark) => bookmark.remote && !localNames.has(bookmark.name)) + .sort((left, right) => { + if (left.name !== right.name) { + return left.name.localeCompare(right.name); + } + if (left.remote === defaultRemoteName) return -1; + if (right.remote === defaultRemoteName) return 1; + return (left.remote ?? "").localeCompare(right.remote ?? ""); + }); + + return dedupeBookmarkRefs([...localBookmarks, ...remoteOnlyBookmarks]); +} + +async function getReviewHeadRevset(pi: ExtensionAPI): Promise { + return (await hasWorkingCopyChanges(pi)) ? "@" : "@-"; +} + +async function getCurrentReviewBookmarks(pi: ExtensionAPI): Promise { + return getBookmarkRefs(pi, { + revset: await getReviewHeadRevset(pi), + includeRemotes: true, + }); +} + +async function getDefaultBookmarkRef(pi: ExtensionAPI): Promise { + const defaultRemoteName = await getDefaultRemoteName(pi); + const trunkBookmarks = await getBookmarkRefs(pi, { revset: "trunk()", includeRemotes: true }); + const trunkBookmark = preferBookmarkRef(trunkBookmarks, defaultRemoteName); + if (trunkBookmark) { + return trunkBookmark; + } + + const bookmarks = await getReviewBookmarks(pi); + const mainBookmark = + bookmarks.find((bookmark) => !bookmark.remote && bookmark.name === "main") ?? + bookmarks.find((bookmark) => !bookmark.remote && bookmark.name === "master") ?? + bookmarks.find((bookmark) => bookmark.remote === defaultRemoteName && bookmark.name === "main") ?? + bookmarks.find((bookmark) => bookmark.remote === defaultRemoteName && bookmark.name === "master"); + if (mainBookmark) { + return mainBookmark; + } + + return bookmarks[0] ?? null; +} + /** - * Get the merge base between the working copy and a bookmark + * Get the merge-base revision between the working copy and a bookmark */ async function getMergeBase( pi: ExtensionAPI, - branch: string, + bookmark: string, + remote?: string, ): Promise { try { - const { stdout: mergeBase, code } = await pi.exec("jj", [ - "log", - "--no-graph", - "-r", - `fork_point(@ | ${branch})`, - "-T", - 'commit_id.short() ++ "\\n"', - ]); - if (code === 0 && mergeBase.trim()) { - return mergeBase.trim(); + const bookmarkRef = await resolveBookmarkRef(pi, bookmark, remote); + if (!bookmarkRef) { + return null; } - return null; + return getSingleRevisionId(pi, `heads(::@ & ::${bookmarkRefToRevset(bookmarkRef)})`); } catch { return null; } } /** - * Get list of local branches + * Get list of recent changes */ -async function getLocalBranches(pi: ExtensionAPI): Promise { - const { stdout, code } = await pi.exec("jj", ["bookmark", "list", "-T", 'name ++ "\\n"']); - if (code !== 0) return []; - return [...new Set(stdout - .trim() - .split("\n") - .filter((b) => b.trim()))]; -} - -/** - * Get list of recent commits - */ -async function getRecentCommits(pi: ExtensionAPI, limit: number = 10): Promise> { +async function getRecentChanges(pi: ExtensionAPI, limit: number = 10): Promise> { const { stdout, code } = await pi.exec("jj", [ "log", + "-n", + `${limit}`, "--no-graph", - "-r", - `latest(::@-, ${limit})`, "-T", - 'commit_id.short() ++ "\t" ++ description.first_line() ++ "\\n"', + 'commit_id ++ "\\t" ++ description.first_line() ++ "\\n"', ]); if (code !== 0) return []; - return stdout - .trim() - .split("\n") + return parseNonEmptyLines(stdout) .filter((line) => line.trim()) .map((line) => { - const [sha, title = ""] = line.trim().split("\t"); - return { sha, title }; + const [sha, ...rest] = line.trim().split("\t"); + return { sha, title: rest.join(" ") }; }); } /** - * Check if there are uncommitted changes (staged, unstaged, or untracked) + * Check if there are working-copy changes */ -async function hasUncommittedChanges(pi: ExtensionAPI): Promise { - const { stdout, code } = await pi.exec("jj", ["log", "--no-graph", "-r", "@", "-T", "empty"]); - return code === 0 && stdout.trim() === "false"; +async function hasWorkingCopyChanges(pi: ExtensionAPI): Promise { + const { stdout, code } = await pi.exec("jj", ["diff", "--summary"]); + return code === 0 && stdout.trim().length > 0; } /** - * Check if there are changes that would prevent switching branches - * (staged or unstaged changes to tracked files - untracked files are fine) + * Check if there are local changes that would make switching bookmarks surprising */ async function hasPendingChanges(pi: ExtensionAPI): Promise { - return hasUncommittedChanges(pi); + return hasWorkingCopyChanges(pi); } /** @@ -624,10 +808,21 @@ function parsePrReference(ref: string): number | null { /** * Get PR information from GitHub CLI */ -async function getPrInfo(pi: ExtensionAPI, prNumber: number): Promise<{ baseBranch: string; title: string; headBranch: string } | null> { +async function getPrInfo( + pi: ExtensionAPI, + prNumber: number, +): Promise<{ + baseBookmark: string; + title: string; + headBookmark: string; + isCrossRepository: boolean; + headRepositoryName?: string; + headRepositoryOwner?: string; + headRepositoryUrl?: string; +} | null> { const { stdout, code } = await pi.exec("gh", [ "pr", "view", String(prNumber), - "--json", "baseRefName,title,headRefName", + "--json", "baseRefName,title,headRefName,isCrossRepository,headRepository,headRepositoryOwner", ]); if (code !== 0) return null; @@ -635,9 +830,13 @@ async function getPrInfo(pi: ExtensionAPI, prNumber: number): Promise<{ baseBran try { const data = JSON.parse(stdout); return { - baseBranch: data.baseRefName, + baseBookmark: data.baseRefName, title: data.title, - headBranch: data.headRefName, + headBookmark: data.headRefName, + isCrossRepository: data.isCrossRepository === true, + headRepositoryName: data.headRepository?.name, + headRepositoryOwner: data.headRepositoryOwner?.login, + headRepositoryUrl: data.headRepository?.url, }; } catch { return null; @@ -645,62 +844,112 @@ async function getPrInfo(pi: ExtensionAPI, prNumber: number): Promise<{ baseBran } /** - * Checkout a PR using Jujutsu + * Get configured jj remotes */ -async function checkoutPr( - pi: ExtensionAPI, - _headPrNumber: number, - headBranch: string, -): Promise<{ success: boolean; error?: string }> { - const fetchResult = await pi.exec("jj", ["git", "fetch", "--branch", headBranch]); - if (fetchResult.code !== 0) { - return { success: false, error: fetchResult.stderr || fetchResult.stdout || "Failed to fetch PR branch" }; - } +async function getJjRemotes(pi: ExtensionAPI): Promise> { + const { stdout, code } = await pi.exec("jj", ["git", "remote", "list"]); + if (code !== 0) return []; - let checkoutResult = await pi.exec("jj", ["new", headBranch]); - if (checkoutResult.code !== 0) { - checkoutResult = await pi.exec("jj", ["new", `${headBranch}@origin`]); - } + return parseNonEmptyLines(stdout) + .map((line) => { + const [name, ...urlParts] = line.split(/\s+/); + return { name, url: urlParts.join(" ") }; + }) + .filter((remote) => remote.name && remote.url); +} - if (checkoutResult.code !== 0) { - return { - success: false, - error: checkoutResult.stderr || checkoutResult.stdout || "Failed to checkout PR branch", - }; - } +function normalizeRemoteUrl(value: string): string { + return value + .trim() + .replace(/^git@github\.com:/, "https://github.com/") + .replace(/^ssh:\/\/git@github\.com\//, "https://github.com/") + .replace(/\.git$/, "") + .toLowerCase(); +} - return { success: true }; +function sanitizeRemoteName(value: string): string { + const sanitized = value.replace(/[^a-zA-Z0-9._-]+/g, "-").replace(/^-+|-+$/g, ""); + return sanitized || "gh-pr"; } /** - * Get the current branch name + * Materialize a PR locally with jj */ -async function getCurrentBranch(pi: ExtensionAPI): Promise { - for (const revision of ["@", "@-"]) { - const { stdout, code } = await pi.exec("jj", ["bookmark", "list", "-r", revision, "-T", 'name ++ "\\n"']); - if (code === 0 && stdout.trim()) { - return stdout.trim().split("\n")[0]; +async function materializePr( + pi: ExtensionAPI, + prNumber: number, + prInfo: { + headBookmark: string; + isCrossRepository: boolean; + headRepositoryName?: string; + headRepositoryOwner?: string; + headRepositoryUrl?: string; + }, +): Promise<{ success: boolean; remote?: string; error?: string }> { + const defaultRemoteName = await getDefaultRemoteName(pi); + if (!defaultRemoteName) { + return { success: false, error: "No jj remotes are configured for this repository" }; + } + + const existingRemotes = await getJjRemotes(pi); + let remoteName = defaultRemoteName; + let addedTemporaryRemote = false; + + if (prInfo.isCrossRepository) { + const repoSlug = prInfo.headRepositoryOwner && prInfo.headRepositoryName + ? `${prInfo.headRepositoryOwner}/${prInfo.headRepositoryName}`.toLowerCase() + : undefined; + const existingRemote = existingRemotes.find((remote) => { + if (prInfo.headRepositoryUrl && normalizeRemoteUrl(remote.url) === normalizeRemoteUrl(prInfo.headRepositoryUrl)) { + return true; + } + return repoSlug ? normalizeRemoteUrl(remote.url).includes(`github.com/${repoSlug}`) : false; + }); + + if (existingRemote) { + remoteName = existingRemote.name; + } else if (prInfo.headRepositoryUrl) { + const remoteBaseName = sanitizeRemoteName( + `gh-pr-${prInfo.headRepositoryOwner ?? "remote"}-${prInfo.headRepositoryName ?? prNumber}`, + ); + const existingRemoteNames = new Set(existingRemotes.map((remote) => remote.name)); + remoteName = remoteBaseName; + let suffix = 2; + while (existingRemoteNames.has(remoteName)) { + remoteName = `${remoteBaseName}-${suffix}`; + suffix += 1; + } + const addRemoteResult = await pi.exec("jj", ["git", "remote", "add", remoteName, prInfo.headRepositoryUrl]); + if (addRemoteResult.code !== 0) { + return { success: false, error: addRemoteResult.stderr || addRemoteResult.stdout || "Failed to add PR remote" }; + } + addedTemporaryRemote = true; + } else { + return { success: false, error: "PR head repository URL is unavailable" }; } } - return null; -} - -/** - * Get the default branch (main or master) - */ -async function getDefaultBranch(pi: ExtensionAPI): Promise { - const { stdout, code } = await pi.exec("jj", ["bookmark", "list", "-r", "trunk()", "-T", 'name ++ "\\n"']); - if (code === 0 && stdout.trim()) { - return stdout.trim().split("\n")[0]; + const fetchResult = await pi.exec("jj", ["git", "fetch", "--remote", remoteName, "--branch", prInfo.headBookmark]); + if (fetchResult.code !== 0) { + if (addedTemporaryRemote) { + await pi.exec("jj", ["git", "remote", "remove", remoteName]); + } + return { success: false, error: fetchResult.stderr || fetchResult.stdout || "Failed to fetch PR bookmark" }; } - // Fall back to checking if main or master exists - const branches = await getLocalBranches(pi); - if (branches.includes("main")) return "main"; - if (branches.includes("master")) return "master"; + const editResult = await pi.exec("jj", ["new", remoteBookmarkRevset(prInfo.headBookmark, remoteName)]); + if (editResult.code !== 0) { + if (addedTemporaryRemote) { + await pi.exec("jj", ["git", "remote", "remove", remoteName]); + } + return { success: false, error: editResult.stderr || editResult.stdout || "Failed to materialize PR locally" }; + } - return "main"; // Default fallback + if (addedTemporaryRemote) { + await pi.exec("jj", ["git", "remote", "remove", remoteName]); + } + + return { success: true, remote: remoteName }; } /** @@ -714,35 +963,39 @@ async function buildReviewPrompt( const includeLocalChanges = options?.includeLocalChanges === true; switch (target.type) { - case "uncommitted": - return UNCOMMITTED_PROMPT; + case "workingCopy": + return WORKING_COPY_PROMPT; - case "baseBranch": { - const mergeBase = await getMergeBase(pi, target.branch); + case "baseBookmark": { + const bookmarkLabel = bookmarkRefToLabel({ name: target.bookmark, remote: target.remote }); + const mergeBase = await getMergeBase(pi, target.bookmark, target.remote); const basePrompt = mergeBase - ? BASE_BRANCH_PROMPT_WITH_MERGE_BASE.replace(/{baseBranch}/g, target.branch).replace(/{mergeBaseSha}/g, mergeBase) - : BASE_BRANCH_PROMPT_FALLBACK.replace(/{branch}/g, target.branch); + ? BASE_BOOKMARK_PROMPT_WITH_MERGE_BASE + .replace(/{baseBookmark}/g, bookmarkLabel) + .replace(/{mergeBaseSha}/g, mergeBase) + : BASE_BOOKMARK_PROMPT_FALLBACK.replace(/{bookmark}/g, bookmarkLabel); return includeLocalChanges ? `${basePrompt} ${LOCAL_CHANGES_REVIEW_INSTRUCTIONS}` : basePrompt; } - case "commit": + case "change": if (target.title) { - return COMMIT_PROMPT_WITH_TITLE.replace("{sha}", target.sha).replace("{title}", target.title); + return CHANGE_PROMPT_WITH_TITLE.replace("{sha}", target.sha).replace("{title}", target.title); } - return COMMIT_PROMPT.replace("{sha}", target.sha); + return CHANGE_PROMPT.replace("{sha}", target.sha); case "pullRequest": { - const mergeBase = await getMergeBase(pi, target.baseBranch); + const baseBookmarkLabel = bookmarkRefToLabel({ name: target.baseBookmark, remote: target.baseRemote }); + const mergeBase = await getMergeBase(pi, target.baseBookmark, target.baseRemote); const basePrompt = mergeBase ? PULL_REQUEST_PROMPT .replace(/{prNumber}/g, String(target.prNumber)) .replace(/{title}/g, target.title) - .replace(/{baseBranch}/g, target.baseBranch) + .replace(/{baseBookmark}/g, baseBookmarkLabel) .replace(/{mergeBaseSha}/g, mergeBase) : PULL_REQUEST_PROMPT_FALLBACK .replace(/{prNumber}/g, String(target.prNumber)) .replace(/{title}/g, target.title) - .replace(/{baseBranch}/g, target.baseBranch); + .replace(/{baseBookmark}/g, baseBookmarkLabel); return includeLocalChanges ? `${basePrompt} ${LOCAL_CHANGES_REVIEW_INSTRUCTIONS}` : basePrompt; } @@ -756,13 +1009,13 @@ async function buildReviewPrompt( */ function getUserFacingHint(target: ReviewTarget): string { switch (target.type) { - case "uncommitted": - return "current changes"; - case "baseBranch": - return `changes against '${target.branch}'`; - case "commit": { + case "workingCopy": + return "working-copy changes"; + case "baseBookmark": + return `changes against '${bookmarkRefToLabel({ name: target.bookmark, remote: target.remote })}'`; + case "change": { const shortSha = target.sha.slice(0, 7); - return target.title ? `commit ${shortSha}: ${target.title}` : `commit ${shortSha}`; + return target.title ? `change ${shortSha}: ${target.title}` : `change ${shortSha}`; } case "pullRequest": { @@ -840,9 +1093,9 @@ async function waitForLoopTurnToStart(ctx: ExtensionContext, previousAssistantId // Review preset options for the selector (keep this order stable) const REVIEW_PRESETS = [ - { value: "uncommitted", label: "Review uncommitted changes", description: "" }, - { value: "baseBranch", label: "Review against a base branch", description: "(local)" }, - { value: "commit", label: "Review a commit", description: "" }, + { value: "workingCopy", label: "Review working-copy changes", description: "" }, + { value: "baseBookmark", label: "Review against a base bookmark", description: "(local)" }, + { value: "change", label: "Review a change", description: "" }, { value: "pullRequest", label: "Review a pull request", description: "(GitHub PR)" }, { value: "folder", label: "Review a folder (or more)", description: "(snapshot, not diff)" }, ] as const; @@ -890,23 +1143,26 @@ export default function reviewExtension(pi: ExtensionAPI) { }); /** - * Determine the smart default review type based on git state + * Determine the smart default review type based on jj state */ - async function getSmartDefault(): Promise<"uncommitted" | "baseBranch" | "commit"> { - // Priority 1: If there are uncommitted changes, default to reviewing them - if (await hasUncommittedChanges(pi)) { - return "uncommitted"; + async function getSmartDefault(): Promise<"workingCopy" | "baseBookmark" | "change"> { + // Priority 1: If there are working-copy changes, default to reviewing them + if (await hasWorkingCopyChanges(pi)) { + return "workingCopy"; } - // Priority 2: If on a feature branch (not the default branch), default to PR-style review - const currentBranch = await getCurrentBranch(pi); - const defaultBranch = await getDefaultBranch(pi); - if (currentBranch && currentBranch !== defaultBranch) { - return "baseBranch"; + // Priority 2: If the current review head differs from trunk/default, default to bookmark-style review + const defaultBookmark = await getDefaultBookmarkRef(pi); + if (defaultBookmark) { + const reviewHeadRevision = await getSingleRevisionId(pi, await getReviewHeadRevset(pi)); + const defaultBookmarkRevision = await getSingleRevisionId(pi, bookmarkRefToRevset(defaultBookmark)); + if (reviewHeadRevision && defaultBookmarkRevision && reviewHeadRevision !== defaultBookmarkRevision) { + return "baseBookmark"; + } } - // Priority 3: Default to reviewing a specific commit - return "commit"; + // Priority 3: Default to reviewing a specific change + return "change"; } /** @@ -1013,21 +1269,21 @@ export default function reviewExtension(pi: ExtensionAPI) { // Handle each preset type switch (result) { - case "uncommitted": - return { type: "uncommitted" }; + case "workingCopy": + return { type: "workingCopy" }; - case "baseBranch": { - const target = await showBranchSelector(ctx); + case "baseBookmark": { + const target = await showBookmarkSelector(ctx); if (target) return target; break; } - case "commit": { + case "change": { if (reviewLoopFixingEnabled) { - ctx.ui.notify("Loop mode does not work with commit review.", "error"); + ctx.ui.notify("Loop mode does not work with change review.", "error"); break; } - const target = await showCommitSelector(ctx); + const target = await showChangeSelector(ctx); if (target) return target; break; } @@ -1051,41 +1307,49 @@ export default function reviewExtension(pi: ExtensionAPI) { } /** - * Show branch selector for base branch review + * Show bookmark selector for base bookmark review */ - async function showBranchSelector(ctx: ExtensionContext): Promise { - const branches = await getLocalBranches(pi); - const currentBranch = await getCurrentBranch(pi); - const defaultBranch = await getDefaultBranch(pi); + async function showBookmarkSelector(ctx: ExtensionContext): Promise { + const bookmarks = await getReviewBookmarks(pi); + const currentBookmarks = await getCurrentReviewBookmarks(pi); + const defaultBookmark = await getDefaultBookmarkRef(pi); - // Never offer the current branch as a base branch (reviewing against itself is meaningless). - const candidateBranches = currentBranch ? branches.filter((b) => b !== currentBranch) : branches; + // Never offer the current review head's bookmark(s) as the base bookmark. + const candidateBookmarks = bookmarks.filter( + (bookmark) => !currentBookmarks.some((currentBookmark) => bookmarkRefsEqual(bookmark, currentBookmark)), + ); - if (candidateBranches.length === 0) { + if (candidateBookmarks.length === 0) { + const currentLabel = currentBookmarks[0] ? bookmarkRefToLabel(currentBookmarks[0]) : undefined; ctx.ui.notify( - currentBranch ? `No other branches found (current branch: ${currentBranch})` : "No branches found", + currentLabel ? `No other bookmarks found (current bookmark: ${currentLabel})` : "No bookmarks found", "error", ); return null; } - // Sort branches with default branch first - const sortedBranches = candidateBranches.sort((a, b) => { - if (a === defaultBranch) return -1; - if (b === defaultBranch) return 1; - return a.localeCompare(b); + // Sort bookmarks with the default bookmark first, then local bookmarks before remote-only ones. + const sortedBookmarks = candidateBookmarks.sort((a, b) => { + if (defaultBookmark && bookmarkRefsEqual(a, defaultBookmark)) return -1; + if (defaultBookmark && bookmarkRefsEqual(b, defaultBookmark)) return 1; + if (!!a.remote !== !!b.remote) return a.remote ? 1 : -1; + return bookmarkRefToLabel(a).localeCompare(bookmarkRefToLabel(b)); }); - const items: SelectItem[] = sortedBranches.map((branch) => ({ - value: branch, - label: branch, - description: branch === defaultBranch ? "(default)" : "", + const items: SelectItem[] = sortedBookmarks.map((bookmark) => ({ + value: bookmarkRefToLabel(bookmark), + label: bookmarkRefToLabel(bookmark), + description: defaultBookmark && bookmarkRefsEqual(bookmark, defaultBookmark) + ? "(default)" + : bookmark.remote + ? `(remote ${bookmark.remote})` + : "", })); - const result = await ctx.ui.custom((tui, theme, _kb, done) => { + const result = await ctx.ui.custom((tui, theme, keybindings, done) => { const container = new Container(); container.addChild(new DynamicBorder((str) => theme.fg("accent", str))); - container.addChild(new Text(theme.fg("accent", theme.bold("Select base branch")))); + container.addChild(new Text(theme.fg("accent", theme.bold("Select base bookmark")))); const searchInput = new Input(); container.addChild(searchInput); @@ -1102,7 +1366,7 @@ export default function reviewExtension(pi: ExtensionAPI) { const updateList = () => { listContainer.clear(); if (filteredItems.length === 0) { - listContainer.addChild(new Text(theme.fg("warning", " No matching branches"))); + listContainer.addChild(new Text(theme.fg("warning", " No matching bookmarks"))); selectList = null; return; } @@ -1138,16 +1402,15 @@ export default function reviewExtension(pi: ExtensionAPI) { container.invalidate(); }, handleInput(data: string) { - const kb = getEditorKeybindings(); if ( - kb.matches(data, "selectUp") || - kb.matches(data, "selectDown") || - kb.matches(data, "selectConfirm") || - kb.matches(data, "selectCancel") + keybindings.matches(data, "tui.select.up") || + keybindings.matches(data, "tui.select.down") || + keybindings.matches(data, "tui.select.confirm") || + keybindings.matches(data, "tui.select.cancel") ) { if (selectList) { selectList.handleInput(data); - } else if (kb.matches(data, "selectCancel")) { + } else if (keybindings.matches(data, "tui.select.cancel")) { done(null); } tui.requestRender(); @@ -1162,30 +1425,31 @@ export default function reviewExtension(pi: ExtensionAPI) { }); if (!result) return null; - return { type: "baseBranch", branch: result }; + const bookmark = parseBookmarkReference(result); + return { type: "baseBookmark", bookmark: bookmark.name, remote: bookmark.remote }; } /** - * Show commit selector + * Show change selector */ - async function showCommitSelector(ctx: ExtensionContext): Promise { - const commits = await getRecentCommits(pi, 20); + async function showChangeSelector(ctx: ExtensionContext): Promise { + const changes = await getRecentChanges(pi, 20); - if (commits.length === 0) { - ctx.ui.notify("No commits found", "error"); + if (changes.length === 0) { + ctx.ui.notify("No changes found", "error"); return null; } - const items: SelectItem[] = commits.map((commit) => ({ - value: commit.sha, - label: `${commit.sha.slice(0, 7)} ${commit.title}`, + const items: SelectItem[] = changes.map((change) => ({ + value: change.sha, + label: `${change.sha.slice(0, 7)} ${change.title}`, description: "", })); - const result = await ctx.ui.custom<{ sha: string; title: string } | null>((tui, theme, _kb, done) => { + const result = await ctx.ui.custom<{ sha: string; title: string } | null>((tui, theme, keybindings, done) => { const container = new Container(); container.addChild(new DynamicBorder((str) => theme.fg("accent", str))); - container.addChild(new Text(theme.fg("accent", theme.bold("Select commit to review")))); + container.addChild(new Text(theme.fg("accent", theme.bold("Select change to review")))); const searchInput = new Input(); container.addChild(searchInput); @@ -1202,7 +1466,7 @@ export default function reviewExtension(pi: ExtensionAPI) { const updateList = () => { listContainer.clear(); if (filteredItems.length === 0) { - listContainer.addChild(new Text(theme.fg("warning", " No matching commits"))); + listContainer.addChild(new Text(theme.fg("warning", " No matching changes"))); selectList = null; return; } @@ -1216,9 +1480,9 @@ export default function reviewExtension(pi: ExtensionAPI) { }); selectList.onSelect = (item) => { - const commit = commits.find((c) => c.sha === item.value); - if (commit) { - done(commit); + const change = changes.find((c) => c.sha === item.value); + if (change) { + done(change); } else { done(null); } @@ -1245,16 +1509,15 @@ export default function reviewExtension(pi: ExtensionAPI) { container.invalidate(); }, handleInput(data: string) { - const kb = getEditorKeybindings(); if ( - kb.matches(data, "selectUp") || - kb.matches(data, "selectDown") || - kb.matches(data, "selectConfirm") || - kb.matches(data, "selectCancel") + keybindings.matches(data, "tui.select.up") || + keybindings.matches(data, "tui.select.down") || + keybindings.matches(data, "tui.select.confirm") || + keybindings.matches(data, "tui.select.cancel") ) { if (selectList) { selectList.handleInput(data); - } else if (kb.matches(data, "selectCancel")) { + } else if (keybindings.matches(data, "tui.select.cancel")) { done(null); } tui.requestRender(); @@ -1269,7 +1532,7 @@ export default function reviewExtension(pi: ExtensionAPI) { }); if (!result) return null; - return { type: "commit", sha: result.sha, title: result.title }; + return { type: "change", sha: result.sha, title: result.title }; } @@ -1297,12 +1560,12 @@ export default function reviewExtension(pi: ExtensionAPI) { } /** - * Show PR input and handle checkout + * Show PR input and materialize the PR locally */ async function showPrInput(ctx: ExtensionContext): Promise { - // First check for pending changes that would prevent branch switching + // First check for pending changes that would make bookmark switching surprising if (await hasPendingChanges(pi)) { - ctx.ui.notify("Cannot checkout PR: you have uncommitted changes. Please commit or stash them first.", "error"); + ctx.ui.notify("Cannot materialize PR: you have local jj changes. Please snapshot or discard them first.", "error"); return null; } @@ -1331,25 +1594,28 @@ export default function reviewExtension(pi: ExtensionAPI) { // Check again for pending changes (in case something changed) if (await hasPendingChanges(pi)) { - ctx.ui.notify("Cannot checkout PR: you have uncommitted changes. Please commit or stash them first.", "error"); + ctx.ui.notify("Cannot materialize PR: you have local jj changes. Please snapshot or discard them first.", "error"); return null; } - // Checkout the PR - ctx.ui.notify(`Checking out PR #${prNumber}...`, "info"); - const checkoutResult = await checkoutPr(pi, prNumber, prInfo.headBranch); + // Materialize the PR locally with jj + ctx.ui.notify(`Materializing PR #${prNumber} with jj...`, "info"); + const materializeResult = await materializePr(pi, prNumber, prInfo); - if (!checkoutResult.success) { - ctx.ui.notify(`Failed to checkout PR: ${checkoutResult.error}`, "error"); + if (!materializeResult.success) { + ctx.ui.notify(`Failed to materialize PR: ${materializeResult.error}`, "error"); return null; } - ctx.ui.notify(`Checked out PR #${prNumber} (${prInfo.headBranch})`, "info"); + ctx.ui.notify(`Materialized PR #${prNumber} (${prInfo.headBookmark}@${materializeResult.remote ?? "origin"})`, "info"); + + const baseBookmarkRef = await resolveBookmarkRef(pi, prInfo.baseBookmark); return { type: "pullRequest", prNumber, - baseBranch: prInfo.baseBranch, + baseBookmark: prInfo.baseBookmark, + baseRemote: baseBookmarkRef?.remote, title: prInfo.title, }; } @@ -1542,20 +1808,24 @@ export default function reviewExtension(pi: ExtensionAPI) { const subcommand = parts[0]?.toLowerCase(); switch (subcommand) { - case "uncommitted": - return { target: { type: "uncommitted" }, extraInstruction }; + case "working-copy": + return { target: { type: "workingCopy" }, extraInstruction }; - case "branch": { - const branch = parts[1]; - if (!branch) return { target: null, extraInstruction }; - return { target: { type: "baseBranch", branch }, extraInstruction }; + case "bookmark": { + const bookmark = parts[1]; + if (!bookmark) return { target: null, extraInstruction }; + const bookmarkRef = parseBookmarkReference(bookmark); + return { + target: { type: "baseBookmark", bookmark: bookmarkRef.name, remote: bookmarkRef.remote }, + extraInstruction, + }; } - case "commit": { + case "change": { const sha = parts[1]; if (!sha) return { target: null, extraInstruction }; const title = parts.slice(2).join(" ") || undefined; - return { target: { type: "commit", sha, title }, extraInstruction }; + return { target: { type: "change", sha, title }, extraInstruction }; } @@ -1577,12 +1847,12 @@ export default function reviewExtension(pi: ExtensionAPI) { } /** - * Handle PR checkout and return a ReviewTarget (or null on failure) + * Materialize a PR locally and return a ReviewTarget (or null on failure) */ async function handlePrCheckout(ctx: ExtensionContext, ref: string): Promise { // First check for pending changes if (await hasPendingChanges(pi)) { - ctx.ui.notify("Cannot checkout PR: you have uncommitted changes. Please commit or stash them first.", "error"); + ctx.ui.notify("Cannot materialize PR: you have local jj changes. Please snapshot or discard them first.", "error"); return null; } @@ -1601,27 +1871,30 @@ export default function reviewExtension(pi: ExtensionAPI) { return null; } - // Checkout the PR - ctx.ui.notify(`Checking out PR #${prNumber}...`, "info"); - const checkoutResult = await checkoutPr(pi, prNumber, prInfo.headBranch); + // Materialize the PR locally with jj + ctx.ui.notify(`Materializing PR #${prNumber} with jj...`, "info"); + const materializeResult = await materializePr(pi, prNumber, prInfo); - if (!checkoutResult.success) { - ctx.ui.notify(`Failed to checkout PR: ${checkoutResult.error}`, "error"); + if (!materializeResult.success) { + ctx.ui.notify(`Failed to materialize PR: ${materializeResult.error}`, "error"); return null; } - ctx.ui.notify(`Checked out PR #${prNumber} (${prInfo.headBranch})`, "info"); + ctx.ui.notify(`Materialized PR #${prNumber} (${prInfo.headBookmark}@${materializeResult.remote ?? "origin"})`, "info"); + + const baseBookmarkRef = await resolveBookmarkRef(pi, prInfo.baseBookmark); return { type: "pullRequest", prNumber, - baseBranch: prInfo.baseBranch, + baseBookmark: prInfo.baseBookmark, + baseRemote: baseBookmarkRef?.remote, title: prInfo.title, }; } function isLoopCompatibleTarget(target: ReviewTarget): boolean { - if (target.type !== "commit") { + if (target.type !== "change") { return true; } @@ -1749,7 +2022,7 @@ export default function reviewExtension(pi: ExtensionAPI) { // Register the /review command pi.registerCommand("review", { - description: "Review code changes (PR, uncommitted, branch, commit, or folder)", + description: "Review code changes (PR, working copy, bookmark, change, or folder)", handler: async (args, ctx) => { if (!ctx.hasUI) { ctx.ui.notify("Review requires interactive mode", "error"); @@ -1787,7 +2060,7 @@ export default function reviewExtension(pi: ExtensionAPI) { if (parsed.target) { if (parsed.target.type === "pr") { - // Handle PR checkout (async operation) + // Materialize the PR locally (async operation) target = await handlePrCheckout(ctx, parsed.target.ref); if (!target) { ctx.ui.notify("PR review failed. Returning to review menu.", "warning"); @@ -1813,7 +2086,7 @@ export default function reviewExtension(pi: ExtensionAPI) { } if (reviewLoopFixingEnabled && !isLoopCompatibleTarget(target)) { - ctx.ui.notify("Loop mode does not work with commit review.", "error"); + ctx.ui.notify("Loop mode does not work with change review.", "error"); if (fromSelector) { target = null; continue;