diff --git a/bench/pr-review-context.ts b/bench/pr-review-context.ts new file mode 100644 index 000000000..63ebe59c3 --- /dev/null +++ b/bench/pr-review-context.ts @@ -0,0 +1,58 @@ +// What `threads.pullRequestFiles` hands a reader for a real pull request. +// The GitHub API is mocked: each squash commit given (default: this repo's +// #614, #621 and #524) is turned into the files API pages GitHub would serve +// (30 files a page, the patch from the first `@@`, none for a binary file), +// and every page is read through PullRequestReviews. No network, no login. +// +// bun bench/pr-review-context.ts [commit ...] +// BENCH_REVIEW_MODULE= bun bench/pr-review-context.ts +// +// Prints, per pull request, the JSON bytes of all pages, of the largest page, +// the patch characters of the largest page and how many files of the PR come +// back with less than their whole patch. +import { resolve } from 'node:path'; + +const commits = process.argv.slice(2).length ? process.argv.slice(2) : ['38d5ca2f7', '9ea83fda5', '060cb4528']; +const modulePath = resolve(process.env.BENCH_REVIEW_MODULE ?? `${import.meta.dir}/../packages/core/src/pull-request-review.ts`); +const { PullRequestReviews } = await import(modulePath) as typeof import('../packages/core/src/pull-request-review.ts'); + +function git(args: string[]): string { + const run = Bun.spawnSync(['git', ...args], { cwd: import.meta.dir, stdout: 'pipe', stderr: 'pipe' }); + if (run.exitCode !== 0) throw new Error(`git ${args.join(' ')}: ${run.stderr.toString()}`); + return run.stdout.toString(); +} + +interface ApiFile { filename: string; previous_filename?: string; status: string; additions: number; deletions: number; patch?: string } + +function apiFiles(commit: string): ApiFile[] { + const diff = git(['diff', '--find-renames', '--no-color', '--no-ext-diff', `${commit}^`, commit]); + return diff.split(/^(?=diff --git )/m).filter(Boolean).map(section => { + const lines = section.replace(/\n$/, '').split('\n'), at = lines.findIndex(line => line.startsWith('@@')); + const header = at < 0 ? lines : lines.slice(0, at), body = at < 0 ? [] : lines.slice(at); + const pick = (prefix: string) => header.find(line => line.startsWith(prefix))?.slice(prefix.length); + const to = pick('rename to ') ?? pick('+++ b/') ?? / b\/(.+)$/.exec(lines[0]!)![1]!, from = pick('rename from ') ?? pick('--- a/'); + const status = header.some(l => l.startsWith('new file')) ? 'added' : header.some(l => l.startsWith('deleted file')) ? 'removed' : pick('rename from ') ? 'renamed' : 'modified'; + return { filename: status === 'removed' ? from! : to, ...(status === 'renamed' ? { previous_filename: from } : {}), status, + additions: body.filter(l => l.startsWith('+')).length, deletions: body.filter(l => l.startsWith('-')).length, ...(body.length ? { patch: body.join('\n') } : {}) }; + }); +} + +const url = 'https://github.com/example/repo/pull/1'; +for (const commit of commits) { + const files = apiFiles(commit), title = git(['log', '-1', '--format=%s', commit]).trim(); + const service = new PullRequestReviews(async (_thread, args) => { + const page = Number(/[?&]page=(\d+)/.exec(args.at(-1)!)![1]); + return JSON.stringify(files.slice((page - 1) * 30, page * 30)); + }); + let bytes = 0, largest = 0, largestPatch = 0, cut = 0, pages = 0; + for (let page = 1; ; page++) { + const result = await service.files('bench', url, page), size = Buffer.byteLength(JSON.stringify(result)); + bytes += size; pages++; + if (size > largest) { largest = size; largestPatch = result.files.reduce((n, f) => n + (f.patch?.length ?? 0), 0); } + cut += result.files.filter((f, i) => f.patch !== (files[(page - 1) * 30 + i]?.patch ?? null)).length; + if (!result.hasMore) break; + } + const raw = files.reduce((n, f) => n + (f.patch?.length ?? 0), 0); + console.log(`${commit} ${title}`); + console.log(` ${files.length} files, ${pages} pages, ${raw} patch chars from git; all pages ${bytes} B, largest page ${largest} B (${largestPatch} patch chars), files not whole ${cut}`); +} diff --git a/docs/performance.md b/docs/performance.md index 25a0d958a..92220f1dc 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -1612,6 +1612,29 @@ A lower threshold costs a summary and a rewritten cache at each extra compaction, which is why the cost falls far less than the context. Automatic compaction is the owner's setting ([context](context.md#automatic-compaction)). +### Pull request file pages + +`threads.pullRequestFiles` returns 30 changed files a page. Their patches +share 60,000 characters, about 15,000 tokens: a file under its fair share +stays whole and leaves the rest to bigger ones, and a bigger file is cut the +same way across its hunks, so small hunks stay whole and long ones keep their +first lines. A cut patch ends with a note giving its page and path; the same +call with that `path` returns the file alone, up to 100,000 characters. Lock +files and generated files, known by their path, keep one stat line. Before, each patch kept up to +100,000 characters, so a page could reach 3 MB and one large file could fill +a reader's context ahead of the small files after it. + +`bench/pr-review-context.ts` rebuilds the files API pages of squash commits of +this repository from git and reads every page through the service, with no +network. Measured on 2026-11-07, before against the previous +`pull-request-review.ts` with `BENCH_REVIEW_MODULE`: + +| Pull request | Files | All pages before | All pages after | Largest page before | Largest page after | +| --- | --- | --- | --- | --- | --- | +| #614, one strings file per part | 185 | 628,053 B | 398,304 B | 271,909 B | 65,752 B | +| #621, one contracts file per domain | 44 | 413,740 B | 126,800 B | 306,462 B | 64,037 B | +| #524, with a 51,000-line results file | 62 | 250,241 B | 131,179 B | 174,421 B | 63,469 B | + ## Benches ```sh @@ -1635,6 +1658,7 @@ bun run bench/queue-admission.ts 1024 16 # one scheduler pass o bun bench/ui-heap.ts --machines --load 4 --tick 25 --switch 0 --minutes 9 # heap, DOM and private bytes of a page left open on busy agents bun bench/process-log-replay.ts # process records a log directory holds, and what this store writes of them bun run bench/checkpoint-store.ts --source # store passes and retention on a copy: bytes, inodes, 20 rewinds +bun bench/pr-review-context.ts [commit ...] # bytes of each pull request file page, from real commits, no network ``` `bench/bandwidth.ts` puts a TCP relay between the client and the core, counts diff --git a/packages/contracts/src/pull-requests.ts b/packages/contracts/src/pull-requests.ts index fe1695f5c..85f0f7300 100644 --- a/packages/contracts/src/pull-requests.ts +++ b/packages/contracts/src/pull-requests.ts @@ -13,7 +13,12 @@ export interface LinkedPullRequest { } export interface PullRequestsRpcMethods { 'threads.pullRequestReview': { params: { threadId: string; url: string }; result: PullRequestReview }; - 'threads.pullRequestFiles': { params: { threadId: string; url: string; page: number }; result: PullRequestFiles }; + /** + * One page of up to 30 changed files. Their patches share a fixed budget, so + * a big patch comes back cut with a note; `path` returns that file of the + * page alone with up to 100,000 characters of its patch. + */ + 'threads.pullRequestFiles': { params: { threadId: string; url: string; page: number; path?: string }; result: PullRequestFiles }; 'threads.pullRequests': { params: { threadId: string; refresh?: boolean }; result: LinkedPullRequest[] }; 'threads.linkPullRequest': { params: { threadId: string; url: string }; result: LinkedPullRequest[] }; 'threads.unlinkPullRequest': { params: { threadId: string; url: string }; result: LinkedPullRequest[] }; @@ -81,7 +86,11 @@ export interface PullRequestReview { url: string; title: string; body: string; state: string; head: string; base: string; checkedAt: number; checks: PullRequestCheck[]; comments: PullRequestComment[]; reviews: PullRequestComment[]; truncated: boolean; } -export interface PullRequestFile { path: string; previousPath?: string; status: string; additions: number; deletions: number; patch: string | null } +export interface PullRequestFile { + path: string; previousPath?: string; status: string; additions: number; deletions: number; patch: string | null; + /** Characters of the patch left out to fit the page's budget; absent when the patch is whole. */ + omitted?: number; +} export interface PullRequestFiles { files: PullRequestFile[]; page: number; hasMore: boolean; truncated: boolean } export interface PullRequestsRpcEvents { 'threads.pullRequestsChanged': { threadId: string; pullRequests: LinkedPullRequest[] }; diff --git a/packages/core/src/pull-request-diff-budget.ts b/packages/core/src/pull-request-diff-budget.ts new file mode 100644 index 000000000..f402b3a4b --- /dev/null +++ b/packages/core/src/pull-request-diff-budget.ts @@ -0,0 +1,108 @@ +/** + * Fits one page of pull request patches into a fixed budget of characters. + * Files share it first: a file under its fair share keeps its whole patch and + * leaves the rest to bigger ones. Inside a file over its share the hunks share + * that file's part the same way: small hunks stay whole, big ones keep their + * first lines. Each cut patch ends with a note naming what it lost and the page + * and path that return it alone, up to REVIEW_FILE_LIMIT characters. Lock files and generated files, known by + * their path, keep only a stat line. Notes and stat lines are paid for before + * the split, so a page never exceeds the budget. + */ + +/** Characters of patch text in one page of files, notes included. About 15,000 tokens; it holds 30 notes many times over. */ +export const REVIEW_DIFF_BUDGET = 60_000; +/** A file read alone by path keeps up to this much of its patch. */ +export const REVIEW_FILE_LIMIT = 100_000; + +export interface BudgetFile { path: string; additions: number; deletions: number; patch: string | null } +export interface BudgetedPatch { patch: string | null; omitted: number } + +const LOCK_FILES = new Set(['bun.lock', 'bun.lockb', 'package-lock.json', 'npm-shrinkwrap.json', 'yarn.lock', 'pnpm-lock.yaml', 'cargo.lock', 'gemfile.lock', + 'poetry.lock', 'pipfile.lock', 'composer.lock', 'go.sum', 'flake.lock', 'uv.lock', 'packages.lock.json', 'pubspec.lock', 'mix.lock', 'podfile.lock']); +const GENERATED_PATH = /(^|\/)(dist|vendor|node_modules|__generated__)\/|\.(lock|min\.js|min\.css|map|snap|pb\.go|g\.dart)$|_pb2\.py$|[._-]generated\.[a-z0-9]+$/i; + +export function generatedFile(path: string): boolean { + return LOCK_FILES.has(path.slice(path.lastIndexOf('/') + 1).toLowerCase()) || GENERATED_PATH.test(path); +} + +/** A path past 200 characters is not repeated: the file entry carries it, and the note stays short. */ +const where = (page: number, path: string) => `read it alone, up to ${REVIEW_FILE_LIMIT} characters, with threads.pullRequestFiles page ${page} path ${path.length > 200 ? 'of this file' : JSON.stringify(path)}`; + +/** + * Max-min fair split: the smallest asks are served whole and what they leave + * goes to the rest. Never hands out more than `budget` in total. + */ +export function fairShares(sizes: number[], budget: number): number[] { + const shares = sizes.map(() => 0), order = sizes.map((_, i) => i).sort((a, b) => sizes[a]! - sizes[b]!); + let left = Math.max(0, budget), waiting = sizes.length; + for (const i of order) { shares[i] = Math.min(sizes[i]!, Math.floor(left / waiting)); left -= shares[i]!; waiting--; } + return shares; +} + +/** Each part first gets its floor, the least it can be shown in; the rest is split fairly over what each still asks. */ +function flooredShares(sizes: number[], floors: number[], budget: number): number[] { + const extra = fairShares(sizes.map((size, i) => size - floors[i]!), budget - floors.reduce((n, f) => n + f, 0)); + return floors.map((floor, i) => floor + extra[i]!); +} + +/** Splits a patch before each `@@` header. Text before the first header stays with the first hunk. */ +function hunks(patch: string): string[][] { + const out: string[][] = []; + for (const line of patch.split('\n')) { + if (line.startsWith('@@') || !out.length) out.push([line]); else out.at(-1)!.push(line); + } + return out; +} + +/** One hunk cut to `limit` characters at a line boundary; the header always stays. */ +const marker = (n: number) => `[${n} more line${n === 1 ? '' : 's'} of this hunk left out]`; + +function cutHunk(lines: string[], limit: number): { lines: string[]; omitted: number } { + const kept = [lines[0]!]; + let used = lines[0]!.length + 1; + const reserve = marker(lines.length).length + 1; + for (const line of lines.slice(1)) { if (used + line.length + 1 + reserve > limit) break; kept.push(line); used += line.length + 1; } + const dropped = lines.slice(kept.length); + return { lines: [...kept, marker(dropped.length)], omitted: dropped.reduce((n, line) => n + line.length + 1, 0) }; +} + +/** The least a cut patch can take: its note alone. */ +function noteOnly(patch: string, page: number, path: string): string { + const count = hunks(patch).length; + return `[patch cut to fit the review budget: ${count} of ${count} hunk${count === 1 ? '' : 's'} shortened, ${patch.length} characters in all; ${where(page, path)}]`; +} + +/** One patch within `limit` characters, cut inside hunks when one of them does not fit whole. Never longer than `limit` once `limit` holds the note alone. */ +export function cutPatch(patch: string, limit: number, page: number, path: string): BudgetedPatch { + if (patch.length <= limit) return { patch, omitted: 0 }; + const parts = hunks(patch), sizes = parts.map(lines => lines.join('\n').length + 1); + const note = (shortened: number) => `[patch cut to fit the review budget: ${shortened} of ${parts.length} hunk${parts.length === 1 ? '' : 's'} shortened, ${patch.length} characters in all; ${where(page, path)}]`; + const room = limit - note(parts.length).length - 1; + // A hunk shows at least its header and a marker; with no room for all of them, the note alone says where the patch is. + const floors = parts.map((lines, i) => Math.min(sizes[i]!, lines[0]!.length + 1 + marker(lines.length).length + 1)); + if (room < floors.reduce((n, f) => n + f, 0)) return { patch: note(parts.length), omitted: patch.length }; + const shares = flooredShares(sizes, floors, room), body: string[] = []; + let shortened = 0, omitted = 0; + parts.forEach((lines, i) => { + if (shares[i]! >= sizes[i]!) { body.push(...lines); return; } + const cut = cutHunk(lines, shares[i]!); + shortened++; omitted += cut.omitted; body.push(...cut.lines); + }); + return { patch: [...body, note(shortened)].join('\n'), omitted }; +} + +/** + * Patches of one page fitted into `budget`. `omitted` counts the characters of + * the original patch that the reader does not get. + */ +export function budgetPatches(files: BudgetFile[], page: number, budget = REVIEW_DIFF_BUDGET): BudgetedPatch[] { + const stats = files.map(f => f.patch !== null && generatedFile(f.path) + ? `[generated or lock file: +${f.additions} -${f.deletions}, patch of ${f.patch.length} characters left out; ${where(page, f.path)}]` : null); + const sizes = files.map((f, i) => stats[i]?.length ?? f.patch?.length ?? 0); + const floors = files.map((f, i) => stats[i] !== null || f.patch === null ? sizes[i]! : Math.min(sizes[i]!, noteOnly(f.patch, page, f.path).length)); + const shares = flooredShares(sizes, floors, budget); + return files.map((f, i) => { + if (f.patch === null) return { patch: null, omitted: 0 }; + return stats[i] !== null ? { patch: stats[i]!, omitted: f.patch.length } : cutPatch(f.patch, shares[i]!, page, f.path); + }); +} diff --git a/packages/core/src/pull-request-review.ts b/packages/core/src/pull-request-review.ts index 092ffd618..0bbd78ba7 100644 --- a/packages/core/src/pull-request-review.ts +++ b/packages/core/src/pull-request-review.ts @@ -1,5 +1,6 @@ import { pullRequestAddress, type PullRequestReview, type PullRequestFiles, type PullRequestComment } from '@boite/contracts'; import { refused } from './errors.ts'; +import { budgetPatches, REVIEW_FILE_LIMIT } from './pull-request-diff-budget.ts'; type Row = Record; const row = (value: unknown): Row => value && typeof value === 'object' && !Array.isArray(value) ? value as Row : {}; @@ -46,15 +47,29 @@ export class PullRequestReviews { truncated: comments.length > 150 || reviews.length >= 100 || checks.length >= 100 || inline.length >= 100 || list(item.comments).length >= 100 || text(item.body, 40001).length > 40000 || [...comments, ...reviews].some(c => text(row(c).body, 12001).length > 12000) }; }); } - files(threadId: string, url: string, page: number): Promise { + /** A page of files whose patches share one budget, or with `path`, that file of the page alone with up to REVIEW_FILE_LIMIT characters of its patch. */ + files(threadId: string, url: string, page: number, path?: string): Promise { const a = this.address(url); if (!Number.isSafeInteger(page) || page < 1 || page > 100) throw refused('pull request files page must be between 1 and 100'); + if (path !== undefined && (typeof path !== 'string' || !path || path.length > 2000)) throw refused('pull request file path must be a file path of the page', { field: 'path', expected: 'a path of 1 to 2000 characters' }); return this.cached(`${threadId}:${a.url}:files:${page}`, async () => { const values = json(await this.run(threadId, ['api', '--hostname', a.host, `repos/${a.repository}/pulls/${a.number}/files?per_page=30&page=${page}`])); if (!Array.isArray(values)) throw refused('GitHub files must be an array'); - return { page, hasMore: values.length === 30 && page < 100, truncated: (page === 100 && values.length === 30) || values.some(v => text(row(v).patch, 100001).length > 100000), - files: values.slice(0, 30).map(value => { const f = row(value); return { path: text(f.filename, 2000), previousPath: f.previous_filename ? text(f.previous_filename, 2000) : undefined, - status: text(f.status, 50), additions: Math.max(0, Number(f.additions) || 0), deletions: Math.max(0, Number(f.deletions) || 0), patch: typeof f.patch === 'string' ? f.patch.slice(0, 100000) : null }; }) }; - }); + return values; + }).then(values => filesPage(values, page, path)); + } +} + +function filesPage(values: unknown[], page: number, path: string | undefined): PullRequestFiles { + const hasMore = values.length === 30 && page < 100, all = values.slice(0, 30).map(value => { const f = row(value); return { path: text(f.filename, 2000), previousPath: f.previous_filename ? text(f.previous_filename, 2000) : undefined, + status: text(f.status, 50), additions: Math.max(0, Number(f.additions) || 0), deletions: Math.max(0, Number(f.deletions) || 0), patch: typeof f.patch === 'string' ? f.patch : null }; }); + if (path !== undefined) { + const file = all.find(f => f.path === path); + if (!file) throw refused(`page ${page} has no file ${path}`, { field: 'path', expected: all.map(f => f.path).join(', ') || 'a page with files' }); + const omitted = Math.max(0, (file.patch?.length ?? 0) - REVIEW_FILE_LIMIT); + return { page, hasMore, truncated: omitted > 0 || (page === 100 && values.length === 30), files: [{ ...file, patch: file.patch?.slice(0, REVIEW_FILE_LIMIT) ?? null, ...(omitted ? { omitted } : {}) }] }; } + const patches = budgetPatches(all, page); + return { page, hasMore, truncated: (page === 100 && values.length === 30) || patches.some(p => p.omitted > 0), + files: all.map((f, i) => ({ ...f, patch: patches[i]!.patch, ...(patches[i]!.omitted ? { omitted: patches[i]!.omitted } : {}) })) }; } diff --git a/packages/core/src/threads/rpc.ts b/packages/core/src/threads/rpc.ts index 662bcb6c2..7c8131649 100644 --- a/packages/core/src/threads/rpc.ts +++ b/packages/core/src/threads/rpc.ts @@ -107,7 +107,7 @@ export function registerThreadMethods(core: Core, probe: ProviderProbe): void { linked.requireLink(p.threadId, url); return result; }); core.router.register('threads.pullRequestFiles', async p => { - const url = linked.requireLink(p.threadId, p.url), result = await pullRequests.reviews.files(p.threadId, url, p.page); + const url = linked.requireLink(p.threadId, p.url), result = await pullRequests.reviews.files(p.threadId, url, p.page, p.path); linked.requireLink(p.threadId, url); return result; }); core.router.register('threads.pullRequests', p => linked.list(p.threadId, p.refresh === true)); diff --git a/packages/core/test/pull-request-review.test.ts b/packages/core/test/pull-request-review.test.ts index 2408dc877..994a969b1 100644 --- a/packages/core/test/pull-request-review.test.ts +++ b/packages/core/test/pull-request-review.test.ts @@ -1,8 +1,9 @@ import { afterEach, expect, test } from 'bun:test'; import { PullRequestReviews } from '../src/pull-request-review.ts'; +import { REVIEW_DIFF_BUDGET } from '../src/pull-request-diff-budget.ts'; import { connect } from '../src/client.ts'; import { echoThread, startTestCore, type TestCore } from './harness.ts'; -import { thrown } from './settle.ts'; +import { rejection, thrown } from './settle.ts'; let harness: TestCore | undefined; afterEach(async () => { await harness?.stop(); harness = undefined; }); const url = 'https://github.com/example/repo/pull/1'; @@ -23,12 +24,41 @@ test('reviews combine discussions and inline feedback, reject unsafe URLs and ex expect(review.checks[0]).toEqual({ name: 'tests', state: 'FAILURE', url: '' }); await service.review('t', url); expect(calls.length).toBe(2); const files = await service.files('t', url, 2); - expect(files.truncated).toBe(true); expect(files.files[0]!.patch!.length).toBe(100000); expect(files.files[1]!.patch).toBeNull(); + expect(files.truncated).toBe(true); expect(files.files[0]!.patch).toContain('page 2 path "a.ts"'); expect(files.files[1]!.patch).toBeNull(); expect(calls.at(-1)?.at(-1)).toBe('repos/example/repo/pulls/1/files?per_page=30&page=2'); + const whole = await service.files('t', url, 2, 'a.ts'); + expect(whole.files.map(f => [f.path, f.patch!.length, f.omitted])).toEqual([['a.ts', 100000, 1]]); expect(calls.length).toBe(3); expect(() => service.files('t', url, 0)).toThrow('page'); + expect((await rejection(service.files('t', url, 2, 'missing.ts'))).reason).toMatchObject({ message: 'page 2 has no file missing.ts', data: { field: 'path', expected: 'a.ts, image.png' } }); expect(() => service.review('t', 'https://evil.example/a/b/pull/1')).toThrow('host'); }); +test('one huge patch no longer starves the small files of its page', async () => { + const hunk = (at: number, lines: number) => [`@@ -${at},${lines} +${at},${lines} @@ function f${at}()`, ...Array.from({ length: lines }, (_, i) => `+ const value${i} = compute(${i}); // line ${i} of the hunk at ${at}`)].join('\n'); + const big = [hunk(1, 3), hunk(100, 1500), hunk(2000, 4)].join('\n'), small = (n: number) => hunk(n, 15); + const values = [{ filename: 'src/big.ts', status: 'modified', additions: 1507, deletions: 0, patch: big }, + { filename: 'bun.lock', status: 'modified', additions: 900, deletions: 850, patch: hunk(1, 900) }, + ...Array.from({ length: 28 }, (_, i) => ({ filename: `src/small-${i}.ts`, status: 'modified', additions: 15, deletions: 0, patch: small(i + 1) }))]; + const service = new PullRequestReviews(async () => JSON.stringify(values)); + const page = await service.files('t', url, 3), [first, lock, ...rest] = page.files; + expect(page.files.reduce((n, f) => n + (f.patch?.length ?? 0), 0)).toBeLessThanOrEqual(REVIEW_DIFF_BUDGET); + expect(rest.map(f => f.patch)).toEqual(values.slice(2).map(v => v.patch)); + expect(rest.every(f => f.omitted === undefined)).toBe(true); + // The big file keeps its two small hunks whole and the head of the long one, then says where the rest is. + expect(first!.patch).toContain(hunk(1, 3)); expect(first!.patch).toContain(hunk(2000, 4)); + expect(first!.patch).toContain(hunk(100, 1500).split('\n').slice(0, 20).join('\n')); + expect(first!.patch).toMatch(/more lines of this hunk left out\]/); + expect(first!.patch!.split('\n').at(-1)).toBe(`[patch cut to fit the review budget: 1 of 3 hunks shortened, ${big.length} characters in all; read it alone, up to 100000 characters, with threads.pullRequestFiles page 3 path "src/big.ts"]`); + expect(first!.omitted).toBeGreaterThan(big.length / 2); + expect(lock!.patch).toBe(`[generated or lock file: +900 -850, patch of ${hunk(1, 900).length} characters left out; read it alone, up to 100000 characters, with threads.pullRequestFiles page 3 path "bun.lock"]`); + expect(page.truncated).toBe(true); + expect((await service.files('t', url, 3, 'src/big.ts')).files[0]!.patch).toBe(big); + // Notes are paid for before the split: 30 cut files with the longest paths still fit. + const long = (i: number) => `${'d'.repeat(1990)}/${i}.ts`; + const crowded = new PullRequestReviews(async () => JSON.stringify(Array.from({ length: 30 }, (_, i) => ({ filename: i % 2 ? long(i) : `${long(i)}.lock`, status: 'modified', additions: 1, deletions: 1, patch: big })))); + expect((await crowded.files('t', url, 1)).files.reduce((n, f) => n + f.patch!.length, 0)).toBeLessThanOrEqual(REVIEW_DIFF_BUDGET); +}); + test('paired phones cannot turn review reads into arbitrary GitHub queries', async () => { harness = await startTestCore(); const owner = await harness.connect(), { threadId } = await echoThread(harness, owner); diff --git a/packages/ui/src/lib/fake-client/pull-requests.ts b/packages/ui/src/lib/fake-client/pull-requests.ts index 80936ea6c..968d32894 100644 --- a/packages/ui/src/lib/fake-client/pull-requests.ts +++ b/packages/ui/src/lib/fake-client/pull-requests.ts @@ -82,10 +82,12 @@ export function pullRequestMethods(ctx: FakeContext): Methods { return { url: pr.url, title: pr.title, body: 'Preview review for the in-memory client.', state: pr.state, head: pr.head, base: pr.base, checkedAt: Date.now(), truncated: false, checks: [{ name: 'UI tests', state: 'SUCCESS', url: pr.url }], comments: [{ author: 'reviewer', body: 'Check the layout on a phone.', at: new Date().toISOString(), url: pr.url }], reviews: [] }; }, - 'threads.pullRequestFiles': async ({ threadId, url, page }) => { + 'threads.pullRequestFiles': async ({ threadId, url, page, path }) => { linked(threadId, url); if (!Number.isSafeInteger(page) || page < 1 || page > 100) throw refusal('pull request files page must be between 1 and 100'); - return { page, hasMore: false, truncated: false, files: page === 1 ? [{ path: 'src/preview.ts', status: 'modified', additions: 1, deletions: 1, patch: '@@ -1 +1 @@\n-const width = 720;\n+const width = 390;' }] : [] }; + const files = page === 1 ? [{ path: 'src/preview.ts', status: 'modified', additions: 1, deletions: 1, patch: '@@ -1 +1 @@\n-const width = 720;\n+const width = 390;' }] : []; + if (path !== undefined && !files.some(f => f.path === path)) throw refusal(`page ${page} has no file ${path}`); + return { page, hasMore: false, truncated: false, files: path === undefined ? files : files.filter(f => f.path === path) }; }, 'threads.pullRequests': async ({ threadId }) => structuredClone(read(threadId)), 'threads.linkPullRequest': async ({ threadId, url }) => {