Merge PR #1942: Add /review PR URL support
This commit is contained in:
@@ -1052,6 +1052,7 @@
|
||||
|
||||
### Added
|
||||
|
||||
- Added `/review` support for explicit GitHub pull request URLs and detected PR URLs from recent conversation context ([#1743](https://github.com/can1357/oh-my-pi/issues/1743)).
|
||||
- Added an optional `fetch` option to `CustomToolContext` so custom tools can use a caller-provided HTTP implementation
|
||||
- Added optional `fetch` overrides to `ModelRegistry` construction and MCP/web search/tool network calls, enabling callers to inject custom HTTP clients instead of relying on global `fetch`
|
||||
- Added a `bash.enabled` setting to disable the model-facing bash tool while leaving user-initiated bang/RPC bash commands available.
|
||||
@@ -1142,6 +1143,10 @@
|
||||
|
||||
- Removed the special Anthropic `claude-opus-4-8` tool-call batch cap; sessions no longer abort an in-flight provider stream after a fixed number of completed tool calls.
|
||||
|
||||
### Added
|
||||
|
||||
- Added `/review` support for explicit GitHub pull request URLs and detected PR URLs from recent conversation context ([#1743](https://github.com/can1357/oh-my-pi/issues/1743)).
|
||||
|
||||
## [15.10.4] - 2026-06-08
|
||||
|
||||
### Added
|
||||
|
||||
@@ -17,6 +17,7 @@ import type { HookCommandContext } from "../../../../extensibility/hooks/types";
|
||||
import reviewCustomRequestTemplate from "../../../../prompts/review-custom-request.md" with { type: "text" };
|
||||
import reviewHeadlessRequestTemplate from "../../../../prompts/review-headless-request.md" with { type: "text" };
|
||||
import reviewRequestTemplate from "../../../../prompts/review-request.md" with { type: "text" };
|
||||
import * as gh from "../../../../tools/gh";
|
||||
import * as git from "../../../../utils/git";
|
||||
import * as jj from "../../../../utils/jj";
|
||||
|
||||
@@ -45,6 +46,25 @@ interface CurrentReviewDiff {
|
||||
mode: string;
|
||||
}
|
||||
|
||||
interface ReviewPrRef {
|
||||
repo: string;
|
||||
number: number;
|
||||
raw: string;
|
||||
kind: "github-url" | "pr-url";
|
||||
}
|
||||
|
||||
interface ParsedReviewArgs {
|
||||
prRef: ReviewPrRef | undefined;
|
||||
extraInstructions: string;
|
||||
}
|
||||
|
||||
type ReviewMenuChoice =
|
||||
| { kind: "detected-pr"; ref: ReviewPrRef }
|
||||
| { kind: "base-branch" }
|
||||
| { kind: "uncommitted" }
|
||||
| { kind: "commit" }
|
||||
| { kind: "custom" };
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Exclusion patterns for noise files
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
@@ -204,6 +224,7 @@ function getDiffPreview(hunks: string, maxLines: number): string {
|
||||
const MAX_DIFF_CHARS = 50_000; // Don't include diff above this
|
||||
const MAX_FILES_FOR_INLINE_DIFF = 20; // Don't include diff if more files than this
|
||||
const DEFAULT_LARGE_DIFF_INSTRUCTION = "MUST run `git diff`/`git show` for assigned files";
|
||||
const DEFAULT_CONTEXT_INSTRUCTION = "MAY read full file context as needed via `read`";
|
||||
const GIT_UNCOMMITTED_DIFF_INSTRUCTION =
|
||||
"MUST run both `git diff -- <path>` and `git diff --cached -- <path>` for assigned files";
|
||||
const JJ_UNCOMMITTED_DIFF_INSTRUCTION = "MUST run `jj --ignore-working-copy diff --git -- <path>` for assigned files";
|
||||
@@ -215,7 +236,7 @@ function buildReviewPrompt(
|
||||
mode: string,
|
||||
stats: DiffStats,
|
||||
rawDiff: string,
|
||||
options: { additionalInstructions?: string; diffInstruction?: string } = {},
|
||||
options: { additionalInstructions?: string; diffInstruction?: string; contextInstruction?: string } = {},
|
||||
): string {
|
||||
const agentCount = getRecommendedAgentCount(stats);
|
||||
const skipDiff = rawDiff.length > MAX_DIFF_CHARS || stats.files.length > MAX_FILES_FOR_INLINE_DIFF;
|
||||
@@ -242,6 +263,7 @@ function buildReviewPrompt(
|
||||
linesPerFile,
|
||||
additionalInstructions: options.additionalInstructions,
|
||||
diffInstruction: options.diffInstruction ?? DEFAULT_LARGE_DIFF_INSTRUCTION,
|
||||
contextInstruction: options.contextInstruction ?? DEFAULT_CONTEXT_INSTRUCTION,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -253,6 +275,203 @@ function buildHeadlessReviewPrompt(focus?: string): string {
|
||||
return prompt.render(reviewHeadlessRequestTemplate, { focus });
|
||||
}
|
||||
|
||||
const REVIEW_CONTEXT_PR_LIMIT = 3;
|
||||
const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9_.-]+$/;
|
||||
const PR_SCHEME_PATTERN = /^pr:\/\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/([1-9]\d*)(?:\/diff(?:\/(?:all|[1-9]\d*))?)?$/;
|
||||
const PR_REF_TEXT_PATTERN = /https:\/\/github\.com\/[^\s<>"']+|pr:\/\/[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\/[^\s<>"']+/g;
|
||||
|
||||
function stripTrailingPrRefPunctuation(text: string): string {
|
||||
return text.replace(/[.,)\]>]+$/g, "");
|
||||
}
|
||||
|
||||
function isValidRepoSegment(segment: string | undefined): segment is string {
|
||||
return segment !== undefined && REPO_SEGMENT_PATTERN.test(segment);
|
||||
}
|
||||
|
||||
function parsePositivePrNumber(value: string | undefined): number | undefined {
|
||||
if (value === undefined || !/^[1-9]\d*$/.test(value)) return undefined;
|
||||
const parsed = Number(value);
|
||||
return Number.isSafeInteger(parsed) ? parsed : undefined;
|
||||
}
|
||||
|
||||
function parseGithubPrUrl(text: string): ReviewPrRef | undefined {
|
||||
let url: URL;
|
||||
try {
|
||||
url = new URL(text);
|
||||
} catch {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
if (url.protocol !== "https:" || url.hostname !== "github.com") return undefined;
|
||||
|
||||
const parts = url.pathname.split("/").filter(Boolean);
|
||||
if (parts.length < 4 || parts[2] !== "pull") return undefined;
|
||||
|
||||
const [owner, repo, , numberPart] = parts;
|
||||
if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return undefined;
|
||||
|
||||
const number = parsePositivePrNumber(numberPart);
|
||||
if (number === undefined) return undefined;
|
||||
|
||||
return { repo: `${owner}/${repo}`, number, raw: text, kind: "github-url" };
|
||||
}
|
||||
|
||||
function parsePrSchemeRef(text: string): ReviewPrRef | undefined {
|
||||
const match = PR_SCHEME_PATTERN.exec(text);
|
||||
if (!match) return undefined;
|
||||
|
||||
const [, owner, repo, numberPart] = match;
|
||||
const number = parsePositivePrNumber(numberPart);
|
||||
if (number === undefined) return undefined;
|
||||
|
||||
return { repo: `${owner}/${repo}`, number, raw: text, kind: "pr-url" };
|
||||
}
|
||||
|
||||
function parseReviewPrRef(text: string): ReviewPrRef | undefined {
|
||||
const candidate = stripTrailingPrRefPunctuation(text);
|
||||
return parseGithubPrUrl(candidate) ?? parsePrSchemeRef(candidate);
|
||||
}
|
||||
|
||||
function buildPrLargeDiffInstruction(ref: ReviewPrRef): string {
|
||||
const prDiffUrl = `pr://${ref.repo}/${ref.number}/diff`;
|
||||
return `MUST read assigned PR file diffs from \`${prDiffUrl}/all\` or per-file \`${prDiffUrl}/<index>\`; NEVER use local \`git diff\`/\`git show\` for PR diff content`;
|
||||
}
|
||||
|
||||
function buildPrContextInstruction(ref: ReviewPrRef): string {
|
||||
const prDiffUrl = `pr://${ref.repo}/${ref.number}/diff`;
|
||||
return `MUST NOT read local workspace files for PR file context; use the fetched PR diff and \`${prDiffUrl}/all\` or per-file \`${prDiffUrl}/<index>\` only`;
|
||||
}
|
||||
|
||||
function extractReviewPrRefFromArgs(args: string[]): ParsedReviewArgs {
|
||||
let prRef: ReviewPrRef | undefined;
|
||||
let prRefIndex = -1;
|
||||
for (const [idx, arg] of args.entries()) {
|
||||
const parsed = parseReviewPrRef(arg);
|
||||
if (parsed) {
|
||||
prRef = parsed;
|
||||
prRefIndex = idx;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
prRef,
|
||||
extraInstructions: args.filter((_, idx) => idx !== prRefIndex).join(" "),
|
||||
};
|
||||
}
|
||||
|
||||
function extractReviewPrRefsFromText(text: string): ReviewPrRef[] {
|
||||
return Array.from(text.matchAll(PR_REF_TEXT_PATTERN), match => parseReviewPrRef(match[0])).filter(
|
||||
(ref): ref is ReviewPrRef => ref !== undefined,
|
||||
);
|
||||
}
|
||||
|
||||
function buildReviewPromptFromDiff(
|
||||
ctx: HookCommandContext,
|
||||
mode: string,
|
||||
diffText: string,
|
||||
extraInstructions: string | undefined,
|
||||
emptyMessage: string,
|
||||
options: { diffInstruction?: string; filteredMessage?: string; contextInstruction?: string } = {},
|
||||
): string | undefined {
|
||||
if (!diffText.trim()) {
|
||||
if (ctx.hasUI) ctx.ui.notify(emptyMessage, "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const stats = parseDiff(diffText);
|
||||
if (stats.files.length === 0) {
|
||||
if (ctx.hasUI)
|
||||
ctx.ui.notify(options.filteredMessage ?? "No reviewable files (all changes filtered out)", "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
return buildReviewPrompt(mode, stats, diffText, {
|
||||
additionalInstructions: extraInstructions,
|
||||
diffInstruction: options.diffInstruction,
|
||||
contextInstruction: options.contextInstruction,
|
||||
});
|
||||
}
|
||||
|
||||
async function buildPrReviewPrompt(
|
||||
api: CustomCommandAPI,
|
||||
ctx: HookCommandContext,
|
||||
ref: ReviewPrRef,
|
||||
extraInstructions: string,
|
||||
): Promise<string | undefined> {
|
||||
let diffText: string;
|
||||
try {
|
||||
const lookup = await gh.getOrFetchPrDiff({ cwd: api.cwd, repo: ref.repo, number: ref.number });
|
||||
diffText = lookup.payload.unified;
|
||||
} catch (err) {
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
const failure = `Failed to fetch PR diff for ${ref.repo}#${ref.number}: ${message}`;
|
||||
if (ctx.hasUI) {
|
||||
ctx.ui.notify(failure, "error");
|
||||
return undefined;
|
||||
}
|
||||
return failure;
|
||||
}
|
||||
|
||||
const promptText = buildReviewPromptFromDiff(
|
||||
ctx,
|
||||
`PR ${ref.repo}#${ref.number}`,
|
||||
diffText,
|
||||
extraInstructions || undefined,
|
||||
`PR ${ref.repo}#${ref.number} has no diff content available`,
|
||||
{ diffInstruction: buildPrLargeDiffInstruction(ref), contextInstruction: buildPrContextInstruction(ref) },
|
||||
);
|
||||
if (promptText !== undefined || ctx.hasUI) return promptText;
|
||||
return `Unable to review PR ${ref.repo}#${ref.number}: no diff content available.`;
|
||||
}
|
||||
|
||||
function isRecord(value: unknown): value is Record<string, unknown> {
|
||||
return typeof value === "object" && value !== null;
|
||||
}
|
||||
|
||||
function getTextContentParts(content: unknown): string[] {
|
||||
if (typeof content === "string") return [content];
|
||||
if (!Array.isArray(content)) return [];
|
||||
|
||||
const parts: string[] = [];
|
||||
for (const item of content) {
|
||||
if (isRecord(item) && item.type === "text" && typeof item.text === "string") {
|
||||
parts.push(item.text);
|
||||
}
|
||||
}
|
||||
return parts;
|
||||
}
|
||||
|
||||
function findRecentPrRefs(ctx: HookCommandContext, limit: number): ReviewPrRef[] {
|
||||
const refs: ReviewPrRef[] = [];
|
||||
const seen = new Set<string>();
|
||||
const entries = ctx.sessionManager.getBranch();
|
||||
|
||||
for (let idx = entries.length - 1; idx >= 0 && refs.length < limit; idx--) {
|
||||
const entry = entries[idx];
|
||||
if (entry?.type !== "message") continue;
|
||||
const message = entry.message;
|
||||
if (message.role !== "user" && message.role !== "assistant") continue;
|
||||
|
||||
const parts = getTextContentParts(message.content);
|
||||
for (let partIdx = parts.length - 1; partIdx >= 0; partIdx--) {
|
||||
const part = parts[partIdx];
|
||||
const partRefs = extractReviewPrRefsFromText(part);
|
||||
for (let refIdx = partRefs.length - 1; refIdx >= 0; refIdx--) {
|
||||
const ref = partRefs[refIdx];
|
||||
const key = `${ref.repo.toLowerCase()}#${ref.number}`;
|
||||
if (seen.has(key)) continue;
|
||||
seen.add(key);
|
||||
refs.push(ref);
|
||||
if (refs.length >= limit) break;
|
||||
}
|
||||
if (refs.length >= limit) break;
|
||||
}
|
||||
}
|
||||
|
||||
return refs;
|
||||
}
|
||||
|
||||
export class ReviewCommand implements CustomCommand {
|
||||
name = "review";
|
||||
description = "Launch interactive code review";
|
||||
@@ -260,36 +479,56 @@ export class ReviewCommand implements CustomCommand {
|
||||
constructor(private api: CustomCommandAPI) {}
|
||||
|
||||
async execute(args: string[], ctx: HookCommandContext): Promise<string | undefined> {
|
||||
if (!ctx.hasUI) {
|
||||
return buildHeadlessReviewPrompt(args.length > 0 ? args.join(" ") : undefined);
|
||||
const parsedArgs = extractReviewPrRefFromArgs(args);
|
||||
if (parsedArgs.prRef) {
|
||||
return buildPrReviewPrompt(this.api, ctx, parsedArgs.prRef, parsedArgs.extraInstructions);
|
||||
}
|
||||
|
||||
// Inline args act as additional instructions appended to the generated prompt.
|
||||
// When present, skip option 4 (editor) — the args already provide the instructions.
|
||||
const extraInstructions = args.length > 0 ? args.join(" ") : undefined;
|
||||
const extraInstructions = parsedArgs.extraInstructions || undefined;
|
||||
if (!ctx.hasUI) {
|
||||
return buildHeadlessReviewPrompt(extraInstructions);
|
||||
}
|
||||
|
||||
const menuItems = extraInstructions
|
||||
? [
|
||||
"1. Review against a base branch (PR Style)",
|
||||
"2. Review uncommitted changes",
|
||||
"3. Review a specific commit",
|
||||
]
|
||||
: [
|
||||
"1. Review against a base branch (PR Style)",
|
||||
"2. Review uncommitted changes",
|
||||
"3. Review a specific commit",
|
||||
"4. Custom review instructions",
|
||||
];
|
||||
const choices: Array<{ label: string; value: ReviewMenuChoice }> = [
|
||||
...findRecentPrRefs(ctx, REVIEW_CONTEXT_PR_LIMIT).map(ref => ({
|
||||
label: `Review PR ${ref.repo}#${ref.number} from conversation`,
|
||||
value: { kind: "detected-pr" as const, ref },
|
||||
})),
|
||||
{
|
||||
label: "1. Review against a base branch (PR Style)",
|
||||
value: { kind: "base-branch" },
|
||||
},
|
||||
{
|
||||
label: "2. Review uncommitted changes",
|
||||
value: { kind: "uncommitted" },
|
||||
},
|
||||
{
|
||||
label: "3. Review a specific commit",
|
||||
value: { kind: "commit" },
|
||||
},
|
||||
];
|
||||
|
||||
const mode = await ctx.ui.select("Review Mode", menuItems);
|
||||
if (!extraInstructions) {
|
||||
choices.push({
|
||||
label: "4. Custom review instructions",
|
||||
value: { kind: "custom" },
|
||||
});
|
||||
}
|
||||
|
||||
if (!mode) return undefined;
|
||||
const selected = await ctx.ui.select(
|
||||
"Review Mode",
|
||||
choices.map(choice => choice.label),
|
||||
);
|
||||
if (!selected) return undefined;
|
||||
|
||||
const modeNum = parseInt(mode[0], 10);
|
||||
const selectedChoice = choices.find(choice => choice.label === selected)?.value;
|
||||
if (!selectedChoice) return undefined;
|
||||
|
||||
switch (modeNum) {
|
||||
case 1: {
|
||||
// PR-style review against base branch
|
||||
switch (selectedChoice.kind) {
|
||||
case "detected-pr":
|
||||
return buildPrReviewPrompt(this.api, ctx, selectedChoice.ref, extraInstructions ?? "");
|
||||
|
||||
case "base-branch": {
|
||||
const branches = await getGitBranches(this.api);
|
||||
if (branches.length === 0) {
|
||||
ctx.ui.notify("No git branches found", "error");
|
||||
@@ -308,62 +547,43 @@ export class ReviewCommand implements CustomCommand {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
if (!diffText.trim()) {
|
||||
ctx.ui.notify(`No changes between ${baseBranch} and ${currentBranch}`, "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const stats = parseDiff(diffText);
|
||||
if (stats.files.length === 0) {
|
||||
ctx.ui.notify("No reviewable files (all changes filtered out)", "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
return buildReviewPrompt(
|
||||
return buildReviewPromptFromDiff(
|
||||
ctx,
|
||||
`Reviewing changes between \`${baseBranch}\` and \`${currentBranch}\` (PR-style)`,
|
||||
stats,
|
||||
diffText,
|
||||
{ additionalInstructions: extraInstructions },
|
||||
extraInstructions,
|
||||
`No changes between ${baseBranch} and ${currentBranch}`,
|
||||
);
|
||||
}
|
||||
|
||||
case 2: {
|
||||
case "uncommitted": {
|
||||
const reviewDiff = await getUncommittedReviewDiff(this.api).catch(err => {
|
||||
ctx.ui.notify(`Failed to get diff: ${err instanceof Error ? err.message : String(err)}`, "error");
|
||||
return undefined;
|
||||
});
|
||||
if (!reviewDiff) return undefined;
|
||||
|
||||
if (!reviewDiff.diffText.trim()) {
|
||||
ctx.ui.notify(reviewDiff.emptyMessage ?? "No diff content found", "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const stats = parseDiff(reviewDiff.diffText);
|
||||
if (stats.files.length === 0) {
|
||||
ctx.ui.notify("No reviewable files (all changes filtered out)", "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
return buildReviewPrompt(reviewDiff.mode, stats, reviewDiff.diffText, {
|
||||
additionalInstructions: extraInstructions,
|
||||
diffInstruction: reviewDiff.diffInstruction,
|
||||
});
|
||||
return buildReviewPromptFromDiff(
|
||||
ctx,
|
||||
reviewDiff.mode,
|
||||
reviewDiff.diffText,
|
||||
extraInstructions,
|
||||
reviewDiff.emptyMessage ?? "No diff content found",
|
||||
{ diffInstruction: reviewDiff.diffInstruction },
|
||||
);
|
||||
}
|
||||
|
||||
case 3: {
|
||||
// Specific commit
|
||||
case "commit": {
|
||||
const commits = await getRecentCommits(this.api, 20);
|
||||
if (commits.length === 0) {
|
||||
ctx.ui.notify("No commits found", "error");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const selected = await ctx.ui.select("Select commit to review", commits);
|
||||
if (!selected) return undefined;
|
||||
const selectedCommit = await ctx.ui.select("Select commit to review", commits);
|
||||
if (!selectedCommit) return undefined;
|
||||
|
||||
// Extract commit hash from selection (format: "abc1234 message")
|
||||
const hash = selected.split(" ")[0];
|
||||
const hash = selectedCommit.split(" ")[0];
|
||||
|
||||
let diffText: string;
|
||||
try {
|
||||
@@ -373,24 +593,17 @@ export class ReviewCommand implements CustomCommand {
|
||||
return undefined;
|
||||
}
|
||||
|
||||
if (!diffText.trim()) {
|
||||
ctx.ui.notify("Commit has no diff content", "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
const stats = parseDiff(diffText);
|
||||
if (stats.files.length === 0) {
|
||||
ctx.ui.notify("No reviewable files in commit (all changes filtered out)", "warning");
|
||||
return undefined;
|
||||
}
|
||||
|
||||
return buildReviewPrompt(`Reviewing commit \`${hash}\``, stats, diffText, {
|
||||
additionalInstructions: extraInstructions,
|
||||
});
|
||||
return buildReviewPromptFromDiff(
|
||||
ctx,
|
||||
`Reviewing commit \`${hash}\``,
|
||||
diffText,
|
||||
extraInstructions,
|
||||
"Commit has no diff content",
|
||||
{ filteredMessage: "No reviewable files in commit (all changes filtered out)" },
|
||||
);
|
||||
}
|
||||
|
||||
case 4: {
|
||||
// Custom instructions with opportunistic current-diff context.
|
||||
case "custom": {
|
||||
const instructions = await ctx.ui.editor(
|
||||
"Enter custom review instructions",
|
||||
"Review the following:\n\n",
|
||||
@@ -403,7 +616,6 @@ export class ReviewCommand implements CustomCommand {
|
||||
|
||||
if (reviewDiff?.diffText.trim()) {
|
||||
const stats = parseDiff(reviewDiff.diffText);
|
||||
// Even if all files filtered, include the custom instructions
|
||||
return buildReviewPrompt(
|
||||
`Custom review: ${instructions.split("\n")[0].slice(0, 60)}…`,
|
||||
stats,
|
||||
@@ -417,9 +629,6 @@ export class ReviewCommand implements CustomCommand {
|
||||
|
||||
return buildCustomReviewPrompt(instructions);
|
||||
}
|
||||
|
||||
default:
|
||||
return undefined;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -37,7 +37,7 @@ Group files by locality, e.g.:
|
||||
Reviewer MUST:
|
||||
1. Focus ONLY on assigned files
|
||||
2. {{#if skipDiff}}{{diffInstruction}}{{else}}MUST use diff hunks below (NEVER re-run git diff){{/if}}
|
||||
3. MAY read full file context as needed via `read`
|
||||
3. {{contextInstruction}}
|
||||
4. Call `report_finding` per issue
|
||||
5. Call `yield` with verdict when done
|
||||
|
||||
|
||||
@@ -1,10 +1,13 @@
|
||||
import { afterEach, describe, expect, it, spyOn } from "bun:test";
|
||||
import { afterEach, describe, expect, it, spyOn, vi } from "bun:test";
|
||||
import * as fs from "node:fs/promises";
|
||||
import * as os from "node:os";
|
||||
import * as path from "node:path";
|
||||
import { ReviewCommand } from "@oh-my-pi/pi-coding-agent/extensibility/custom-commands/bundled/review";
|
||||
import type { CustomCommandAPI } from "@oh-my-pi/pi-coding-agent/extensibility/custom-commands/types";
|
||||
import type { HookCommandContext } from "@oh-my-pi/pi-coding-agent/extensibility/hooks/types";
|
||||
import type { SessionEntry } from "@oh-my-pi/pi-coding-agent/session/session-manager";
|
||||
import type { PrDiffPayload, ViewLookupResult } from "@oh-my-pi/pi-coding-agent/tools/gh";
|
||||
import * as gh from "@oh-my-pi/pi-coding-agent/tools/gh";
|
||||
import * as git from "@oh-my-pi/pi-coding-agent/utils/git";
|
||||
import * as jj from "@oh-my-pi/pi-coding-agent/utils/jj";
|
||||
|
||||
@@ -16,6 +19,61 @@ const SAMPLE_JJ_DIFF = `diff --git a/src/workspace.ts b/src/workspace.ts
|
||||
+export const value = 2;
|
||||
`;
|
||||
|
||||
const SAMPLE_PR_DIFF = `diff --git a/src/pr.ts b/src/pr.ts
|
||||
--- a/src/pr.ts
|
||||
+++ b/src/pr.ts
|
||||
@@ -1 +1 @@
|
||||
-export const pr = false;
|
||||
+export const pr = true;
|
||||
`;
|
||||
|
||||
function makeManyFileDiff(fileCount: number): string {
|
||||
return Array.from(
|
||||
{ length: fileCount },
|
||||
(_, idx) => `diff --git a/src/pr-${idx}.ts b/src/pr-${idx}.ts
|
||||
--- a/src/pr-${idx}.ts
|
||||
+++ b/src/pr-${idx}.ts
|
||||
@@ -1 +1 @@
|
||||
-export const pr${idx} = false;
|
||||
+export const pr${idx} = true;
|
||||
`,
|
||||
).join("\n");
|
||||
}
|
||||
|
||||
interface SelectCall {
|
||||
title: string;
|
||||
options: string[];
|
||||
}
|
||||
|
||||
interface NotifyCall {
|
||||
message: string;
|
||||
type: "info" | "warning" | "error" | undefined;
|
||||
}
|
||||
|
||||
function makePrDiffLookup(unified: string): ViewLookupResult<PrDiffPayload> {
|
||||
return {
|
||||
rendered: unified,
|
||||
sourceUrl: undefined,
|
||||
payload: { unified, files: [] },
|
||||
status: "fresh",
|
||||
fetchedAt: Date.now(),
|
||||
};
|
||||
}
|
||||
|
||||
function makeUserEntry(id: string, content: string): SessionEntry {
|
||||
return {
|
||||
type: "message",
|
||||
id,
|
||||
parentId: null,
|
||||
timestamp: "2026-06-05T00:00:00.000Z",
|
||||
message: {
|
||||
role: "user",
|
||||
content,
|
||||
timestamp: Date.now(),
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
interface EditorCall {
|
||||
title: string;
|
||||
prefill: string | undefined;
|
||||
@@ -26,6 +84,7 @@ describe("ReviewCommand", () => {
|
||||
let tmpDir: string | undefined;
|
||||
|
||||
afterEach(async () => {
|
||||
vi.restoreAllMocks();
|
||||
if (tmpDir) {
|
||||
await fs.rm(tmpDir, { recursive: true, force: true });
|
||||
tmpDir = undefined;
|
||||
@@ -39,13 +98,28 @@ describe("ReviewCommand", () => {
|
||||
|
||||
function createContext(options?: {
|
||||
selectedMode?: string;
|
||||
selectResults?: string[];
|
||||
editorValue?: string | undefined;
|
||||
sessionEntries?: SessionEntry[];
|
||||
branchEntries?: SessionEntry[];
|
||||
onEditorCall?: (call: EditorCall) => void;
|
||||
onSelectCall?: (call: SelectCall) => void;
|
||||
onNotify?: (call: NotifyCall) => void;
|
||||
}): HookCommandContext {
|
||||
const selectResults = [...(options?.selectResults ?? [])];
|
||||
return {
|
||||
hasUI: true,
|
||||
sessionManager: {
|
||||
getEntries: () => options?.sessionEntries ?? [],
|
||||
getBranch: () => options?.branchEntries ?? options?.sessionEntries ?? [],
|
||||
},
|
||||
ui: {
|
||||
select: () => Promise.resolve(options?.selectedMode ?? "4. Custom review instructions"),
|
||||
select: (title: string, selectOptions: string[]) => {
|
||||
options?.onSelectCall?.({ title, options: selectOptions });
|
||||
return Promise.resolve(
|
||||
selectResults.shift() ?? options?.selectedMode ?? "4. Custom review instructions",
|
||||
);
|
||||
},
|
||||
editor: (
|
||||
title: string,
|
||||
prefill?: string,
|
||||
@@ -55,7 +129,9 @@ describe("ReviewCommand", () => {
|
||||
options?.onEditorCall?.({ title, prefill, editorOptions });
|
||||
return Promise.resolve(options?.editorValue);
|
||||
},
|
||||
notify: () => {},
|
||||
notify: (message: string, type?: "info" | "warning" | "error") => {
|
||||
options?.onNotify?.({ message, type });
|
||||
},
|
||||
},
|
||||
} as unknown as HookCommandContext;
|
||||
}
|
||||
@@ -130,6 +206,7 @@ describe("ReviewCommand", () => {
|
||||
const promptText = result!;
|
||||
expect(promptText).toContain("src/workspace.ts");
|
||||
expect(promptText).toContain("+1/-1");
|
||||
expect(promptText).toContain("MAY read full file context as needed via `read`");
|
||||
expect(jjDiffSpy).toHaveBeenCalledWith(dir);
|
||||
expect(gitStatusSpy).not.toHaveBeenCalled();
|
||||
expect(gitDiffSpy).not.toHaveBeenCalled();
|
||||
@@ -169,6 +246,350 @@ describe("ReviewCommand", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("parses supported explicit PR URL formats", async () => {
|
||||
const dir = await createTempDir();
|
||||
const diffSpy = spyOn(gh, "getOrFetchPrDiff").mockResolvedValue(makePrDiffLookup(SAMPLE_PR_DIFF));
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = { hasUI: false } as unknown as HookCommandContext;
|
||||
|
||||
const cases = [
|
||||
"https://github.com/owner/repo/pull/123",
|
||||
"https://github.com/owner/repo/pull/123/",
|
||||
"https://github.com/owner/repo/pull/123?tab=files",
|
||||
"https://github.com/owner/repo/pull/123#discussion_r123",
|
||||
"https://github.com/owner/repo/pull/123/files",
|
||||
"https://github.com/owner/repo/pull/123/commits",
|
||||
"pr://owner/repo/123/diff/all",
|
||||
"pr://owner/repo/123/diff/1",
|
||||
];
|
||||
|
||||
for (const url of cases) {
|
||||
const result = await command.execute([url], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain("PR owner/repo#123");
|
||||
expect(diffSpy).toHaveBeenCalledWith({ cwd: dir, repo: "owner/repo", number: 123 });
|
||||
}
|
||||
});
|
||||
|
||||
it("prevents local file reads for PR URL reviews", async () => {
|
||||
const dir = await createTempDir();
|
||||
spyOn(gh, "getOrFetchPrDiff").mockResolvedValue(makePrDiffLookup(SAMPLE_PR_DIFF));
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = { hasUI: false } as unknown as HookCommandContext;
|
||||
|
||||
const result = await command.execute(["https://github.com/owner/repo/pull/123"], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain("MUST NOT read local workspace files for PR file context");
|
||||
expect(result!).toContain("`pr://owner/repo/123/diff/all`");
|
||||
expect(result!).toContain("per-file `pr://owner/repo/123/diff/<index>`");
|
||||
expect(result!).not.toContain("MAY read full file context as needed via `read`");
|
||||
});
|
||||
|
||||
it("uses PR diff URLs for omitted large PR diff instructions", async () => {
|
||||
const dir = await createTempDir();
|
||||
spyOn(gh, "getOrFetchPrDiff").mockResolvedValue(makePrDiffLookup(makeManyFileDiff(21)));
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = { hasUI: false } as unknown as HookCommandContext;
|
||||
|
||||
const result = await command.execute(["https://github.com/owner/repo/pull/123"], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain("MUST read assigned PR file diffs from `pr://owner/repo/123/diff/all`");
|
||||
expect(result!).toContain("per-file `pr://owner/repo/123/diff/<index>`");
|
||||
expect(result!).toContain("NEVER use local `git diff`/`git show` for PR diff content");
|
||||
expect(result!).not.toContain("MUST run `git diff`/`git show` for assigned files");
|
||||
});
|
||||
|
||||
it("rejects unsupported PR-like URL formats as normal instructions", async () => {
|
||||
const diffSpy = spyOn(gh, "getOrFetchPrDiff").mockResolvedValue(makePrDiffLookup(SAMPLE_PR_DIFF));
|
||||
const command = new ReviewCommand({ cwd: "/tmp" } as unknown as CustomCommandAPI);
|
||||
const ctx = { hasUI: false } as unknown as HookCommandContext;
|
||||
|
||||
const cases = [
|
||||
"https://github.com/owner/repo/issues/123",
|
||||
"https://github.com/owner/repo/commit/abc123",
|
||||
"https://example.com/owner/repo/pull/123",
|
||||
"pr://123",
|
||||
"https://github.com/owner/repo/pull/0",
|
||||
"https://github.com/owner/repo/pull/-1",
|
||||
"https://github.com/owner/repo/pull/not-a-number",
|
||||
];
|
||||
|
||||
for (const url of cases) {
|
||||
const result = await command.execute([url], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain(url);
|
||||
}
|
||||
expect(diffSpy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("removes only the first valid PR URL from extra instructions", async () => {
|
||||
const dir = await createTempDir();
|
||||
const diffSpy = spyOn(gh, "getOrFetchPrDiff").mockResolvedValue(makePrDiffLookup(SAMPLE_PR_DIFF));
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = { hasUI: false } as unknown as HookCommandContext;
|
||||
const secondUrl = "https://github.com/owner/repo/pull/456";
|
||||
|
||||
const result = await command.execute(["focus", "https://github.com/owner/repo/pull/123", "on", secondUrl], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain("focus on https://github.com/owner/repo/pull/456");
|
||||
expect(diffSpy).toHaveBeenCalledWith({ cwd: dir, repo: "owner/repo", number: 123 });
|
||||
});
|
||||
|
||||
it("bypasses the interactive menu for explicit PR URLs", async () => {
|
||||
const dir = await createTempDir();
|
||||
const diffSpy = spyOn(gh, "getOrFetchPrDiff").mockResolvedValue(makePrDiffLookup(SAMPLE_PR_DIFF));
|
||||
let selectCalled = false;
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
onSelectCall: () => {
|
||||
selectCalled = true;
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute(["https://github.com/owner/repo/pull/123", "focus", "on", "CLI", "UX"], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain("focus on CLI UX");
|
||||
expect(selectCalled).toBe(false);
|
||||
expect(diffSpy).toHaveBeenCalledWith({ cwd: dir, repo: "owner/repo", number: 123 });
|
||||
});
|
||||
|
||||
it("notifies and stops when explicit PR diff fetching fails", async () => {
|
||||
const dir = await createTempDir();
|
||||
spyOn(gh, "getOrFetchPrDiff").mockRejectedValue(new Error("authentication required"));
|
||||
const notifications: NotifyCall[] = [];
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
onNotify: call => {
|
||||
notifications.push(call);
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute(["https://github.com/owner/repo/pull/123"], ctx);
|
||||
|
||||
expect(result).toBeUndefined();
|
||||
expect(notifications).toEqual([
|
||||
{
|
||||
message: "Failed to fetch PR diff for owner/repo#123: authentication required",
|
||||
type: "error",
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
it("notifies and stops when explicit PR diff content is empty", async () => {
|
||||
const dir = await createTempDir();
|
||||
spyOn(gh, "getOrFetchPrDiff").mockResolvedValue(makePrDiffLookup(" \n"));
|
||||
const notifications: NotifyCall[] = [];
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
onNotify: call => {
|
||||
notifications.push(call);
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute(["https://github.com/owner/repo/pull/123"], ctx);
|
||||
|
||||
expect(result).toBeUndefined();
|
||||
expect(notifications).toEqual([
|
||||
{
|
||||
message: "PR owner/repo#123 has no diff content available",
|
||||
type: "warning",
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
it("reviews a detected PR from recent conversation context", async () => {
|
||||
const dir = await createTempDir();
|
||||
const diffSpy = spyOn(gh, "getOrFetchPrDiff").mockResolvedValue(makePrDiffLookup(SAMPLE_PR_DIFF));
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
selectedMode: "Review PR owner/example#77 from conversation",
|
||||
sessionEntries: [makeUserEntry("u1", "Please review https://github.com/owner/example/pull/77.")],
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain("PR owner/example#77");
|
||||
expect(result!).toContain("src/pr.ts");
|
||||
expect(diffSpy).toHaveBeenCalledWith({ cwd: dir, repo: "owner/example", number: 77 });
|
||||
});
|
||||
|
||||
it("does not detect PR URLs from entries outside the current branch", async () => {
|
||||
const dir = await createTempDir();
|
||||
let reviewModeOptions: string[] = [];
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
editorValue: "Review docs",
|
||||
sessionEntries: [makeUserEntry("stale", "Stale https://github.com/owner/example/pull/77")],
|
||||
branchEntries: [],
|
||||
onSelectCall: call => {
|
||||
if (call.title === "Review Mode") reviewModeOptions = call.options;
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(reviewModeOptions).not.toContain("Review PR owner/example#77 from conversation");
|
||||
});
|
||||
|
||||
it("detects only PR URLs from the active branch path", async () => {
|
||||
const dir = await createTempDir();
|
||||
let reviewModeOptions: string[] = [];
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
editorValue: "Review docs",
|
||||
sessionEntries: [
|
||||
makeUserEntry("stale", "Stale https://github.com/owner/example/pull/77"),
|
||||
makeUserEntry("active", "Active https://github.com/owner/example/pull/78"),
|
||||
],
|
||||
branchEntries: [makeUserEntry("active", "Active https://github.com/owner/example/pull/78")],
|
||||
onSelectCall: call => {
|
||||
if (call.title === "Review Mode") reviewModeOptions = call.options;
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(reviewModeOptions).toContain("Review PR owner/example#78 from conversation");
|
||||
expect(reviewModeOptions).not.toContain("Review PR owner/example#77 from conversation");
|
||||
});
|
||||
|
||||
it("deduplicates detected PR menu entries", async () => {
|
||||
const dir = await createTempDir();
|
||||
let reviewModeOptions: string[] = [];
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
editorValue: "Review docs",
|
||||
sessionEntries: [
|
||||
makeUserEntry("u1", "Review https://github.com/owner/example/pull/77 and pr://owner/example/77/diff/1"),
|
||||
],
|
||||
onSelectCall: call => {
|
||||
if (call.title === "Review Mode") reviewModeOptions = call.options;
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(
|
||||
reviewModeOptions.filter(option => option === "Review PR owner/example#77 from conversation"),
|
||||
).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("orders detected PR menu entries by most recent mention", async () => {
|
||||
const dir = await createTempDir();
|
||||
let reviewModeOptions: string[] = [];
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
editorValue: "Review docs",
|
||||
sessionEntries: [
|
||||
makeUserEntry("u1", "Older https://github.com/owner/example/pull/77"),
|
||||
makeUserEntry("u2", "Newer https://github.com/owner/example/pull/78"),
|
||||
],
|
||||
onSelectCall: call => {
|
||||
if (call.title === "Review Mode") reviewModeOptions = call.options;
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(reviewModeOptions.slice(0, 2)).toEqual([
|
||||
"Review PR owner/example#78 from conversation",
|
||||
"Review PR owner/example#77 from conversation",
|
||||
]);
|
||||
});
|
||||
|
||||
it("orders detected PR menu entries by rightmost mention within one message", async () => {
|
||||
const dir = await createTempDir();
|
||||
let reviewModeOptions: string[] = [];
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
editorValue: "Review docs",
|
||||
sessionEntries: [
|
||||
makeUserEntry(
|
||||
"u1",
|
||||
"Older https://github.com/owner/example/pull/77 newer https://github.com/owner/example/pull/78",
|
||||
),
|
||||
],
|
||||
onSelectCall: call => {
|
||||
if (call.title === "Review Mode") reviewModeOptions = call.options;
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(reviewModeOptions.slice(0, 2)).toEqual([
|
||||
"Review PR owner/example#78 from conversation",
|
||||
"Review PR owner/example#77 from conversation",
|
||||
]);
|
||||
});
|
||||
|
||||
it("preserves the existing menu shape when no recent PR is detected", async () => {
|
||||
const dir = await createTempDir();
|
||||
let reviewModeOptions: string[] = [];
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
editorValue: "Review docs",
|
||||
onSelectCall: call => {
|
||||
if (call.title === "Review Mode") reviewModeOptions = call.options;
|
||||
},
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(reviewModeOptions).toEqual([
|
||||
"1. Review against a base branch (PR Style)",
|
||||
"2. Review uncommitted changes",
|
||||
"3. Review a specific commit",
|
||||
"4. Custom review instructions",
|
||||
]);
|
||||
});
|
||||
|
||||
it("keeps base branch review mode working", async () => {
|
||||
const dir = await createTempDir();
|
||||
spyOn(git.branch, "list").mockResolvedValue(["main"]);
|
||||
spyOn(git.branch, "current").mockResolvedValue("feature");
|
||||
const diffSpy = spyOn(git, "diff").mockResolvedValue(SAMPLE_PR_DIFF);
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
selectResults: ["1. Review against a base branch (PR Style)", "main"],
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain("Reviewing changes between `main` and `feature`");
|
||||
expect(result!).toContain("src/pr.ts");
|
||||
expect(diffSpy).toHaveBeenCalledWith(dir, { base: "main...feature" });
|
||||
});
|
||||
|
||||
it("keeps specific commit review mode working", async () => {
|
||||
const dir = await createTempDir();
|
||||
spyOn(git.log, "onelines").mockResolvedValue(["abc1234 Fix review command"]);
|
||||
const showSpy = spyOn(git, "show").mockResolvedValue(SAMPLE_PR_DIFF);
|
||||
const command = new ReviewCommand({ cwd: dir } as unknown as CustomCommandAPI);
|
||||
const ctx = createContext({
|
||||
selectResults: ["3. Review a specific commit", "abc1234 Fix review command"],
|
||||
});
|
||||
|
||||
const result = await command.execute([], ctx);
|
||||
|
||||
expect(result).toBeDefined();
|
||||
expect(result!).toContain("Reviewing commit `abc1234`");
|
||||
expect(result!).toContain("src/pr.ts");
|
||||
expect(showSpy).toHaveBeenCalledWith(dir, "abc1234", { format: "" });
|
||||
});
|
||||
it("renders headless review requests through the reviewer task prompt", async () => {
|
||||
const command = new ReviewCommand({ cwd: "/tmp" } as unknown as CustomCommandAPI);
|
||||
const ctx = { hasUI: false } as unknown as HookCommandContext;
|
||||
|
||||
Reference in New Issue
Block a user