From 441b93f55ceffc8802cc53f93a48998e1514ef70 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Fable 5)" Date: Wed, 2 Sep 2026 21:43:45 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat:=20the=20inbox=20page=20=E2=80=94=20a?= =?UTF-8?q?=20pinned=20tab=20that=20opens=20a=20prepared=20review=20in=20o?= =?UTF-8?q?ne=20click?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Serves a self-contained dashboard at the daemon's port: it polls /api/inbox and shows what's ready (smallest first), what's preparing, and what was skipped or failed. Clicking a ready pull request hits /open/, which brings its prepared review up as a live diffity session — a server over the kept worktree, diffing against the pull request's base, with the prepared findings imported — and redirects the browser to it. A healthy session already serving that worktree is reused; the import is idempotent and best-effort, so a stale anchor never blocks opening the diff. Part of #79 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- package-lock.json | 12 +- packages/api/package.json | 2 +- packages/cli/package.json | 2 +- packages/cli/src/inbox/daemon.ts | 49 +++++++- packages/cli/src/inbox/open-session.ts | 88 ++++++++++++++ packages/cli/src/inbox/open.ts | 24 ++++ packages/cli/src/inbox/page.ts | 143 +++++++++++++++++++++++ packages/cli/tests/inbox-open.test.ts | 135 +++++++++++++++++++++ packages/cli/tests/inbox-prepare.test.ts | 3 +- packages/git/package.json | 2 +- packages/github/package.json | 2 +- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- 13 files changed, 447 insertions(+), 19 deletions(-) create mode 100644 packages/cli/src/inbox/open-session.ts create mode 100644 packages/cli/src/inbox/open.ts create mode 100644 packages/cli/src/inbox/page.ts create mode 100644 packages/cli/tests/inbox-open.test.ts diff --git a/package-lock.json b/package-lock.json index ec6a2403..7411c76d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.9", + "version": "0.10.10", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.9", + "version": "0.10.10", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.9", + "version": "0.10.10", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.9", + "version": "0.10.10", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.9", + "version": "0.10.10", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.9", + "version": "0.10.10", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/packages/api/package.json b/packages/api/package.json index cc66cfec..f785f821 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.9", + "version": "0.10.10", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index 025e0c3d..0f9df398 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.9", + "version": "0.10.10", "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 92842357..63d27605 100644 --- a/packages/cli/src/inbox/daemon.ts +++ b/packages/cli/src/inbox/daemon.ts @@ -1,4 +1,4 @@ -import { createServer, type Server } from 'node:http'; +import { createServer, 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'; @@ -10,6 +10,9 @@ import { removeWorktree, cloneDir } from './worktree.js'; import { InboxStore } from './store.js'; import { runTick, type Forge } from './tick.js'; import { buildView } from './view.js'; +import { resolveOpen } from './open.js'; +import { openPreparedSession, realOpenSessionDeps, type OpenSessionDeps } from './open-session.js'; +import { inboxPage } from './page.js'; const realForge: Forge = { viewerLogin: getViewerLogin, @@ -32,6 +35,8 @@ export interface DaemonOptions { once?: boolean; /** The forge to poll; defaults to the real GitHub one. Overridden only by tests. */ forge?: Forge; + /** How a prepared review is brought up as a session; defaults to the real one. Tests override it. */ + openDeps?: OpenSessionDeps; } /** @@ -85,7 +90,8 @@ export async function runDaemon( // Bind the port first: it is the daemon's singleton lock, so a second daemon exits here (via the // server's error handler) before it can reclaim and kill the first one's in-flight servers. - const server = await bindInboxServer(store, config, log); + const openDeps = options.openDeps ?? realOpenSessionDeps(nodePath, entry); + const server = await bindInboxServer(store, config, log, openDeps); reclaimLeftoverServers(log); const timer = setInterval(() => void tick(), config.pollMinutes * 60_000); void tick(); @@ -135,15 +141,26 @@ function reclaimLeftoverServers(log: (message: string) => void): void { } } -export function startInboxServer(store: InboxStore, config: InboxConfig, log: (message: string) => void): Server { +export function startInboxServer(store: InboxStore, config: InboxConfig, log: (message: string) => void, openDeps: OpenSessionDeps): Server { const server = createServer((req, res) => { const openBase = `http://localhost:${config.port}`; - if (req.method === 'GET' && (req.url === '/api/inbox' || req.url === '/api/inbox/')) { + const url = req.url ?? '/'; + + if (req.method === 'GET' && (url === '/' || url === '/index.html')) { + res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }); + res.end(inboxPage()); + return; + } + if (req.method === 'GET' && (url === '/api/inbox' || url === '/api/inbox/')) { const view = buildView(store, openBase, new Date().toISOString()); res.writeHead(200, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(view)); return; } + if (req.method === 'GET' && url.startsWith('/open/')) { + void handleOpen(store, decodeURIComponent(url.slice('/open/'.length)), openDeps, log, res); + return; + } res.writeHead(404, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ error: 'not found' })); }); @@ -159,8 +176,28 @@ export function startInboxServer(store: InboxStore, config: InboxConfig, log: (m return server; } +/** Brings a prepared review up as a live session and redirects the browser to it. */ +async function handleOpen(store: InboxStore, id: string, openDeps: OpenSessionDeps, log: (message: string) => void, res: ServerResponse): Promise { + const resolution = resolveOpen(store, id); + if (!resolution.ok) { + res.writeHead(resolution.status, { 'Content-Type': 'text/plain; charset=utf-8' }); + res.end(resolution.message); + return; + } + try { + log(`opening ${id}`); + const { url } = await openPreparedSession(resolution.pr.worktreePath!, resolution.pr.bundlePath!, openDeps); + res.writeHead(302, { Location: url }); + res.end(); + } catch (err) { + log(`could not open ${id}: ${err instanceof Error ? err.message : err}`); + res.writeHead(500, { 'Content-Type': 'text/plain; charset=utf-8' }); + res.end(`Could not open ${id}: ${err instanceof Error ? err.message : err}`); + } +} + /** Resolves once the port is held; a clash exits through the server's own error handler first. */ -function bindInboxServer(store: InboxStore, config: InboxConfig, log: (message: string) => void): Promise { - const server = startInboxServer(store, config, log); +function bindInboxServer(store: InboxStore, config: InboxConfig, log: (message: string) => void, openDeps: OpenSessionDeps): Promise { + const server = startInboxServer(store, config, log, openDeps); return new Promise(resolve => server.once('listening', () => resolve(server))); } diff --git a/packages/cli/src/inbox/open-session.ts b/packages/cli/src/inbox/open-session.ts new file mode 100644 index 00000000..3c8f53b9 --- /dev/null +++ b/packages/cli/src/inbox/open-session.ts @@ -0,0 +1,88 @@ +import { spawn, execFileSync } from 'node:child_process'; +import { readFileSync, realpathSync } from 'node:fs'; +import { createHash } from 'node:crypto'; +import { checkInstanceHealth, findInstanceForRepo } from '../registry.js'; + +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; + /** Adds the prepared review's threads and tours to the running session. */ + importBundle(worktree: string, bundlePath: string): void; +} + +/** + * 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. + */ +export async function openPreparedSession(worktree: string, bundlePath: string, deps: OpenSessionDeps): Promise<{ url: string; imported: boolean }> { + const ref = deps.baseRefOf(bundlePath); + const port = await deps.ensureServer(worktree, ref); + let imported = true; + try { + deps.importBundle(worktree, bundlePath); + } catch { + imported = false; + } + return { url: `http://localhost:${port}/`, imported }; +} + +/** The base commit a bundle was built against; the session diffs the worktree against it. */ +export function baseRefOf(bundlePath: string): string { + const bundle = JSON.parse(readFileSync(bundlePath, 'utf-8')) as { baseSha?: string | null }; + if (!bundle.baseSha) { + throw new Error(`The bundle at ${bundlePath} records no base commit.`); + } + return bundle.baseSha; +} + +/** + * The real `ensureServer`: a healthy diffity already serving this worktree is reused, otherwise one + * is started in the reviewer's own diffity (no data-dir override), so it shows up in `diffity list` + * and behaves like any session they opened themselves. + */ +export function realOpenSessionDeps(nodePath: string, entry: string): OpenSessionDeps { + return { + baseRefOf, + ensureServer: (worktree, ref) => ensureServer(nodePath, entry, worktree, ref), + importBundle: (worktree, bundlePath) => { + execFileSync(nodePath, [entry, '--repo', worktree, 'agent', 'import-bundle', bundlePath], { stdio: 'pipe' }); + }, + }; +} + +async function ensureServer(nodePath: string, entry: string, worktree: string, ref: string, waitMs = 30_000): Promise { + const hash = repoHash(worktree); + const existing = findInstanceForRepo(hash); + if (existing && await checkInstanceHealth(existing.port)) { + return existing.port; + } + + const child = spawn(nodePath, [entry, '--repo', worktree, '--no-open', '--quiet', ref], { detached: true, stdio: 'ignore' }); + child.unref(); + + const deadline = Date.now() + waitMs; + while (Date.now() < deadline) { + await sleep(400); + const entryRow = findInstanceForRepo(hash); + if (entryRow && await checkInstanceHealth(entryRow.port)) { + return entryRow.port; + } + } + try { if (child.pid) process.kill(child.pid, 'SIGTERM'); } catch { /* already gone */ } + throw new Error(`diffity did not start for ${worktree} within ${waitMs / 1000}s`); +} + +/** The server registers under the hash of its resolved repo root, so resolve symlinks before hashing. */ +function repoHash(worktree: string): string { + let root = worktree; + try { root = realpathSync(worktree); } catch { /* not yet on disk; hash the path as given */ } + return createHash('sha256').update(root).digest('hex').slice(0, 12); +} + +function sleep(ms: number): Promise { + return new Promise(resolve => setTimeout(resolve, ms)); +} diff --git a/packages/cli/src/inbox/open.ts b/packages/cli/src/inbox/open.ts new file mode 100644 index 00000000..b5ba547e --- /dev/null +++ b/packages/cli/src/inbox/open.ts @@ -0,0 +1,24 @@ +import type { InboxPr, InboxStore } from './store.js'; + +export type OpenResolution = + | { ok: true; pr: InboxPr } + | { ok: false; status: number; message: string }; + +/** + * Whether a pull request can be opened, and why not when it can't. Only a prepared (or stale but + * 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 { + const pr = store.get(id); + if (!pr) { + return { ok: false, status: 404, message: `No pull request ${id} in the inbox.` }; + } + if (pr.status !== 'prepared' && pr.status !== 'stale') { + return { ok: false, status: 409, message: `${id} is ${pr.status}, not ready to open.` }; + } + if (!pr.worktreePath || !pr.bundlePath) { + return { ok: false, status: 409, message: `${id} has no prepared worktree to open.` }; + } + return { ok: true, pr }; +} diff --git a/packages/cli/src/inbox/page.ts b/packages/cli/src/inbox/page.ts new file mode 100644 index 00000000..ae892234 --- /dev/null +++ b/packages/cli/src/inbox/page.ts @@ -0,0 +1,143 @@ +/** + * The inbox page, served at `/`. Self-contained (no build step, no external requests): it polls + * `/api/inbox` and renders the three groups, opening a prepared review in a new tab via `/open/:id`. + */ +export function inboxPage(): string { + return ` + + + + +diffity inbox + + + +
+

diffity inbox

+ loading… +
+
+ + + + + +
+ + +`; +} diff --git a/packages/cli/tests/inbox-open.test.ts b/packages/cli/tests/inbox-open.test.ts new file mode 100644 index 00000000..0701c6c7 --- /dev/null +++ b/packages/cli/tests/inbox-open.test.ts @@ -0,0 +1,135 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { resolveOpen } from '../src/inbox/open.js'; +import { openPreparedSession, baseRefOf, type OpenSessionDeps } from '../src/inbox/open-session.js'; +import { startInboxServer } from '../src/inbox/daemon.js'; +import { InboxStore } from '../src/inbox/store.js'; +import type { PrSnapshot } from '@diffity/github'; + +let root: string; + +function snapshot(): PrSnapshot { + return { + owner: 'o', repo: 'r', number: 4, title: 'A change', url: 'https://github.com/o/r/pull/4', + author: 'alice', isBot: false, isDraft: false, state: 'OPEN', headSha: 'aaa', baseRef: 'main', + additions: 3, deletions: 1, changedFiles: 2, updatedAt: 'now', + }; +} + +function preparedStore(): InboxStore { + const store = new InboxStore(':memory:'); + store.observe(snapshot(), true, 'now'); + store.markPrepared('o/r#4', { headSha: 'aaa', bundlePath: '/b.json', worktreePath: '/wt', logPath: '/l', at: 'now' }); + return store; +} + +beforeEach(() => { root = mkdtempSync(join(tmpdir(), 'diffity-open-')); }); +afterEach(() => { rmSync(root, { recursive: true, force: true }); }); + +describe('resolveOpen', () => { + it('accepts a prepared pull request with a worktree and bundle', () => { + const store = preparedStore(); + const r = resolveOpen(store, 'o/r#4'); + expect(r.ok).toBe(true); + store.close(); + }); + + it('refuses an unknown, a not-ready, or a worktree-less pull request', () => { + const store = preparedStore(); + expect(resolveOpen(store, 'o/r#9')).toMatchObject({ ok: false, status: 404 }); + + store.observe({ ...snapshot(), number: 5 }, true, 'now'); + store.setStatus('o/r#5', 'queued'); + expect(resolveOpen(store, 'o/r#5')).toMatchObject({ ok: false, status: 409 }); + store.close(); + }); +}); + +describe('baseRefOf', () => { + it('reads the base commit from a bundle and refuses one without it', () => { + const withBase = join(root, 'a.json'); + writeFileSync(withBase, JSON.stringify({ baseSha: 'b'.repeat(40) })); + expect(baseRefOf(withBase)).toBe('b'.repeat(40)); + + const noBase = join(root, 'b.json'); + writeFileSync(noBase, JSON.stringify({ baseSha: null })); + expect(() => baseRefOf(noBase)).toThrow(/no base commit/); + }); +}); + +describe('openPreparedSession', () => { + it('starts the session at the base and imports the bundle, returning its url', async () => { + const calls: string[] = []; + const deps: OpenSessionDeps = { + baseRefOf: () => 'basesha', + ensureServer: (wt, ref) => { calls.push(`ensure ${wt} ${ref}`); return Promise.resolve(5599); }, + importBundle: (wt, bundle) => { calls.push(`import ${wt} ${bundle}`); }, + }; + + const result = await openPreparedSession('/wt', '/b.json', deps); + + expect(result).toEqual({ url: 'http://localhost:5599/', imported: true }); + expect(calls).toEqual(['ensure /wt basesha', 'import /wt /b.json']); + }); + + it('still opens the diff when the import fails, flagging it', async () => { + const deps: OpenSessionDeps = { + baseRefOf: () => 'basesha', + ensureServer: () => Promise.resolve(5599), + importBundle: () => { throw new Error('head moved'); }, + }; + const result = await openPreparedSession('/wt', '/b.json', deps); + expect(result).toEqual({ url: 'http://localhost:5599/', imported: false }); + }); +}); + +describe('the inbox server routes', () => { + const stubOpen: OpenSessionDeps = { + baseRefOf: () => 'basesha', + ensureServer: () => Promise.resolve(7788), + importBundle: () => {}, + }; + + async function serve(store: InboxStore) { + const config = { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', prepare: ['x'], prepareTimeoutMinutes: 30 }; + const server = startInboxServer(store, config, () => {}, stubOpen); + await new Promise(resolve => server.on('listening', resolve)); + const { port } = server.address() as { port: number }; + return { port, server }; + } + + it('serves the page at / and the view at /api/inbox', async () => { + const store = preparedStore(); + const { port, server } = await serve(store); + try { + const page = await fetch(`http://127.0.0.1:${port}/`); + expect(page.headers.get('content-type')).toContain('text/html'); + expect(await page.text()).toContain('diffity inbox'); + + const api = await (await fetch(`http://127.0.0.1:${port}/api/inbox`)).json(); + expect(api.ready[0].id).toBe('o/r#4'); + } finally { + server.close(); + store.close(); + } + }); + + it('redirects /open/ to the opened session, and 409s a not-ready one', async () => { + const store = preparedStore(); + const { port, server } = await serve(store); + try { + const res = await fetch(`http://127.0.0.1:${port}/open/${encodeURIComponent('o/r#4')}`, { redirect: 'manual' }); + expect(res.status).toBe(302); + expect(res.headers.get('location')).toBe('http://localhost:7788/'); + + store.observe({ ...snapshot(), number: 8 }, true, 'now'); + const notReady = await fetch(`http://127.0.0.1:${port}/open/${encodeURIComponent('o/r#8')}`, { redirect: 'manual' }); + expect(notReady.status).toBe(409); + } finally { + server.close(); + store.close(); + } + }); +}); diff --git a/packages/cli/tests/inbox-prepare.test.ts b/packages/cli/tests/inbox-prepare.test.ts index 70be20a5..f5ad7eff 100644 --- a/packages/cli/tests/inbox-prepare.test.ts +++ b/packages/cli/tests/inbox-prepare.test.ts @@ -137,7 +137,8 @@ describe('the inbox JSON server', () => { const store = new InboxStore(':memory:'); store.observe({ ...snapshot(), headSha: 'aaa' }, true, 'now'); store.markPrepared('o/demo#4', { headSha: 'aaa', bundlePath: '/b.json', worktreePath: '/wt', logPath: '/l', at: 'now' }); - const server = startInboxServer(store, { ...config(), port: 0 }, () => {}); + const noOpenDeps = { baseRefOf: () => 'x', ensureServer: () => Promise.resolve(1), importBundle: () => {} }; + const server = startInboxServer(store, { ...config(), port: 0 }, () => {}, noOpenDeps); await new Promise(resolve => server.on('listening', resolve)); const { port } = server.address() as { port: number }; diff --git a/packages/git/package.json b/packages/git/package.json index 337b6dbb..1774424e 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.10.9", + "version": "0.10.10", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index e9435407..917b2dd9 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.10.9", + "version": "0.10.10", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index 060c5d7b..acff2675 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.10.9", + "version": "0.10.10", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 56e97242..68e72558 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.10.9", + "version": "0.10.10", "type": "module", "private": true, "scripts": { From 84ce999e5f240532f0935389067b7cc216980d18 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Fable 5)" Date: Wed, 2 Sep 2026 21:56:59 +0200 Subject: [PATCH 2/3] fix: a malformed open URL no longer crashes the daemon, and the surface is guarded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The whole request handler is wrapped, and a bad percent-escape in /open answers 400 instead of throwing uncaught and taking the long-running daemon down. Every request must carry this loopback server's own Host, so a page that rebinds its hostname to 127.0.0.1 cannot read the reviewer's pull requests; a state-changing /open refuses a cross-site fetch. An import that fails on open is now logged with its reason rather than swallowed. The real ensureServer path gains tests — the hash it looks a server up by matches what diffity registers, and the start times out. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- packages/cli/src/inbox/daemon.ts | 80 +++++++++++++---- packages/cli/src/inbox/open-session.ts | 40 +++++---- packages/cli/tests/inbox-open.test.ts | 118 ++++++++++++++++++++++++- 3 files changed, 200 insertions(+), 38 deletions(-) diff --git a/packages/cli/src/inbox/daemon.ts b/packages/cli/src/inbox/daemon.ts index 63d27605..6f3cec1b 100644 --- a/packages/cli/src/inbox/daemon.ts +++ b/packages/cli/src/inbox/daemon.ts @@ -143,26 +143,60 @@ function reclaimLeftoverServers(log: (message: string) => void): void { export function startInboxServer(store: InboxStore, config: InboxConfig, log: (message: string) => void, openDeps: OpenSessionDeps): Server { const server = createServer((req, res) => { - const openBase = `http://localhost:${config.port}`; - const url = req.url ?? '/'; + // The whole handler is guarded: an unhandled throw here (a malformed percent-escape, say) would + // otherwise have no catch and take the long-running daemon down with it. + try { + // Loopback binding is not enough on its own: a page on another site can rebind its own + // hostname to 127.0.0.1, so a stranger's Host header must not reach the reviewer's PR list. + // Judged against the connection's own port, which is the port actually bound. + if (!isLocalHost(req.headers.host, req.socket.localPort)) { + res.writeHead(403, { 'Content-Type': 'text/plain; charset=utf-8' }); + res.end('forbidden'); + return; + } - if (req.method === 'GET' && (url === '/' || url === '/index.html')) { - res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }); - res.end(inboxPage()); - return; - } - if (req.method === 'GET' && (url === '/api/inbox' || url === '/api/inbox/')) { - const view = buildView(store, openBase, new Date().toISOString()); - res.writeHead(200, { 'Content-Type': 'application/json' }); - res.end(JSON.stringify(view)); - return; - } - if (req.method === 'GET' && url.startsWith('/open/')) { - void handleOpen(store, decodeURIComponent(url.slice('/open/'.length)), openDeps, log, res); - return; + const openBase = `http://localhost:${config.port}`; + const url = req.url ?? '/'; + + if (req.method === 'GET' && (url === '/' || url === '/index.html')) { + res.writeHead(200, { 'Content-Type': 'text/html; charset=utf-8' }); + res.end(inboxPage()); + return; + } + if (req.method === 'GET' && (url === '/api/inbox' || url === '/api/inbox/')) { + const view = buildView(store, openBase, new Date().toISOString()); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify(view)); + 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; + } + 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; + } + 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' }); + res.end(JSON.stringify({ error: 'not found' })); + } catch (err) { + log(`request handler error: ${err instanceof Error ? err.message : err}`); + if (!res.headersSent) { + res.writeHead(500, { 'Content-Type': 'text/plain; charset=utf-8' }); + } + res.end('internal error'); } - res.writeHead(404, { 'Content-Type': 'application/json' }); - res.end(JSON.stringify({ error: 'not found' })); }); server.on('error', err => { const code = (err as NodeJS.ErrnoException).code; @@ -186,7 +220,10 @@ async function handleOpen(store: InboxStore, id: string, openDeps: OpenSessionDe } try { log(`opening ${id}`); - const { url } = await openPreparedSession(resolution.pr.worktreePath!, resolution.pr.bundlePath!, openDeps); + const { url, imported, importError } = await openPreparedSession(resolution.pr.worktreePath!, resolution.pr.bundlePath!, openDeps); + if (!imported) { + log(`opened ${id} but its findings did not import: ${importError}`); + } res.writeHead(302, { Location: url }); res.end(); } catch (err) { @@ -196,6 +233,11 @@ async function handleOpen(store: InboxStore, id: string, openDeps: OpenSessionDe } } +/** 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}`); +} + /** Resolves once the port is held; a clash exits through the server's own error handler first. */ function bindInboxServer(store: InboxStore, config: InboxConfig, log: (message: string) => void, openDeps: OpenSessionDeps): Promise { const server = startInboxServer(store, config, log, openDeps); diff --git a/packages/cli/src/inbox/open-session.ts b/packages/cli/src/inbox/open-session.ts index 3c8f53b9..3910883f 100644 --- a/packages/cli/src/inbox/open-session.ts +++ b/packages/cli/src/inbox/open-session.ts @@ -3,31 +3,21 @@ import { readFileSync, realpathSync } from 'node:fs'; import { createHash } from 'node:crypto'; import { checkInstanceHealth, findInstanceForRepo } from '../registry.js'; -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; - /** Adds the prepared review's threads and tours to the running session. */ - importBundle(worktree: string, bundlePath: string): void; -} - /** * 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. */ -export async function openPreparedSession(worktree: string, bundlePath: string, deps: OpenSessionDeps): Promise<{ url: string; imported: boolean }> { +export async function openPreparedSession(worktree: string, bundlePath: string, deps: OpenSessionDeps): Promise { const ref = deps.baseRefOf(bundlePath); const port = await deps.ensureServer(worktree, ref); - let imported = true; try { deps.importBundle(worktree, bundlePath); - } catch { - imported = false; + } catch (err) { + return { url: `http://localhost:${port}/`, imported: false, importError: err instanceof Error ? err.message : String(err) }; } - return { url: `http://localhost:${port}/`, imported }; + return { url: `http://localhost:${port}/`, imported: true }; } /** The base commit a bundle was built against; the session diffs the worktree against it. */ @@ -54,8 +44,10 @@ export function realOpenSessionDeps(nodePath: string, entry: string): OpenSessio }; } -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, 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. const existing = findInstanceForRepo(hash); if (existing && await checkInstanceHealth(existing.port)) { return existing.port; @@ -77,7 +69,7 @@ async function ensureServer(nodePath: string, entry: string, worktree: string, r } /** The server registers under the hash of its resolved repo root, so resolve symlinks before hashing. */ -function repoHash(worktree: string): string { +export function repoHash(worktree: string): string { let root = worktree; try { root = realpathSync(worktree); } catch { /* not yet on disk; hash the path as given */ } return createHash('sha256').update(root).digest('hex').slice(0, 12); @@ -86,3 +78,19 @@ function repoHash(worktree: string): string { function sleep(ms: number): Promise { return new Promise(resolve => setTimeout(resolve, ms)); } + +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; + /** Adds the prepared review's threads and tours to the running session. */ + importBundle(worktree: string, bundlePath: string): void; +} + +export interface OpenedSession { + url: string; + imported: boolean; + /** Why the import did not happen, when it didn't; the diff is still opened. */ + importError?: string; +} diff --git a/packages/cli/tests/inbox-open.test.ts b/packages/cli/tests/inbox-open.test.ts index 0701c6c7..93374037 100644 --- a/packages/cli/tests/inbox-open.test.ts +++ b/packages/cli/tests/inbox-open.test.ts @@ -1,13 +1,19 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { request } from 'node:http'; +import { execFileSync } from 'node:child_process'; import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { tmpdir } from 'node:os'; import { resolveOpen } from '../src/inbox/open.js'; -import { openPreparedSession, baseRefOf, type OpenSessionDeps } from '../src/inbox/open-session.js'; +import { openPreparedSession, baseRefOf, ensureServer, repoHash, type OpenSessionDeps } from '../src/inbox/open-session.js'; import { startInboxServer } from '../src/inbox/daemon.js'; import { InboxStore } from '../src/inbox/store.js'; +import { readRegistry } from '../src/registry.js'; import type { PrSnapshot } from '@diffity/github'; +const ENTRY = join(dirname(fileURLToPath(import.meta.url)), '..', 'dist', 'index.js'); + let root: string; function snapshot(): PrSnapshot { @@ -28,6 +34,18 @@ function preparedStore(): InboxStore { beforeEach(() => { root = mkdtempSync(join(tmpdir(), 'diffity-open-')); }); afterEach(() => { rmSync(root, { recursive: true, force: true }); }); +/** A GET with forged headers fetch() will not set, resolving to the status code. */ +function rawGet(port: number, path: string, headers: Record): Promise { + return new Promise((resolve, reject) => { + const req = request({ host: '127.0.0.1', port, path, method: 'GET', headers }, res => { + res.resume(); + resolve(res.statusCode ?? 0); + }); + req.on('error', reject); + req.end(); + }); +} + describe('resolveOpen', () => { it('accepts a prepared pull request with a worktree and bundle', () => { const store = preparedStore(); @@ -81,10 +99,49 @@ describe('openPreparedSession', () => { importBundle: () => { throw new Error('head moved'); }, }; const result = await openPreparedSession('/wt', '/b.json', deps); - expect(result).toEqual({ url: 'http://localhost:5599/', imported: false }); + expect(result).toEqual({ url: 'http://localhost:5599/', imported: false, importError: 'head moved' }); }); }); +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; + process.env.DIFFITY_DATA_DIR = join(root, 'data'); + const repo = join(root, 'repo'); + execFileSync('git', ['init', '-b', 'main', repo], { stdio: 'pipe' }); + execFileSync('git', ['config', 'user.email', 't@t'], { cwd: repo, stdio: 'pipe' }); + execFileSync('git', ['config', 'user.name', 'T'], { cwd: repo, stdio: 'pipe' }); + writeFileSync(join(repo, 'a.ts'), 'const a = 1;\n'); + execFileSync('git', ['add', '.'], { cwd: repo, stdio: 'pipe' }); + execFileSync('git', ['commit', '-m', 'init'], { cwd: repo, stdio: 'pipe' }); + + let port = 0; + try { + port = await ensureServer(process.execPath, ENTRY, repo, 'work', 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(); + expect(entry!.repoHash).toBe(repoHash(repo)); + } finally { + const entry = readRegistry().find(e => e.port === port); + if (entry) { try { process.kill(entry.pid, 'SIGKILL'); } catch { /* gone */ } } + if (prev === undefined) delete process.env.DIFFITY_DATA_DIR; else process.env.DIFFITY_DATA_DIR = prev; + } + }, 30_000); + + it('throws when nothing registers before the deadline', async () => { + const prev = process.env.DIFFITY_DATA_DIR; + process.env.DIFFITY_DATA_DIR = join(root, 'empty-data'); + 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/); + } finally { + if (prev === undefined) delete process.env.DIFFITY_DATA_DIR; else process.env.DIFFITY_DATA_DIR = prev; + } + }, 10_000); +}); + describe('the inbox server routes', () => { const stubOpen: OpenSessionDeps = { baseRefOf: () => 'basesha', @@ -132,4 +189,59 @@ describe('the inbox server routes', () => { store.close(); } }); + + it('answers a malformed /open URL with 400 and keeps running', async () => { + const store = preparedStore(); + const { port, server } = await serve(store); + try { + const bad = await fetch(`http://127.0.0.1:${port}/open/%`); + expect(bad.status).toBe(400); + // The daemon is still up and serving afterwards. + const ok = await fetch(`http://127.0.0.1:${port}/api/inbox`); + expect(ok.status).toBe(200); + } finally { + server.close(); + store.close(); + } + }); + + it('rejects a foreign Host header and a cross-site open', async () => { + const store = preparedStore(); + const { port, server } = await serve(store); + try { + // fetch() forbids setting Host and Sec-Fetch-*, so a raw request is needed to forge them. + const rebind = await rawGet(port, '/api/inbox', { Host: 'evil.example.com:1234' }); + expect(rebind).toBe(403); + + const driveBy = await rawGet(port, `/open/${encodeURIComponent('o/r#4')}`, { + Host: `127.0.0.1:${port}`, 'Sec-Fetch-Site': 'cross-site', + }); + expect(driveBy).toBe(403); + } finally { + server.close(); + store.close(); + } + }); + + it('surfaces an import failure in the log but still redirects', async () => { + const store = preparedStore(); + const logs: string[] = []; + const failingOpen: OpenSessionDeps = { + baseRefOf: () => 'basesha', + ensureServer: () => Promise.resolve(7788), + importBundle: () => { throw new Error('head moved'); }, + }; + const config = { pollMinutes: 5, port: 0, reposDir: root, worktreesDir: root, filter: '', prepare: ['x'], prepareTimeoutMinutes: 30 }; + const server = startInboxServer(store, config, m => logs.push(m), failingOpen); + await new Promise(resolve => server.on('listening', resolve)); + const { port } = server.address() as { port: number }; + try { + const res = await fetch(`http://127.0.0.1:${port}/open/${encodeURIComponent('o/r#4')}`, { redirect: 'manual' }); + expect(res.status).toBe(302); + expect(logs.some(l => l.includes('did not import') && l.includes('head moved'))).toBe(true); + } finally { + server.close(); + store.close(); + } + }); }); From a525240ab5365f72fe15421e1136764a68cb4c71 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Fable 5)" Date: Wed, 2 Sep 2026 22:06:42 +0200 Subject: [PATCH 3/3] fix: page links follow the host used, and a store error can't hang open The page's links are built from the host the reader actually reached it by, so opening from 127.0.0.1 is not a cross-site click against a localhost link. resolveOpen moves inside the open handler's try, so a store failure answers 500 rather than leaving the request to hang. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --- packages/cli/src/inbox/daemon.ts | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/inbox/daemon.ts b/packages/cli/src/inbox/daemon.ts index 6f3cec1b..905d5261 100644 --- a/packages/cli/src/inbox/daemon.ts +++ b/packages/cli/src/inbox/daemon.ts @@ -155,7 +155,9 @@ export function startInboxServer(store: InboxStore, config: InboxConfig, log: (m return; } - const openBase = `http://localhost:${config.port}`; + // The host the reader actually used — localhost or 127.0.0.1, already checked — so the links + // on the page point back at the same origin and a click on them is not cross-site. + const openBase = `http://${req.headers.host}`; const url = req.url ?? '/'; if (req.method === 'GET' && (url === '/' || url === '/index.html')) { @@ -212,13 +214,13 @@ export function startInboxServer(store: InboxStore, config: InboxConfig, log: (m /** Brings a prepared review up as a live session and redirects the browser to it. */ async function handleOpen(store: InboxStore, id: string, openDeps: OpenSessionDeps, log: (message: string) => void, res: ServerResponse): Promise { - const resolution = resolveOpen(store, id); - if (!resolution.ok) { - res.writeHead(resolution.status, { 'Content-Type': 'text/plain; charset=utf-8' }); - res.end(resolution.message); - return; - } try { + const resolution = resolveOpen(store, id); + if (!resolution.ok) { + res.writeHead(resolution.status, { 'Content-Type': 'text/plain; charset=utf-8' }); + res.end(resolution.message); + return; + } log(`opening ${id}`); const { url, imported, importError } = await openPreparedSession(resolution.pr.worktreePath!, resolution.pr.bundlePath!, openDeps); if (!imported) { @@ -228,8 +230,10 @@ async function handleOpen(store: InboxStore, id: string, openDeps: OpenSessionDe res.end(); } catch (err) { log(`could not open ${id}: ${err instanceof Error ? err.message : err}`); - res.writeHead(500, { 'Content-Type': 'text/plain; charset=utf-8' }); - res.end(`Could not open ${id}: ${err instanceof Error ? err.message : err}`); + if (!res.headersSent) { + res.writeHead(500, { 'Content-Type': 'text/plain; charset=utf-8' }); + res.end(`Could not open ${id}: ${err instanceof Error ? err.message : err}`); + } } }