mirror of
https://github.com/garrytan/gstack.git
synced 2026-08-22 14:07:14 +02:00
evals: selection sees uncommitted work; git errors fail closed
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
820afa26e8
commit
0a292dc063
@@ -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']);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -3,8 +3,10 @@
|
|||||||
*
|
*
|
||||||
* Each test declares which source files it depends on ("touchfiles") in
|
* Each test declares which source files it depends on ("touchfiles") in
|
||||||
* ./touchfiles-data.ts (literals only — see the note there). The test runner
|
* ./touchfiles-data.ts (literals only — see the note there). The test runner
|
||||||
* checks `git diff` and only runs tests whose dependencies were modified.
|
* computes changed files as the union of committed diff, staged + unstaged
|
||||||
* Override with EVALS_ALL=1 to run everything.
|
* 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
|
* 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
|
* 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[] {
|
function runGitOrThrow(args: string[], cwd: string, spawnImpl: typeof spawnSync): string {
|
||||||
const result = spawnSync('git', ['diff', '--name-only', `${baseBranch}...HEAD`], {
|
const result = spawnImpl('git', args, {
|
||||||
cwd, stdio: 'pipe', timeout: 5000,
|
cwd, stdio: 'pipe', timeout: 10000, maxBuffer: 8 * 1024 * 1024,
|
||||||
});
|
});
|
||||||
if (result.status !== 0) return [];
|
if (result.error || result.status !== 0) {
|
||||||
return result.stdout.toString().trim().split('\n').filter(Boolean);
|
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 <base>...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 ---
|
// --- Touchfile map diffing ---
|
||||||
|
|||||||
Reference in New Issue
Block a user