diff --git a/README.md b/README.md index 9658e8c8..42ecfb76 100644 --- a/README.md +++ b/README.md @@ -218,6 +218,14 @@ This checks out the PR and opens the diff against **the commit the pull request Above the diff you get the pull request's description and every review already on it, so you are not re-deriving intent from the code or repeating a point someone else has made. +When the checkout cannot name its pull request — a detached worktree at the PR head, which is what the review inbox prepares — pass the number and the commit the pull request is based on: + +```bash +diffity --pr 123 +``` + +The diff is pinned to that base, and the description, the reviews and the submit dialog appear as they do for a URL. + ### Submitting a review The forge dialog is a composer, not a push button: diff --git a/package-lock.json b/package-lock.json index 21723e93..8bf30df8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.12", + "version": "0.10.13", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.12", + "version": "0.10.13", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.12", + "version": "0.10.13", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.12", + "version": "0.10.13", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.12", + "version": "0.10.13", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.12", + "version": "0.10.13", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/packages/api/package.json b/packages/api/package.json index fba82eb1..6e6b63b1 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.12", + "version": "0.10.13", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index 48434772..c8507e86 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.12", + "version": "0.10.13", "description": "Agent-agnostic, GitHub-style diff viewer and code review tool with a live agent loop", "type": "module", "bin": { diff --git a/packages/cli/src/inbox/daemon.ts b/packages/cli/src/inbox/daemon.ts index d0f88490..325b4140 100644 --- a/packages/cli/src/inbox/daemon.ts +++ b/packages/cli/src/inbox/daemon.ts @@ -220,7 +220,7 @@ async function handleOpen(store: InboxStore, id: string, openDeps: OpenSessionDe return; } log(`opening ${id}`); - const { url, imported, importError } = await openPreparedSession(resolution.pr.worktreePath!, resolution.pr.bundlePath!, openDeps); + const { url, imported, importError } = await openPreparedSession(resolution.pr.worktreePath!, resolution.pr.bundlePath!, resolution.pr.number, openDeps); if (!imported) { log(`opened ${id} but its findings did not import: ${importError}`); } diff --git a/packages/cli/src/inbox/open-session.ts b/packages/cli/src/inbox/open-session.ts index 2a4674fb..20373dbc 100644 --- a/packages/cli/src/inbox/open-session.ts +++ b/packages/cli/src/inbox/open-session.ts @@ -5,13 +5,13 @@ import { checkInstanceHealth, findInstanceForRepo } from '../registry.js'; /** * Brings a prepared review up as a live diffity session the reviewer can open: a server over the - * worktree, diffing against the pull request's base, with the prepared findings imported. Returns - * the URL to send the browser to. Import failure is not fatal — the diff is still worth opening — - * but it is surfaced to the caller. + * worktree, diffing against the pull request's base and told which pull request it shows, with the + * prepared findings imported. Returns the URL to send the browser to. Import failure is not fatal — + * the diff is still worth opening — but it is surfaced to the caller. */ -export async function openPreparedSession(worktree: string, bundlePath: string, deps: OpenSessionDeps): Promise { +export async function openPreparedSession(worktree: string, bundlePath: string, prNumber: number, deps: OpenSessionDeps): Promise { const ref = deps.baseRefOf(bundlePath); - const port = await deps.ensureServer(worktree, ref); + const port = await deps.ensureServer(worktree, ref, prNumber); const url = sessionUrl(port, ref); try { deps.importBundle(worktree, bundlePath); @@ -46,14 +46,14 @@ export function baseRefOf(bundlePath: string): string { export function realOpenSessionDeps(nodePath: string, entry: string): OpenSessionDeps { return { baseRefOf, - ensureServer: (worktree, ref) => ensureServer(nodePath, entry, worktree, ref), + ensureServer: (worktree, ref, prNumber) => ensureServer(nodePath, entry, worktree, ref, prNumber), importBundle: (worktree, bundlePath) => { execFileSync(nodePath, [entry, '--repo', worktree, 'agent', 'import-bundle', bundlePath], { stdio: 'pipe' }); }, }; } -export async function ensureServer(nodePath: string, entry: string, worktree: string, ref: string, waitMs = 30_000): Promise { +export async function ensureServer(nodePath: string, entry: string, worktree: string, ref: string, prNumber: number | undefined, waitMs = 30_000): Promise { const hash = repoHash(worktree); // A healthy server already on this worktree is reused as-is. The worktree lives under the inbox's // own directory and is only ever served at the pull request's base, so its ref is the one wanted. @@ -62,7 +62,7 @@ export async function ensureServer(nodePath: string, entry: string, worktree: st return existing.port; } - const child = spawn(nodePath, [entry, '--repo', worktree, '--no-open', '--quiet', ref], { detached: true, stdio: 'ignore' }); + const child = spawn(nodePath, serverArgs(entry, worktree, ref, prNumber), { detached: true, stdio: 'ignore' }); child.unref(); const deadline = Date.now() + waitMs; @@ -77,6 +77,15 @@ export async function ensureServer(nodePath: string, entry: string, worktree: st throw new Error(`diffity did not start for ${worktree} within ${waitMs / 1000}s`); } +/** + * The argv that brings a worktree up as a session at the ref. A detached worktree cannot name its + * pull request, so the number goes along: it is what puts the description, the reviews and the + * submit dialog on the page. + */ +export function serverArgs(entry: string, worktree: string, ref: string, prNumber: number | undefined): string[] { + return [entry, '--repo', worktree, '--no-open', '--quiet', ...(prNumber !== undefined ? ['--pr', String(prNumber)] : []), ref]; +} + /** The server registers under the hash of its resolved repo root, so resolve symlinks before hashing. */ export function repoHash(worktree: string): string { let root = worktree; @@ -91,8 +100,8 @@ function sleep(ms: number): Promise { export interface OpenSessionDeps { /** Reads the base ref recorded in the bundle, so the session diffs the same change. */ baseRefOf(bundlePath: string): string; - /** Ensures a diffity server for the worktree at that ref and returns its port. */ - ensureServer(worktree: string, ref: string): Promise; + /** Ensures a diffity server for the worktree at that ref, told its pull request, and returns its port. */ + ensureServer(worktree: string, ref: string, prNumber: number): Promise; /** Adds the prepared review's threads and tours to the running session. */ importBundle(worktree: string, bundlePath: string): void; } diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 7156745a..4aeb6b3a 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -6,6 +6,7 @@ import { createRequire } from 'node:module'; import open from 'open'; import pc from 'picocolors'; import { isGitRepo, isValidGitRef, getRepoRoot, getRepoName, normalizeRef, getDiffityDirPath, isDataDirUntracked, WORKING_TREE_REFS } from '@diffity/git'; +import { pullRequestNumber } from './pr-number.js'; import type { PrBase } from '@diffity/github'; import { isGitHubPrUrl, @@ -76,6 +77,7 @@ program .option('--new', 'Stop existing instance and start fresh') .option('--work', 'You are working on this branch, so the agent may change code') .option('--review', 'You are reviewing it, so the agent may not — even if you wrote it') + .option('--pr ', 'The pull request this diff reviews, when the checkout cannot name it (a detached worktree at its head)', pullRequestNumber) .addHelpText('after', ` Common usage: $ diffity See all uncommitted changes @@ -87,6 +89,7 @@ Common usage: $ diffity staged Only staged changes $ diffity unstaged Only unstaged changes $ diffity https://github.com/owner/repo/pull/123 Review a GitHub PR + $ diffity --pr 123 Review a PR from a detached checkout at its head $ diffity --dark --unified Dark mode, unified view $ diffity --new Force restart existing instance @@ -210,6 +213,23 @@ range syntax (main..feature, main...feature) also work.`) } } + if (opts.pr !== undefined) { + if (parsedPrNumber !== undefined) { + console.error(pc.red('Error: Pass either a pull request URL or --pr, not both.')); + process.exit(1); + } + if (refs.length !== 1 || WORKING_TREE_REFS.has(refs[0])) { + console.error(pc.red('Error: --pr needs the commit the pull request is based on.')); + console.log(` Example: ${pc.cyan('diffity --pr 123 ')}`); + process.exit(1); + } + if (!detectRemote()) { + console.error(pc.red('Error: No GitHub remote detected for this repository.')); + process.exit(1); + } + parsedPrNumber = opts.pr; + } + for (let i = 0; i < refs.length; i++) { if (refs[i] === '.') { refs[i] = 'work'; @@ -332,7 +352,8 @@ range syntax (main..feature, main...feature) also work.`) diffArgs, description, effectiveRef, - pinnedRef: prBase?.oid, + // A session that names its pull request shows that pull request: /diff always comes back to its base. + pinnedRef: prBase?.oid ?? (opts.pr !== undefined ? effectiveRef : undefined), prNumber: parsedPrNumber, version: pkg.version, registryInfo: { repoRoot, repoHash, repoName }, diff --git a/packages/cli/src/pr-number.ts b/packages/cli/src/pr-number.ts new file mode 100644 index 00000000..e6b8e726 --- /dev/null +++ b/packages/cli/src/pr-number.ts @@ -0,0 +1,10 @@ +import { InvalidArgumentError } from 'commander'; + +/** What `--pr` accepts: the number of the pull request a checkout cannot name itself. */ +export function pullRequestNumber(value: string): number { + const parsed = Number(value); + if (!Number.isInteger(parsed) || parsed < 1) { + throw new InvalidArgumentError('A pull request number is a positive integer.'); + } + return parsed; +} diff --git a/packages/cli/tests/inbox-open.test.ts b/packages/cli/tests/inbox-open.test.ts index 3a10dbb6..c642fcab 100644 --- a/packages/cli/tests/inbox-open.test.ts +++ b/packages/cli/tests/inbox-open.test.ts @@ -6,7 +6,7 @@ import { dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { tmpdir } from 'node:os'; import { resolveDismiss, resolveOpen } from '../src/inbox/open.js'; -import { openPreparedSession, baseRefOf, ensureServer, repoHash, type OpenSessionDeps } from '../src/inbox/open-session.js'; +import { openPreparedSession, baseRefOf, ensureServer, repoHash, serverArgs, type OpenSessionDeps } from '../src/inbox/open-session.js'; import { startInboxServer } from '../src/inbox/daemon.js'; import { InboxStore } from '../src/inbox/store.js'; import { readRegistry, registerInstance } from '../src/registry.js'; @@ -107,14 +107,14 @@ describe('openPreparedSession', () => { const calls: string[] = []; const deps: OpenSessionDeps = { baseRefOf: () => 'basesha', - ensureServer: (wt, ref) => { calls.push(`ensure ${wt} ${ref}`); return Promise.resolve(5599); }, + ensureServer: (wt, ref, pr) => { calls.push(`ensure ${wt} ${ref} #${pr}`); return Promise.resolve(5599); }, importBundle: (wt, bundle) => { calls.push(`import ${wt} ${bundle}`); }, }; - const result = await openPreparedSession('/wt', '/b.json', deps); + const result = await openPreparedSession('/wt', '/b.json', 4, deps); expect(result).toEqual({ url: 'http://localhost:5599/diff?ref=basesha', imported: true }); - expect(calls).toEqual(['ensure /wt basesha', 'import /wt /b.json']); + expect(calls).toEqual(['ensure /wt basesha #4', 'import /wt /b.json']); }); it('still opens the diff when the import fails, flagging it', async () => { @@ -123,11 +123,20 @@ describe('openPreparedSession', () => { ensureServer: () => Promise.resolve(5599), importBundle: () => { throw new Error('head moved'); }, }; - const result = await openPreparedSession('/wt', '/b.json', deps); + const result = await openPreparedSession('/wt', '/b.json', 4, deps); expect(result).toEqual({ url: 'http://localhost:5599/diff?ref=basesha', imported: false, importError: 'head moved' }); }); }); +describe('serverArgs', () => { + it('names the pull request when it has one, and only then', () => { + expect(serverArgs('/e.js', '/wt', 'basesha', 14502)) + .toEqual(['/e.js', '--repo', '/wt', '--no-open', '--quiet', '--pr', '14502', 'basesha']); + expect(serverArgs('/e.js', '/wt', 'work', undefined)) + .toEqual(['/e.js', '--repo', '/wt', '--no-open', '--quiet', 'work']); + }); +}); + describe('the real ensureServer', () => { it('hashes a worktree the same way the diffity server it starts registers it', async () => { const prev = process.env.DIFFITY_DATA_DIR; @@ -142,7 +151,7 @@ describe('the real ensureServer', () => { let port = 0; try { - port = await ensureServer(process.execPath, ENTRY, repo, 'work', 20_000); + port = await ensureServer(process.execPath, ENTRY, repo, 'work', undefined, 20_000); // The entry the server registered must carry the hash open-session looks it up by. const entry = readRegistry().find(e => e.port === port); expect(entry).toBeDefined(); @@ -160,7 +169,7 @@ describe('the real ensureServer', () => { const idle = join(root, 'idle.mjs'); writeFileSync(idle, 'setInterval(() => {}, 1000);\n'); try { - await expect(ensureServer(process.execPath, idle, join(root, 'wt'), 'work', 800)).rejects.toThrow(/did not start/); + await expect(ensureServer(process.execPath, idle, join(root, 'wt'), 'work', undefined, 800)).rejects.toThrow(/did not start/); } finally { if (prev === undefined) delete process.env.DIFFITY_DATA_DIR; else process.env.DIFFITY_DATA_DIR = prev; } diff --git a/packages/cli/tests/pr-number-flag.test.ts b/packages/cli/tests/pr-number-flag.test.ts new file mode 100644 index 00000000..cfb6354c --- /dev/null +++ b/packages/cli/tests/pr-number-flag.test.ts @@ -0,0 +1,123 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { InvalidArgumentError } from 'commander'; +import { execFileSync, spawn, spawnSync, type ChildProcess } from 'node:child_process'; +import { existsSync, mkdtempSync, readFileSync, realpathSync, writeFileSync, rmSync, mkdirSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { tmpdir } from 'node:os'; +import { pullRequestNumber } from '../src/pr-number.js'; + +const ENTRY = join(dirname(fileURLToPath(import.meta.url)), '..', 'dist', 'index.js'); + +describe('what --pr accepts', () => { + it('takes a positive whole number, however written', () => { + expect(pullRequestNumber('14502')).toBe(14502); + expect(pullRequestNumber('1e2')).toBe(100); + }); + + it('rejects everything else with the raw input left to commander', () => { + for (const raw of ['0', '-1', '4.5', '42abc', 'abc', '']) { + expect(() => pullRequestNumber(raw)).toThrowError(InvalidArgumentError); + } + }); +}); + +describe('--pr on the command line', () => { + let root: string; + /** A repository with a GitHub remote, as a pull request's checkout has. */ + let repo: string; + /** One without any remote. */ + let plain: string; + + function initRepo(dir: string): void { + mkdirSync(dir); + execFileSync('git', ['init', '-b', 'main', dir], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: dir, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: dir, stdio: 'pipe' }); + writeFileSync(join(dir, 'a.ts'), 'const a = 1;\n'); + execFileSync('git', ['add', '.'], { cwd: dir, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'init'], { cwd: dir, stdio: 'pipe' }); + } + + beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'diffity-pr-flag-')); + repo = join(root, 'repo'); + plain = join(root, 'plain'); + initRepo(repo); + initRepo(plain); + execFileSync('git', ['remote', 'add', 'origin', 'git@github.com:o/r.git'], { cwd: repo, stdio: 'pipe' }); + }); + + afterAll(() => { rmSync(root, { recursive: true, force: true }); }); + + const env = () => ({ ...process.env, DIFFITY_DATA_DIR: join(root, 'data') }); + + function run(dir: string, args: string[]) { + return spawnSync(process.execPath, [ENTRY, '--repo', dir, '--no-open', ...args], { encoding: 'utf-8', env: env() }); + } + + /** Starts a server on a free port and resolves with it once it has registered. */ + async function start(args: string[]): Promise<{ child: ChildProcess; port: number; ref: string }> { + const child = spawn(process.execPath, [ENTRY, '--repo', repo, '--no-open', '--quiet', '--port', '0', ...args], { env: env(), stdio: 'ignore' }); + const registry = join(root, 'data', 'registry.json'); + const repoRoot = realpathSync(repo); + const deadline = Date.now() + 20_000; + while (Date.now() < deadline) { + if (existsSync(registry)) { + const entry = (JSON.parse(readFileSync(registry, 'utf-8')) as { repoRoot: string; port: number; ref: string }[]) + .find(e => e.repoRoot === repoRoot); + if (entry) { + return { child, port: entry.port, ref: entry.ref }; + } + } + if (child.exitCode !== null) { + throw new Error(`the server exited with ${child.exitCode} before registering`); + } + await new Promise(resolve => setTimeout(resolve, 100)); + } + child.kill('SIGTERM'); + throw new Error('the server did not register in time'); + } + + it('needs the commit the pull request is based on', () => { + const noRef = run(repo, ['--pr', '7']); + expect(noRef.status).toBe(1); + expect(noRef.stderr).toContain('--pr needs the commit the pull request is based on'); + + const workingTree = run(repo, ['--pr', '7', 'work']); + expect(workingTree.status).toBe(1); + expect(workingTree.stderr).toContain('--pr needs the commit the pull request is based on'); + }); + + it('refuses a number that is not one', () => { + const bad = run(repo, ['--pr', 'seven', 'HEAD']); + expect(bad.status).not.toBe(0); + expect(bad.stderr).toContain('A pull request number is a positive integer'); + }); + + it('refuses a repository with no GitHub remote, where the number could show nothing', () => { + const noRemote = run(plain, ['--pr', '7', 'HEAD']); + expect(noRemote.status).toBe(1); + expect(noRemote.stderr).toContain('No GitHub remote detected'); + }); + + it('pins /diff to the base it was given', async () => { + const { child, port } = await start(['--pr', '7', 'HEAD']); + try { + const res = await fetch(`http://127.0.0.1:${port}/diff`, { redirect: 'manual' }); + expect(res.status).toBe(302); + expect(res.headers.get('location')).toBe('/diff?ref=HEAD'); + } finally { + child.kill('SIGTERM'); + } + }, 30_000); + + it('takes the base as --base too', async () => { + const { child, ref } = await start(['--pr', '7', '--base', 'HEAD']); + try { + expect(ref).toBe('HEAD'); + } finally { + child.kill('SIGTERM'); + } + }, 30_000); +}); diff --git a/packages/git/package.json b/packages/git/package.json index f7c622d1..7500403d 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.10.12", + "version": "0.10.13", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index acd48cb0..82234e4c 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.10.12", + "version": "0.10.13", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index 4a234e55..4162a789 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.10.12", + "version": "0.10.13", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 9d14e3be..f22aacdf 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.10.12", + "version": "0.10.13", "type": "module", "private": true, "scripts": {