From 39e8e789bfaae005546630ae5791b2378e0f7ee3 Mon Sep 17 00:00:00 2001 From: Sinabina Date: Sun, 2 Aug 2026 07:04:58 +0800 Subject: [PATCH] fix(evals): stop the harness grading itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit f3140b5245221fff7fb9411c7ec07c2ca11587b5) --- test/helpers/eval-store.test.ts | 99 +++++++++++++++++++++++++++++++++ test/helpers/eval-store.ts | 33 ++++++++++- 2 files changed, 129 insertions(+), 3 deletions(-) diff --git a/test/helpers/eval-store.test.ts b/test/helpers/eval-store.test.ts index c6aff1c95..067a56281 100644 --- a/test/helpers/eval-store.test.ts +++ b/test/helpers/eval-store.test.ts @@ -58,6 +58,19 @@ function makeResult(overrides?: Partial): EvalResult { }; } +/** Capture everything a block writes to stderr (finalize prints there). */ +async function captureStderr(fn: () => Promise): Promise { + 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 --- describe('EvalCollector', () => { @@ -119,6 +132,41 @@ describe('EvalCollector', () => { 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 () => { const collector = new EvalCollector('llm-judge', tmpDir); const filepath = await collector.finalize(); @@ -259,6 +307,34 @@ describe('findPreviousRun', () => { 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', () => { fs.writeFileSync( 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); }); + 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', () => { const c: ComparisonResult = { before_file: 'a.json', after_file: 'b.json', diff --git a/test/helpers/eval-store.ts b/test/helpers/eval-store.ts index 9a801ae1c..7a42a9bde 100644 --- a/test/helpers/eval-store.ts +++ b/test/helpers/eval-store.ts @@ -131,6 +131,9 @@ export interface ComparisonResult { unchanged: number; tool_count_before: 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 --- @@ -171,8 +174,14 @@ export function extractToolSummary(transcript: any[]): Record { } /** - * 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. + * + * 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( evalDir: string, @@ -196,6 +205,7 @@ export function findPreviousRun( const raw = fs.readFileSync(fullPath, 'utf-8'); // Quick parse — only grab the fields we need const data = JSON.parse(raw); + if (data._partial) continue; // in-progress run, not a baseline if (data.tier !== tier) continue; entries.push({ file: fullPath, branch: data.branch || '', timestamp: data.timestamp || '' }); } catch { continue; } @@ -226,6 +236,7 @@ export function compareEvalResults( const deltas: TestDelta[] = []; let improved = 0, regressed = 0, unchanged = 0; let toolCountBefore = 0, toolCountAfter = 0; + let matched = 0; // Index before tests by name const beforeMap = new Map(); @@ -246,6 +257,7 @@ export function compareEvalResults( let statusChange: TestDelta['status_change'] = 'unchanged'; if (beforeTest) { + matched++; if (!beforeTest.passed && afterTest.passed) { statusChange = 'improved'; improved++; } else if (beforeTest.passed && !afterTest.passed) { statusChange = 'regressed'; regressed++; } else { unchanged++; } @@ -314,6 +326,7 @@ export function compareEvalResults( unchanged, tool_count_before: toolCountBefore, 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) { const overallParts: string[] = []; @@ -742,7 +765,11 @@ export class EvalCollector { const comparison = compareEvalResults(prevResult, result, prevFile, filepath); process.stderr.write(formatComparison(comparison) + '\n'); } 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) { process.stderr.write(`\nCompare error: ${err.message}\n`);