diff --git a/README.md b/README.md index c7d13b2b..9658e8c8 100644 --- a/README.md +++ b/README.md @@ -324,7 +324,7 @@ running past the end would otherwise be counted and highlighted with nothing to ## The review inbox -`diffity inbox` watches the pull requests awaiting your review and prepares each one ahead of time, so the review is ready the moment you look. It polls GitHub (`gh search prs --review-requested=@me`), and for each pull request worth your attention it cuts a worktree at the PR head, runs a diffity session over the diff, has an agent prepare a review with a walkthrough, and saves the result as a bundle. New commits redo a stale review; a merged, closed, or no-longer-requested PR is retired. +`diffity inbox` watches the pull requests awaiting your review and prepares each one ahead of time, so the review is ready the moment you look. It polls GitHub (`gh search prs --review-requested=@me`), and for each pull request worth your attention it cuts a worktree at the PR head, runs a diffity session over the diff, has an agent prepare a review with a walkthrough, and saves the result as a bundle. New commits redo a stale review; a merged, closed, or no-longer-requested PR is retired. At most `maxPrepared` reviews are kept prepared at a time — the rest wait in the queue, smallest first — and a prepared review leaves the inbox once you have posted it (GitHub withdraws the request) or dismissed it from the page. The daemon never posts your prepared reviews to GitHub — they are local drafts you open and submit yourself — and it runs the review agent with your GitHub credentials stripped from its environment. That said, the agent executes the pull request's own repository code (see the warning below), so treat the "never posts" behaviour as the daemon's design, not a sandbox. @@ -346,6 +346,7 @@ On first run it writes `~/.diffity/inbox/config.json`: | `filter` | Your own words on what does and doesn't need your attention, handed to the agent — it answers with a skip instead of reviewing when a PR matches (e.g. "Skip payments-focused PRs"). | | `prepare` | The review agent, as a command and its arguments. It runs in the PR's worktree and reads its prompt on stdin. | | `prepareTimeoutMinutes` | How long one preparation may take before it's abandoned. | +| `maxPrepared` | How many prepared reviews may wait for you at once (default 5). Each preparation is an agent run; the rest of the queue waits until a prepared review is posted or dismissed. | > ⚠️ The `prepare` command runs inside a checkout the pull request's author controls, so it executes their repository scripts. The daemon runs it without the forge's credentials in its environment, but you should still only point `prepare` at an agent you're willing to run on untrusted code. diff --git a/package-lock.json b/package-lock.json index 9cee55aa..21723e93 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.11", + "version": "0.10.12", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.11", + "version": "0.10.12", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.11", + "version": "0.10.12", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.11", + "version": "0.10.12", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.11", + "version": "0.10.12", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.11", + "version": "0.10.12", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/packages/api/package.json b/packages/api/package.json index a3aa43d2..fba82eb1 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.11", + "version": "0.10.12", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index f41730cc..48434772 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.11", + "version": "0.10.12", "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/commands/inbox.ts b/packages/cli/src/commands/inbox.ts index 87034fc9..ae39a95d 100644 --- a/packages/cli/src/commands/inbox.ts +++ b/packages/cli/src/commands/inbox.ts @@ -79,8 +79,8 @@ export function registerInboxCommand(program: Command): void { section('Ready to review', view.ready.map(row => ` ${sizeBadge(row)} ${pc.bold(`${row.repo}#${row.number}`)} ${row.title}${row.stale ? pc.yellow(' (stale — new commits)') : ''}`, )); - section('Preparing', view.working.map(row => - ` ${pc.dim(row.status.padEnd(9))} ${row.repo}#${row.number} ${row.title}`, + section('Queue', view.working.map(row => + ` ${pc.dim(row.status.padEnd(9))} ${row.repo}#${row.number} ${row.title} ${pc.dim(row.statusReason ?? '')}`, )); section('Other', view.other.map(row => ` ${pc.dim(row.status.padEnd(9))} ${row.repo}#${row.number} ${pc.dim(row.statusReason ?? '')}`, diff --git a/packages/cli/src/inbox/config.ts b/packages/cli/src/inbox/config.ts index 6e23970c..a8bac856 100644 --- a/packages/cli/src/inbox/config.ts +++ b/packages/cli/src/inbox/config.ts @@ -23,6 +23,11 @@ export interface InboxConfig { */ prepare: string[]; prepareTimeoutMinutes: number; + /** + * How many prepared reviews may wait for the reviewer at once. Each preparation spends an agent + * run, so the queue beyond this waits for a prepared review to be posted or dismissed. + */ + maxPrepared: number; } export const DEFAULT_INBOX_CONFIG: InboxConfig = { @@ -38,6 +43,7 @@ export const DEFAULT_INBOX_CONFIG: InboxConfig = { '--disallowedTools', 'Bash(gh pr review:*)', 'Bash(gh pr comment:*)', 'Bash(gh pr merge:*)', 'Bash(gh api:*)', ], prepareTimeoutMinutes: 30, + maxPrepared: 5, }; /** @@ -94,6 +100,9 @@ export function parseInboxConfig(raw: unknown, source = 'inbox config'): InboxCo if (obj.prepareTimeoutMinutes !== undefined) { config.prepareTimeoutMinutes = positive(obj.prepareTimeoutMinutes, 'prepareTimeoutMinutes', source); } + if (obj.maxPrepared !== undefined) { + config.maxPrepared = positiveInteger(obj.maxPrepared, 'maxPrepared', source); + } return config; } @@ -104,6 +113,13 @@ function positive(value: unknown, key: string, source: string): number { return value; } +function positiveInteger(value: unknown, key: string, source: string): number { + if (typeof value !== 'number' || !Number.isInteger(value) || value < 1) { + throw new Error(`${source}: ${key} must be a positive integer`); + } + return value; +} + function port(value: unknown, source: string): number { if (typeof value !== 'number' || !Number.isInteger(value) || value < 1 || value > 65535) { throw new Error(`${source}: port must be an integer between 1 and 65535`); diff --git a/packages/cli/src/inbox/daemon.ts b/packages/cli/src/inbox/daemon.ts index 905d5261..d0f88490 100644 --- a/packages/cli/src/inbox/daemon.ts +++ b/packages/cli/src/inbox/daemon.ts @@ -1,4 +1,4 @@ -import { createServer, type Server, type ServerResponse } from 'node:http'; +import { createServer, type IncomingMessage, type Server, type ServerResponse } from 'node:http'; import { existsSync, readdirSync, readFileSync, rmSync } from 'node:fs'; import { basename, join } from 'node:path'; import { getViewerLogin, searchReviewRequested, viewPr } from '@diffity/github'; @@ -7,10 +7,12 @@ import { inboxDir } from './paths.js'; import { preparePr, type PrepareDeps } from './prepare.js'; import { realPrepareDeps, type Inflight } from './runtime.js'; import { removeWorktree, cloneDir } from './worktree.js'; +import { findInstanceForRepo, killInstance } from '../registry.js'; +import { repoHash } from './open-session.js'; import { InboxStore } from './store.js'; import { runTick, type Forge } from './tick.js'; import { buildView } from './view.js'; -import { resolveOpen } from './open.js'; +import { resolveDismiss, resolveOpen } from './open.js'; import { openPreparedSession, realOpenSessionDeps, type OpenSessionDeps } from './open-session.js'; import { inboxPage } from './page.js'; @@ -60,10 +62,11 @@ export async function runDaemon( const deps = { forge: options.forge ?? realForge, prepare: (snapshot: Parameters[0]) => preparePr(snapshot, config, prepareDeps), - removeWorktree: (worktree: string, repo: string) => removeWorktree(cloneDir(config.reposDir, repo), worktree), + removeWorktree: (worktree: string, repo: string) => reclaimWorktree(config, worktree, repo), log, now: () => new Date().toISOString(), shouldContinue: () => !stopping, + maxPrepared: config.maxPrepared, }; const tick = async () => { @@ -172,22 +175,17 @@ export function startInboxServer(store: InboxStore, config: InboxConfig, log: (m return; } if (req.method === 'GET' && url.startsWith('/open/')) { - // A state-changing GET, so a cross-site fetch — a drive-by trying to spawn a session — is - // refused; a click from the inbox page itself is same-origin, and a direct navigation none. - if (req.headers['sec-fetch-site'] === 'cross-site') { - res.writeHead(403, { 'Content-Type': 'text/plain; charset=utf-8' }); - res.end('forbidden'); - return; + const id = stateChangingId(req, res, '/open/'); + if (id !== null) { + void handleOpen(store, id, openDeps, log, res).catch(err => log(`open failed: ${err instanceof Error ? err.message : err}`)); } - let id: string; - try { - id = decodeURIComponent(url.slice('/open/'.length)); - } catch { - res.writeHead(400, { 'Content-Type': 'text/plain; charset=utf-8' }); - res.end('bad request'); - return; + return; + } + if (req.method === 'POST' && url.startsWith('/dismiss/')) { + const id = stateChangingId(req, res, '/dismiss/'); + if (id !== null) { + handleDismiss(store, config, id, log, res); } - void handleOpen(store, id, openDeps, log, res).catch(err => log(`open failed: ${err instanceof Error ? err.message : err}`)); return; } res.writeHead(404, { 'Content-Type': 'application/json' }); @@ -237,6 +235,59 @@ async function handleOpen(store: InboxStore, id: string, openDeps: OpenSessionDe } } +/** + * The id a state-changing route was asked about, or null once the request has been answered: a + * cross-site fetch — a drive-by trying to spawn a session or dismiss a review — is refused, and a + * malformed escape is a bad request. A click from the inbox page is same-origin, a direct + * navigation has no site. + */ +function stateChangingId(req: IncomingMessage, res: ServerResponse, prefix: string): string | null { + if (req.headers['sec-fetch-site'] === 'cross-site') { + res.writeHead(403, { 'Content-Type': 'text/plain; charset=utf-8' }); + res.end('forbidden'); + return null; + } + try { + return decodeURIComponent((req.url ?? '').slice(prefix.length)); + } catch { + res.writeHead(400, { 'Content-Type': 'text/plain; charset=utf-8' }); + res.end('bad request'); + return null; + } +} + +/** Marks a pull request as one the reviewer will not review, and reclaims its worktree. */ +function handleDismiss(store: InboxStore, config: InboxConfig, id: string, log: (message: string) => void, res: ServerResponse): void { + const resolution = resolveDismiss(store, id); + if (!resolution.ok) { + res.writeHead(resolution.status, { 'Content-Type': 'text/plain; charset=utf-8' }); + res.end(resolution.message); + return; + } + const { pr } = resolution; + if (pr.worktreePath) { + reclaimWorktree(config, pr.worktreePath, pr.repo); + store.setPaths(pr.id, { worktreePath: null }); + } + store.setStatus(pr.id, 'dismissed', 'dismissed by the reviewer'); + log(`dismissed ${pr.id}`); + res.writeHead(204); + res.end(); +} + +/** + * Removes a pull request's worktree, first stopping any diffity server the reviewer opened on it. + * That session lives in the reviewer's own registry and would otherwise keep serving a directory + * that no longer exists. + */ +export function reclaimWorktree(config: InboxConfig, worktree: string, repo: string): void { + const instance = findInstanceForRepo(repoHash(worktree)); + if (instance) { + killInstance(instance); + } + removeWorktree(cloneDir(config.reposDir, repo), worktree); +} + /** A request whose Host is this loopback server's own address (localhost or 127.0.0.1, right port). */ function isLocalHost(host: string | undefined, port: number | undefined): boolean { return port != null && (host === `localhost:${port}` || host === `127.0.0.1:${port}`); diff --git a/packages/cli/src/inbox/open.ts b/packages/cli/src/inbox/open.ts index b5ba547e..168e8b48 100644 --- a/packages/cli/src/inbox/open.ts +++ b/packages/cli/src/inbox/open.ts @@ -1,6 +1,6 @@ import type { InboxPr, InboxStore } from './store.js'; -export type OpenResolution = +export type Resolution = | { ok: true; pr: InboxPr } | { ok: false; status: number; message: string }; @@ -9,7 +9,7 @@ export type OpenResolution = * still prepared) review has a worktree and a bundle to open; a queued, skipped or failed one has * nothing to show yet. */ -export function resolveOpen(store: InboxStore, id: string): OpenResolution { +export function resolveOpen(store: InboxStore, id: string): Resolution { const pr = store.get(id); if (!pr) { return { ok: false, status: 404, message: `No pull request ${id} in the inbox.` }; @@ -22,3 +22,18 @@ export function resolveOpen(store: InboxStore, id: string): OpenResolution { } return { ok: true, pr }; } + +/** + * Whether a pull request can be dismissed right now. One being prepared cannot: the run in flight + * would finish and mark it prepared over the dismissal. + */ +export function resolveDismiss(store: InboxStore, id: string): Resolution { + const pr = store.get(id); + if (!pr) { + return { ok: false, status: 404, message: `No pull request ${id} in the inbox.` }; + } + if (pr.status === 'preparing') { + return { ok: false, status: 409, message: `${id} is being prepared right now; dismiss it once that has finished.` }; + } + return { ok: true, pr }; +} diff --git a/packages/cli/src/inbox/page.ts b/packages/cli/src/inbox/page.ts index ae892234..8bfe9014 100644 --- a/packages/cli/src/inbox/page.ts +++ b/packages/cli/src/inbox/page.ts @@ -32,6 +32,11 @@ export function inboxPage(): string { margin: 0 0 8px; font-weight: 600; } .row { display: flex; align-items: center; gap: 12px; background: var(--panel); border: 1px solid var(--line); border-radius: 10px; padding: 11px 14px; margin-bottom: 8px; } + .entry { display: flex; align-items: stretch; gap: 8px; margin-bottom: 8px; } + .entry .row { flex: 1; margin-bottom: 0; } + .dismiss { flex: none; width: 38px; border: 1px solid var(--line); border-radius: 10px; background: var(--panel); + color: var(--muted); font-size: 16px; cursor: pointer; } + .dismiss:hover { color: var(--bad); border-color: var(--bad); } .row.open { cursor: pointer; } .row.open:hover { border-color: var(--accent); } .size { font-variant-numeric: tabular-nums; color: var(--muted); font-size: 12px; @@ -62,7 +67,7 @@ export function inboxPage(): string {