From 917fdce4d3fdb3faf36227081f648351e234d0f7 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Mon, 7 Sep 2026 21:04:31 +0200 Subject: [PATCH 1/3] feat: a script re-prepares a pull request with another model and compares the findings with the bundle it has `scripts/inbox-compare.ts [--model m] [--effort e]` takes the newest bundle the inbox has for a pull request as the baseline, prepares the same head again with the candidate settings, and prints a markdown table of what was reproduced, what is new, and what the run spent. The candidate gets its own scratch worktree and diffity data directory under a temp directory, so nothing is written into ~/.diffity and a running daemon is undisturbed. A worktree can only be cut at whatever refs/pull//head points at, so the script asks for that head before spending an agent run and refuses when the pull request has moved past the baseline. The scripts directory is now typechecked too, which is what keeps this one honest. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- development.md | 33 +++ package-lock.json | 12 +- package.json | 2 +- packages/api/package.json | 2 +- packages/cli/package.json | 2 +- packages/git/package.json | 2 +- packages/github/package.json | 2 +- packages/parser/package.json | 2 +- packages/ui/package.json | 2 +- scripts/inbox-compare.test.ts | 386 ++++++++++++++++++++++++ scripts/inbox-compare.ts | 542 ++++++++++++++++++++++++++++++++++ tsconfig.scripts.json | 8 + 12 files changed, 982 insertions(+), 13 deletions(-) create mode 100644 scripts/inbox-compare.test.ts create mode 100644 scripts/inbox-compare.ts create mode 100644 tsconfig.scripts.json diff --git a/development.md b/development.md index 867ecb4f..321ecb6e 100644 --- a/development.md +++ b/development.md @@ -126,6 +126,39 @@ npm run test:watch -w @diffity/parser npm run test:watch -w @diffity/ui ``` +## Comparing review models + +`scripts/inbox-compare.ts` re-prepares a pull request the inbox has already reviewed, with a +different model or effort, and puts the two sets of findings side by side. It is how a change to +`agent.model` is decided: by what the candidate finds, not by what it costs. + +```bash +npm run build # the script runs the built CLI, so build first +npx tsx scripts/inbox-compare.ts NaturalCycles/NCBackend3#14550 --model opus +npx tsx scripts/inbox-compare.ts NaturalCycles/NCBackend3#14550 --effort medium --out /tmp/14550.md +``` + +The baseline is the newest bundle for that pull request under `~/.diffity/inbox/bundles` +(`--bundles-dir` to look elsewhere, `--head ` to pick an older one). The candidate gets a +scratch worktree and its own diffity data directory under a fresh temp directory (`--scratch` to +name it), so nothing is written into `~/.diffity` and a running `diffity inbox` is undisturbed. One +invocation is one agent run, and it takes as long as a real preparation — up to half an hour. + +A worktree can only be cut at whatever `refs/pull//head` points at, so the script refuses, +before spending the run, when the pull request has moved past the baseline's head. + +Reading the table: each severity row is `baseline count | reproduced, new`. *Reproduced* means a +candidate finding landed on the same file with an overlapping line range — a one-line finding +counts as its line give or take two. *New* counts candidate findings no baseline finding covers; +some are real, some are noise, which is what the finding list underneath is for. The `cost / time` +row is the candidate's own run: two models are not comparable on the baseline's, which predates +the run log. Exit code 0 is a completed comparison, 2 a candidate that skipped or failed, 1 a +usage error. `--json` prints the same numbers as one object. + +The baseline bundles were prepared by the old pipeline, which loaded the reviewer's own settings, +skills and MCP servers and let the agent run the repository's toolchain. A difference between the +columns is therefore prompt *and* model, not model alone. + ## CLI Usage (for reference while developing) ```bash diff --git a/package-lock.json b/package-lock.json index 18ef9d32..858c31d5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8457,7 +8457,7 @@ }, "packages/api": { "name": "@diffity/api", - "version": "0.10.25", + "version": "0.10.26", "dependencies": { "@diffity/parser": "*" }, @@ -8468,7 +8468,7 @@ }, "packages/cli": { "name": "@naturalcycles/diffity", - "version": "0.10.25", + "version": "0.10.26", "license": "MIT", "dependencies": { "commander": "^14.0.3", @@ -8492,7 +8492,7 @@ }, "packages/git": { "name": "@diffity/git", - "version": "0.10.25", + "version": "0.10.26", "devDependencies": { "@types/node": "^25.5.0", "typescript": "^5.9.3", @@ -8501,7 +8501,7 @@ }, "packages/github": { "name": "@diffity/github", - "version": "0.10.25", + "version": "0.10.26", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*" @@ -8514,7 +8514,7 @@ }, "packages/parser": { "name": "@diffity/parser", - "version": "0.10.25", + "version": "0.10.26", "devDependencies": { "typescript": "^5.9.3", "vitest": "^4.1.0" @@ -8522,7 +8522,7 @@ }, "packages/ui": { "name": "@diffity/ui", - "version": "0.10.25", + "version": "0.10.26", "dependencies": { "@diffity/api": "*", "@diffity/parser": "*", diff --git a/package.json b/package.json index 35820d01..cb0c347b 100644 --- a/package.json +++ b/package.json @@ -19,7 +19,7 @@ "link-dev": "tsx scripts/link-dev.ts", "dev": "tsx scripts/dev.ts", "test:scripts": "vitest run scripts", - "typecheck": "npm run typecheck -w @diffity/parser && npm run typecheck -w @diffity/api && npm run typecheck -w @diffity/git && npm run typecheck -w @diffity/github && npm run typecheck -w @diffity/ui && npm run typecheck -w @naturalcycles/diffity" + "typecheck": "npm run typecheck -w @diffity/parser && npm run typecheck -w @diffity/api && npm run typecheck -w @diffity/git && npm run typecheck -w @diffity/github && npm run typecheck -w @diffity/ui && npm run typecheck -w @naturalcycles/diffity && tsc -p tsconfig.scripts.json" }, "keywords": [ "git", diff --git a/packages/api/package.json b/packages/api/package.json index 568606ec..e9e73daf 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/api", - "version": "0.10.25", + "version": "0.10.26", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index b5fd0290..3c255049 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@naturalcycles/diffity", - "version": "0.10.25", + "version": "0.10.26", "description": "Agent-agnostic, GitHub-style diff viewer and code review tool with a live agent loop", "type": "module", "bin": { diff --git a/packages/git/package.json b/packages/git/package.json index 488cfafd..a9171142 100644 --- a/packages/git/package.json +++ b/packages/git/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/git", - "version": "0.10.25", + "version": "0.10.26", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/github/package.json b/packages/github/package.json index 6154e74a..b829470d 100644 --- a/packages/github/package.json +++ b/packages/github/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/github", - "version": "0.10.25", + "version": "0.10.26", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/parser/package.json b/packages/parser/package.json index 11af21d4..93b32f17 100644 --- a/packages/parser/package.json +++ b/packages/parser/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/parser", - "version": "0.10.25", + "version": "0.10.26", "private": true, "type": "module", "main": "./dist/index.js", diff --git a/packages/ui/package.json b/packages/ui/package.json index 2f611472..d88235d7 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@diffity/ui", - "version": "0.10.25", + "version": "0.10.26", "type": "module", "private": true, "scripts": { diff --git a/scripts/inbox-compare.test.ts b/scripts/inbox-compare.test.ts new file mode 100644 index 00000000..c23a4baf --- /dev/null +++ b/scripts/inbox-compare.test.ts @@ -0,0 +1,386 @@ +import { describe, expect, it } from 'vitest'; +import type { BundleThread } from '@diffity/api'; +import { + UsageError, + bundleNamesFor, + candidateLabel, + compareFindings, + findingsOf, + firstSentence, + formatTokens, + matches, + newestBundle, + parseOptions, + parsePrSpec, + renderMarkdown, + renderSpend, + sameHead, + spendOf, + type Finding, +} from './inbox-compare.js'; + +const AGENT = { name: 'agent', type: 'agent' } as const; + +const REF = { owner: 'NaturalCycles', repo: 'NCBackend3', number: 14550 }; + +function finding(over: Partial = {}): Finding { + return { severity: 'P1', filePath: 'src/a.ts', startLine: 10, endLine: 10, sentence: 'Something is off.', ...over }; +} + +function thread(over: Partial = {}): BundleThread { + return { + filePath: 'src/a.ts', + side: 'new', + startLine: 10, + endLine: 12, + status: 'open', + anchorContent: null, + comments: [{ author: AGENT, body: 'P1: it breaks. And more.', kind: 'review', createdAt: '2026-09-07T08:00:00.000Z' }], + ...over, + }; +} + +describe('parseOptions', () => { + it('takes the pull request and leaves the defaults', () => { + const options = parseOptions(['NaturalCycles/NCBackend3#14550']); + + expect(options.ref).toEqual(REF); + expect(options.head).toBeNull(); + expect(options.model).toBeNull(); + expect(options.json).toBe(false); + expect(options.keep).toBe(false); + expect(options.bundlesDir).toMatch(/\/\.diffity\/inbox\/bundles$/); + expect(options.reposDir).toMatch(/\/nc\/repos$/); + expect(options.scratch).toBeNull(); + }); + + it('reads every flag', () => { + const options = parseOptions([ + 'o/r#7', '--head', 'ABCDEF1234567', '--model', 'opus', '--effort', 'medium', + '--bundles-dir', '/b', '--repos-dir', '/r', '--scratch', '/s', '--out', '/o.md', '--json', '--keep', + ]); + + expect(options).toEqual({ + ref: { owner: 'o', repo: 'r', number: 7 }, + head: 'ABCDEF1234567', + model: 'opus', + effort: 'medium', + bundlesDir: '/b', + reposDir: '/r', + scratch: '/s', + out: '/o.md', + json: true, + keep: true, + }); + }); + + it('refuses what it cannot act on', () => { + expect(() => parseOptions([])).toThrow(UsageError); + expect(() => parseOptions(['o/r#1', '--nope'])).toThrow(/unknown option --nope/); + expect(() => parseOptions(['o/r#1', 'o/r#2'])).toThrow(/one pull request at a time/); + expect(() => parseOptions(['o/r#1', '--model'])).toThrow(/--model needs a value/); + expect(() => parseOptions(['o/r#1', '--model', '--json'])).toThrow(/--model needs a value/); + expect(() => parseOptions(['o/r#1', '--effort', 'gentle'])).toThrow(/low\|medium\|high\|xhigh\|max/); + expect(() => parseOptions(['o/r#1', '--head', 'zzz'])).toThrow(/at least 7 hex digits/); + }); +}); + +describe('parsePrSpec', () => { + it('reads owner, repository and number', () => { + expect(parsePrSpec('NaturalCycles/NCBackend3#14550')).toEqual(REF); + }); + + it('refuses anything else', () => { + for (const spec of ['NCBackend3#1', 'o/r', 'o/r#', 'o/r#x', '#1']) { + expect(() => parsePrSpec(spec)).toThrow(UsageError); + } + }); +}); + +describe('bundleNamesFor', () => { + it('keeps the pull request its own, dashes in the names and all', () => { + const names = [ + 'NaturalCycles-NCBackend3-14550-879ffcdc4ebf.json', + 'NaturalCycles-NCBackend3-14550-0e0e0c6ba442.json', + 'NaturalCycles-NCBackend3-1455-879ffcdc4ebf.json', + 'NaturalCycles-NCBackend3-145500-879ffcdc4ebf.json', + 'NaturalCycles-admin3-14550-879ffcdc4ebf.json', + 'NaturalCycles-NCBackend3-14550-879ffcdc4ebf.log', + 'NaturalCycles-NCBackend3-14550-notahash.json', + ]; + + expect(bundleNamesFor(names, REF)).toEqual([ + 'NaturalCycles-NCBackend3-14550-879ffcdc4ebf.json', + 'NaturalCycles-NCBackend3-14550-0e0e0c6ba442.json', + ]); + }); +}); + +describe('newestBundle', () => { + const bundles = [ + { path: '/b/old.json', headSha: 'aaaaaaaaaaaa1111', createdAt: '2026-09-01T10:00:00.000Z' }, + { path: '/b/new.json', headSha: 'bbbbbbbbbbbb2222', createdAt: '2026-09-07T10:00:00.000Z' }, + { path: '/b/mid.json', headSha: 'aaaaaaaaaaaa1111', createdAt: '2026-09-03T10:00:00.000Z' }, + ]; + + it('takes the newest when no head is named', () => { + expect(newestBundle(bundles, null)?.path).toBe('/b/new.json'); + }); + + it('takes the newest at the named head', () => { + expect(newestBundle(bundles, 'aaaaaaaaaaaa')?.path).toBe('/b/mid.json'); + }); + + it('is null when nothing is at that head', () => { + expect(newestBundle(bundles, 'cccccccccccc')).toBeNull(); + expect(newestBundle([], null)).toBeNull(); + }); +}); + +describe('sameHead', () => { + it('compares on the shorter sha', () => { + expect(sameHead('879ffcdc4ebf09fec4466d6c2c8b94d6909e282b', '879ffcdc4ebf')).toBe(true); + expect(sameHead('879FFCDC4EBF', '879ffcdc4ebf09fe')).toBe(true); + expect(sameHead('879ffcdc4ebf', '879ffcdc0000')).toBe(false); + }); + + it('will not call six digits a match', () => { + expect(sameHead('879ffc', '879ffcdc4ebf')).toBe(false); + }); +}); + +describe('findingsOf', () => { + it('labels each finding by the severity it opens with', () => { + const findings = findingsOf([ + thread(), + thread({ filePath: 'docs/x.md', startLine: 40, endLine: 40, comments: [{ author: AGENT, body: '[suggestion] rename it.', kind: 'review', createdAt: '2026-09-07T08:00:00.000Z' }] }), + thread({ comments: [{ author: AGENT, body: 'no marker at all.', kind: 'review', createdAt: '2026-09-07T08:00:00.000Z' }] }), + ]); + + expect(findings.map(item => item.severity)).toEqual(['P1', 'suggestion', 'other']); + expect(findings[0]).toEqual({ severity: 'P1', filePath: 'src/a.ts', startLine: 10, endLine: 12, sentence: 'it breaks.' }); + }); + + it('leaves out the general summary and the threads nobody has to act on', () => { + const findings = findingsOf([ + thread(), + thread({ filePath: '__general__', startLine: 0, endLine: 0 }), + thread({ filePath: 'src/b.ts', status: 'dismissed' }), + thread({ filePath: 'src/c.ts', status: 'resolved' }), + ]); + + expect(findings.map(item => item.filePath)).toEqual(['src/a.ts']); + }); + + it('takes the review comment rather than whatever was added later', () => { + const findings = findingsOf([thread({ + comments: [ + { author: AGENT, body: 'a note.', kind: 'aside', createdAt: '2026-09-07T08:00:00.000Z' }, + { author: AGENT, body: 'P2: the real finding.', kind: 'review', createdAt: '2026-09-07T08:01:00.000Z' }, + ], + })]); + + expect(findings[0].severity).toBe('P2'); + expect(findings[0].sentence).toBe('the real finding.'); + }); +}); + +describe('firstSentence', () => { + it('drops the severity marker', () => { + expect(firstSentence('P1: this is wrong. And this too.')).toBe('this is wrong.'); + expect(firstSentence('[must-fix] this is wrong.')).toBe('this is wrong.'); + }); + + it('does not break a sentence on a decimal point', () => { + expect(firstSentence('P3: the threshold is 0.197 here. The other one differs.')).toBe('the threshold is 0.197 here.'); + }); + + it('collapses the newlines a finding is written over', () => { + expect(firstSentence('P2: one\n two three')).toBe('one two three'); + }); + + it('cuts a long opening', () => { + expect(firstSentence(`P1: ${'x'.repeat(300)}`, 20)).toBe(`${'x'.repeat(20)}…`); + }); +}); + +describe('matches', () => { + it('needs the same file', () => { + expect(matches(finding(), finding({ filePath: 'src/b.ts' }))).toBe(false); + }); + + it('allows a one-line finding two lines of slack either way', () => { + expect(matches(finding(), finding({ startLine: 12, endLine: 12 }))).toBe(true); + expect(matches(finding(), finding({ startLine: 14, endLine: 14 }))).toBe(true); + expect(matches(finding(), finding({ startLine: 15, endLine: 15 }))).toBe(false); + }); + + it('takes a range as written', () => { + const range = finding({ startLine: 20, endLine: 30 }); + + expect(matches(range, finding({ startLine: 30, endLine: 40 }))).toBe(true); + expect(matches(range, finding({ startLine: 31, endLine: 40 }))).toBe(false); + expect(matches(range, finding({ startLine: 32, endLine: 32 }))).toBe(true); + }); +}); + +describe('compareFindings', () => { + const spend = { costUsd: 1.2, minutes: 6.1, turns: 22, outputTokens: 14_000 }; + + function compare(baseline: Finding[], drafted: Finding[]) { + return compareFindings({ pr: 'o/r#1', head: '879ffcdc4ebf', candidate: 'opus', baseline, drafted, spend }); + } + + it('counts reproduced against the baseline and new against the candidate', () => { + const comparison = compare( + [ + finding({ severity: 'P1', filePath: 'src/a.ts', startLine: 12, endLine: 14 }), + finding({ severity: 'P2', filePath: 'docs/x.md', startLine: 40, endLine: 40 }), + finding({ severity: 'P3', filePath: 'src/c.ts', startLine: 5, endLine: 5 }), + ], + [ + finding({ severity: 'P1', filePath: 'src/a.ts', startLine: 12, endLine: 12 }), + finding({ severity: 'P3', filePath: 'src/c.ts', startLine: 6, endLine: 6 }), + finding({ severity: 'P2', filePath: 'src/x.ts', startLine: 80, endLine: 80 }), + finding({ severity: 'P3', filePath: 'src/y.ts', startLine: 1, endLine: 1 }), + finding({ severity: 'P3', filePath: 'src/z.ts', startLine: 1, endLine: 1 }), + ], + ); + + expect(comparison.severities).toEqual([ + { severity: 'P1', baseline: 1, reproduced: 1, added: 0 }, + { severity: 'P2', baseline: 1, reproduced: 0, added: 1 }, + { severity: 'P3', baseline: 1, reproduced: 1, added: 2 }, + ]); + expect(comparison.baseline.map(row => row.reproducedBy?.filePath ?? null)).toEqual(['src/a.ts', null, 'src/c.ts']); + expect(comparison.added.map(row => row.filePath)).toEqual(['src/x.ts', 'src/y.ts', 'src/z.ts']); + }); + + it('lists only the severities either side has', () => { + const comparison = compare([finding({ severity: 'P1' })], [finding({ severity: 'other', filePath: 'src/q.ts' })]); + + expect(comparison.severities.map(row => row.severity)).toEqual(['P1', 'other']); + }); + + it('matches on the lines whatever the severities say', () => { + const comparison = compare([finding({ severity: 'P1' })], [finding({ severity: 'P3' })]); + + expect(comparison.severities).toEqual([{ severity: 'P1', baseline: 1, reproduced: 1, added: 0 }]); + expect(comparison.baseline[0].reproducedBy?.severity).toBe('P3'); + expect(comparison.added).toEqual([]); + }); + + it('carries what the run spent through', () => { + expect(compare([], []).spend).toEqual(spend); + }); +}); + +describe('spendOf', () => { + it('is all unknown when the agent reported nothing', () => { + expect(spendOf(null)).toEqual({ costUsd: null, minutes: null, turns: null, outputTokens: null }); + }); + + it('reads minutes out of the duration', () => { + const spend = spendOf({ + costUsd: 1.2, durationMs: 366_000, turns: 22, inputTokens: 100, outputTokens: 14_000, + cacheReadTokens: 0, cacheWriteTokens: 0, models: ['opus'], isError: false, subtype: 'success', + }); + + expect(spend).toEqual({ costUsd: 1.2, minutes: 6.1, turns: 22, outputTokens: 14_000 }); + }); +}); + +describe('renderSpend', () => { + it('writes an unknown as a dash', () => { + expect(renderSpend({ costUsd: null, minutes: null, turns: null, outputTokens: null })).toBe('— / — / — / —'); + }); + + it('writes what is known', () => { + expect(renderSpend({ costUsd: 1.2, minutes: 6.14, turns: 22, outputTokens: 14_400 })).toBe('$1.20 / 6.1 min / 22 turns / 14k out'); + }); +}); + +describe('formatTokens', () => { + it('rounds to thousands once there are thousands', () => { + expect(formatTokens(999)).toBe('999'); + expect(formatTokens(1499)).toBe('1k'); + expect(formatTokens(27_400)).toBe('27k'); + }); +}); + +describe('candidateLabel', () => { + it('names the model, or says it is the default one', () => { + expect(candidateLabel('opus', null)).toBe('opus'); + expect(candidateLabel(null, null)).toBe('default model'); + expect(candidateLabel(null, 'medium')).toBe('default model · effort medium'); + }); +}); + +describe('renderMarkdown', () => { + it('renders the block to paste into the issue', () => { + const comparison = compareFindings({ + pr: 'NaturalCycles/NCBackend3#14550', + head: '879ffcdc4ebf', + candidate: 'opus', + baseline: [ + finding({ severity: 'P1', filePath: 'src/b1/config.ts', startLine: 12, endLine: 14, sentence: 'the baseline is stale.' }), + finding({ severity: 'P2', filePath: 'docs/vogon/b1Service.md', startLine: 40, endLine: 40, sentence: 'the document is not updated.' }), + ], + drafted: [ + finding({ severity: 'P1', filePath: 'src/b1/config.ts', startLine: 12, endLine: 12, sentence: 'the baseline is stale.' }), + finding({ severity: 'P2', filePath: 'src/x.ts', startLine: 80, endLine: 80, sentence: 'this leaks.' }), + ], + spend: { costUsd: 1.2, minutes: 6.1, turns: 22, outputTokens: 14_000 }, + }); + + expect(renderMarkdown(comparison)).toBe([ + '### NaturalCycles/NCBackend3#14550 · head 879ffcdc4ebf · candidate: opus', + '| | baseline (Fable, old pipeline) | candidate |', + '|---|---|---|', + '| P1 | 1 | 1 reproduced, 0 new |', + '| P2 | 1 | 0 reproduced, 1 new |', + '| cost / time | — | $1.20 / 6.1 min / 22 turns / 14k out |', + '', + 'Baseline findings:', + '- P1 src/b1/config.ts:12-14 — the baseline is stale. → reproduced by P1 src/b1/config.ts:12', + '- P2 docs/vogon/b1Service.md:40 — the document is not updated. → not reproduced', + 'New in candidate:', + '- P2 src/x.ts:80 — this leaks.', + '', + ].join('\n')); + }); + + it('says so when a side found nothing', () => { + const comparison = compareFindings({ + pr: 'o/r#1', head: 'abcdef123456', candidate: 'default model', + baseline: [], drafted: [], spend: spendOf(null), + }); + + expect(renderMarkdown(comparison)).toContain('Baseline findings:\n- none\nNew in candidate:\n- none'); + expect(renderMarkdown(comparison)).toContain('| cost / time | — | — / — / — / — |'); + }); +}); + +describe('the JSON shape', () => { + it('survives a round trip through JSON', () => { + const comparison = compareFindings({ + pr: 'o/r#1', head: 'abcdef123456', candidate: 'opus', + baseline: [finding()], + drafted: [finding({ severity: 'P2', startLine: 11, endLine: 11 })], + spend: { costUsd: 1.2, minutes: 6.1, turns: 22, outputTokens: 14_000 }, + }); + + expect(JSON.parse(JSON.stringify(comparison))).toEqual({ + pr: 'o/r#1', + head: 'abcdef123456', + candidate: 'opus', + severities: [{ severity: 'P1', baseline: 1, reproduced: 1, added: 0 }], + baseline: [{ + severity: 'P1', filePath: 'src/a.ts', startLine: 10, endLine: 10, sentence: 'Something is off.', + reproducedBy: { severity: 'P2', filePath: 'src/a.ts', startLine: 11, endLine: 11, sentence: 'Something is off.' }, + }], + added: [], + spend: { costUsd: 1.2, minutes: 6.1, turns: 22, outputTokens: 14_000 }, + }); + }); +}); diff --git a/scripts/inbox-compare.ts b/scripts/inbox-compare.ts new file mode 100644 index 00000000..b05d7b37 --- /dev/null +++ b/scripts/inbox-compare.ts @@ -0,0 +1,542 @@ +#!/usr/bin/env node + +/** + * Re-prepares one pull request with a candidate model and puts its findings beside the bundle the + * inbox already has for that head, so a cheaper drafter is judged on what it finds rather than on + * what it costs. One agent run per invocation; everything it writes stays in a scratch directory. + */ + +import { execFile } from 'node:child_process'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { basename, dirname, join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { promisify } from 'node:util'; +import { GENERAL_THREAD_FILE_PATH, parseReviewBundle, type BundleThread, type ReviewBundle } from '@diffity/api'; +import { viewPr } from '@diffity/github'; +import type { RunStats } from '../packages/cli/src/inbox/agent-output.js'; +import { DEFAULT_INBOX_CONFIG, expandHome, type InboxConfig } from '../packages/cli/src/inbox/config.js'; +import { preparePr, type PrepareResult } from '../packages/cli/src/inbox/prepare.js'; +import { realPrepareDeps } from '../packages/cli/src/inbox/runtime.js'; +import { severityOf } from '../packages/cli/src/inbox/summary.js'; +import { cloneDir, removeWorktree } from '../packages/cli/src/inbox/worktree.js'; + +const root = resolve(dirname(fileURLToPath(import.meta.url)), '..'); + +export const USAGE = 'Usage: npx tsx scripts/inbox-compare.ts [--head ] [--model ]' + + ' [--effort ] [--bundles-dir ] [--repos-dir ] [--scratch ]' + + ' [--keep] [--json] [--out ]'; + +/** What the caller asked for, over what it did not name. */ +export class UsageError extends Error {} + +export interface PrRefSpec { + owner: string; + repo: string; + number: number; +} + +export interface Options { + ref: PrRefSpec; + /** The baseline bundle's head, when the newest one is not the wanted one. */ + head: string | null; + model: string | null; + effort: string | null; + bundlesDir: string; + reposDir: string; + /** Where the worktree and the run's diffity data go; null means a fresh temporary directory. */ + scratch: string | null; + json: boolean; + out: string | null; + /** Leave the worktree behind, for reading the candidate's session in the browser. */ + keep: boolean; +} + +const EFFORTS = ['low', 'medium', 'high', 'xhigh', 'max']; + +/** The severities a finding can open with, in the order the table lists them. */ +const SEVERITY_ORDER = ['P1', 'P2', 'P3', 'must-fix', 'suggestion', 'question', 'other']; + +export function parseOptions(argv: string[]): Options { + let spec: string | null = null; + let head: string | null = null; + let model: string | null = null; + let effort: string | null = null; + let bundlesDir = '~/.diffity/inbox/bundles'; + let reposDir = '~/nc/repos'; + let scratch: string | null = null; + let json = false; + let out: string | null = null; + let keep = false; + + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + const value = (): string => { + const next = argv[++i]; + if (next === undefined || next.startsWith('--')) { + throw new UsageError(`${arg} needs a value`); + } + return next; + }; + switch (arg) { + case '--head': head = value(); break; + case '--model': model = value(); break; + case '--effort': effort = value(); break; + case '--bundles-dir': bundlesDir = value(); break; + case '--repos-dir': reposDir = value(); break; + case '--scratch': scratch = value(); break; + case '--out': out = value(); break; + case '--json': json = true; break; + case '--keep': keep = true; break; + default: + if (arg.startsWith('-')) { + throw new UsageError(`unknown option ${arg}`); + } + if (spec !== null) { + throw new UsageError('one pull request at a time'); + } + spec = arg; + } + } + + if (spec === null) { + throw new UsageError('name the pull request to compare, as owner/repo#number'); + } + if (head !== null && !/^[0-9a-f]{7,40}$/i.test(head)) { + throw new UsageError(`--head ${head} is not a commit sha of at least 7 hex digits`); + } + if (effort !== null && !EFFORTS.includes(effort)) { + throw new UsageError(`--effort must be one of ${EFFORTS.join('|')}`); + } + return { + ref: parsePrSpec(spec), + head, model, effort, json, keep, + bundlesDir: expandHome(bundlesDir), + reposDir: expandHome(reposDir), + scratch: scratch === null ? null : expandHome(scratch), + out: out === null ? null : expandHome(out), + }; +} + +export function parsePrSpec(spec: string): PrRefSpec { + const match = /^([A-Za-z0-9._-]+)\/([A-Za-z0-9._-]+)#(\d+)$/.exec(spec); + if (!match) { + throw new UsageError(`"${spec}" is not a pull request; write it as owner/repo#number`); + } + return { owner: match[1], repo: match[2], number: Number(match[3]) }; +} + +/** + * The bundle files in the directory that belong to this pull request. Matched by the whole prefix + * rather than by splitting on the dashes: an owner and a repository name may each hold one. + */ +export function bundleNamesFor(names: string[], ref: PrRefSpec): string[] { + const prefix = `${ref.owner}-${ref.repo}-${ref.number}-`; + return names.filter(name => name.startsWith(prefix) && /^[0-9a-f]{7,40}\.json$/i.test(name.slice(prefix.length))); +} + +export interface BundleRef { + path: string; + headSha: string; + createdAt: string; +} + +/** The newest bundle of the ones given, or the newest at `head` when a head is named. */ +export function newestBundle(bundles: T[], head: string | null): T | null { + const matching = head === null ? bundles : bundles.filter(bundle => sameHead(bundle.headSha, head)); + return [...matching].sort((a, b) => b.createdAt.localeCompare(a.createdAt))[0] ?? null; +} + +/** Two shas name the same commit when the shorter is a prefix of the longer. */ +export function sameHead(a: string, b: string): boolean { + const shared = Math.min(a.length, b.length); + return shared >= 7 && a.slice(0, shared).toLowerCase() === b.slice(0, shared).toLowerCase(); +} + +export interface Finding { + severity: string; + filePath: string; + startLine: number; + endLine: number; + /** The finding's own opening sentence, so a row says what it is about. */ + sentence: string; +} + +type ThreadShape = Pick; + +/** + * The findings a reviewer still has to act on: the general summary is not one, and neither is a + * thread the checking pass dismissed or resolved. + */ +export function findingsOf(threads: ThreadShape[]): Finding[] { + const findings: Finding[] = []; + for (const thread of threads) { + if (thread.filePath === GENERAL_THREAD_FILE_PATH || thread.status !== 'open') { + continue; + } + const comment = thread.comments.find(item => item.kind === 'review') ?? thread.comments[0]; + if (!comment) { + continue; + } + findings.push({ + severity: severityOf(comment.body), + filePath: thread.filePath, + startLine: thread.startLine, + endLine: thread.endLine, + sentence: firstSentence(comment.body), + }); + } + return findings; +} + +/** The first sentence without the severity marker the severity column already carries. */ +export function firstSentence(body: string, limit = 160): string { + const text = body + .replace(/^\s*(?:P[1-3]\b:?|\[(?:must-fix|suggestion|question)\]:?)\s*/i, '') + .replace(/\s+/g, ' ') + .trim(); + // A full stop only ends a sentence when something breaks after it, so `0.197` stays whole. + const match = /^.*?[.!?](?=\s|$)/.exec(text); + const sentence = (match ? match[0] : text).trim(); + return sentence.length > limit ? `${sentence.slice(0, limit).trimEnd()}…` : sentence; +} + +/** How far off a one-line finding may be and still be the same one: two models rarely agree on the line. */ +const LINE_SLACK = 2; + +export function rangeOf(finding: Pick): [number, number] { + return finding.startLine === finding.endLine + ? [finding.startLine - LINE_SLACK, finding.endLine + LINE_SLACK] + : [finding.startLine, finding.endLine]; +} + +/** The same finding: the same file, and line ranges that overlap. */ +export function matches(a: Finding, b: Finding): boolean { + if (a.filePath !== b.filePath) { + return false; + } + const [aStart, aEnd] = rangeOf(a); + const [bStart, bEnd] = rangeOf(b); + return aStart <= bEnd && bStart <= aEnd; +} + +/** What one agent run spent, as the table prints it. */ +export interface Spend { + costUsd: number | null; + minutes: number | null; + turns: number | null; + outputTokens: number | null; +} + +export function spendOf(stats: RunStats | null): Spend { + return { + costUsd: stats?.costUsd ?? null, + minutes: stats?.durationMs == null ? null : stats.durationMs / 60_000, + turns: stats?.turns ?? null, + outputTokens: stats === null ? null : stats.outputTokens, + }; +} + +export interface BaselineRow extends Finding { + reproducedBy: Finding | null; +} + +export interface SeverityRow { + severity: string; + baseline: number; + reproduced: number; + added: number; +} + +export interface Comparison { + pr: string; + head: string; + candidate: string; + severities: SeverityRow[]; + baseline: BaselineRow[]; + /** The candidate's findings that no baseline finding covers. */ + added: Finding[]; + spend: Spend; +} + +export function compareFindings(input: { + pr: string; + head: string; + candidate: string; + baseline: Finding[]; + drafted: Finding[]; + spend: Spend; +}): Comparison { + // Not a pairing: a baseline finding counts as reproduced when any candidate finding lands on it, + // and a candidate finding is new when none of the baseline's does. + const baseline: BaselineRow[] = input.baseline.map(finding => ({ + ...finding, + reproducedBy: input.drafted.find(drafted => matches(finding, drafted)) ?? null, + })); + const added = input.drafted.filter(drafted => !input.baseline.some(finding => matches(finding, drafted))); + return { + pr: input.pr, + head: input.head, + candidate: input.candidate, + severities: severityRows(baseline, added), + baseline, + added, + spend: input.spend, + }; +} + +function severityRows(baseline: BaselineRow[], added: Finding[]): SeverityRow[] { + return SEVERITY_ORDER + .filter(severity => baseline.some(row => row.severity === severity) || added.some(row => row.severity === severity)) + .map(severity => ({ + severity, + baseline: baseline.filter(row => row.severity === severity).length, + reproduced: baseline.filter(row => row.severity === severity && row.reproducedBy !== null).length, + added: added.filter(row => row.severity === severity).length, + })); +} + +export function candidateLabel(model: string | null, effort: string | null): string { + const parts = [model ?? 'default model']; + if (effort !== null) { + parts.push(`effort ${effort}`); + } + return parts.join(' · '); +} + +export function location(finding: Pick): string { + return finding.startLine === finding.endLine + ? `${finding.filePath}:${finding.startLine}` + : `${finding.filePath}:${finding.startLine}-${finding.endLine}`; +} + +export function renderSpend(spend: Spend): string { + return [ + spend.costUsd === null ? '—' : `$${spend.costUsd.toFixed(2)}`, + spend.minutes === null ? '—' : `${spend.minutes.toFixed(1)} min`, + spend.turns === null ? '—' : `${spend.turns} turns`, + spend.outputTokens === null ? '—' : `${formatTokens(spend.outputTokens)} out`, + ].join(' / '); +} + +export function formatTokens(tokens: number): string { + return tokens >= 1000 ? `${Math.round(tokens / 1000)}k` : String(tokens); +} + +/** The block to paste into the issue: the tally, then every finding either side has. */ +export function renderMarkdown(comparison: Comparison): string { + const lines = [ + `### ${comparison.pr} · head ${comparison.head} · candidate: ${comparison.candidate}`, + '| | baseline (Fable, old pipeline) | candidate |', + '|---|---|---|', + ]; + for (const row of comparison.severities) { + lines.push(`| ${row.severity} | ${row.baseline} | ${row.reproduced} reproduced, ${row.added} new |`); + } + lines.push(`| cost / time | — | ${renderSpend(comparison.spend)} |`, ''); + + lines.push('Baseline findings:'); + if (comparison.baseline.length === 0) { + lines.push('- none'); + } + for (const row of comparison.baseline) { + const outcome = row.reproducedBy === null + ? 'not reproduced' + : `reproduced by ${row.reproducedBy.severity} ${location(row.reproducedBy)}`; + lines.push(`- ${row.severity} ${location(row)} — ${row.sentence} → ${outcome}`); + } + + lines.push('New in candidate:'); + if (comparison.added.length === 0) { + lines.push('- none'); + } + for (const row of comparison.added) { + lines.push(`- ${row.severity} ${location(row)} — ${row.sentence}`); + } + return `${lines.join('\n')}\n`; +} + +function readBundle(path: string): ReviewBundle | null { + let raw: unknown; + try { + raw = JSON.parse(readFileSync(path, 'utf-8')); + } catch { + return null; + } + const parsed = parseReviewBundle(raw); + return parsed.ok ? parsed.value : null; +} + +/** The bundle the candidate is measured against, by head when one is named and by age otherwise. */ +function readBaseline(options: Options): { path: string; bundle: ReviewBundle } { + if (!existsSync(options.bundlesDir)) { + throw new UsageError(`no bundles directory at ${options.bundlesDir}`); + } + const found = bundleNamesFor(readdirSync(options.bundlesDir), options.ref) + .map(name => join(options.bundlesDir, name)) + .flatMap(path => { + const bundle = readBundle(path); + return bundle === null ? [] : [{ path, bundle, headSha: bundle.headSha, createdAt: bundle.createdAt }]; + }); + const newest = newestBundle(found, options.head); + if (!newest) { + const at = options.head === null ? '' : ` at head ${options.head}`; + throw new UsageError(`no bundle for ${prName(options.ref)}${at} in ${options.bundlesDir}; there is nothing to compare against`); + } + return newest; +} + +/** + * The head `refs/pull//head` points at, fetched into the clone. This is the only head a + * worktree can be cut at, so it decides whether the baseline's head is still reachable as a + * checkout — and it is asked before an agent run is spent rather than after. + */ +async function fetchPrHead(clone: string, number: number): Promise { + const git = (args: string[]) => promisify(execFile)('git', ['-c', 'core.hooksPath=/dev/null', ...args], { + cwd: clone, encoding: 'utf-8', env: { ...process.env, GIT_TERMINAL_PROMPT: '0' }, + }); + await git(['fetch', 'origin', `refs/pull/${number}/head`]); + const { stdout } = await git(['rev-parse', 'FETCH_HEAD']); + return stdout.trim(); +} + +function prName(ref: PrRefSpec): string { + return `${ref.owner}/${ref.repo}#${ref.number}`; +} + +function worktreeOf(result: PrepareResult): string | null { + return 'worktree' in result ? result.worktree : null; +} + +async function main(): Promise { + let options: Options; + try { + options = parseOptions(process.argv.slice(2)); + } catch (err) { + console.error(`❌ ${err instanceof Error ? err.message : err}`); + console.error(USAGE); + return 1; + } + + const entry = resolve(root, 'packages/cli/dist/index.js'); + let baseline: { path: string; bundle: ReviewBundle }; + const clone = cloneDir(options.reposDir, options.ref.repo); + try { + if (!existsSync(entry)) { + throw new UsageError(`${entry} is missing — run \`npm run build\` first`); + } + if (!existsSync(clone)) { + throw new UsageError(`no clone at ${clone}; clone ${options.ref.owner}/${options.ref.repo} there first`); + } + baseline = readBaseline(options); + } catch (err) { + console.error(`❌ ${err instanceof Error ? err.message : err}`); + return 1; + } + + const head = baseline.bundle.headSha; + const short = head.slice(0, 12); + console.error(`🔍 ${prName(options.ref)} · baseline ${basename(baseline.path)} of ${baseline.bundle.createdAt}`); + + let current: string; + try { + current = await fetchPrHead(clone, options.ref.number); + } catch (err) { + console.error(`⏭ ${clone} could not fetch refs/pull/${options.ref.number}/head: ${err instanceof Error ? err.message : err}`); + return 2; + } + if (!sameHead(current, head)) { + console.error(`⏭ the pull request's head is now ${current.slice(0, 12)}, and a worktree can only be cut at that one; re-run against a bundle at that head, or pick another pull request.`); + return 2; + } + + const snapshot = await viewPr(options.ref); + if (!snapshot) { + console.error(`⏭ gh could not read ${prName(options.ref)}`); + return 2; + } + + const scratch = options.scratch ?? mkdtempSync(join(tmpdir(), 'diffity-compare-')); + mkdirSync(scratch, { recursive: true }); + // Everything preparing writes — the exported bundle, the agent's log, each session's data — is + // rooted here, so a comparison never touches the running inbox's own directory. + process.env.DIFFITY_DATA_DIR = scratch; + + const config: InboxConfig = { + ...DEFAULT_INBOX_CONFIG, + reposDir: options.reposDir, + worktreesDir: join(scratch, 'worktrees'), + filter: '', + alertWhen: '', + alertPaths: [], + agent: { model: options.model, effort: options.effort, mcpAllow: [], extraArgs: [], maxBudgetUsd: null }, + validate: { ...DEFAULT_INBOX_CONFIG.validate, model: null }, + }; + const candidate = candidateLabel(options.model, options.effort); + const deps = realPrepareDeps( + process.execPath, entry, + worktree => join(scratch, 'data', basename(worktree)), + config, message => console.error(` ${message}`), + ); + + console.error(`🤖 preparing ${prName(options.ref)} at ${short} with ${candidate} — agent running…`); + const startedAt = Date.now(); + const elapsed = () => (Date.now() - startedAt) / 60_000; + const ticker = setInterval(() => console.error(` … ${elapsed().toFixed(0)} min`), 60_000); + let result: PrepareResult; + try { + result = await preparePr({ ...snapshot, headSha: head }, config, deps, { bumped: true }); + } finally { + clearInterval(ticker); + } + console.error(`⏱️ the agent stopped after ${elapsed().toFixed(1)} min`); + + const worktree = worktreeOf(result); + try { + if (result.kind !== 'prepared') { + const what = result.kind === 'skipped' ? 'skipped this pull request' : 'failed'; + console.error(`⏭ the candidate ${what}: ${result.reason}`); + return 2; + } + if (!sameHead(result.headSha, head)) { + console.error(`⏭ the worktree ended up at ${result.headSha.slice(0, 12)}, not the baseline's ${short}; the findings would not be about the same code.`); + return 2; + } + const drafted = readBundle(result.bundlePath); + if (!drafted) { + console.error(`⏭ the candidate's bundle at ${result.bundlePath} could not be read`); + return 2; + } + + const comparison = compareFindings({ + pr: prName(options.ref), + head: short, + candidate, + baseline: findingsOf(baseline.bundle.threads), + drafted: findingsOf(drafted.threads), + spend: spendOf(result.run.stats), + }); + const markdown = renderMarkdown(comparison); + if (options.out !== null) { + mkdirSync(dirname(options.out), { recursive: true }); + writeFileSync(options.out, markdown); + console.error(`📝 ${options.out}`); + } + console.log(options.json ? JSON.stringify(comparison, null, 2) : markdown.trimEnd()); + return 0; + } finally { + if (worktree !== null && !options.keep) { + try { + await removeWorktree(clone, worktree); + } catch (err) { + console.error(` the worktree at ${worktree} is still there: ${err instanceof Error ? err.message : err}`); + } + } + console.error(`🗂️ the run's data is under ${scratch}`); + } +} + +// Only when run as a script: the test beside this file imports it for the pure functions above. +if (process.argv[1] !== undefined && resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { + process.exitCode = await main(); +} diff --git a/tsconfig.scripts.json b/tsconfig.scripts.json new file mode 100644 index 00000000..856becb6 --- /dev/null +++ b/tsconfig.scripts.json @@ -0,0 +1,8 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "types": ["node"] + }, + "include": ["scripts/**/*.ts"] +} From ebac6482be27ffcc8773f55d11582f3006916e07 Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Mon, 7 Sep 2026 21:05:45 +0200 Subject: [PATCH 2/3] docs: --keep says what it keeps Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- development.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/development.md b/development.md index 321ecb6e..96f1bb6f 100644 --- a/development.md +++ b/development.md @@ -141,8 +141,10 @@ npx tsx scripts/inbox-compare.ts NaturalCycles/NCBackend3#14550 --effort medium The baseline is the newest bundle for that pull request under `~/.diffity/inbox/bundles` (`--bundles-dir` to look elsewhere, `--head ` to pick an older one). The candidate gets a scratch worktree and its own diffity data directory under a fresh temp directory (`--scratch` to -name it), so nothing is written into `~/.diffity` and a running `diffity inbox` is undisturbed. One -invocation is one agent run, and it takes as long as a real preparation — up to half an hour. +name it), so nothing is written into `~/.diffity` and a running `diffity inbox` is undisturbed. The +worktree is removed afterwards unless `--keep` is passed, which leaves it in place so the +candidate's own review can be opened in the browser. One invocation is one agent run, and it takes +as long as a real preparation — up to half an hour. A worktree can only be cut at whatever `refs/pull//head` points at, so the script refuses, before spending the run, when the pull request has moved past the baseline's head. From 59f2d4d6807c3f1f066abd58ed815c8d5b9d43fe Mon Sep 17 00:00:00 2001 From: "Fredrik Liljegren (Claude Code Claude Opus 5)" Date: Mon, 7 Sep 2026 21:15:10 +0200 Subject: [PATCH 3/3] fix: a comparison pins the baseline's head instead of refusing to run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A worktree is not limited to whatever refs/pull//head points at now: an earlier head of the pull request is usually an ancestor of the current one, and the forge serves any commit reachable from an advertised ref by sha. `prepareWorktree` takes an optional pinHead and checks that commit out — present already, fetched by sha, or refused as force-pushed away — and `preparePr` passes it through from PrepareOpts. The daemon sets neither and behaves exactly as before. The compare script pins the baseline's head and drops the pre-flight refusal, so the six bundles on this machine whose pull requests have moved on are comparable again — including #14550, one of the two P1-in-a-tiny-diff cases the adoption gate needs. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Bwp5QefjsjMFeq6CK6cT6w --- development.md | 7 ++- packages/cli/src/inbox/prepare.ts | 7 ++- packages/cli/src/inbox/worktree.ts | 28 ++++++++++- packages/cli/tests/inbox-prepare.test.ts | 19 +++++++ packages/cli/tests/inbox-worktree.test.ts | 60 ++++++++++++++++++++++- scripts/inbox-compare.ts | 30 +----------- 6 files changed, 116 insertions(+), 35 deletions(-) diff --git a/development.md b/development.md index 96f1bb6f..7d94faa9 100644 --- a/development.md +++ b/development.md @@ -146,8 +146,11 @@ worktree is removed afterwards unless `--keep` is passed, which leaves it in pla candidate's own review can be opened in the browser. One invocation is one agent run, and it takes as long as a real preparation — up to half an hour. -A worktree can only be cut at whatever `refs/pull//head` points at, so the script refuses, -before spending the run, when the pull request has moved past the baseline's head. +The candidate is pinned to the baseline's head, so a pull request that has moved on since — or has +merged — can still be compared. An earlier head usually comes along with the pull request's own +ref; when it does not, it is fetched by sha, which the forge serves for any commit reachable from a +ref it advertises. A head that was force-pushed away is gone for good, and the run is refused with +`the baseline's head is no longer reachable from origin`. Reading the table: each severity row is `baseline count | reproduced, new`. *Reproduced* means a candidate finding landed on the same file with an overlapping line range — a one-line finding diff --git a/packages/cli/src/inbox/prepare.ts b/packages/cli/src/inbox/prepare.ts index 8605936b..4c831b32 100644 --- a/packages/cli/src/inbox/prepare.ts +++ b/packages/cli/src/inbox/prepare.ts @@ -94,6 +94,11 @@ export function logsDir(): string { export interface PrepareOpts { /** The reviewer asked for this one by name, so the filter does not get a say. */ bumped?: boolean; + /** + * Review this commit rather than wherever the pull request has got to. Only a comparison against + * an earlier review sets it; the daemon always takes the current head. + */ + pinHead?: string; } export async function preparePr(snapshot: PrSnapshot, config: InboxConfig, deps: PrepareDeps, opts: PrepareOpts = {}): Promise { @@ -107,7 +112,7 @@ export async function preparePr(snapshot: PrSnapshot, config: InboxConfig, deps: let head: string; let diffRef: string; try { - ({ head, diffRef } = await prepareWorktree(clone, dest, snapshot, snapshot.baseRef)); + ({ head, diffRef } = await prepareWorktree(clone, dest, snapshot, snapshot.baseRef, opts.pinHead)); } catch (err) { return { kind: 'failed', failure: 'worktree', reason: err instanceof Error ? err.message : String(err), worktree: null, logPath: null, run }; } diff --git a/packages/cli/src/inbox/worktree.ts b/packages/cli/src/inbox/worktree.ts index 28b92b53..55852d9c 100644 --- a/packages/cli/src/inbox/worktree.ts +++ b/packages/cli/src/inbox/worktree.ts @@ -38,8 +38,11 @@ async function runGit(cwd: string, args: string[]): Promise { * diff against — the fetched base, so a diffity session over the worktree shows the same change as * the pull request without asking the forge anything. Idempotent and self-healing: an existing * worktree, even one a killed agent left dirty, is forced to the new head rather than re-created. + * + * `pinHead` cuts the worktree at that commit instead of wherever the pull request has got to, + * which is how a review is re-run against the head an earlier one was written at. */ -export async function prepareWorktree(clone: string, dest: string, ref: PrRef, baseRef: string): Promise<{ head: string; diffRef: string }> { +export async function prepareWorktree(clone: string, dest: string, ref: PrRef, baseRef: string, pinHead?: string): Promise<{ head: string; diffRef: string }> { if (!existsSync(clone)) { throw new Error(`No local clone at ${clone}. Clone ${ref.owner}/${ref.repo} there first.`); } @@ -49,7 +52,9 @@ export async function prepareWorktree(clone: string, dest: string, ref: PrRef, b await requireMatchingOrigin(clone, ref); await runGit(clone, ['fetch', 'origin', `refs/pull/${ref.number}/head`]); - const head = await runGit(clone, ['rev-parse', 'FETCH_HEAD']); + const head = pinHead === undefined + ? await runGit(clone, ['rev-parse', 'FETCH_HEAD']) + : await reachable(clone, pinHead); // `refs/heads/` so a tag sharing the branch's name cannot be fetched in its place. await runGit(clone, ['fetch', 'origin', `refs/heads/${baseRef}`]); const diffRef = await runGit(clone, ['rev-parse', 'FETCH_HEAD']); @@ -77,6 +82,25 @@ export async function prepareWorktree(clone: string, dest: string, ref: PrRef, b return { head, diffRef }; } +/** + * The pinned commit, as a full sha, with the object in the clone. Fetching the pull request's ref + * usually brings it along already — an earlier head is an ancestor of a later one unless the branch + * was rewritten — and otherwise the forge serves any commit reachable from a ref it advertises, so + * it is asked for by sha. A force-push is what puts a commit out of reach for good. + */ +async function reachable(clone: string, pinHead: string): Promise { + try { + await runGit(clone, ['cat-file', '-e', `${pinHead}^{commit}`]); + } catch { + try { + await runGit(clone, ['fetch', 'origin', pinHead]); + } catch { + throw new Error(`the baseline's head ${pinHead.slice(0, 12)} is no longer reachable from origin (force-pushed?)`); + } + } + return runGit(clone, ['rev-parse', pinHead]); +} + /** The clone must actually be the pull request's repository, not another of the same name. */ async function requireMatchingOrigin(clone: string, ref: PrRef): Promise { let url: string; diff --git a/packages/cli/tests/inbox-prepare.test.ts b/packages/cli/tests/inbox-prepare.test.ts index 41a06720..f6e3caab 100644 --- a/packages/cli/tests/inbox-prepare.test.ts +++ b/packages/cli/tests/inbox-prepare.test.ts @@ -483,6 +483,25 @@ describe('the inbox JSON server', () => { store.close(); }); + it('reviews a pinned head the pull request has moved past', async () => { + // A second push, so the snapshot's head is no longer where the pull request points. + const upstream = join(root, 'remotes', 'o', 'demo'); + writeFileSync(join(upstream, 'b.ts'), 'const b = 2;\n'); + git(upstream, ['add', '.']); + git(upstream, ['commit', '-m', 'more']); + git(upstream, ['update-ref', 'refs/pull/4/head', 'HEAD']); + const moved = git(upstream, ['rev-parse', 'HEAD']); + + const result = await preparePr(snapshot(), config(), deps(), { pinHead: head }); + + expect(result.kind).toBe('prepared'); + if (result.kind !== 'prepared') return; + expect(result.headSha).toBe(head); + expect(result.headSha).not.toBe(moved); + expect(existsSync(join(result.worktree, 'b.ts'))).toBe(false); + expect(result.bundlePath).toContain(head.slice(0, 12)); + }); + it('sets the filter aside for a bumped pull request', async () => { prompts = []; const withFilter = { ...config(), filter: 'Skip payments-focused PRs' }; diff --git a/packages/cli/tests/inbox-worktree.test.ts b/packages/cli/tests/inbox-worktree.test.ts index a392561c..5953a53e 100644 --- a/packages/cli/tests/inbox-worktree.test.ts +++ b/packages/cli/tests/inbox-worktree.test.ts @@ -6,6 +6,7 @@ import { tmpdir } from 'node:os'; import { prepareWorktree, removeWorktree } from '../src/inbox/worktree.js'; let root: string; +let upstream: string; let clone: string; let dest: string; let head: string; @@ -17,7 +18,7 @@ function git(cwd: string, args: string[]): string { beforeEach(() => { root = mkdtempSync(join(tmpdir(), 'diffity-worktree-')); - const upstream = join(root, 'remotes', 'o', 'demo'); + upstream = join(root, 'remotes', 'o', 'demo'); execFileSync('git', ['init', '-b', 'main', upstream], { stdio: 'pipe' }); git(upstream, ['config', 'user.email', 't@t']); git(upstream, ['config', 'user.name', 'T']); @@ -80,6 +81,63 @@ describe('prepareWorktree', () => { expect(existsSync(join(dest, 'debris.txt'))).toBe(false); expect(git(dest, ['rev-parse', 'HEAD'])).toBe(head); }); + + describe('with a pinned head', () => { + /** A second push on the pull request, leaving the head captured in `head` behind. */ + function pushOnTop(): string { + writeFileSync(join(upstream, 'b.ts'), 'const b = 2;\n'); + git(upstream, ['add', '.']); + git(upstream, ['commit', '-m', 'more']); + git(upstream, ['update-ref', `refs/pull/${ref.number}/head`, 'HEAD']); + return git(upstream, ['rev-parse', 'HEAD']); + } + + it('cuts the worktree at an earlier head the pull request has moved past', async () => { + const moved = pushOnTop(); + + const cut = await prepareWorktree(clone, dest, ref, 'main', head); + + expect(cut.head).toBe(head); + expect(git(dest, ['rev-parse', 'HEAD'])).toBe(head); + // The later commit's file is not in the tree, so the review is about the pinned code. + expect(existsSync(join(dest, 'b.ts'))).toBe(false); + expect(moved).not.toBe(head); + }); + + it('takes the pull request as it stands when nothing is pinned', async () => { + const moved = pushOnTop(); + + const cut = await prepareWorktree(clone, dest, ref, 'main'); + + expect(cut.head).toBe(moved); + expect(existsSync(join(dest, 'b.ts'))).toBe(true); + }); + + it('asks origin by sha for a pinned commit the clone has never fetched', async () => { + // What GitHub allows: any commit reachable from a ref it advertises can be asked for by sha. + git(upstream, ['config', 'uploadpack.allowReachableSHA1InWant', 'true']); + // On a branch of its own, made after the clone, so no fetch of main or the pull ref brings it. + git(upstream, ['checkout', '-q', '-b', 'other']); + writeFileSync(join(upstream, 'c.ts'), 'const c = 3;\n'); + git(upstream, ['add', '.']); + git(upstream, ['commit', '-m', 'elsewhere']); + const elsewhere = git(upstream, ['rev-parse', 'HEAD']); + expect(() => git(clone, ['cat-file', '-e', `${elsewhere}^{commit}`])).toThrow(); + + const cut = await prepareWorktree(clone, dest, ref, 'main', elsewhere); + + expect(cut.head).toBe(elsewhere); + expect(existsSync(join(dest, 'c.ts'))).toBe(true); + }); + + it('says so when the pinned head is gone from origin', async () => { + const forcePushedAway = 'deadbeef'.repeat(5); + + await expect(prepareWorktree(clone, dest, ref, 'main', forcePushedAway)) + .rejects.toThrow(/the baseline's head deadbeefdead is no longer reachable from origin \(force-pushed\?\)/); + expect(existsSync(dest)).toBe(false); + }); + }); }); describe('removeWorktree', () => { diff --git a/scripts/inbox-compare.ts b/scripts/inbox-compare.ts index b05d7b37..1f9e52e0 100644 --- a/scripts/inbox-compare.ts +++ b/scripts/inbox-compare.ts @@ -6,12 +6,10 @@ * what it costs. One agent run per invocation; everything it writes stays in a scratch directory. */ -import { execFile } from 'node:child_process'; import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { basename, dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { promisify } from 'node:util'; import { GENERAL_THREAD_FILE_PATH, parseReviewBundle, type BundleThread, type ReviewBundle } from '@diffity/api'; import { viewPr } from '@diffity/github'; import type { RunStats } from '../packages/cli/src/inbox/agent-output.js'; @@ -386,20 +384,6 @@ function readBaseline(options: Options): { path: string; bundle: ReviewBundle } return newest; } -/** - * The head `refs/pull//head` points at, fetched into the clone. This is the only head a - * worktree can be cut at, so it decides whether the baseline's head is still reachable as a - * checkout — and it is asked before an agent run is spent rather than after. - */ -async function fetchPrHead(clone: string, number: number): Promise { - const git = (args: string[]) => promisify(execFile)('git', ['-c', 'core.hooksPath=/dev/null', ...args], { - cwd: clone, encoding: 'utf-8', env: { ...process.env, GIT_TERMINAL_PROMPT: '0' }, - }); - await git(['fetch', 'origin', `refs/pull/${number}/head`]); - const { stdout } = await git(['rev-parse', 'FETCH_HEAD']); - return stdout.trim(); -} - function prName(ref: PrRefSpec): string { return `${ref.owner}/${ref.repo}#${ref.number}`; } @@ -438,18 +422,6 @@ async function main(): Promise { const short = head.slice(0, 12); console.error(`🔍 ${prName(options.ref)} · baseline ${basename(baseline.path)} of ${baseline.bundle.createdAt}`); - let current: string; - try { - current = await fetchPrHead(clone, options.ref.number); - } catch (err) { - console.error(`⏭ ${clone} could not fetch refs/pull/${options.ref.number}/head: ${err instanceof Error ? err.message : err}`); - return 2; - } - if (!sameHead(current, head)) { - console.error(`⏭ the pull request's head is now ${current.slice(0, 12)}, and a worktree can only be cut at that one; re-run against a bundle at that head, or pick another pull request.`); - return 2; - } - const snapshot = await viewPr(options.ref); if (!snapshot) { console.error(`⏭ gh could not read ${prName(options.ref)}`); @@ -485,7 +457,7 @@ async function main(): Promise { const ticker = setInterval(() => console.error(` … ${elapsed().toFixed(0)} min`), 60_000); let result: PrepareResult; try { - result = await preparePr({ ...snapshot, headSha: head }, config, deps, { bumped: true }); + result = await preparePr({ ...snapshot, headSha: head }, config, deps, { bumped: true, pinHead: head }); } finally { clearInterval(ticker); }