From ff33c59c20771df5667555ecc30c469a20e861fa Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Fri, 5 Jun 2026 21:56:48 +0530 Subject: [PATCH 1/7] Add review command PR URL handling --- packages/coding-agent/CHANGELOG.md | 4 + .../custom-commands/bundled/review/index.ts | 349 ++++++++++++++---- 2 files changed, 274 insertions(+), 79 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index c624557ce..f4c51d7f8 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -14,6 +14,10 @@ ## [15.9.3] - 2026-06-05 +### 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)). + ### Fixed - Fixed `@`-mention auto-read injecting an unrelated, same-named file when a mention did not point at a real path — e.g. an npm scope like `@scope/`, a partial path, or a bare token. `generateFileMentionMessages` resolution previously fell back to prefix and repo-wide fuzzy matching (globbing the whole project on every such mention) and auto-read the single "best" guess. Resolution is now exact-only: a mention is auto-read only when it resolves to an existing file or directory; otherwise it is left as prose. The TUI `@`-selector already inserts the real, complete path before send, so post-send guessing was both unnecessary and the source of the wrong-file reads. Directories still resolve and are listed. Removes the per-mention `**/*` project scan. diff --git a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts index d66734a18..5296fbc8c 100644 --- a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts +++ b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts @@ -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 // ───────────────────────────────────────────────────────────────────────────── @@ -253,6 +273,187 @@ 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)?)?$/; +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 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 } = {}, +): 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, + }); +} + +async function buildPrReviewPrompt( + api: CustomCommandAPI, + ctx: HookCommandContext, + ref: ReviewPrRef, + extraInstructions: string, +): Promise { + 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`, + ); + 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 { + 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(); + const entries = ctx.sessionManager.getEntries(); + + 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; + + for (const part of getTextContentParts(message.content)) { + for (const ref of extractReviewPrRefsFromText(part)) { + 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 +461,56 @@ export class ReviewCommand implements CustomCommand { constructor(private api: CustomCommandAPI) {} async execute(args: string[], ctx: HookCommandContext): Promise { - 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 +529,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 +575,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 +598,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 +611,6 @@ export class ReviewCommand implements CustomCommand { return buildCustomReviewPrompt(instructions); } - - default: - return undefined; } } } From f109cd0e338ed1dbd5a6c7873fd57202f5b341f1 Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Fri, 5 Jun 2026 21:56:52 +0530 Subject: [PATCH 2/7] Cover review command PR URL flows --- .../custom-commands/review.test.ts | 310 +++++++++++++++++- 1 file changed, 307 insertions(+), 3 deletions(-) diff --git a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts index 48b1974b9..3dd388f9f 100644 --- a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts +++ b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts @@ -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 "../../../src/extensibility/custom-commands/bundled/review"; import type { CustomCommandAPI } from "../../../src/extensibility/custom-commands/types"; import type { HookCommandContext } from "../../../src/extensibility/hooks/types"; +import type { SessionEntry } from "../../../src/session/session-manager"; +import type { PrDiffPayload, ViewLookupResult } from "../../../src/tools/gh"; +import * as gh from "../../../src/tools/gh"; import * as git from "../../../src/utils/git"; import * as jj from "../../../src/utils/jj"; @@ -16,6 +19,48 @@ 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; +`; + +interface SelectCall { + title: string; + options: string[]; +} + +interface NotifyCall { + message: string; + type: "info" | "warning" | "error" | undefined; +} + +function makePrDiffLookup(unified: string): ViewLookupResult { + 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 +71,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 +85,26 @@ describe("ReviewCommand", () => { function createContext(options?: { selectedMode?: string; + selectResults?: string[]; editorValue?: string | undefined; + sessionEntries?: 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 ?? [], + }, 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 +114,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; } @@ -169,6 +230,249 @@ 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", + "pr://owner/repo/123/diff/all", + ]; + + 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("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("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/all"), + ], + 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("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; From 7f2c21eeb84b6ed7675866d03084d79834e17290 Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Fri, 5 Jun 2026 22:51:25 +0530 Subject: [PATCH 3/7] Address review command PR detection feedback --- .../custom-commands/bundled/review/index.ts | 12 ++-- .../custom-commands/review.test.ts | 72 +++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts index 5296fbc8c..f1108e068 100644 --- a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts +++ b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts @@ -303,7 +303,7 @@ function parseGithubPrUrl(text: string): ReviewPrRef | 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; + if (parts.length < 4 || parts[2] !== "pull") return undefined; const [owner, repo, , numberPart] = parts; if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) return undefined; @@ -431,7 +431,7 @@ function getTextContentParts(content: unknown): string[] { function findRecentPrRefs(ctx: HookCommandContext, limit: number): ReviewPrRef[] { const refs: ReviewPrRef[] = []; const seen = new Set(); - const entries = ctx.sessionManager.getEntries(); + const entries = ctx.sessionManager.getBranch(); for (let idx = entries.length - 1; idx >= 0 && refs.length < limit; idx--) { const entry = entries[idx]; @@ -439,8 +439,12 @@ function findRecentPrRefs(ctx: HookCommandContext, limit: number): ReviewPrRef[] const message = entry.message; if (message.role !== "user" && message.role !== "assistant") continue; - for (const part of getTextContentParts(message.content)) { - for (const ref of extractReviewPrRefsFromText(part)) { + 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); diff --git a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts index 3dd388f9f..3fe3e9f10 100644 --- a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts +++ b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts @@ -88,6 +88,7 @@ describe("ReviewCommand", () => { selectResults?: string[]; editorValue?: string | undefined; sessionEntries?: SessionEntry[]; + branchEntries?: SessionEntry[]; onEditorCall?: (call: EditorCall) => void; onSelectCall?: (call: SelectCall) => void; onNotify?: (call: NotifyCall) => void; @@ -97,6 +98,7 @@ describe("ReviewCommand", () => { hasUI: true, sessionManager: { getEntries: () => options?.sessionEntries ?? [], + getBranch: () => options?.branchEntries ?? options?.sessionEntries ?? [], }, ui: { select: (title: string, selectOptions: string[]) => { @@ -241,6 +243,8 @@ describe("ReviewCommand", () => { "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", ]; @@ -371,6 +375,48 @@ describe("ReviewCommand", () => { 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[] = []; @@ -417,6 +463,32 @@ describe("ReviewCommand", () => { ]); }); + 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[] = []; From 02bd6c52ba96a374e718e6e57250e77cf1cd5cce Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Fri, 5 Jun 2026 23:19:52 +0530 Subject: [PATCH 4/7] Support review command PR diff slice URLs --- .../src/extensibility/custom-commands/bundled/review/index.ts | 2 +- .../test/extensibility/custom-commands/review.test.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts index f1108e068..5231249e7 100644 --- a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts +++ b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts @@ -275,7 +275,7 @@ function buildHeadlessReviewPrompt(focus?: string): string { 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)?)?$/; +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 { diff --git a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts index 3fe3e9f10..2075409d6 100644 --- a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts +++ b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts @@ -246,6 +246,7 @@ describe("ReviewCommand", () => { "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) { @@ -424,7 +425,7 @@ describe("ReviewCommand", () => { const ctx = createContext({ editorValue: "Review docs", sessionEntries: [ - makeUserEntry("u1", "Review https://github.com/owner/example/pull/77 and pr://owner/example/77/diff/all"), + 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; From 2a4eb25365acdc4bd5bb6d8a96f083381a684358 Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Sat, 6 Jun 2026 02:47:30 +0530 Subject: [PATCH 5/7] Use PR diff URLs for large review prompts --- .../custom-commands/bundled/review/index.ts | 6 ++++ .../custom-commands/review.test.ts | 28 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts index 5231249e7..fa79e706b 100644 --- a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts +++ b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts @@ -330,6 +330,11 @@ function parseReviewPrRef(text: string): ReviewPrRef | undefined { 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}/\`; NEVER use local \`git diff\`/\`git show\` for PR diff content`; +} + function extractReviewPrRefFromArgs(args: string[]): ParsedReviewArgs { let prRef: ReviewPrRef | undefined; let prRefIndex = -1; @@ -406,6 +411,7 @@ async function buildPrReviewPrompt( diffText, extraInstructions || undefined, `PR ${ref.repo}#${ref.number} has no diff content available`, + { diffInstruction: buildPrLargeDiffInstruction(ref) }, ); if (promptText !== undefined || ctx.hasUI) return promptText; return `Unable to review PR ${ref.repo}#${ref.number}: no diff content available.`; diff --git a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts index 2075409d6..54b95fac3 100644 --- a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts +++ b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts @@ -27,6 +27,19 @@ const SAMPLE_PR_DIFF = `diff --git a/src/pr.ts b/src/pr.ts +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[]; @@ -258,6 +271,21 @@ describe("ReviewCommand", () => { } }); + 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/`"); + 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); From 586e9435002481557ef110b4d08a0d2e8eb9ed42 Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Sat, 6 Jun 2026 03:05:24 +0530 Subject: [PATCH 6/7] Move review URL changelog entry to Unreleased --- packages/coding-agent/CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f4c51d7f8..64bc5fc57 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### 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.9.4] - 2026-06-05 ### Fixed @@ -14,10 +18,6 @@ ## [15.9.3] - 2026-06-05 -### 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)). - ### Fixed - Fixed `@`-mention auto-read injecting an unrelated, same-named file when a mention did not point at a real path — e.g. an npm scope like `@scope/`, a partial path, or a bare token. `generateFileMentionMessages` resolution previously fell back to prefix and repo-wide fuzzy matching (globbing the whole project on every such mention) and auto-read the single "best" guess. Resolution is now exact-only: a mention is auto-read only when it resolves to an existing file or directory; otherwise it is left as prose. The TUI `@`-selector already inserts the real, complete path before send, so post-send guessing was both unnecessary and the source of the wrong-file reads. Directories still resolve and are listed. Removes the per-mention `**/*` project scan. From b2a68398f0c3bcff6b7dcc24994bb07b77cc5f5c Mon Sep 17 00:00:00 2001 From: Mathews-Tom Date: Sat, 6 Jun 2026 21:13:46 +0530 Subject: [PATCH 7/7] Avoid local reads in PR review prompts --- .../custom-commands/bundled/review/index.ts | 14 +++++++++++--- .../coding-agent/src/prompts/review-request.md | 2 +- .../extensibility/custom-commands/review.test.ts | 16 ++++++++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts index fa79e706b..a320e3f86 100644 --- a/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts +++ b/packages/coding-agent/src/extensibility/custom-commands/bundled/review/index.ts @@ -224,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 -- ` and `git diff --cached -- ` for assigned files"; const JJ_UNCOMMITTED_DIFF_INSTRUCTION = "MUST run `jj --ignore-working-copy diff --git -- ` for assigned files"; @@ -235,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; @@ -262,6 +263,7 @@ function buildReviewPrompt( linesPerFile, additionalInstructions: options.additionalInstructions, diffInstruction: options.diffInstruction ?? DEFAULT_LARGE_DIFF_INSTRUCTION, + contextInstruction: options.contextInstruction ?? DEFAULT_CONTEXT_INSTRUCTION, }); } @@ -335,6 +337,11 @@ function buildPrLargeDiffInstruction(ref: ReviewPrRef): string { return `MUST read assigned PR file diffs from \`${prDiffUrl}/all\` or per-file \`${prDiffUrl}/\`; 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}/\` only`; +} + function extractReviewPrRefFromArgs(args: string[]): ParsedReviewArgs { let prRef: ReviewPrRef | undefined; let prRefIndex = -1; @@ -365,7 +372,7 @@ function buildReviewPromptFromDiff( diffText: string, extraInstructions: string | undefined, emptyMessage: string, - options: { diffInstruction?: string; filteredMessage?: string } = {}, + options: { diffInstruction?: string; filteredMessage?: string; contextInstruction?: string } = {}, ): string | undefined { if (!diffText.trim()) { if (ctx.hasUI) ctx.ui.notify(emptyMessage, "warning"); @@ -382,6 +389,7 @@ function buildReviewPromptFromDiff( return buildReviewPrompt(mode, stats, diffText, { additionalInstructions: extraInstructions, diffInstruction: options.diffInstruction, + contextInstruction: options.contextInstruction, }); } @@ -411,7 +419,7 @@ async function buildPrReviewPrompt( diffText, extraInstructions || undefined, `PR ${ref.repo}#${ref.number} has no diff content available`, - { diffInstruction: buildPrLargeDiffInstruction(ref) }, + { 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.`; diff --git a/packages/coding-agent/src/prompts/review-request.md b/packages/coding-agent/src/prompts/review-request.md index 655852556..b0c001d64 100644 --- a/packages/coding-agent/src/prompts/review-request.md +++ b/packages/coding-agent/src/prompts/review-request.md @@ -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 diff --git a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts index 54b95fac3..3dde8dcb2 100644 --- a/packages/coding-agent/test/extensibility/custom-commands/review.test.ts +++ b/packages/coding-agent/test/extensibility/custom-commands/review.test.ts @@ -206,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(); @@ -271,6 +272,21 @@ describe("ReviewCommand", () => { } }); + 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/`"); + 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)));