feat: --pr names the pull request a detached checkout cannot, and the inbox passes it

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 <base-sha>
+```
+
+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:

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",

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": {

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}`);
     }

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<OpenedSession> {
+export async function openPreparedSession(worktree: string, bundlePath: string, prNumber: number, deps: OpenSessionDeps): Promise<OpenedSession> {
   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<number> {
+export async function ensureServer(nodePath: string, entry: string, worktree: string, ref: string, prNumber: number | undefined, waitMs = 30_000): Promise<number> {
   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<void> {
 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<number>;
+  /** 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<number>;
   /** Adds the prepared review's threads and tours to the running session. */
   importBundle(worktree: string, bundlePath: string): void;
 }

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 <number>', '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 <base-sha>          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 <base-sha>')}`);
+        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 },

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;
+}

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;
     }