From 8c30ee95958c2d10e42c1603d664ce13c429536b Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 29 Sep 2026 15:19:50 +0000 Subject: [PATCH] fix(test): stop the eng batching eval once its floor is proven The case's only verdict is reviewCount >= FLOOR (3). Run 36385945043 had three distinct acknowledged review decisions at 6m41s but kept answering until the ceiling (7) at 12m13s. The registration now passes the runner's existing isCollectionComplete stop once FLOOR non-setup, non-administrative review decisions are acknowledged; the floor check, ceiling, budget and counter are unchanged. A child-process registration test proves the stop predicate and that below-floor and timeout outcomes still fail. --- test/eng-batching-saved-ledger.test.ts | 52 +++++++++++++++++++ ...2e-plan-eng-multi-finding-batching.test.ts | 13 +++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/test/eng-batching-saved-ledger.test.ts b/test/eng-batching-saved-ledger.test.ts index d343e2c1e..d9dd7eb57 100644 --- a/test/eng-batching-saved-ledger.test.ts +++ b/test/eng-batching-saved-ledger.test.ts @@ -432,3 +432,55 @@ for (const context of ['## History','## Archived source','Quoted source:\n']) plan=plan.replace(target,'').replace('## Decision ledger',`${context}\n\n${target}\n\n## Decision ledger`); expect(inlineEvaluate(call,plan)).toBe(false); }); + +// Import the actual paid registration in an isolated Bun child; only its native +// runner is controlled. Collection stops once FLOOR distinct review decisions are +// acknowledged, and the unchanged floor verdict still decides the outcome. +test.each([ + { scenario: 'floor settled', outcome: 'collection_complete', reviewCount: 3, passes: true }, + { scenario: 'batched below floor', outcome: 'plan_ready', reviewCount: 2, passes: false }, + { scenario: 'ceiling', outcome: 'ceiling_reached', reviewCount: 7, passes: true }, + { scenario: 'timeout', outcome: 'timeout', reviewCount: 3, passes: false }, +])('actual batching registration stops at the proven floor: $scenario', async ({ outcome, reviewCount, passes }) => { + const ROOT = path.resolve(import.meta.dir, '..'); + const temp = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'batching-registration-'))); + const factsPath = path.join(temp, 'facts.json'); + const runner = path.join(ROOT, 'test/helpers/claude-pty-runner.ts'); + const script = path.join(temp, 'registration.test.ts'); + fs.writeFileSync(script, ` +import { describe, expect, mock } from 'bun:test'; +import * as fs from 'node:fs'; +import * as real from ${JSON.stringify(runner)}; +const facts = { runs: 0, stops: [] as boolean[], ceiling: 0 }; +const save = () => fs.writeFileSync(${JSON.stringify(factsPath)}, JSON.stringify(facts)); +const fp = (signature: string, preReview: boolean, administrative?: string) => ({ signature, preReview, administrative, promptSnippet: signature, options: [], observedAtMs: 1 }); +mock.module(${JSON.stringify(path.join(ROOT, 'test/helpers/e2e-gate.ts'))}, () => ({ + describeE2ETier: (tier: string) => { expect(tier).toBe('periodic'); return describe; }, +})); +mock.module(${JSON.stringify(runner)}, () => ({ ...real, runPlanSkillCounting: async (opts: any) => { + facts.runs++; facts.ceiling = opts.reviewCountCeiling; + const setup = [fp('s1', true), fp('s2', true)]; + const review = [fp('r1', false), fp('r2', false), fp('r3', false)]; + facts.stops = [ + opts.isCollectionComplete({ status: 'ready', calls: [], assistantMessages: [] }, [...setup, ...review.slice(0, 2)]), + opts.isCollectionComplete({ status: 'ready', calls: [], assistantMessages: [] }, [...setup, ...review.slice(0, 2), fp('h', false, 'completion-handoff')]), + opts.isCollectionComplete({ status: 'ready', calls: [], assistantMessages: [] }, [...setup, ...review]), + ]; + save(); + return { outcome: ${JSON.stringify(outcome)}, summary: 'controlled', evidence: 'controlled', elapsedMs: 1, + fingerprints: [...setup, ...review].slice(0, 2 + ${reviewCount}), step0Count: 2, reviewCount: ${reviewCount}, administrativeCount: 0 }; +} })); +await import(${JSON.stringify(path.join(ROOT, 'test/skill-e2e-plan-eng-multi-finding-batching.test.ts'))}); +`); + try { + const child = Bun.spawn([process.execPath, 'test', script], { cwd: ROOT, stdout: 'pipe', stderr: 'pipe', timeout: 10_000, + env: { PATH: process.env.PATH ?? '', HOME: temp, TMPDIR: temp, TEMP: temp, TMP: temp, GIT_CONFIG_NOSYSTEM: '1', EVALS_HERMETIC: '1', + ...(process.env.SystemRoot ? { SystemRoot: process.env.SystemRoot } : {}) } }); + const [exit, out, err] = await Promise.all([child.exited, new Response(child.stdout).text(), new Response(child.stderr).text()]); + const facts = JSON.parse(fs.readFileSync(factsPath, 'utf8')); + expect(exit, out + err).toBe(passes ? 0 : 1); + expect(facts).toEqual({ runs: 1, stops: [false, false, true], ceiling: 7 }); + } finally { + fs.rmSync(temp, { recursive: true, force: true }); + } +}); diff --git a/test/skill-e2e-plan-eng-multi-finding-batching.test.ts b/test/skill-e2e-plan-eng-multi-finding-batching.test.ts index d78f81968..424b3d6f1 100644 --- a/test/skill-e2e-plan-eng-multi-finding-batching.test.ts +++ b/test/skill-e2e-plan-eng-multi-finding-batching.test.ts @@ -12,7 +12,8 @@ * would pass that test trivially. * - This test uses runPlanSkillCounting at periodic tier (~25 min budget, * N-AUQ tracking, ceiling-bounded retries) to actually count distinct - * review-phase AUQs and assert the model fires one per finding. + * review-phase AUQs and assert the model fires one per finding. Collection + * stops as soon as the floor is proven (~7 min observed). * * Why a separate test from skill-e2e-plan-eng-finding-count (the existing * 5-finding count test): @@ -21,7 +22,7 @@ * This is the tightest regression test for the original bug class — * not a band-around-N test, but a "did the agent batch?" test. * - * Tier: periodic (~25 min, ~$5/run). Sequential by default. + * Tier: periodic (~7 min observed; 25 min budget). Sequential by default. */ import { test } from 'bun:test'; @@ -80,12 +81,18 @@ describeE2E('/plan-eng-review multi-finding batching regression (periodic)', () isSetupAUQ: engSetupAUQ, isFirstReviewAUQ: engFirstReviewAUQ, isReviewAUQ: findings.isReviewAUQ, + // The only verdict is the floor. Once FLOOR distinct acknowledged + // review decisions exist, a batching regression can no longer occur + // in this attempt; stop instead of letting the review run to the + // ceiling (run 36385945043: floor at 6m41s, ceiling at 12m13s). + isCollectionComplete: (_transcript, fingerprints) => + fingerprints.filter(fp => !fp.preReview && !fp.administrative).length >= FLOOR, reviewCountCeiling: N + 3, // hard cap above floor + tolerance timeoutMs: 1_500_000, // 25 min env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' }, }); - if (!['plan_ready', 'completion_summary', 'ceiling_reached'].includes(obs.outcome)) { + if (!['plan_ready', 'completion_summary', 'collection_complete', 'ceiling_reached'].includes(obs.outcome)) { throw new Error( `multi-finding batching test FAILED: outcome=${obs.outcome}\n` + `step0=${obs.step0Count} review=${obs.reviewCount} elapsed=${obs.elapsedMs}ms\n` +