From d09f4131e220aed6cf6fc83c9c341b92a479587a Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 07:27:46 +0000 Subject: [PATCH 01/14] fix(runner): resolve unresolved review feedback on write anchoring and local review scope Addresses the unresolved Greptile threads from PRs #2, #7, #16, and #19: - packages/runner: fail closed when /proc/self/fd is unavailable instead of renaming through a re-resolved mutable pathname (PR #16 thread). - packages/runner: range-less local reviews now include index-only changes and untracked files in both the changed-file list and the diff context (PR #19 thread). - packages/agent: planned writes are restricted to the files the validated finding cites; review-diff membership alone no longer authorizes a write (PR #19 thread). - .env.example: drop AGENT_ZERO_PORT, which referenced the removed Nitro server (PR #2 threads). - PR #7 thread (hook workflow docs) is already covered by CONTRIBUTING.md. Co-authored-by: Codesmith --- .env.example | 1 - packages/agent/src/agent.test.ts | 23 ++++++++++ packages/agent/src/agent.ts | 19 +++----- packages/runner/src/boundary.ts | 74 ++++++++++++++++++++++--------- packages/runner/src/index.test.ts | 71 ++++++++++++++++++++++++++++- 5 files changed, 151 insertions(+), 37 deletions(-) diff --git a/.env.example b/.env.example index 3ae050b..800af52 100644 --- a/.env.example +++ b/.env.example @@ -1,5 +1,4 @@ OPENAI_API_KEY= AGENT_ZERO_MODEL=gpt-5 -AGENT_ZERO_PORT=4040 GITHUB_TOKEN= GITHUB_WEBHOOK_SECRET= diff --git a/packages/agent/src/agent.test.ts b/packages/agent/src/agent.test.ts index 5eb32a2..4e53fea 100644 --- a/packages/agent/src/agent.test.ts +++ b/packages/agent/src/agent.test.ts @@ -438,6 +438,29 @@ describe('narrow scope', () => { expect(writes).toEqual([]); }); + it('refuses a proactive change to a review file the finding does not cite', async () => { + const { agent, writes } = harness({ + reviewFiles: ['src/user.ts', 'src/other.ts'], + files: { + 'src/user.ts': sourceFile, + 'src/other.ts': 'export const other = true;\n', + 'package.json': JSON.stringify({ scripts: { test: 'vitest run' } }), + 'pnpm-lock.yaml': '', + }, + decisions: [ + decision({ + changes: [ + { path: 'src/other.ts', content: 'export const other = false;\n', reason: 'drive-by' }, + ], + }), + ], + }); + const result = await agent.run({ repository: '/checkout', mode: 'fix', trigger: 'proactive' }); + expect(result.state).toBe('needs-human'); + expect(result.summary).toContain('outside the validated scope'); + expect(writes).toEqual([]); + }); + it('refuses a change that tries to leave the checkout', async () => { const { agent, writes } = harness({ decisions: [ diff --git a/packages/agent/src/agent.ts b/packages/agent/src/agent.ts index 7a4eb2a..90d374c 100644 --- a/packages/agent/src/agent.ts +++ b/packages/agent/src/agent.ts @@ -142,12 +142,7 @@ export class AgentZero { if (repairRefusal) return run.finish(repairRefusal.state, repairRefusal.summary); } - const scoped = scopeChanges( - decision.changes, - finding, - effectiveInput, - config.agent.maxChangedFiles, - ); + const scoped = scopeChanges(decision.changes, finding, config.agent.maxChangedFiles); if ('reason' in scoped) return run.finish('needs-human', scoped.reason); run.emit('executing', `Applying ${String(scoped.changes.length)} planned change(s)`, attempt); @@ -352,13 +347,15 @@ class Run { /** * Restrict a change set to the scope the validated finding established. * - * A fix is only narrow if it touches the files the evidence pointed at. Anything else, including a - * plausible-looking refactor of an unrelated file, is refused and handed to a human. + * A fix is only narrow if it touches the files the evidence pointed at, so only the files the + * finding itself cites are writable. Merely appearing in the review diff is not enough: the + * validation accepted a claim about the cited files, not about every file the diff happens to + * touch. Anything else, including a plausible-looking refactor of an unrelated file, is refused + * and handed to a human. */ export function scopeChanges( changes: readonly ProposedChange[], finding: Finding, - input: ReviewInput, maxChangedFiles: number, ): { changes: ProposedChange[] } | { reason: string } { if (changes.length === 0) @@ -369,9 +366,7 @@ export function scopeChanges( }; const scope = new Set( - [...finding.files, ...(input.files ?? [])] - .filter((path) => isRepositoryRelativePath(path)) - .map(normalizePath), + finding.files.filter((path) => isRepositoryRelativePath(path)).map(normalizePath), ); const accepted: ProposedChange[] = []; for (const change of changes) { diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index bfb4300..939096c 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -160,7 +160,10 @@ export abstract class RepositoryBoundary implements Runner { const diffRange = contextDiffRange(options); const files = await this.git(['ls-files']); const changedFiles = await this.reviewFiles(options); - const diff = await this.git(['diff', '--no-ext-diff', ...diffRange, '--']); + const diff = + diffRange.length > 0 + ? (await this.git(['diff', '--no-ext-diff', ...diffRange, '--'])).stdout + : await this.pendingDiff(); return [ 'FILES', truncateTail(files.stdout, MAX_FILE_LIST), @@ -169,17 +172,45 @@ export abstract class RepositoryBoundary implements Runner { truncateTail(changedFiles.join('\n'), MAX_FILE_LIST), '', 'DIFF', - truncateTail(diff.stdout, MAX_DIFF), + truncateTail(diff, MAX_DIFF), ].join('\n'); } async reviewFiles(options: RepositoryContextOptions = {}): Promise { const diffRange = contextDiffRange(options); - const outcome = await this.git(['diff', '--name-only', ...diffRange, '--']); - return outcome.stdout - .split('\n') - .map((path) => path.trim()) - .filter((path) => path.length > 0 && isRepositoryRelativePath(path)); + // A committed pull-request range fixes the reviewed set. Without one, the pending local + // changes are the review target, and a plain `git diff` alone would silently omit index-only + // changes and untracked files. + const listings = + diffRange.length > 0 + ? [await this.git(['diff', '--name-only', ...diffRange, '--'])] + : [ + await this.git(['diff', '--name-only', '--cached', '--']), + await this.git(['diff', '--name-only', '--']), + await this.git(['ls-files', '--others', '--exclude-standard']), + ]; + const seen = new Set(); + const paths: string[] = []; + for (const listing of listings) + for (const line of listing.stdout.split('\n')) { + const path = line.trim(); + if (path.length === 0 || !isRepositoryRelativePath(path) || seen.has(path)) continue; + seen.add(path); + paths.push(path); + } + return paths; + } + + /** + * The full pending local diff: staged content first, then working-tree edits. + * + * `git diff` alone reads only the working tree against the index, so index-only changes would + * be reviewed as if they did not exist. + */ + private async pendingDiff(): Promise { + const staged = await this.git(['diff', '--no-ext-diff', '--cached', '--']); + const unstaged = await this.git(['diff', '--no-ext-diff', '--']); + return [staged.stdout, unstaged.stdout].filter((part) => part.length > 0).join('\n'); } async changedFiles(): Promise { @@ -265,18 +296,19 @@ export abstract class RepositoryBoundary implements Runner { * atomic rename, and both names resolve through the held descriptor (`/proc/self/fd` on Linux), * never through re-walked path components. The target inode itself is never written, so a * concurrent rename carrying it outside the checkout after validation moves nothing but the - * previous content. + * previous content. Platforms that cannot anchor the rename to the descriptor refuse the write + * (see {@link directoryAnchor}). * * Protected so that tests can interleave an adversarial rename at exactly this point. */ protected async replaceInside( directory: FileHandle, - fallbackParent: string, + parent: string, targetName: string, content: string, original: string, ): Promise { - const anchor = await directoryAnchor(directory, fallbackParent, original); + const anchor = await directoryAnchor(directory, original); const temporaryName = `.agent-zero-${randomUUID()}.tmp`; const temporary = join(anchor, temporaryName); const target = join(anchor, targetName); @@ -390,24 +422,22 @@ async function descriptorPath( /** * Resolve a stable path through the directory handle itself. Linux exposes descriptors under * `/proc/self/fd`, so renaming the directory cannot redirect the mutation through a different path. - * On platforms without that facility we re-resolve the fallback immediately before use and verify - * that it is still the same directory inode held by the descriptor. + * + * Platforms without that facility get no fallback: any pathname alternative re-walks mutable + * components, so a concurrent task could swap the verified directory for a symlink between the + * inode comparison and the rename and redirect the write outside the checkout. Node exposes no + * descriptor-relative create or rename, so the write fails closed instead. */ -async function directoryAnchor( - directory: FileHandle, - fallback: string, - original: string, -): Promise { +async function directoryAnchor(directory: FileHandle, original: string): Promise { const descriptor = `/proc/self/fd/${directory.fd}`; try { await lstat(descriptor); return descriptor; } catch { - const real = await realpath(fallback); - const [expected, current] = await Promise.all([directory.stat(), stat(real)]); - if (expected.dev !== current.dev || expected.ino !== current.ino) - throw new PathEscapeError(original, 'parent replaced while writing'); - return real; + throw new RunnerWriteDeniedError( + original, + 'descriptor-anchored writes are not supported on this platform', + ); } } diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index d94e657..12b40b1 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -71,6 +71,23 @@ class TargetRenamingRunner extends LocalRunner { } } +/** + * Simulates a platform without `/proc/self/fd` by anchoring the mutation to a dead descriptor: + * the kernel cannot report where it points, exactly as on systems that lack the facility. + */ +class ProclessRunner extends LocalRunner { + protected override async replaceInside( + directory: FileHandle, + parent: string, + targetName: string, + content: string, + original: string, + ): Promise { + const dead = { fd: -1 } as unknown as FileHandle; + return super.replaceInside(dead, parent, targetName, content, original); + } +} + interface Invocation { program: string; args: string[]; @@ -161,6 +178,15 @@ describe('path boundary', () => { await expect(access(join(outside, 'escape.txt'))).rejects.toThrow('ENOENT'); }); + it('fails closed instead of renaming through a mutable path when descriptor anchoring is unavailable', async () => { + const runner = new ProclessRunner(root, { writable: true }); + await expect(runner.write('src/user.ts', 'payload')).rejects.toThrow( + 'descriptor-anchored writes are not supported on this platform', + ); + // The refused write must leave the target untouched. + await expect(runner.read('src/user.ts')).resolves.toContain('export const user'); + }); + it('keeps a write contained when the validated target inode is renamed outside before it lands', async () => { const outside = await mkdtemp(join(tmpdir(), 'agent-zero-outside-')); const victim = join(outside, 'victim.txt'); @@ -236,7 +262,7 @@ describe('command execution', () => { }); describe('git inspection', () => { - it('collects the file list and diff through fixed arguments', async () => { + it('collects the file list and every pending local change through fixed arguments', async () => { const { runner: process, calls } = recordingProcess({ git: { exitCode: 0, stdout: 'src/user.ts', stderr: '' }, }); @@ -244,7 +270,48 @@ describe('git inspection', () => { expect(context).toContain('FILES'); expect(context).toContain('CHANGED FILES'); expect(context).toContain('DIFF'); - expect(calls.map((call) => call.args[0])).toEqual(['ls-files', 'diff', 'diff']); + expect(calls.map((call) => call.args.join(' '))).toEqual([ + 'ls-files', + 'diff --name-only --cached --', + 'diff --name-only --', + 'ls-files --others --exclude-standard', + 'diff --no-ext-diff --cached --', + 'diff --no-ext-diff --', + ]); + }); + + it('includes staged and untracked files in a range-less local review', async () => { + const outputs: Record = { + 'diff --name-only --cached --': 'src/staged.ts\nsrc/both.ts\n', + 'diff --name-only --': 'src/edited.ts\nsrc/both.ts\n', + 'ls-files --others --exclude-standard': 'src/untracked.ts\n', + }; + const process: ProcessRunner = async (program, args) => ({ + exitCode: 0, + stdout: program === 'git' ? (outputs[args.join(' ')] ?? '') : '', + stderr: '', + }); + await expect(new LocalRunner(root, { process }).reviewFiles()).resolves.toEqual([ + 'src/staged.ts', + 'src/both.ts', + 'src/edited.ts', + 'src/untracked.ts', + ]); + }); + + it('feeds staged content into the range-less review diff', async () => { + const outputs: Record = { + 'diff --no-ext-diff --cached --': '+const staged = true;', + 'diff --no-ext-diff --': '+const unstaged = true;', + }; + const process: ProcessRunner = async (program, args) => ({ + exitCode: 0, + stdout: program === 'git' ? (outputs[args.join(' ')] ?? '') : '', + stderr: '', + }); + const context = await new LocalRunner(root, { process }).context(); + expect(context).toContain('+const staged = true;'); + expect(context).toContain('+const unstaged = true;'); }); it('collects a committed pull-request diff from the fixed merge-base range', async () => { From c43fdf325adcc6e5d811519456dd163f5b082f3d Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 07:30:53 +0000 Subject: [PATCH 02/14] fix(runner): drop unsafe FileHandle assertion in dead-descriptor test Co-authored-by: Codesmith --- packages/runner/src/index.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index 12b40b1..e814bfc 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -73,7 +73,8 @@ class TargetRenamingRunner extends LocalRunner { /** * Simulates a platform without `/proc/self/fd` by anchoring the mutation to a dead descriptor: - * the kernel cannot report where it points, exactly as on systems that lack the facility. + * closing the held handle first leaves it with `fd` -1, so the kernel cannot report where it + * points, exactly as on systems that lack the facility. */ class ProclessRunner extends LocalRunner { protected override async replaceInside( @@ -83,8 +84,8 @@ class ProclessRunner extends LocalRunner { content: string, original: string, ): Promise { - const dead = { fd: -1 } as unknown as FileHandle; - return super.replaceInside(dead, parent, targetName, content, original); + await directory.close(); + return super.replaceInside(directory, parent, targetName, content, original); } } From 1fbb9ad075b783e13eab2b6b71e00c0e134047d5 Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 07:41:23 +0000 Subject: [PATCH 03/14] fix(runner): render pending review diff as final-state patches with untracked content Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 54 +++++++++++++++++++++++++++---- packages/runner/src/index.test.ts | 44 ++++++++++++++++++++++--- 2 files changed, 87 insertions(+), 11 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index 939096c..c17cf4e 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -202,15 +202,57 @@ export abstract class RepositoryBoundary implements Runner { } /** - * The full pending local diff: staged content first, then working-tree edits. + * The full pending local diff, one final-state patch per file. * - * `git diff` alone reads only the working tree against the index, so index-only changes would - * be reviewed as if they did not exist. + * A single HEAD-to-working-tree diff covers staged and unstaged edits together; joining the + * `--cached` and working-tree layers instead would emit two overlapping patches for a + * partially-staged file and expose the intermediate staged value as though it were a separate + * edit. Untracked files appear in no git diff at all, so each one gets a synthetic + * creation patch (see {@link untrackedPatches}). */ private async pendingDiff(): Promise { - const staged = await this.git(['diff', '--no-ext-diff', '--cached', '--']); - const unstaged = await this.git(['diff', '--no-ext-diff', '--']); - return [staged.stdout, unstaged.stdout].filter((part) => part.length > 0).join('\n'); + const tracked = await this.git(['diff', '--no-ext-diff', 'HEAD', '--']); + const parts = + tracked.exitCode === 0 + ? [tracked.stdout] + : // No commit to diff against (unborn HEAD): the index and working tree are the only + // layers, so show them directly rather than dropping staged content. + [ + (await this.git(['diff', '--no-ext-diff', '--cached', '--'])).stdout, + (await this.git(['diff', '--no-ext-diff', '--'])).stdout, + ]; + parts.push(...(await this.untrackedPatches())); + return parts.filter((part) => part.length > 0).join('\n'); + } + + /** + * A synthetic creation patch for each untracked file included in a range-less review. + * + * {@link reviewFiles} lists untracked paths as review targets, so their content must reach the + * reviewer too; `git diff --no-index` against `/dev/null` renders the same new-file patch a + * commit would produce. Collection stops once the diff budget is exhausted, since anything + * further would be truncated away regardless. + */ + private async untrackedPatches(): Promise { + const listing = await this.git(['ls-files', '--others', '--exclude-standard']); + const patches: string[] = []; + let total = 0; + for (const line of listing.stdout.split('\n')) { + const path = line.trim(); + if (path.length === 0 || !isRepositoryRelativePath(path)) continue; + if (total > MAX_DIFF) break; + // `--no-index` exits 1 when the paths differ, which is the expected outcome here; only + // larger codes report a real failure, and those degrade to omitting the patch. + const outcome = await this.process( + 'git', + ['diff', '--no-ext-diff', '--no-index', '--', '/dev/null', path], + { cwd: this.root, timeoutMs: GIT_TIMEOUT_MS, maxOutputBytes: this.maxOutputBytes }, + ); + if (outcome.exitCode > 1 || outcome.stdout.length === 0) continue; + patches.push(outcome.stdout); + total += outcome.stdout.length; + } + return patches; } async changedFiles(): Promise { diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index e814bfc..2047c66 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -276,8 +276,9 @@ describe('git inspection', () => { 'diff --name-only --cached --', 'diff --name-only --', 'ls-files --others --exclude-standard', - 'diff --no-ext-diff --cached --', - 'diff --no-ext-diff --', + 'diff --no-ext-diff HEAD --', + 'ls-files --others --exclude-standard', + 'diff --no-ext-diff --no-index -- /dev/null src/user.ts', ]); }); @@ -300,10 +301,11 @@ describe('git inspection', () => { ]); }); - it('feeds staged content into the range-less review diff', async () => { + it('feeds one consolidated final-state patch into the range-less review diff', async () => { const outputs: Record = { - 'diff --no-ext-diff --cached --': '+const staged = true;', - 'diff --no-ext-diff --': '+const unstaged = true;', + 'diff --no-ext-diff HEAD --': '+const final = true;', + // The staged layer of a partially-staged file must never surface as a separate patch. + 'diff --no-ext-diff --cached --': '+const intermediate = true;', }; const process: ProcessRunner = async (program, args) => ({ exitCode: 0, @@ -311,6 +313,38 @@ describe('git inspection', () => { stderr: '', }); const context = await new LocalRunner(root, { process }).context(); + expect(context).toContain('+const final = true;'); + expect(context).not.toContain('+const intermediate = true;'); + }); + + it('includes a synthetic creation patch for each untracked file in the range-less review diff', async () => { + const process: ProcessRunner = async (program, args) => { + if (program !== 'git') return { exitCode: 0, stdout: '', stderr: '' }; + const argv = args.join(' '); + if (argv === 'ls-files --others --exclude-standard') + return { exitCode: 0, stdout: 'src/untracked.ts\n', stderr: '' }; + if (argv === 'diff --no-ext-diff --no-index -- /dev/null src/untracked.ts') + return { exitCode: 1, stdout: '+const untracked = true;', stderr: '' }; + return { exitCode: 0, stdout: '', stderr: '' }; + }; + const context = await new LocalRunner(root, { process }).context(); + expect(context).toContain('src/untracked.ts'); + expect(context).toContain('+const untracked = true;'); + }); + + it('falls back to the staged and working-tree layers when there is no commit to diff against', async () => { + const process: ProcessRunner = async (program, args) => { + if (program !== 'git') return { exitCode: 0, stdout: '', stderr: '' }; + const argv = args.join(' '); + if (argv === 'diff --no-ext-diff HEAD --') + return { exitCode: 128, stdout: '', stderr: 'unknown revision HEAD' }; + if (argv === 'diff --no-ext-diff --cached --') + return { exitCode: 0, stdout: '+const staged = true;', stderr: '' }; + if (argv === 'diff --no-ext-diff --') + return { exitCode: 0, stdout: '+const unstaged = true;', stderr: '' }; + return { exitCode: 0, stdout: '', stderr: '' }; + }; + const context = await new LocalRunner(root, { process }).context(); expect(context).toContain('+const staged = true;'); expect(context).toContain('+const unstaged = true;'); }); From 6df8afc81f637d37454ddf5c7a42ddb817ea73cd Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 07:43:17 +0000 Subject: [PATCH 04/14] fix(runner): capture fake git outputs in test process closures Co-authored-by: Codesmith --- packages/runner/src/index.test.ts | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index 2047c66..bb30d53 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -318,14 +318,16 @@ describe('git inspection', () => { }); it('includes a synthetic creation patch for each untracked file in the range-less review diff', async () => { + const outputs: Record = { + 'ls-files --others --exclude-standard': 'src/untracked.ts\n', + 'diff --no-ext-diff --no-index -- /dev/null src/untracked.ts': '+const untracked = true;', + }; const process: ProcessRunner = async (program, args) => { - if (program !== 'git') return { exitCode: 0, stdout: '', stderr: '' }; const argv = args.join(' '); - if (argv === 'ls-files --others --exclude-standard') - return { exitCode: 0, stdout: 'src/untracked.ts\n', stderr: '' }; - if (argv === 'diff --no-ext-diff --no-index -- /dev/null src/untracked.ts') - return { exitCode: 1, stdout: '+const untracked = true;', stderr: '' }; - return { exitCode: 0, stdout: '', stderr: '' }; + const stdout = program === 'git' ? (outputs[argv] ?? '') : ''; + // `--no-index` reports "the paths differ" with exit code 1, like real git. + const exitCode = argv.includes('--no-index') && stdout.length > 0 ? 1 : 0; + return { exitCode, stdout, stderr: '' }; }; const context = await new LocalRunner(root, { process }).context(); expect(context).toContain('src/untracked.ts'); @@ -333,16 +335,15 @@ describe('git inspection', () => { }); it('falls back to the staged and working-tree layers when there is no commit to diff against', async () => { + const outputs: Record = { + 'diff --no-ext-diff --cached --': '+const staged = true;', + 'diff --no-ext-diff --': '+const unstaged = true;', + }; const process: ProcessRunner = async (program, args) => { - if (program !== 'git') return { exitCode: 0, stdout: '', stderr: '' }; const argv = args.join(' '); if (argv === 'diff --no-ext-diff HEAD --') return { exitCode: 128, stdout: '', stderr: 'unknown revision HEAD' }; - if (argv === 'diff --no-ext-diff --cached --') - return { exitCode: 0, stdout: '+const staged = true;', stderr: '' }; - if (argv === 'diff --no-ext-diff --') - return { exitCode: 0, stdout: '+const unstaged = true;', stderr: '' }; - return { exitCode: 0, stdout: '', stderr: '' }; + return { exitCode: 0, stdout: program === 'git' ? (outputs[argv] ?? '') : '', stderr: '' }; }; const context = await new LocalRunner(root, { process }).context(); expect(context).toContain('+const staged = true;'); From 74fe75ba960be0416968b75abdff7b7b04113200 Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 07:54:07 +0000 Subject: [PATCH 05/14] fix(runner): consolidate unborn-repository pending diff into final-state patches Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 25 ++++++++++++++++--------- packages/runner/src/index.test.ts | 12 +++++++++--- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index c17cf4e..1a6d733 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -212,19 +212,26 @@ export abstract class RepositoryBoundary implements Runner { */ private async pendingDiff(): Promise { const tracked = await this.git(['diff', '--no-ext-diff', 'HEAD', '--']); - const parts = - tracked.exitCode === 0 - ? [tracked.stdout] - : // No commit to diff against (unborn HEAD): the index and working tree are the only - // layers, so show them directly rather than dropping staged content. - [ - (await this.git(['diff', '--no-ext-diff', '--cached', '--'])).stdout, - (await this.git(['diff', '--no-ext-diff', '--'])).stdout, - ]; + const parts = tracked.exitCode === 0 ? [tracked.stdout] : [await this.unbornDiff()]; parts.push(...(await this.untrackedPatches())); return parts.filter((part) => part.length > 0).join('\n'); } + /** + * The pending diff of a repository whose HEAD is unborn (no commit to diff against). + * + * Joining the `--cached` and working-tree layers here would emit two overlapping patches for a + * partially-staged file and expose the staged intermediate value as though it were a separate + * edit. Diffing the empty tree against the working tree instead renders one final-state + * creation patch per tracked file. The empty-tree id is computed rather than hardcoded so the + * fallback also holds in SHA-256 repositories. + */ + private async unbornDiff(): Promise { + const emptyTree = await this.git(['hash-object', '-t', 'tree', '/dev/null']); + if (emptyTree.exitCode !== 0) return ''; + return (await this.git(['diff', '--no-ext-diff', emptyTree.stdout.trim(), '--'])).stdout; + } + /** * A synthetic creation patch for each untracked file included in a range-less review. * diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index bb30d53..386c2bf 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -334,8 +334,13 @@ describe('git inspection', () => { expect(context).toContain('+const untracked = true;'); }); - it('falls back to the staged and working-tree layers when there is no commit to diff against', async () => { + it('consolidates the unborn-repository fallback into one final-state patch per file', async () => { + const emptyTree = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'; const outputs: Record = { + 'hash-object -t tree /dev/null': `${emptyTree}\n`, + [`diff --no-ext-diff ${emptyTree} --`]: '+const final = true;', + // The staged layer of a partially-staged file must never surface as a separate + // overlapping patch that exposes its intermediate value. 'diff --no-ext-diff --cached --': '+const staged = true;', 'diff --no-ext-diff --': '+const unstaged = true;', }; @@ -346,8 +351,9 @@ describe('git inspection', () => { return { exitCode: 0, stdout: program === 'git' ? (outputs[argv] ?? '') : '', stderr: '' }; }; const context = await new LocalRunner(root, { process }).context(); - expect(context).toContain('+const staged = true;'); - expect(context).toContain('+const unstaged = true;'); + expect(context).toContain('+const final = true;'); + expect(context).not.toContain('+const staged = true;'); + expect(context).not.toContain('+const unstaged = true;'); }); it('collects a committed pull-request diff from the fixed merge-base range', async () => { From bac813793e870565ec5321906bf49b691e4121d3 Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 08:03:05 +0000 Subject: [PATCH 06/14] fix(runner): list review paths NUL-delimited so special filenames survive Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 26 ++++++++++++---------- packages/runner/src/index.test.ts | 37 +++++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index 1a6d733..f7ecde5 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -180,20 +180,21 @@ export abstract class RepositoryBoundary implements Runner { const diffRange = contextDiffRange(options); // A committed pull-request range fixes the reviewed set. Without one, the pending local // changes are the review target, and a plain `git diff` alone would silently omit index-only - // changes and untracked files. + // changes and untracked files. Every listing is NUL-delimited (`-z`): newline-delimited git + // output C-quotes names containing characters such as newlines or tabs, and that display + // representation is not a filesystem path. const listings = diffRange.length > 0 - ? [await this.git(['diff', '--name-only', ...diffRange, '--'])] + ? [await this.git(['diff', '--name-only', '-z', ...diffRange, '--'])] : [ - await this.git(['diff', '--name-only', '--cached', '--']), - await this.git(['diff', '--name-only', '--']), - await this.git(['ls-files', '--others', '--exclude-standard']), + await this.git(['diff', '--name-only', '-z', '--cached', '--']), + await this.git(['diff', '--name-only', '-z', '--']), + await this.git(['ls-files', '-z', '--others', '--exclude-standard']), ]; const seen = new Set(); const paths: string[] = []; for (const listing of listings) - for (const line of listing.stdout.split('\n')) { - const path = line.trim(); + for (const path of listing.stdout.split('\0')) { if (path.length === 0 || !isRepositoryRelativePath(path) || seen.has(path)) continue; seen.add(path); paths.push(path); @@ -237,15 +238,16 @@ export abstract class RepositoryBoundary implements Runner { * * {@link reviewFiles} lists untracked paths as review targets, so their content must reach the * reviewer too; `git diff --no-index` against `/dev/null` renders the same new-file patch a - * commit would produce. Collection stops once the diff budget is exhausted, since anything - * further would be truncated away regardless. + * commit would produce. The listing is NUL-delimited (`-z`) and parsed verbatim: git C-quotes + * names containing characters such as newlines or tabs in newline-delimited output, and that + * display representation would not open as a filesystem path. Collection stops once the diff + * budget is exhausted, since anything further would be truncated away regardless. */ private async untrackedPatches(): Promise { - const listing = await this.git(['ls-files', '--others', '--exclude-standard']); + const listing = await this.git(['ls-files', '-z', '--others', '--exclude-standard']); const patches: string[] = []; let total = 0; - for (const line of listing.stdout.split('\n')) { - const path = line.trim(); + for (const path of listing.stdout.split('\0')) { if (path.length === 0 || !isRepositoryRelativePath(path)) continue; if (total > MAX_DIFF) break; // `--no-index` exits 1 when the paths differ, which is the expected outcome here; only diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index 386c2bf..b2e7de9 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -273,20 +273,20 @@ describe('git inspection', () => { expect(context).toContain('DIFF'); expect(calls.map((call) => call.args.join(' '))).toEqual([ 'ls-files', - 'diff --name-only --cached --', - 'diff --name-only --', - 'ls-files --others --exclude-standard', + 'diff --name-only -z --cached --', + 'diff --name-only -z --', + 'ls-files -z --others --exclude-standard', 'diff --no-ext-diff HEAD --', - 'ls-files --others --exclude-standard', + 'ls-files -z --others --exclude-standard', 'diff --no-ext-diff --no-index -- /dev/null src/user.ts', ]); }); it('includes staged and untracked files in a range-less local review', async () => { const outputs: Record = { - 'diff --name-only --cached --': 'src/staged.ts\nsrc/both.ts\n', - 'diff --name-only --': 'src/edited.ts\nsrc/both.ts\n', - 'ls-files --others --exclude-standard': 'src/untracked.ts\n', + 'diff --name-only -z --cached --': 'src/staged.ts\0src/both.ts\0', + 'diff --name-only -z --': 'src/edited.ts\0src/both.ts\0', + 'ls-files -z --others --exclude-standard': 'src/untracked.ts\0', }; const process: ProcessRunner = async (program, args) => ({ exitCode: 0, @@ -319,7 +319,7 @@ describe('git inspection', () => { it('includes a synthetic creation patch for each untracked file in the range-less review diff', async () => { const outputs: Record = { - 'ls-files --others --exclude-standard': 'src/untracked.ts\n', + 'ls-files -z --others --exclude-standard': 'src/untracked.ts\0', 'diff --no-ext-diff --no-index -- /dev/null src/untracked.ts': '+const untracked = true;', }; const process: ProcessRunner = async (program, args) => { @@ -334,6 +334,25 @@ describe('git inspection', () => { expect(context).toContain('+const untracked = true;'); }); + it('keeps an untracked filename with special characters usable through NUL-delimited listings', async () => { + // Newline-delimited git output would C-quote this name into a non-path display string. + const weird = 'src/untracked\nfile.ts'; + const outputs: Record = { + 'ls-files -z --others --exclude-standard': `${weird}\0`, + [`diff --no-ext-diff --no-index -- /dev/null ${weird}`]: '+const weird = true;', + }; + const process: ProcessRunner = async (program, args) => { + const argv = args.join(' '); + const stdout = program === 'git' ? (outputs[argv] ?? '') : ''; + // `--no-index` reports "the paths differ" with exit code 1, like real git. + const exitCode = argv.includes('--no-index') && stdout.length > 0 ? 1 : 0; + return { exitCode, stdout, stderr: '' }; + }; + const runner = new LocalRunner(root, { process }); + await expect(runner.reviewFiles()).resolves.toContain(weird); + await expect(runner.context()).resolves.toContain('+const weird = true;'); + }); + it('consolidates the unborn-repository fallback into one final-state patch per file', async () => { const emptyTree = '4b825dc642cb6eb9a060e54bf8d69288fbee4904'; const outputs: Record = { @@ -362,7 +381,7 @@ describe('git inspection', () => { const headSha = 'a'.repeat(40); await new LocalRunner(root, { process }).context({ baseSha, headSha }); const range = `${baseSha}...${headSha}`; - expect(calls[1]?.args).toEqual(['diff', '--name-only', range, '--']); + expect(calls[1]?.args).toEqual(['diff', '--name-only', '-z', range, '--']); expect(calls[2]?.args).toEqual(['diff', '--no-ext-diff', range, '--']); }); From 90b630dbfa4c04ddf482c04c5fe99920c6405b12 Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 08:11:19 +0000 Subject: [PATCH 07/14] chore: keep AGENT_ZERO_PORT in the environment example The Nitro control plane is being restored in #24 and honors this variable, so dropping it as a leftover of the removed server no longer applies. --- .env.example | 1 + 1 file changed, 1 insertion(+) diff --git a/.env.example b/.env.example index 800af52..3ae050b 100644 --- a/.env.example +++ b/.env.example @@ -1,4 +1,5 @@ OPENAI_API_KEY= AGENT_ZERO_MODEL=gpt-5 +AGENT_ZERO_PORT=4040 GITHUB_TOKEN= GITHUB_WEBHOOK_SECRET= From 22eb3daeb128e98543973c59ee70c10e7f7f931d Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 08:20:29 +0000 Subject: [PATCH 08/14] fix(runner): collect every untracked patch and truncate the review diff once Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 9 ++++----- packages/runner/src/index.test.ts | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index f7ecde5..d624599 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -240,16 +240,16 @@ export abstract class RepositoryBoundary implements Runner { * reviewer too; `git diff --no-index` against `/dev/null` renders the same new-file patch a * commit would produce. The listing is NUL-delimited (`-z`) and parsed verbatim: git C-quotes * names containing characters such as newlines or tabs in newline-delimited output, and that - * display representation would not open as a filesystem path. Collection stops once the diff - * budget is exhausted, since anything further would be truncated away regardless. + * display representation would not open as a filesystem path. Every listed file is collected: + * the diff budget is applied once, by {@link context}'s final tail-keeping truncation, so an + * early stop here would silently drop later files' patches that the truncation would have kept + * while {@link reviewFiles} still publishes their paths as review targets. */ private async untrackedPatches(): Promise { const listing = await this.git(['ls-files', '-z', '--others', '--exclude-standard']); const patches: string[] = []; - let total = 0; for (const path of listing.stdout.split('\0')) { if (path.length === 0 || !isRepositoryRelativePath(path)) continue; - if (total > MAX_DIFF) break; // `--no-index` exits 1 when the paths differ, which is the expected outcome here; only // larger codes report a real failure, and those degrade to omitting the patch. const outcome = await this.process( @@ -259,7 +259,6 @@ export abstract class RepositoryBoundary implements Runner { ); if (outcome.exitCode > 1 || outcome.stdout.length === 0) continue; patches.push(outcome.stdout); - total += outcome.stdout.length; } return patches; } diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index b2e7de9..35b73fb 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -334,6 +334,25 @@ describe('git inspection', () => { expect(context).toContain('+const untracked = true;'); }); + it('keeps collecting untracked patches after one file exhausts the diff budget', async () => { + // The final tail-keeping truncation in context() keeps later content, so stopping the + // collection early would silently drop a patch for a path reviewFiles() still publishes. + const outputs: Record = { + 'ls-files -z --others --exclude-standard': 'src/huge.ts\0src/late.ts\0', + 'diff --no-ext-diff --no-index -- /dev/null src/huge.ts': `+${'x'.repeat(150_000)}`, + 'diff --no-ext-diff --no-index -- /dev/null src/late.ts': '+const late = true;', + }; + const process: ProcessRunner = async (program, args) => { + const argv = args.join(' '); + const stdout = program === 'git' ? (outputs[argv] ?? '') : ''; + // `--no-index` reports "the paths differ" with exit code 1, like real git. + const exitCode = argv.includes('--no-index') && stdout.length > 0 ? 1 : 0; + return { exitCode, stdout, stderr: '' }; + }; + const context = await new LocalRunner(root, { process }).context(); + expect(context).toContain('+const late = true;'); + }); + it('keeps an untracked filename with special characters usable through NUL-delimited listings', async () => { // Newline-delimited git output would C-quote this name into a non-path display string. const weird = 'src/untracked\nfile.ts'; From 1a4ff3b3e1b65f2497b7a74de4d8c406e52e95df Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 08:32:01 +0000 Subject: [PATCH 09/14] fix(runner): allocate the review diff budget per file with truncation markers Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 64 +++++++++++++++++++++++++------ packages/runner/src/index.test.ts | 26 ++++++++++++- 2 files changed, 77 insertions(+), 13 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index d624599..437b8a9 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -18,6 +18,7 @@ import { isRepositoryRelativePath, redactSecrets, secretValuesFromEnvironment, + truncateHead, truncateTail, type CheckResult, type NetworkPolicy, @@ -160,10 +161,10 @@ export abstract class RepositoryBoundary implements Runner { const diffRange = contextDiffRange(options); const files = await this.git(['ls-files']); const changedFiles = await this.reviewFiles(options); - const diff = + const patches = diffRange.length > 0 - ? (await this.git(['diff', '--no-ext-diff', ...diffRange, '--'])).stdout - : await this.pendingDiff(); + ? splitFilePatches((await this.git(['diff', '--no-ext-diff', ...diffRange, '--'])).stdout) + : await this.pendingPatches(); return [ 'FILES', truncateTail(files.stdout, MAX_FILE_LIST), @@ -172,7 +173,7 @@ export abstract class RepositoryBoundary implements Runner { truncateTail(changedFiles.join('\n'), MAX_FILE_LIST), '', 'DIFF', - truncateTail(diff, MAX_DIFF), + boundedDiff(patches, MAX_DIFF), ].join('\n'); } @@ -211,11 +212,13 @@ export abstract class RepositoryBoundary implements Runner { * edit. Untracked files appear in no git diff at all, so each one gets a synthetic * creation patch (see {@link untrackedPatches}). */ - private async pendingDiff(): Promise { + private async pendingPatches(): Promise { const tracked = await this.git(['diff', '--no-ext-diff', 'HEAD', '--']); - const parts = tracked.exitCode === 0 ? [tracked.stdout] : [await this.unbornDiff()]; - parts.push(...(await this.untrackedPatches())); - return parts.filter((part) => part.length > 0).join('\n'); + const patches = splitFilePatches( + tracked.exitCode === 0 ? tracked.stdout : await this.unbornDiff(), + ); + patches.push(...(await this.untrackedPatches())); + return patches; } /** @@ -241,9 +244,9 @@ export abstract class RepositoryBoundary implements Runner { * commit would produce. The listing is NUL-delimited (`-z`) and parsed verbatim: git C-quotes * names containing characters such as newlines or tabs in newline-delimited output, and that * display representation would not open as a filesystem path. Every listed file is collected: - * the diff budget is applied once, by {@link context}'s final tail-keeping truncation, so an - * early stop here would silently drop later files' patches that the truncation would have kept - * while {@link reviewFiles} still publishes their paths as review targets. + * the diff budget is allocated per file by {@link boundedDiff}, so an early stop here would + * silently drop later files' patches that the allocation would have kept while + * {@link reviewFiles} still publishes their paths as review targets. */ private async untrackedPatches(): Promise { const listing = await this.git(['ls-files', '-z', '--others', '--exclude-standard']); @@ -437,6 +440,45 @@ function contextDiffRange(options: RepositoryContextOptions): string[] { return [`${baseSha}...${headSha}`]; } +/** + * Split a multi-file git diff into one patch per file. + * + * `diff --git` headers only ever start a line at column zero; every content line carries a + * one-character prefix, so the split cannot fire inside a patch body. + */ +function splitFilePatches(diff: string): string[] { + if (diff.length === 0) return []; + return diff.split(/\n(?=diff --git )/).filter((patch) => patch.length > 0); +} + +/** + * Join per-file patches under a shared budget without letting one file evict another. + * + * A single tail-keeping truncation of the joined diff would let one oversized patch push an + * earlier file's patch out entirely while the changed-file list still names that file as a review + * target. Instead the budget is allocated per patch: small patches keep everything, the surplus is + * shared among the larger ones, and each truncated patch keeps its head (the header naming the + * file) with an explicit truncation marker. + */ +function boundedDiff(patches: readonly string[], budget: number): string { + if (patches.length === 0) return ''; + const joined = patches.join('\n'); + if (joined.length <= budget) return joined; + let remaining = Math.max(budget - (patches.length - 1), 0); + let left = patches.length; + const allocations = new Array(patches.length); + const bySize = patches + .map((patch, index) => ({ length: patch.length, index })) + .toSorted((a, b) => a.length - b.length); + for (const { length, index } of bySize) { + const taken = Math.min(length, Math.floor(remaining / left)); + allocations[index] = taken; + remaining -= taken; + left -= 1; + } + return patches.map((patch, index) => truncateHead(patch, allocations[index] ?? 0)).join('\n'); +} + function assertInside(root: string, candidate: string, original: string): void { const rel = relative(root, candidate); if (rel.length > 0 && (isAbsolute(rel) || rel.replaceAll('\\', '/').split('/').includes('..'))) diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index 35b73fb..f76b071 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -335,8 +335,8 @@ describe('git inspection', () => { }); it('keeps collecting untracked patches after one file exhausts the diff budget', async () => { - // The final tail-keeping truncation in context() keeps later content, so stopping the - // collection early would silently drop a patch for a path reviewFiles() still publishes. + // The diff budget is allocated per file in context(), so stopping the collection early + // would silently drop a patch for a path reviewFiles() still publishes. const outputs: Record = { 'ls-files -z --others --exclude-standard': 'src/huge.ts\0src/late.ts\0', 'diff --no-ext-diff --no-index -- /dev/null src/huge.ts': `+${'x'.repeat(150_000)}`, @@ -353,6 +353,28 @@ describe('git inspection', () => { expect(context).toContain('+const late = true;'); }); + it('keeps a tracked patch in the review diff when an oversized untracked patch follows it', async () => { + // A single tail-keeping truncation of the joined diff would evict the earlier tracked patch + // while reviewFiles() still lists the tracked file; the per-file budget instead truncates + // only the oversized patch, with an explicit marker. + const outputs: Record = { + 'diff --no-ext-diff HEAD --': + 'diff --git a/src/tracked.ts b/src/tracked.ts\n+const tracked = true;', + 'ls-files -z --others --exclude-standard': 'src/huge.ts\0', + 'diff --no-ext-diff --no-index -- /dev/null src/huge.ts': `+${'x'.repeat(150_000)}`, + }; + const process: ProcessRunner = async (program, args) => { + const argv = args.join(' '); + const stdout = program === 'git' ? (outputs[argv] ?? '') : ''; + // `--no-index` reports "the paths differ" with exit code 1, like real git. + const exitCode = argv.includes('--no-index') && stdout.length > 0 ? 1 : 0; + return { exitCode, stdout, stderr: '' }; + }; + const context = await new LocalRunner(root, { process }).context(); + expect(context).toContain('+const tracked = true;'); + expect(context).toContain('[truncated'); + }); + it('keeps an untracked filename with special characters usable through NUL-delimited listings', async () => { // Newline-delimited git output would C-quote this name into a non-path display string. const weird = 'src/untracked\nfile.ts'; From 9ddcac9023076b3f0f8aeda2f787bd48f48654ab Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 08:34:08 +0000 Subject: [PATCH 10/14] fix(runner): hoist the patch-boundary regex and drop the new Array allocation Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index 437b8a9..86092d7 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -84,6 +84,9 @@ const MAX_FILE_LIST = 30_000; const MAX_DIFF = 100_000; const GIT_TIMEOUT_MS = 30_000; const COMMIT_SHA = /^[0-9a-f]{7,64}$/i; +// `diff --git` headers only ever start a line at column zero; every content line carries a +// one-character prefix, so this boundary cannot fire inside a patch body. +const FILE_PATCH_BOUNDARY = /\n(?=diff --git )/; // Not defined on every platform; opening still works there, the descriptor re-check remains. const O_NOFOLLOW = constants.O_NOFOLLOW ?? 0; const O_DIRECTORY = constants.O_DIRECTORY ?? 0; @@ -440,15 +443,10 @@ function contextDiffRange(options: RepositoryContextOptions): string[] { return [`${baseSha}...${headSha}`]; } -/** - * Split a multi-file git diff into one patch per file. - * - * `diff --git` headers only ever start a line at column zero; every content line carries a - * one-character prefix, so the split cannot fire inside a patch body. - */ +/** Split a multi-file git diff into one patch per file (see {@link FILE_PATCH_BOUNDARY}). */ function splitFilePatches(diff: string): string[] { if (diff.length === 0) return []; - return diff.split(/\n(?=diff --git )/).filter((patch) => patch.length > 0); + return diff.split(FILE_PATCH_BOUNDARY).filter((patch) => patch.length > 0); } /** @@ -466,7 +464,7 @@ function boundedDiff(patches: readonly string[], budget: number): string { if (joined.length <= budget) return joined; let remaining = Math.max(budget - (patches.length - 1), 0); let left = patches.length; - const allocations = new Array(patches.length); + const allocations = patches.map(() => 0); const bySize = patches .map((patch, index) => ({ length: patch.length, index })) .toSorted((a, b) => a.length - b.length); From 186471bba5ca2f291c7d9700eb08d705393072c5 Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 08:45:56 +0000 Subject: [PATCH 11/14] fix(runner): keep every rendered review patch attributable to its file Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 48 ++++++++++++++++++++++++------- packages/runner/src/index.test.ts | 30 +++++++++++++++++-- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index 86092d7..a0b4b82 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -454,27 +454,53 @@ function splitFilePatches(diff: string): string[] { * * A single tail-keeping truncation of the joined diff would let one oversized patch push an * earlier file's patch out entirely while the changed-file list still names that file as a review - * target. Instead the budget is allocated per patch: small patches keep everything, the surplus is - * shared among the larger ones, and each truncated patch keeps its head (the header naming the - * file) with an explicit truncation marker. + * target. Instead the budget is allocated per patch: small patches keep everything and the + * surplus is shared among the larger ones. Every rendered patch keeps at least its complete first + * line (the `diff --git` header naming the file), because a fragment shorter than the header + * could not be attributed to any changed-file entry; when the budget cannot fit every header, the + * trailing patches are dropped whole behind an explicit omission marker rather than surfacing as + * anonymous fragments. */ function boundedDiff(patches: readonly string[], budget: number): string { if (patches.length === 0) return ''; const joined = patches.join('\n'); if (joined.length <= budget) return joined; - let remaining = Math.max(budget - (patches.length - 1), 0); - let left = patches.length; - const allocations = patches.map(() => 0); - const bySize = patches + const headers = patches.map((patch) => { + const end = patch.indexOf('\n'); + return end === -1 ? patch.length : end; + }); + // Retain the leading patches whose complete header lines all fit within the budget. + let reserved = 0; + let retained = 0; + while (retained < patches.length) { + const cost = (headers[retained] ?? 0) + (retained > 0 ? 1 : 0); + if (reserved + cost > budget) break; + reserved += cost; + retained += 1; + } + const kept = patches.slice(0, retained); + let remaining = budget - reserved; + let left = retained; + const allocations = kept.map((_, index) => headers[index] ?? 0); + const bySize = kept .map((patch, index) => ({ length: patch.length, index })) .toSorted((a, b) => a.length - b.length); for (const { length, index } of bySize) { - const taken = Math.min(length, Math.floor(remaining / left)); - allocations[index] = taken; - remaining -= taken; + const extra = Math.min( + Math.max(length - (allocations[index] ?? 0), 0), + Math.floor(remaining / left), + ); + allocations[index] = (allocations[index] ?? 0) + extra; + remaining -= extra; left -= 1; } - return patches.map((patch, index) => truncateHead(patch, allocations[index] ?? 0)).join('\n'); + const rendered = kept.map((patch, index) => truncateHead(patch, allocations[index] ?? 0)); + const omitted = patches.length - retained; + if (omitted > 0) { + const noun = omitted === 1 ? 'file patch' : 'file patches'; + rendered.push(`[omitted ${String(omitted)} ${noun} beyond the diff budget]`); + } + return rendered.join('\n'); } function assertInside(root: string, candidate: string, original: string): void { diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index f76b071..00de334 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -339,7 +339,7 @@ describe('git inspection', () => { // would silently drop a patch for a path reviewFiles() still publishes. const outputs: Record = { 'ls-files -z --others --exclude-standard': 'src/huge.ts\0src/late.ts\0', - 'diff --no-ext-diff --no-index -- /dev/null src/huge.ts': `+${'x'.repeat(150_000)}`, + 'diff --no-ext-diff --no-index -- /dev/null src/huge.ts': `diff --git a/src/huge.ts b/src/huge.ts\n+${'x'.repeat(150_000)}`, 'diff --no-ext-diff --no-index -- /dev/null src/late.ts': '+const late = true;', }; const process: ProcessRunner = async (program, args) => { @@ -361,7 +361,7 @@ describe('git inspection', () => { 'diff --no-ext-diff HEAD --': 'diff --git a/src/tracked.ts b/src/tracked.ts\n+const tracked = true;', 'ls-files -z --others --exclude-standard': 'src/huge.ts\0', - 'diff --no-ext-diff --no-index -- /dev/null src/huge.ts': `+${'x'.repeat(150_000)}`, + 'diff --no-ext-diff --no-index -- /dev/null src/huge.ts': `diff --git a/src/huge.ts b/src/huge.ts\n+${'x'.repeat(150_000)}`, }; const process: ProcessRunner = async (program, args) => { const argv = args.join(' '); @@ -375,6 +375,32 @@ describe('git inspection', () => { expect(context).toContain('[truncated'); }); + it('keeps every rendered review patch attributable to its file under allocation pressure', async () => { + // With enough long-path patches, a fair split of the diff budget is shorter than one + // `diff --git` header line; a partial header could not be associated with any CHANGED FILES + // entry, so every rendered patch must keep its complete header and the overflow must be + // declared instead of surfacing as anonymous fragments. + const patches = Array.from({ length: 2_000 }, (_, index) => { + const path = `src/${'directory/'.repeat(12)}file-${String(index)}.ts`; + return `diff --git a/${path} b/${path}\n+${'x'.repeat(400)}`; + }); + const outputs: Record = { + 'diff --no-ext-diff HEAD --': patches.join('\n'), + }; + const process: ProcessRunner = async (program, args) => ({ + exitCode: 0, + stdout: program === 'git' ? (outputs[args.join(' ')] ?? '') : '', + stderr: '', + }); + const context = await new LocalRunner(root, { process }).context(); + const diff = context.slice(context.indexOf('\nDIFF\n')); + const headerLines = diff.split('\n').filter((line) => line.includes('diff --git')); + expect(headerLines.length).toBeGreaterThan(0); + for (const line of headerLines) expect(line).toMatch(/^diff --git a\/\S+ b\/\S+$/); + expect(diff).toContain('[omitted'); + expect(diff).toContain('file patches beyond the diff budget]'); + }); + it('keeps an untracked filename with special characters usable through NUL-delimited listings', async () => { // Newline-delimited git output would C-quote this name into a non-path display string. const weird = 'src/untracked\nfile.ts'; From 3c3355c63233f5d4b92a45729490834d4e1e6588 Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 08:47:14 +0000 Subject: [PATCH 12/14] fix(runner): hoist the patch-header regex in the allocation-pressure test Co-authored-by: Codesmith --- packages/runner/src/index.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index 00de334..667e00d 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -25,6 +25,9 @@ import { type ProcessRunner, } from './index.js'; +/** A complete `diff --git` header line, as rendered for the whitespace-free test paths below. */ +const COMPLETE_PATCH_HEADER = /^diff --git a\/\S+ b\/\S+$/; + /** * Reproduces the validate-then-swap race deterministically: validation passes against a real * directory, then the directory is replaced with a symlink before the filesystem operation runs. @@ -396,7 +399,7 @@ describe('git inspection', () => { const diff = context.slice(context.indexOf('\nDIFF\n')); const headerLines = diff.split('\n').filter((line) => line.includes('diff --git')); expect(headerLines.length).toBeGreaterThan(0); - for (const line of headerLines) expect(line).toMatch(/^diff --git a\/\S+ b\/\S+$/); + for (const line of headerLines) expect(line).toMatch(COMPLETE_PATCH_HEADER); expect(diff).toContain('[omitted'); expect(diff).toContain('file patches beyond the diff budget]'); }); From 27fda308cc707c4e5331f02e4925cbd605cc4584 Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 08:58:14 +0000 Subject: [PATCH 13/14] fix(runner): mark untracked review files whose patch cannot be produced Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 29 +++++++++++++++++++++++++---- packages/runner/src/index.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index a0b4b82..651cd4f 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -87,6 +87,8 @@ const COMMIT_SHA = /^[0-9a-f]{7,64}$/i; // `diff --git` headers only ever start a line at column zero; every content line carries a // one-character prefix, so this boundary cannot fire inside a patch body. const FILE_PATCH_BOUNDARY = /\n(?=diff --git )/; +// Characters that would break a synthetic single-line `diff --git` header (see unavailablePatch). +const HEADER_UNSAFE = /[\n\r\t"\\]/; // Not defined on every platform; opening still works there, the descriptor re-check remains. const O_NOFOLLOW = constants.O_NOFOLLOW ?? 0; const O_DIRECTORY = constants.O_DIRECTORY ?? 0; @@ -249,7 +251,9 @@ export abstract class RepositoryBoundary implements Runner { * display representation would not open as a filesystem path. Every listed file is collected: * the diff budget is allocated per file by {@link boundedDiff}, so an early stop here would * silently drop later files' patches that the allocation would have kept while - * {@link reviewFiles} still publishes their paths as review targets. + * {@link reviewFiles} still publishes their paths as review targets. For the same reason a + * file whose diff fails or renders nothing gets an explicit stand-in patch (see + * {@link unavailablePatch}) rather than a silent omission. */ private async untrackedPatches(): Promise { const listing = await this.git(['ls-files', '-z', '--others', '--exclude-standard']); @@ -257,14 +261,16 @@ export abstract class RepositoryBoundary implements Runner { for (const path of listing.stdout.split('\0')) { if (path.length === 0 || !isRepositoryRelativePath(path)) continue; // `--no-index` exits 1 when the paths differ, which is the expected outcome here; only - // larger codes report a real failure, and those degrade to omitting the patch. + // larger codes report a real failure. const outcome = await this.process( 'git', ['diff', '--no-ext-diff', '--no-index', '--', '/dev/null', path], { cwd: this.root, timeoutMs: GIT_TIMEOUT_MS, maxOutputBytes: this.maxOutputBytes }, ); - if (outcome.exitCode > 1 || outcome.stdout.length === 0) continue; - patches.push(outcome.stdout); + if (outcome.exitCode > 1) patches.push(unavailablePatch(path, 'git diff --no-index failed')); + else if (outcome.stdout.length === 0) + patches.push(unavailablePatch(path, 'git diff --no-index rendered no patch')); + else patches.push(outcome.stdout); } return patches; } @@ -503,6 +509,21 @@ function boundedDiff(patches: readonly string[], budget: number): string { return rendered.join('\n'); } +/** + * An explicit stand-in patch for an untracked file whose synthetic diff could not be produced. + * + * {@link reviewFiles} publishes every untracked path as a review target, so silently skipping a + * failed or empty `git diff --no-index` would leave a target selectable without any reviewable + * content behind it. The stand-in keeps the `diff --git` header shape that {@link boundedDiff} + * preserves for attribution and declares the omission instead of hiding it. A name containing + * characters such as newlines is rendered in its quoted display form so the header stays a + * single attributable line. + */ +function unavailablePatch(path: string, reason: string): string { + const name = HEADER_UNSAFE.test(path) ? JSON.stringify(path) : path; + return `diff --git a/${name} b/${name}\n[untracked file patch unavailable: ${reason}]`; +} + function assertInside(root: string, candidate: string, original: string): void { const rel = relative(root, candidate); if (rel.length > 0 && (isAbsolute(rel) || rel.replaceAll('\\', '/').split('/').includes('..'))) diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index 667e00d..8155ba1 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -404,6 +404,29 @@ describe('git inspection', () => { expect(diff).toContain('file patches beyond the diff budget]'); }); + it('marks an untracked file whose synthetic patch could not be produced instead of dropping it', async () => { + // reviewFiles() publishes both paths as review targets, so a failed or empty `--no-index` + // diff must surface an explicit marker rather than leaving a listed target without any + // reviewable content in DIFF. + const outputs: Record = { + 'ls-files -z --others --exclude-standard': 'src/failed.ts\0src/empty.ts\0', + 'diff --no-ext-diff --no-index -- /dev/null src/empty.ts': '', + }; + const process: ProcessRunner = async (program, args) => { + const argv = args.join(' '); + if (argv === 'diff --no-ext-diff --no-index -- /dev/null src/failed.ts') + return { exitCode: 2, stdout: '', stderr: 'boom' }; + return { exitCode: 0, stdout: program === 'git' ? (outputs[argv] ?? '') : '', stderr: '' }; + }; + const context = await new LocalRunner(root, { process }).context(); + expect(context).toContain( + 'diff --git a/src/failed.ts b/src/failed.ts\n[untracked file patch unavailable: git diff --no-index failed]', + ); + expect(context).toContain( + 'diff --git a/src/empty.ts b/src/empty.ts\n[untracked file patch unavailable: git diff --no-index rendered no patch]', + ); + }); + it('keeps an untracked filename with special characters usable through NUL-delimited listings', async () => { // Newline-delimited git output would C-quote this name into a non-path display string. const weird = 'src/untracked\nfile.ts'; From 493cc190b122519af7c995db0320fb122cbceacc Mon Sep 17 00:00:00 2001 From: RedStar071 Date: Mon, 10 Aug 2026 09:10:58 +0000 Subject: [PATCH 14/14] fix(runner): render omitted review patches with their diff headers Co-authored-by: Codesmith --- packages/runner/src/boundary.ts | 15 ++++++++------- packages/runner/src/index.test.ts | 10 +++++----- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/packages/runner/src/boundary.ts b/packages/runner/src/boundary.ts index 651cd4f..b272558 100644 --- a/packages/runner/src/boundary.ts +++ b/packages/runner/src/boundary.ts @@ -463,9 +463,11 @@ function splitFilePatches(diff: string): string[] { * target. Instead the budget is allocated per patch: small patches keep everything and the * surplus is shared among the larger ones. Every rendered patch keeps at least its complete first * line (the `diff --git` header naming the file), because a fragment shorter than the header - * could not be attributed to any changed-file entry; when the budget cannot fit every header, the - * trailing patches are dropped whole behind an explicit omission marker rather than surfacing as - * anonymous fragments. + * could not be attributed to any changed-file entry. When the budget cannot fit every header, the + * trailing patches lose their bodies but never their identity: each one still renders its + * complete header line over an explicit omission marker, so every path the changed-file list + * names stays attributable in the diff. That overrun is deliberate and bounded by the same + * per-file header data the changed-file list already carries. */ function boundedDiff(patches: readonly string[], budget: number): string { if (patches.length === 0) return ''; @@ -501,10 +503,9 @@ function boundedDiff(patches: readonly string[], budget: number): string { left -= 1; } const rendered = kept.map((patch, index) => truncateHead(patch, allocations[index] ?? 0)); - const omitted = patches.length - retained; - if (omitted > 0) { - const noun = omitted === 1 ? 'file patch' : 'file patches'; - rendered.push(`[omitted ${String(omitted)} ${noun} beyond the diff budget]`); + for (let index = retained; index < patches.length; index += 1) { + const header = (patches[index] ?? '').slice(0, headers[index] ?? 0); + rendered.push(`${header}\n[file patch omitted beyond the diff budget]`); } return rendered.join('\n'); } diff --git a/packages/runner/src/index.test.ts b/packages/runner/src/index.test.ts index 8155ba1..25566f7 100644 --- a/packages/runner/src/index.test.ts +++ b/packages/runner/src/index.test.ts @@ -381,8 +381,9 @@ describe('git inspection', () => { it('keeps every rendered review patch attributable to its file under allocation pressure', async () => { // With enough long-path patches, a fair split of the diff budget is shorter than one // `diff --git` header line; a partial header could not be associated with any CHANGED FILES - // entry, so every rendered patch must keep its complete header and the overflow must be - // declared instead of surfacing as anonymous fragments. + // entry, so every patch must render its complete header even when its body is omitted, and + // each omission must be declared instead of surfacing as an anonymous fragment or a bare + // count that leaves reviewers unable to tell which named files lack diff content. const patches = Array.from({ length: 2_000 }, (_, index) => { const path = `src/${'directory/'.repeat(12)}file-${String(index)}.ts`; return `diff --git a/${path} b/${path}\n+${'x'.repeat(400)}`; @@ -398,10 +399,9 @@ describe('git inspection', () => { const context = await new LocalRunner(root, { process }).context(); const diff = context.slice(context.indexOf('\nDIFF\n')); const headerLines = diff.split('\n').filter((line) => line.includes('diff --git')); - expect(headerLines.length).toBeGreaterThan(0); + expect(headerLines.length).toBe(patches.length); for (const line of headerLines) expect(line).toMatch(COMPLETE_PATCH_HEADER); - expect(diff).toContain('[omitted'); - expect(diff).toContain('file patches beyond the diff budget]'); + expect(diff).toContain('[file patch omitted beyond the diff budget]'); }); it('marks an untracked file whose synthetic patch could not be produced instead of dropping it', async () => {