From 0286a285f8215fecef128582723cea526f4a73f8 Mon Sep 17 00:00:00 2001 From: Gal Be Date: Tue, 15 Sep 2026 11:54:54 +0300 Subject: [PATCH] Say what the pre-pending marker test actually proves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test claimed its 8s gate turned "noticed late" into an unmissably slow run. It does not. Without the evalOnce entry checkpoint the poll still fires at ~500ms, and the abort listener Runner registers kills the gate subprocess mid-flight, so the same verdict arrives at roughly the same speed by an older route — the whole-branch review traced this and measured it at ~840ms. So the test passes with or without the fix it sat next to, and its comment implied otherwise. That is the worse failure: a reader trusting it would believe the checkpoint was covered here, and delete the unit test in pipeline.test.js that actually discriminates. Both stay, and the comments now say which does what. The timing ceiling keeps its point — an eval that sat through the gate would blow it whichever route noticed the marker — and says to raise it rather than trim the gate if windows-latest ever flakes. Comment-only; no test or source behaviour changes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FV5Usy3xLiz4ngEp6zPC5L --- test/cli/eval.test.js | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/test/cli/eval.test.js b/test/cli/eval.test.js index 4234e87..70968d4 100644 --- a/test/cli/eval.test.js +++ b/test/cli/eval.test.js @@ -270,10 +270,16 @@ describe('eval', () => { // Every abort test above interrupts a run already under way. The sticky // marker's whole point is the opposite case: `stop --force` left the // marker behind after an earlier eval aborted, and it is STILL there, - // unconsumed, when the next eval starts — spec says that must be caught - // within one poll interval (~500ms), not after a full correctness gate. - // A repo whose test gate takes 8s if ever reached turns "checked late" - // into an unmissably slow test rather than a subtle wrong verdict. + // unconsumed, when the next eval starts. + // + // This covers that end to end — exit 2, ABORTED, no row, claim released — + // and NOT the entry checkpoint in evalOnce that makes it prompt. It cannot: + // without that checkpoint the poll still fires at ~500ms, and the abort + // listener Runner registers kills the gate subprocess mid-flight, so the + // same verdict arrives at roughly the same speed by an older route. The + // checkpoint itself is covered by the unit test in pipeline.test.js, which + // calls evalOnce with an already-aborted signal and genuinely fails + // without it. Keep both: this one guards the contract, that one the fix. const dir = await makeBenchRepo() await runCli(['init', '-C', dir]) await writeFiles(dir, { @@ -305,9 +311,11 @@ describe('eval', () => { const code = await new Promise((resolve) => child.on('close', resolve)) const elapsed = Date.now() - start - // Generous ceiling, not a tight bound: well under the 8s the test gate - // would cost if the marker were only noticed after claiming and gating, - // with headroom for windows-latest, the slowest runner in the matrix. + // Generous ceiling, not a tight bound. The 8s gate above is what makes it + // mean anything: an eval that sat through it would blow this, whichever + // route noticed the marker. Headroom is for windows-latest, the slowest + // runner in the matrix — raise the ceiling if it ever flakes there rather + // than trimming the gate, which is the part doing the work. expect(elapsed).toBeLessThan(6000) expect(code).toBe(2) const payload = JSON.parse(out)