diff --git a/.changeset/review-pull-request-by-number.md b/.changeset/review-pull-request-by-number.md new file mode 100644 index 000000000..0dc1f8613 --- /dev/null +++ b/.changeset/review-pull-request-by-number.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": minor +--- + +Review a GitHub pull request directly with `hunk diff --pr `. Hunk shells out to the authenticated GitHub CLI (`gh pr diff --patch`) and feeds the result through the existing patch pipeline, so a PR opens without a manual `gh pr checkout`. Pass `--repo ` to target a repository other than the current directory. diff --git a/src/core/cli.test.ts b/src/core/cli.test.ts index 33cf1bc81..b7bf3f8d0 100644 --- a/src/core/cli.test.ts +++ b/src/core/cli.test.ts @@ -186,6 +186,41 @@ describe("parseCli", () => { expect(cached).toMatchObject({ kind: "vcs", staged: true }); }); + test("parses a GitHub pull request into a patch review", async () => { + const originalSpawn = Bun.spawn; + const mutableBun = Bun as unknown as { spawn: typeof Bun.spawn }; + const patch = "diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -1 +1 @@\n-x\n+y\n"; + + mutableBun.spawn = ((command: string[]) => + originalSpawn( + [ + process.execPath, + "--eval", + `process.stdout.write(${JSON.stringify(patch)});` + + `globalThis.__hunkPrArgs = ${JSON.stringify(command)};`, + ], + { stdin: "ignore", stdout: "pipe", stderr: "pipe" }, + )) as typeof Bun.spawn; + + try { + const parsed = await parseCli(["bun", "hunk", "diff", "--pr", "68"]); + + expect(parsed).toMatchObject({ + kind: "patch", + text: patch, + label: "PR #68", + }); + } finally { + mutableBun.spawn = originalSpawn; + } + }); + + test("rejects combining --pr with a target", async () => { + await expect(parseCli(["bun", "hunk", "diff", "--pr", "68", "HEAD~1"])).rejects.toThrow( + /--pr` cannot be combined/, + ); + }); + test("parses untracked file toggles for git diff", async () => { const excluded = await parseCli(["bun", "hunk", "diff", "--exclude-untracked"]); const included = await parseCli(["bun", "hunk", "diff", "--no-exclude-untracked"]); diff --git a/src/core/cli.ts b/src/core/cli.ts index 38efebbb9..d8a6ebe3e 100644 --- a/src/core/cli.ts +++ b/src/core/cli.ts @@ -31,6 +31,7 @@ import { RELOAD_SEPARATOR_MESSAGE, } from "../session/agent/errors"; import { detectVcs } from "./vcs"; +import { fetchPullRequestPatch } from "./github"; import { DEFAULT_TAB_WIDTH, parseTabWidth } from "./tabWidth"; import { resolveCliVersion } from "./version"; @@ -96,6 +97,14 @@ export const WATCH_OPTION = { const DIFF_OPTIONS = [ { flag: "--staged", description: "show staged changes instead of the working tree" }, { flag: "--cached", description: "alias for --staged" }, + { + flag: "--pr ", + description: "review a GitHub pull request via `gh pr diff` (requires gh)", + }, + { + flag: "--repo ", + description: "target repository for --pr (defaults to the current directory)", + }, AUXILIARY_AGENT_OPTIONS.excludeUntracked, { flag: `--no-${AUXILIARY_AGENT_OPTIONS.excludeUntracked.flag.slice(2)}`, @@ -113,6 +122,7 @@ export const CLI_REFERENCE_COMMANDS = { "hunk diff [target] [-- ]", "hunk diff --staged [-- ]", "hunk diff ", + "hunk diff --pr [--repo ]", ], options: DIFF_OPTIONS, commonReviewOptions: true, @@ -350,6 +360,7 @@ function renderCliHelp() { " hunk diff [target] [-- ] review working tree changes or compare against a target", " hunk diff --staged [-- ] review staged changes", " hunk diff compare two concrete files", + " hunk diff --pr review a GitHub pull request (requires gh)", " hunk show [target] [-- ] review the last commit or a given target", " hunk stash show [ref] review a stash entry (git only)", " hunk patch [file] review a patch file or stdin", @@ -630,6 +641,17 @@ async function parseDiffCommand(tokens: string[], argv: string[]): Promise 0 ? pathspecs : undefined; + const prRef = typeof parsedOptions.pr === "string" ? parsedOptions.pr : undefined; + if (prRef !== undefined) { + if (parsedTargets.length > 0 || staged || normalizedPathspecs) { + throw new Error("`--pr` cannot be combined with targets, `--staged`, or pathspecs."); + } + + const repo = typeof parsedOptions.repo === "string" ? parsedOptions.repo : undefined; + const { text, label } = await fetchPullRequestPatch(prRef, repo); + return { kind: "patch", text, label, options }; + } + if (parsedTargets.length === 0) { return { kind: "vcs", diff --git a/src/core/github.test.ts b/src/core/github.test.ts new file mode 100644 index 000000000..55f1599dd --- /dev/null +++ b/src/core/github.test.ts @@ -0,0 +1,101 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { fetchPullRequestPatch, GitHubCliError } from "./github"; + +const originalSpawn = Bun.spawn; +const mutableBun = Bun as unknown as { spawn: typeof Bun.spawn }; + +afterEach(() => { + mutableBun.spawn = originalSpawn; +}); + +/** + * Replace Bun.spawn with a fake that emits controlled stdout/stderr/exit via a + * real Node subprocess, recording the argv the code under test requested. + */ +function stubSpawn(options: { + stdout?: string; + stderr?: string; + exitCode?: number; + throwOnSpawn?: boolean; +}): { calls: string[][] } { + const calls: string[][] = []; + const { stdout = "", stderr = "", exitCode = 0, throwOnSpawn = false } = options; + + mutableBun.spawn = ((command: string[]) => { + calls.push(command); + if (throwOnSpawn) { + throw new Error("spawn ENOENT"); + } + + const script = + `process.stdout.write(${JSON.stringify(stdout)});` + + `process.stderr.write(${JSON.stringify(stderr)});` + + `process.exit(${exitCode});`; + + return originalSpawn([process.execPath, "--eval", script], { + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + }); + }) as typeof Bun.spawn; + + return { calls }; +} + +const SAMPLE_PATCH = `diff --git a/a.txt b/a.txt +index 0000000..1111111 100644 +--- a/a.txt ++++ b/a.txt +@@ -1 +1 @@ +-old ++new +`; + +describe("fetchPullRequestPatch", () => { + test("returns the patch text and a numbered label", async () => { + const { calls } = stubSpawn({ stdout: SAMPLE_PATCH }); + + const result = await fetchPullRequestPatch("68"); + + expect(result.text).toBe(SAMPLE_PATCH); + expect(result.label).toBe("PR #68"); + expect(calls[0]).toEqual(["gh", "pr", "diff", "68", "--patch"]); + }); + + test("passes --repo through to gh when provided", async () => { + const { calls } = stubSpawn({ stdout: SAMPLE_PATCH }); + + await fetchPullRequestPatch("68", "modem-dev/hunk"); + + expect(calls[0]).toEqual(["gh", "pr", "diff", "68", "--patch", "--repo", "modem-dev/hunk"]); + }); + + test("labels non-numeric refs without a hash", async () => { + stubSpawn({ stdout: SAMPLE_PATCH }); + + const result = await fetchPullRequestPatch("https://github.com/modem-dev/hunk/pull/68"); + + expect(result.label).toBe("PR https://github.com/modem-dev/hunk/pull/68"); + }); + + test("throws a friendly error when gh is not installed", async () => { + stubSpawn({ throwOnSpawn: true }); + + await expect(fetchPullRequestPatch("68")).rejects.toBeInstanceOf(GitHubCliError); + await expect(fetchPullRequestPatch("68")).rejects.toThrow(/GitHub CLI \(gh\)/); + }); + + test("surfaces gh stderr on a non-zero exit", async () => { + stubSpawn({ stderr: "no pull requests found for branch", exitCode: 1 }); + + await expect(fetchPullRequestPatch("999")).rejects.toThrow( + /Failed to fetch PR #999.*no pull requests found/s, + ); + }); + + test("rejects an empty diff", async () => { + stubSpawn({ stdout: "\n" }); + + await expect(fetchPullRequestPatch("68")).rejects.toThrow(/no diff to review/); + }); +}); diff --git a/src/core/github.ts b/src/core/github.ts new file mode 100644 index 000000000..2b43ec6db --- /dev/null +++ b/src/core/github.ts @@ -0,0 +1,81 @@ +/** + * Minimal GitHub CLI bridge used by `hunk diff --pr`. + * + * Hunk stays a local diff viewer; it does not talk to the GitHub API itself. + * Instead it shells out to the user's authenticated `gh` and reuses the + * existing patch pipeline, so a pull request becomes just another patch source. + */ + +/** One resolved pull request patch, ready to feed the patch loader. */ +export interface PullRequestPatch { + /** Unified diff text produced by `gh pr diff --patch`. */ + text: string; + /** Human-readable label such as `PR #68` shown in the review header. */ + label: string; +} + +/** Raised when `gh` is unavailable or the pull request cannot be fetched. */ +export class GitHubCliError extends Error { + constructor(message: string) { + super(message); + this.name = "GitHubCliError"; + } +} + +/** Normalize a `--pr` value into the label shown in the review header. */ +function describePullRequest(ref: string): string { + const trimmed = ref.trim(); + if (/^\d+$/.test(trimmed)) { + return `PR #${trimmed}`; + } + + return `PR ${trimmed}`; +} + +/** + * Fetch one pull request as a unified diff via `gh pr diff --patch`. + * + * `ref` accepts anything `gh` accepts: a PR number, URL, or branch name. + * `repo` maps to `gh --repo `; when omitted `gh` resolves the + * repository from the current directory. + */ +export async function fetchPullRequestPatch( + ref: string, + repo?: string, + ghExecutable = "gh", +): Promise { + const args = ["pr", "diff", ref, "--patch"]; + if (repo) { + args.push("--repo", repo); + } + + let proc: Bun.ReadableSubprocess; + try { + proc = Bun.spawn([ghExecutable, ...args], { + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + }); + } catch { + throw new GitHubCliError( + "`hunk diff --pr` requires the GitHub CLI (gh). Install it from https://cli.github.com and run `gh auth login`.", + ); + } + + const [exitCode, stdout, stderr] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]); + + if (exitCode !== 0) { + const detail = stderr.trim() || `gh exited with code ${exitCode}`; + throw new GitHubCliError(`Failed to fetch ${describePullRequest(ref)} via gh: ${detail}`); + } + + if (stdout.trim().length === 0) { + throw new GitHubCliError(`${describePullRequest(ref)} has no diff to review.`); + } + + return { text: stdout, label: describePullRequest(ref) }; +} diff --git a/src/core/loaders.ts b/src/core/loaders.ts index df1f3c3dd..8a8d74438 100644 --- a/src/core/loaders.ts +++ b/src/core/loaders.ts @@ -432,7 +432,7 @@ async function loadPatchChangeset( ? await new Response(Bun.stdin.stream()).text() : await Bun.file(resolvePath(cwd, input.file)).text()); - const label = input.file && input.file !== "-" ? input.file : "stdin patch"; + const label = input.file && input.file !== "-" ? input.file : (input.label ?? "stdin patch"); return normalizePatchChangeset( patchText, `Patch review: ${basename(label)}`, diff --git a/src/core/types.ts b/src/core/types.ts index fb9842418..4f8e75fa6 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -317,6 +317,11 @@ export interface PatchCommandInput { kind: "patch"; file?: string; text?: string; + /** + * Header label for in-memory patches that have no backing file, such as a + * pull request fetched via `hunk diff --pr`. Ignored when `file` is set. + */ + label?: string; options: CommonOptions; } diff --git a/website/src/content/docs/docs/reference/cli.md b/website/src/content/docs/docs/reference/cli.md index e8f0cb6b2..b997834ac 100644 --- a/website/src/content/docs/docs/reference/cli.md +++ b/website/src/content/docs/docs/reference/cli.md @@ -49,6 +49,7 @@ review diffs or compare two concrete files hunk diff [target] [-- ] hunk diff --staged [-- ] hunk diff +hunk diff --pr [--repo ] ``` ### Command-specific options @@ -57,6 +58,8 @@ hunk diff | ------------------------ | --------------------------------------------------------------------------------------------- | | `--staged` | show staged changes instead of the working tree | | `--cached` | alias for --staged | +| `--pr ` | review a GitHub pull request via `gh pr diff` (requires gh) | +| `--repo ` | target repository for --pr (defaults to the current directory) | | `--exclude-untracked` | exclude untracked files from working tree reviews | | `--no-exclude-untracked` | include untracked files in working tree reviews Compatibility inverse; omitted from `--help`. |