Commit 68769e69b1fb
Changed files (1)
dots
pi
agent
extensions
dots/pi/agent/extensions/review.ts
@@ -270,12 +270,15 @@ To extract findings and create TODOs, use the org-todos extension.
// Module-level state means only one review can be active at a time.
// This is intentional - the UI and /end-review command assume a single active review.
let reviewOriginId: string | undefined = undefined;
+// Temporary clone directory for cross-repo PR reviews (cleaned up on /end-review)
+let reviewCloneDir: string | undefined = undefined;
const REVIEW_STATE_TYPE = "review-session";
type ReviewSessionState = {
active: boolean;
originId?: string;
+ cloneDir?: string;
};
function setReviewWidget(ctx: ExtensionContext, active: boolean) {
@@ -314,21 +317,44 @@ function applyReviewState(ctx: ExtensionContext) {
if (state?.active && state.originId) {
reviewOriginId = state.originId;
+ reviewCloneDir = state.cloneDir;
setReviewWidget(ctx, true);
return;
}
reviewOriginId = undefined;
+ reviewCloneDir = undefined;
setReviewWidget(ctx, false);
}
+/**
+ * Clean up review state and optionally remove the temporary clone directory.
+ */
+async function clearReviewState(ctx: ExtensionContext, pi: ExtensionAPI) {
+ setReviewWidget(ctx, false);
+ reviewOriginId = undefined;
+
+ // Clean up temporary clone directory for cross-repo PR reviews
+ if (reviewCloneDir) {
+ try {
+ await fs.rm(reviewCloneDir, { recursive: true, force: true });
+ ctx.ui.notify(`Cleaned up temporary clone: ${reviewCloneDir}`, "info");
+ } catch {
+ ctx.ui.notify(`Warning: Failed to clean up ${reviewCloneDir}`, "warning");
+ }
+ reviewCloneDir = undefined;
+ }
+
+ pi.appendEntry(REVIEW_STATE_TYPE, { active: false });
+}
+
// Review target types (matching Codex's approach)
type ReviewTarget =
| { type: "uncommitted" }
| { type: "baseBranch"; branch: string }
| { type: "commit"; sha: string; title?: string }
| { type: "custom"; instructions: string }
- | { type: "pullRequest"; prNumber: number; baseBranch: string; title: string; repo?: string }
+ | { type: "pullRequest"; prNumber: number; baseBranch: string; title: string; repo?: string; cloneDir?: string }
| { type: "folder"; paths: string[] };
// Prompts (adapted from Codex)
@@ -460,24 +486,26 @@ async function loadProjectReviewGuidelines(cwd: string): Promise<string | null>
async function getMergeBase(
pi: ExtensionAPI,
branch: string,
+ cwd?: string,
): Promise<string | null> {
+ const execOpts = cwd ? { cwd } : undefined;
try {
// First try to get the upstream tracking branch
const { stdout: upstream, code: upstreamCode } = await pi.exec("git", [
"rev-parse",
"--abbrev-ref",
`${branch}@{upstream}`,
- ]);
+ ], execOpts);
if (upstreamCode === 0 && upstream.trim()) {
- const { stdout: mergeBase, code } = await pi.exec("git", ["merge-base", "HEAD", upstream.trim()]);
+ const { stdout: mergeBase, code } = await pi.exec("git", ["merge-base", "HEAD", upstream.trim()], execOpts);
if (code === 0 && mergeBase.trim()) {
return mergeBase.trim();
}
}
// Fall back to using the branch directly
- const { stdout: mergeBase, code } = await pi.exec("git", ["merge-base", "HEAD", branch]);
+ const { stdout: mergeBase, code } = await pi.exec("git", ["merge-base", "HEAD", branch], execOpts);
if (code === 0 && mergeBase.trim()) {
return mergeBase.trim();
}
@@ -677,18 +705,24 @@ async function buildReviewPrompt(pi: ExtensionAPI, target: ReviewTarget): Promis
return target.instructions;
case "pullRequest": {
- const mergeBase = await getMergeBase(pi, target.baseBranch);
+ const mergeBase = await getMergeBase(pi, target.baseBranch, target.cloneDir);
+ let prompt: string;
if (mergeBase) {
- return PULL_REQUEST_PROMPT
+ prompt = PULL_REQUEST_PROMPT
.replace(/{prNumber}/g, String(target.prNumber))
.replace(/{title}/g, target.title)
.replace(/{baseBranch}/g, target.baseBranch)
.replace(/{mergeBaseSha}/g, mergeBase);
+ } else {
+ prompt = PULL_REQUEST_PROMPT_FALLBACK
+ .replace(/{prNumber}/g, String(target.prNumber))
+ .replace(/{title}/g, target.title)
+ .replace(/{baseBranch}/g, target.baseBranch);
}
- return PULL_REQUEST_PROMPT_FALLBACK
- .replace(/{prNumber}/g, String(target.prNumber))
- .replace(/{title}/g, target.title)
- .replace(/{baseBranch}/g, target.baseBranch);
+ if (target.cloneDir) {
+ prompt += `\n\nIMPORTANT: This is a cross-repository PR. The repository has been cloned to \`${target.cloneDir}\`. You MUST \`cd ${target.cloneDir}\` before running any git or file commands. All file paths are relative to that directory.`;
+ }
+ return prompt;
}
case "folder":
@@ -1079,12 +1113,6 @@ export default function reviewExtension(pi: ExtensionAPI) {
* Show PR selector with list of open PRs + manual entry option
*/
async function showPrInput(ctx: ExtensionContext): Promise<ReviewTarget | null> {
- // First check for pending changes that would prevent branch switching
- if (await hasPendingChanges(pi)) {
- ctx.ui.notify("Cannot checkout PR: you have uncommitted changes. Please commit or stash them first.", "error");
- return null;
- }
-
// Fetch open PRs
ctx.ui.notify("Fetching open PRs...", "info");
const prs = await fetchOpenPRs();
@@ -1222,62 +1250,29 @@ export default function reviewExtension(pi: ExtensionAPI) {
if (!selected) return null;
- // Manual entry fallback
- let prNumber: number;
+ // Build the ref string for handlePrCheckout
+ let prRef: string;
let prRepo: string | undefined;
if (selected === "__manual__") {
- const prRef = await ctx.ui.editor(
+ const input = await ctx.ui.editor(
"Enter PR number, owner/repo#number, or URL (e.g. 123, tektoncd/pipeline#456, or https://github.com/owner/repo/pull/123):",
"",
);
- if (!prRef?.trim()) return null;
+ if (!input?.trim()) return null;
- const parsed = parsePrReference(prRef);
+ const parsed = parsePrReference(input);
if (!parsed) {
ctx.ui.notify("Invalid PR reference. Enter a number, owner/repo#number, or GitHub PR URL.", "error");
return null;
}
- prNumber = parsed.number;
+ prRef = input.trim();
prRepo = parsed.repo;
} else {
- prNumber = parseInt(selected, 10);
+ prRef = selected;
}
- const repoLabel = prRepo ? ` (${prRepo})` : "";
-
- // Get PR info from GitHub
- ctx.ui.notify(`Fetching PR #${prNumber} info${repoLabel}...`, "info");
- const prInfo = await getPrInfo(pi, prNumber, prRepo);
-
- if (!prInfo) {
- ctx.ui.notify(`Could not find PR #${prNumber}${repoLabel}. Make sure gh is authenticated and the PR exists.`, "error");
- return null;
- }
-
- // 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");
- return null;
- }
-
- // Checkout the PR
- ctx.ui.notify(`Checking out PR #${prNumber}${repoLabel}...`, "info");
- const checkoutResult = await checkoutPr(pi, prNumber, prRepo);
-
- if (!checkoutResult.success) {
- ctx.ui.notify(`Failed to checkout PR: ${checkoutResult.error}`, "error");
- return null;
- }
-
- ctx.ui.notify(`Checked out PR #${prNumber} (${prInfo.headBranch})${repoLabel}`, "info");
-
- return {
- type: "pullRequest",
- prNumber,
- baseBranch: prInfo.baseBranch,
- title: prInfo.title,
- repo: prRepo,
- };
+ // Delegate to handlePrCheckout which handles both local and cross-repo PRs
+ return await handlePrCheckout(ctx, prRef, prRepo);
}
/**
@@ -1290,12 +1285,17 @@ export default function reviewExtension(pi: ExtensionAPI) {
return;
}
+ // Track clone directory for cross-repo PR reviews
+ const cloneDir = target.type === "pullRequest" ? target.cloneDir : undefined;
+ reviewCloneDir = cloneDir;
+
// Handle fresh session mode
if (useFreshSession) {
// Store current position (where we'll return to)
const originId = ctx.sessionManager.getLeafId() ?? undefined;
if (!originId) {
ctx.ui.notify("Failed to determine review origin. Try again from a session with messages.", "error");
+ reviewCloneDir = undefined;
return;
}
reviewOriginId = originId;
@@ -1312,6 +1312,7 @@ export default function reviewExtension(pi: ExtensionAPI) {
if (!firstUserMessage) {
ctx.ui.notify("No user message found in session", "error");
reviewOriginId = undefined;
+ reviewCloneDir = undefined;
return;
}
@@ -1321,11 +1322,13 @@ export default function reviewExtension(pi: ExtensionAPI) {
const result = await ctx.navigateTree(firstUserMessage.id, { summarize: false, label: "code-review" });
if (result.cancelled) {
reviewOriginId = undefined;
+ reviewCloneDir = undefined;
return;
}
} catch (error) {
// Clean up state if navigation fails
reviewOriginId = undefined;
+ reviewCloneDir = undefined;
ctx.ui.notify(`Failed to start review: ${error instanceof Error ? error.message : String(error)}`, "error");
return;
}
@@ -1340,7 +1343,7 @@ export default function reviewExtension(pi: ExtensionAPI) {
setReviewWidget(ctx, true);
// Persist review state so tree navigation can restore/reset it
- pi.appendEntry(REVIEW_STATE_TYPE, { active: true, originId: lockedOriginId });
+ pi.appendEntry(REVIEW_STATE_TYPE, { active: true, originId: lockedOriginId, cloneDir });
}
const prompt = await buildReviewPrompt(pi, target);
@@ -1443,14 +1446,11 @@ export default function reviewExtension(pi: ExtensionAPI) {
/**
* Handle PR checkout and return a ReviewTarget (or null on failure)
+ *
+ * For cross-repo PRs (when repo is set), clones the repository to a temp
+ * directory and checks out the PR there instead of in the current repo.
*/
async function handlePrCheckout(ctx: ExtensionContext, ref: string, repo?: string): Promise<ReviewTarget | null> {
- // 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");
- return null;
- }
-
const prRef = parsePrReference(ref);
if (!prRef) {
ctx.ui.notify("Invalid PR reference. Enter a number or GitHub PR URL.", "error");
@@ -1460,6 +1460,13 @@ export default function reviewExtension(pi: ExtensionAPI) {
// Use repo from the parsed reference if not explicitly provided
const effectiveRepo = repo ?? prRef.repo;
const repoLabel = effectiveRepo ? ` (${effectiveRepo})` : "";
+ const isCrossRepo = !!effectiveRepo;
+
+ // For local repo PRs, check for pending changes (cross-repo clones to temp dir so no conflict)
+ if (!isCrossRepo && await hasPendingChanges(pi)) {
+ ctx.ui.notify("Cannot checkout PR: you have uncommitted changes. Please commit or stash them first.", "error");
+ return null;
+ }
// Get PR info
ctx.ui.notify(`Fetching PR #${prRef.number} info${repoLabel}...`, "info");
@@ -1470,23 +1477,69 @@ export default function reviewExtension(pi: ExtensionAPI) {
return null;
}
- // Checkout the PR
- ctx.ui.notify(`Checking out PR #${prRef.number}${repoLabel}...`, "info");
- const checkoutResult = await checkoutPr(pi, prRef.number, effectiveRepo);
+ let cloneDir: string | undefined;
- if (!checkoutResult.success) {
- ctx.ui.notify(`Failed to checkout PR: ${checkoutResult.error}`, "error");
- return null;
+ if (isCrossRepo) {
+ // Clone the repository to a temp directory for cross-repo PRs
+ const tmpBase = path.join(process.env.TMPDIR || "/tmp", "pi-review");
+ await fs.mkdir(tmpBase, { recursive: true });
+ const sanitizedRepo = effectiveRepo!.replace(/\//g, "-");
+ cloneDir = path.join(tmpBase, `${sanitizedRepo}-pr-${prRef.number}`);
+
+ // Remove existing clone if present (stale from previous review)
+ try {
+ await fs.rm(cloneDir, { recursive: true, force: true });
+ } catch {
+ // Ignore
+ }
+
+ ctx.ui.notify(`Cloning ${effectiveRepo} to ${cloneDir}...`, "info");
+ const { stderr: cloneErr, code: cloneCode } = await pi.exec("gh", [
+ "repo", "clone", effectiveRepo!, cloneDir, "--", "--depth=1",
+ ]);
+
+ if (cloneCode !== 0) {
+ ctx.ui.notify(`Failed to clone ${effectiveRepo}: ${cloneErr}`, "error");
+ return null;
+ }
+
+ // Checkout the PR inside the clone
+ ctx.ui.notify(`Checking out PR #${prRef.number} in clone...`, "info");
+ const { stderr: coErr, code: coCode } = await pi.exec("gh", [
+ "pr", "checkout", String(prRef.number), "-R", effectiveRepo!,
+ ], { cwd: cloneDir });
+
+ if (coCode !== 0) {
+ ctx.ui.notify(`Failed to checkout PR: ${coErr}`, "error");
+ // Clean up failed clone
+ await fs.rm(cloneDir, { recursive: true, force: true }).catch(() => {});
+ return null;
+ }
+
+ // Fetch the base branch so merge-base works
+ await pi.exec("git", ["fetch", "origin", prInfo.baseBranch], { cwd: cloneDir });
+
+ ctx.ui.notify(`Checked out PR #${prRef.number} (${prInfo.headBranch}) in ${cloneDir}`, "info");
+ } else {
+ // Local repo checkout
+ ctx.ui.notify(`Checking out PR #${prRef.number}...`, "info");
+ const checkoutResult = await checkoutPr(pi, prRef.number);
+
+ if (!checkoutResult.success) {
+ ctx.ui.notify(`Failed to checkout PR: ${checkoutResult.error}`, "error");
+ return null;
+ }
+
+ ctx.ui.notify(`Checked out PR #${prRef.number} (${prInfo.headBranch})`, "info");
}
- ctx.ui.notify(`Checked out PR #${prRef.number} (${prInfo.headBranch})${repoLabel}`, "info");
-
return {
type: "pullRequest",
prNumber: prRef.number,
baseBranch: prInfo.baseBranch,
title: prInfo.title,
repo: effectiveRepo,
+ cloneDir,
};
}
@@ -1626,9 +1679,9 @@ Preserve exact file paths, function names, and error messages.
const state = getReviewState(ctx);
if (state?.active && state.originId) {
reviewOriginId = state.originId;
+ reviewCloneDir = state.cloneDir;
} else if (state?.active) {
- setReviewWidget(ctx, false);
- pi.appendEntry(REVIEW_STATE_TYPE, { active: false });
+ await clearReviewState(ctx, pi);
ctx.ui.notify("Review state was missing origin info; cleared review status.", "warning");
return;
} else {
@@ -1682,9 +1735,7 @@ Preserve exact file paths, function names, and error messages.
}
// Clear state only on success
- setReviewWidget(ctx, false);
- reviewOriginId = undefined;
- pi.appendEntry(REVIEW_STATE_TYPE, { active: false });
+ await clearReviewState(ctx, pi);
if (result.cancelled) {
ctx.ui.notify("Navigation cancelled", "info");
@@ -1709,9 +1760,7 @@ Preserve exact file paths, function names, and error messages.
}
// Clear state only on success
- setReviewWidget(ctx, false);
- reviewOriginId = undefined;
- pi.appendEntry(REVIEW_STATE_TYPE, { active: false });
+ await clearReviewState(ctx, pi);
ctx.ui.notify("Review complete! Returned to original position.", "info");
} catch (error) {
// Keep state so they can try again