mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-18 02:42:25 +02:00
fix(evals): stop the harness grading itself
findPreviousRun excluded only the file being written, by name, so every suite compared against _partial-e2e.json — the current run's own accumulator, relabelled with the current tier just before each flush. That is why every block read '+$0.00, +0s, Stable run, no regressions.' This harness has never been able to detect a regression, and reassuring output that cannot fail is worse than none. In-progress runs are now excluded by role, and a run with nothing to compare against says NO BASELINE instead of claiming stability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit f3140b5245221fff7fb9411c7ec07c2ca11587b5)
This commit is contained in:
@@ -58,6 +58,19 @@ function makeResult(overrides?: Partial<EvalResult>): EvalResult {
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Capture everything a block writes to stderr (finalize prints there). */
|
||||||
|
async function captureStderr(fn: () => Promise<void>): Promise<string> {
|
||||||
|
const original = process.stderr.write.bind(process.stderr);
|
||||||
|
let captured = '';
|
||||||
|
(process.stderr as any).write = (chunk: any) => { captured += String(chunk); return true; };
|
||||||
|
try {
|
||||||
|
await fn();
|
||||||
|
} finally {
|
||||||
|
(process.stderr as any).write = original;
|
||||||
|
}
|
||||||
|
return captured;
|
||||||
|
}
|
||||||
|
|
||||||
// --- EvalCollector tests ---
|
// --- EvalCollector tests ---
|
||||||
|
|
||||||
describe('EvalCollector', () => {
|
describe('EvalCollector', () => {
|
||||||
@@ -119,6 +132,41 @@ describe('EvalCollector', () => {
|
|||||||
expect(fs.readdirSync(tmpDir).filter(f => f.endsWith('.json') && !f.startsWith('_partial'))).toHaveLength(1);
|
expect(fs.readdirSync(tmpDir).filter(f => f.endsWith('.json') && !f.startsWith('_partial'))).toHaveLength(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('with no completed prior run, says NO BASELINE instead of comparing against its own partial', async () => {
|
||||||
|
// addTest writes the in-progress accumulator into the same dir. If that
|
||||||
|
// counted as a baseline, the run would compare against itself and print a
|
||||||
|
// reassuring all-clear forever.
|
||||||
|
const collector = new EvalCollector('e2e', tmpDir);
|
||||||
|
collector.addTest(makeEntry({ name: 'test-1', passed: true }));
|
||||||
|
|
||||||
|
const output = await captureStderr(async () => { await collector.finalize(); });
|
||||||
|
|
||||||
|
expect(fs.existsSync(path.join(tmpDir, '_partial-e2e.json'))).toBe(true); // the trap exists
|
||||||
|
expect(output).toContain('NO BASELINE');
|
||||||
|
expect(output).not.toContain('vs previous');
|
||||||
|
expect(output).not.toContain('Stable run');
|
||||||
|
});
|
||||||
|
|
||||||
|
test('with a genuine prior run, reports the real delta', async () => {
|
||||||
|
fs.writeFileSync(
|
||||||
|
path.join(tmpDir, '0.3.5-main-e2e-20260312-100000.json'),
|
||||||
|
JSON.stringify(makeResult({
|
||||||
|
timestamp: '2026-03-12T10:00:00Z',
|
||||||
|
tests: [makeEntry({ name: 'test-1', passed: true, turns_used: 5 })],
|
||||||
|
})),
|
||||||
|
);
|
||||||
|
|
||||||
|
const collector = new EvalCollector('e2e', tmpDir);
|
||||||
|
collector.addTest(makeEntry({ name: 'test-1', passed: false, turns_used: 5 }));
|
||||||
|
|
||||||
|
const output = await captureStderr(async () => { await collector.finalize(); });
|
||||||
|
|
||||||
|
expect(output).toContain('vs previous');
|
||||||
|
expect(output).toContain('REGRESSION');
|
||||||
|
expect(output).toContain('1 regressed');
|
||||||
|
expect(output).not.toContain('NO BASELINE');
|
||||||
|
});
|
||||||
|
|
||||||
test('empty collector writes valid file', async () => {
|
test('empty collector writes valid file', async () => {
|
||||||
const collector = new EvalCollector('llm-judge', tmpDir);
|
const collector = new EvalCollector('llm-judge', tmpDir);
|
||||||
const filepath = await collector.finalize();
|
const filepath = await collector.finalize();
|
||||||
@@ -259,6 +307,34 @@ describe('findPreviousRun', () => {
|
|||||||
expect(result).toBeNull(); // only file is excluded
|
expect(result).toBeNull(); // only file is excluded
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('never returns the in-progress accumulator as a baseline', () => {
|
||||||
|
// The current run's own partial carries the current tier + branch and the
|
||||||
|
// freshest timestamp. If it were a candidate, every run would compare
|
||||||
|
// against itself and report "no regressions" forever.
|
||||||
|
fs.writeFileSync(
|
||||||
|
path.join(tmpDir, '_partial-e2e.json'),
|
||||||
|
JSON.stringify(makeResult({ branch: 'main', timestamp: '2026-03-14T10:00:00Z', _partial: true })),
|
||||||
|
);
|
||||||
|
|
||||||
|
const result = findPreviousRun(tmpDir, 'e2e', 'main', path.join(tmpDir, 'current.json'));
|
||||||
|
expect(result).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
test('prefers a completed run over a newer in-progress accumulator', () => {
|
||||||
|
fs.writeFileSync(
|
||||||
|
path.join(tmpDir, '0.3.5-main-e2e-20260312-100000.json'),
|
||||||
|
JSON.stringify(makeResult({ branch: 'main', timestamp: '2026-03-12T10:00:00Z' })),
|
||||||
|
);
|
||||||
|
// Newer, same tier + branch, but in-progress — must lose to the older completed run.
|
||||||
|
fs.writeFileSync(
|
||||||
|
path.join(tmpDir, '_partial-e2e.json'),
|
||||||
|
JSON.stringify(makeResult({ branch: 'main', timestamp: '2026-03-14T10:00:00Z', _partial: true })),
|
||||||
|
);
|
||||||
|
|
||||||
|
const result = findPreviousRun(tmpDir, 'e2e', 'main', path.join(tmpDir, 'current.json'));
|
||||||
|
expect(result).toContain('0.3.5-main-e2e');
|
||||||
|
});
|
||||||
|
|
||||||
test('filters by tier', () => {
|
test('filters by tier', () => {
|
||||||
fs.writeFileSync(
|
fs.writeFileSync(
|
||||||
path.join(tmpDir, '0.3.6-main-llm-judge-20260314-100000.json'),
|
path.join(tmpDir, '0.3.6-main-llm-judge-20260314-100000.json'),
|
||||||
@@ -524,6 +600,29 @@ describe('generateCommentary', () => {
|
|||||||
expect(notes.some(n => n.includes('No regressions'))).toBe(true);
|
expect(notes.some(n => n.includes('No regressions'))).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('says NO BASELINE instead of "stable" when nothing matched the prior run', () => {
|
||||||
|
// A baseline file existed but shares no test names (renamed/retired suite),
|
||||||
|
// so zero tests were actually compared. Claiming stability here is a lie.
|
||||||
|
const c: ComparisonResult = {
|
||||||
|
before_file: 'a.json', after_file: 'b.json',
|
||||||
|
before_branch: 'main', after_branch: 'main',
|
||||||
|
before_timestamp: '', after_timestamp: '',
|
||||||
|
deltas: [
|
||||||
|
{ name: 'a', before: { passed: false, cost_usd: 0 }, after: { passed: true, cost_usd: 0.10 }, status_change: 'unchanged' },
|
||||||
|
{ name: 'b', before: { passed: false, cost_usd: 0 }, after: { passed: true, cost_usd: 0.10 }, status_change: 'unchanged' },
|
||||||
|
{ name: 'c', before: { passed: false, cost_usd: 0 }, after: { passed: true, cost_usd: 0.10 }, status_change: 'unchanged' },
|
||||||
|
],
|
||||||
|
total_cost_delta: 0.30, total_duration_delta: 0,
|
||||||
|
improved: 0, regressed: 0, unchanged: 3,
|
||||||
|
tool_count_before: 0, tool_count_after: 0,
|
||||||
|
matched: 0,
|
||||||
|
};
|
||||||
|
|
||||||
|
const notes = generateCommentary(c);
|
||||||
|
expect(notes.some(n => n.includes('NO BASELINE'))).toBe(true);
|
||||||
|
expect(notes.some(n => n.includes('Stable run'))).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
test('returns empty for stable run with no significant changes', () => {
|
test('returns empty for stable run with no significant changes', () => {
|
||||||
const c: ComparisonResult = {
|
const c: ComparisonResult = {
|
||||||
before_file: 'a.json', after_file: 'b.json',
|
before_file: 'a.json', after_file: 'b.json',
|
||||||
|
|||||||
@@ -131,6 +131,9 @@ export interface ComparisonResult {
|
|||||||
unchanged: number;
|
unchanged: number;
|
||||||
tool_count_before: number;
|
tool_count_before: number;
|
||||||
tool_count_after: number;
|
tool_count_after: number;
|
||||||
|
/** After-tests that had a same-named entry in the before run. 0 = nothing was
|
||||||
|
* actually compared, so no stability claim is warranted. */
|
||||||
|
matched?: number;
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- Shared helpers ---
|
// --- Shared helpers ---
|
||||||
@@ -171,8 +174,14 @@ export function extractToolSummary(transcript: any[]): Record<string, number> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Find the most recent prior eval file for comparison.
|
* Find the most recent prior COMPLETED eval file for comparison.
|
||||||
* Prefers same branch, falls back to any branch.
|
* Prefers same branch, falls back to any branch.
|
||||||
|
*
|
||||||
|
* In-progress accumulators (`_partial: true`, written by savePartial after every
|
||||||
|
* test) are never candidates: the current run's own partial carries the current
|
||||||
|
* tier + branch and the freshest timestamp, so including it made every run
|
||||||
|
* compare against itself and report "no regressions" unconditionally. The
|
||||||
|
* exclusion is by role (the `_partial` flag), not by filename.
|
||||||
*/
|
*/
|
||||||
export function findPreviousRun(
|
export function findPreviousRun(
|
||||||
evalDir: string,
|
evalDir: string,
|
||||||
@@ -196,6 +205,7 @@ export function findPreviousRun(
|
|||||||
const raw = fs.readFileSync(fullPath, 'utf-8');
|
const raw = fs.readFileSync(fullPath, 'utf-8');
|
||||||
// Quick parse — only grab the fields we need
|
// Quick parse — only grab the fields we need
|
||||||
const data = JSON.parse(raw);
|
const data = JSON.parse(raw);
|
||||||
|
if (data._partial) continue; // in-progress run, not a baseline
|
||||||
if (data.tier !== tier) continue;
|
if (data.tier !== tier) continue;
|
||||||
entries.push({ file: fullPath, branch: data.branch || '', timestamp: data.timestamp || '' });
|
entries.push({ file: fullPath, branch: data.branch || '', timestamp: data.timestamp || '' });
|
||||||
} catch { continue; }
|
} catch { continue; }
|
||||||
@@ -226,6 +236,7 @@ export function compareEvalResults(
|
|||||||
const deltas: TestDelta[] = [];
|
const deltas: TestDelta[] = [];
|
||||||
let improved = 0, regressed = 0, unchanged = 0;
|
let improved = 0, regressed = 0, unchanged = 0;
|
||||||
let toolCountBefore = 0, toolCountAfter = 0;
|
let toolCountBefore = 0, toolCountAfter = 0;
|
||||||
|
let matched = 0;
|
||||||
|
|
||||||
// Index before tests by name
|
// Index before tests by name
|
||||||
const beforeMap = new Map<string, EvalTestEntry>();
|
const beforeMap = new Map<string, EvalTestEntry>();
|
||||||
@@ -246,6 +257,7 @@ export function compareEvalResults(
|
|||||||
|
|
||||||
let statusChange: TestDelta['status_change'] = 'unchanged';
|
let statusChange: TestDelta['status_change'] = 'unchanged';
|
||||||
if (beforeTest) {
|
if (beforeTest) {
|
||||||
|
matched++;
|
||||||
if (!beforeTest.passed && afterTest.passed) { statusChange = 'improved'; improved++; }
|
if (!beforeTest.passed && afterTest.passed) { statusChange = 'improved'; improved++; }
|
||||||
else if (beforeTest.passed && !afterTest.passed) { statusChange = 'regressed'; regressed++; }
|
else if (beforeTest.passed && !afterTest.passed) { statusChange = 'regressed'; regressed++; }
|
||||||
else { unchanged++; }
|
else { unchanged++; }
|
||||||
@@ -314,6 +326,7 @@ export function compareEvalResults(
|
|||||||
unchanged,
|
unchanged,
|
||||||
tool_count_before: toolCountBefore,
|
tool_count_before: toolCountBefore,
|
||||||
tool_count_after: toolCountAfter,
|
tool_count_after: toolCountAfter,
|
||||||
|
matched,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -512,7 +525,17 @@ export function generateCommentary(c: ComparisonResult): string[] {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// 4. Overall summary
|
// 4. No baseline — say so. A run with nothing to compare against must never
|
||||||
|
// read as "stable"; silence or a false all-clear is worse than no output.
|
||||||
|
if (c.matched === 0 && c.deltas.length > 0) {
|
||||||
|
notes.push(
|
||||||
|
`NO BASELINE: none of these ${c.deltas.length} test(s) appear in ${path.basename(c.before_file)}. ` +
|
||||||
|
'Nothing was compared, so this run says nothing about regressions.',
|
||||||
|
);
|
||||||
|
return notes;
|
||||||
|
}
|
||||||
|
|
||||||
|
// 5. Overall summary
|
||||||
if (c.deltas.length >= 3 && regressions.length === 0) {
|
if (c.deltas.length >= 3 && regressions.length === 0) {
|
||||||
const overallParts: string[] = [];
|
const overallParts: string[] = [];
|
||||||
|
|
||||||
@@ -742,7 +765,11 @@ export class EvalCollector {
|
|||||||
const comparison = compareEvalResults(prevResult, result, prevFile, filepath);
|
const comparison = compareEvalResults(prevResult, result, prevFile, filepath);
|
||||||
process.stderr.write(formatComparison(comparison) + '\n');
|
process.stderr.write(formatComparison(comparison) + '\n');
|
||||||
} else {
|
} else {
|
||||||
process.stderr.write('\nFirst run — no comparison available.\n');
|
process.stderr.write(
|
||||||
|
`\nNO BASELINE: no completed prior ${this.tier} run found in ${this.evalDir}` +
|
||||||
|
' (the in-progress accumulator is not a baseline). Nothing compared —' +
|
||||||
|
' this run says nothing about regressions.\n',
|
||||||
|
);
|
||||||
}
|
}
|
||||||
} catch (err: any) {
|
} catch (err: any) {
|
||||||
process.stderr.write(`\nCompare error: ${err.message}\n`);
|
process.stderr.write(`\nCompare error: ${err.message}\n`);
|
||||||
|
|||||||
Reference in New Issue
Block a user