From 0a292dc06342364fcdd997ad93aab0bd392e27ec Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 15 Aug 2026 08:34:21 -0700 Subject: [PATCH] evals: selection sees uncommitted work; git errors fail closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getChangedFiles is now the deduped union of committed (base...HEAD), staged+unstaged (git diff HEAD), and untracked (git status --porcelain --untracked-files=all) — an agent that edits files and runs evals BEFORE committing no longer gets the full $38 suite every time because the committed diff looked empty. Clean tree still returns [] (run-all by design for main-branch/periodic runs). Git failures now THROW with the failing command, stderr, and 'set EVALS_ALL=1 to deliberately run the full suite' — the old return [] silently became run-all, which is silently expensive. 11 new free tests cover every source, dedupe, quoted paths, and both failure shapes via an injectable spawn seam. Co-Authored-By: Claude Fable 5 --- test/changed-files-union.test.ts | 155 +++++++++++++++++++++++++++++++ test/helpers/test-selection.ts | 67 +++++++++++-- 2 files changed, 214 insertions(+), 8 deletions(-) create mode 100644 test/changed-files-union.test.ts diff --git a/test/changed-files-union.test.ts b/test/changed-files-union.test.ts new file mode 100644 index 000000000..d94ea9185 --- /dev/null +++ b/test/changed-files-union.test.ts @@ -0,0 +1,155 @@ +/** + * getChangedFiles union semantics: committed + staged + unstaged + untracked. + * Free (no API calls), runs with `bun test`. + * + * Change-set 3 of the eval-selection work: an agent that edits files and + * runs evals BEFORE committing used to get an empty committed-diff → run-all + * → full paid suite. getChangedFiles now unions the committed diff with the + * working-tree diff and untracked files, and FAILS CLOSED (throws, naming + * EVALS_ALL=1) on any git error instead of silently returning []. + */ + +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +import { getChangedFiles } from './helpers/touchfiles'; + +describe('getChangedFiles union', () => { + let repo: string; + + const git = (args: string[]) => { + const result = spawnSync( + 'git', + ['-c', 'user.email=test@test', '-c', 'user.name=test', '-c', 'commit.gpgsign=false', ...args], + { cwd: repo, stdio: 'pipe', timeout: 10000 }, + ); + if (result.status !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${result.stderr?.toString()}`); + } + }; + + const write = (rel: string, content: string) => { + const filePath = path.join(repo, rel); + fs.mkdirSync(path.dirname(filePath), { recursive: true }); + fs.writeFileSync(filePath, content); + }; + + beforeEach(() => { + repo = fs.mkdtempSync(path.join(os.tmpdir(), 'changed-files-union-')); + git(['init', '-q']); + write('a.txt', 'a\n'); + write('b.txt', 'b\n'); + git(['add', 'a.txt', 'b.txt']); + git(['commit', '-q', '-m', 'base']); + git(['tag', 'base']); + }); + + afterEach(() => { + fs.rmSync(repo, { recursive: true, force: true }); + }); + + test('committed-only change', () => { + write('a.txt', 'a2\n'); + git(['add', 'a.txt']); + git(['commit', '-q', '-m', 'change a']); + expect(getChangedFiles('base', repo)).toEqual(['a.txt']); + }); + + test('staged-only change', () => { + write('a.txt', 'a2\n'); + git(['add', 'a.txt']); + expect(getChangedFiles('base', repo)).toEqual(['a.txt']); + }); + + test('unstaged-only change', () => { + write('b.txt', 'b2\n'); + expect(getChangedFiles('base', repo)).toEqual(['b.txt']); + }); + + test('untracked-only file', () => { + write('new-dir/new.txt', 'new\n'); + expect(getChangedFiles('base', repo)).toEqual(['new-dir/new.txt']); + }); + + test('mixed sources — each file exactly once', () => { + // committed change to a.txt... + write('a.txt', 'a2\n'); + git(['add', 'a.txt']); + git(['commit', '-q', '-m', 'change a']); + // ...PLUS an unstaged edit to the same file (dedupe check), + write('a.txt', 'a3\n'); + // a staged edit to b.txt, + write('b.txt', 'b2\n'); + git(['add', 'b.txt']); + // and an untracked file. + write('c.txt', 'c\n'); + + const result = getChangedFiles('base', repo); + expect(result.sort()).toEqual(['a.txt', 'b.txt', 'c.txt']); + expect(result.filter(f => f === 'a.txt').length).toBe(1); // deduped + }); + + test('clean tree → empty union (run-all semantics preserved by callers)', () => { + expect(getChangedFiles('base', repo)).toEqual([]); + }); + + test('non-repo cwd → throws naming EVALS_ALL', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'changed-files-nonrepo-')); + try { + expect(() => getChangedFiles('main', dir)).toThrow(/EVALS_ALL=1/); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + test('missing base ref → throws naming EVALS_ALL and the failing command', () => { + expect(() => getChangedFiles('no-such-ref', repo)).toThrow(/EVALS_ALL=1/); + expect(() => getChangedFiles('no-such-ref', repo)).toThrow(/git diff --name-only no-such-ref\.\.\.HEAD/); + }); + + test('injected spawn failure → throws with stderr in the message', () => { + const failingSpawn = ((_cmd: string, args: string[]) => ({ + status: 128, + error: undefined, + stdout: Buffer.from(''), + stderr: Buffer.from(`fatal: injected failure for ${args[0]}`), + })) as unknown as typeof spawnSync; + + let message = ''; + try { + getChangedFiles('base', repo, failingSpawn); + } catch (err) { + message = (err as Error).message; + } + expect(message).toContain('EVALS_ALL=1'); + expect(message).toContain('injected failure'); + expect(message).toContain('exit 128'); + }); + + test('injected spawn error object (git binary missing) → throws', () => { + const errorSpawn = (() => ({ + status: null, + error: new Error('spawn git ENOENT'), + stdout: Buffer.from(''), + stderr: Buffer.from(''), + })) as unknown as typeof spawnSync; + + let message = ''; + try { + getChangedFiles('base', repo, errorSpawn); + } catch (err) { + message = (err as Error).message; + } + expect(message).toContain('EVALS_ALL=1'); + expect(message).toContain('spawn git ENOENT'); + expect(message).toContain('spawn-error'); + }); + + test('untracked path with spaces (git quotes it) is unquoted', () => { + write('has space.txt', 'x\n'); + expect(getChangedFiles('base', repo)).toEqual(['has space.txt']); + }); +}); diff --git a/test/helpers/test-selection.ts b/test/helpers/test-selection.ts index 4037c49d6..e9c7d33f4 100644 --- a/test/helpers/test-selection.ts +++ b/test/helpers/test-selection.ts @@ -3,8 +3,10 @@ * * Each test declares which source files it depends on ("touchfiles") in * ./touchfiles-data.ts (literals only — see the note there). The test runner - * checks `git diff` and only runs tests whose dependencies were modified. - * Override with EVALS_ALL=1 to run everything. + * computes changed files as the union of committed diff, staged + unstaged + * diff, and untracked files — uncommitted work selects tests too — and only + * runs tests whose dependencies were modified. Override with EVALS_ALL=1 to + * run everything. * * When touchfiles-data.ts itself changed, selection uses MAP-DIFF instead of * a global run-all: the old version of the data file is loaded from git and @@ -71,14 +73,63 @@ export function detectBaseBranch(cwd: string): string | null { } /** - * Get list of files changed between base branch and HEAD. + * Run a git command and return stdout. FAIL-CLOSED: any failure (spawn + * error, non-zero exit) throws — a broken git environment must abort the + * suite loudly instead of silently degrading into a full (paid) run. */ -export function getChangedFiles(baseBranch: string, cwd: string): string[] { - const result = spawnSync('git', ['diff', '--name-only', `${baseBranch}...HEAD`], { - cwd, stdio: 'pipe', timeout: 5000, +function runGitOrThrow(args: string[], cwd: string, spawnImpl: typeof spawnSync): string { + const result = spawnImpl('git', args, { + cwd, stdio: 'pipe', timeout: 10000, maxBuffer: 8 * 1024 * 1024, }); - if (result.status !== 0) return []; - return result.stdout.toString().trim().split('\n').filter(Boolean); + if (result.error || result.status !== 0) { + const stderr = result.stderr?.toString().trim() || result.error?.message || 'unknown error'; + throw new Error( + `getChangedFiles: \`git ${args.join(' ')}\` failed in ${cwd} ` + + `(exit ${result.status ?? 'spawn-error'}): ${stderr}\n` + + 'Diff-based test selection cannot proceed. Fix the git environment, ' + + 'or set EVALS_ALL=1 to deliberately run the full suite.', + ); + } + return result.stdout.toString(); +} + +/** + * Get the list of files changed relative to the base branch, INCLUDING + * uncommitted work. Union of three sources, deduped: + * 1. committed: `git diff --name-only ...HEAD` + * 2. staged + unstaged: `git diff --name-only HEAD` + * 3. untracked: `git status --porcelain --untracked-files=all` ('?? ' lines) + * + * Without 2 and 3, an agent that edits files and runs evals BEFORE + * committing gets an empty diff → run-all → the full paid suite every time. + * + * An empty UNION still means "no changes" and callers keep their intentional + * run-all semantics for it (main-branch / periodic full runs depend on that). + * + * Git failures THROW (see runGitOrThrow) instead of returning [] — the old + * behavior made a broken git environment indistinguishable from a clean tree. + * + * `spawnImpl` is injectable for tests. + */ +export function getChangedFiles( + baseBranch: string, + cwd: string, + spawnImpl: typeof spawnSync = spawnSync, +): string[] { + const committed = runGitOrThrow(['diff', '--name-only', `${baseBranch}...HEAD`], cwd, spawnImpl) + .trim().split('\n').filter(Boolean); + const uncommitted = runGitOrThrow(['diff', '--name-only', 'HEAD'], cwd, spawnImpl) + .trim().split('\n').filter(Boolean); + const untracked = runGitOrThrow(['status', '--porcelain', '--untracked-files=all'], cwd, spawnImpl) + .split('\n') + .filter(line => line.startsWith('?? ')) + .map(line => { + let p = line.slice(3); + // git quotes paths containing special characters + if (p.startsWith('"') && p.endsWith('"')) p = p.slice(1, -1); + return p; + }); + return [...new Set([...committed, ...uncommitted, ...untracked])]; } // --- Touchfile map diffing ---