From 6dbf6e6883ec66fd91a87b93b807aed501426334 Mon Sep 17 00:00:00 2001 From: montjeffrey Date: Sun, 4 Oct 2026 22:34:30 -0400 Subject: [PATCH] fix(gateguard): report fresh-process failures and fail the gate on them The previous fix stopped a throwing hook from aborting the cold replay by recording it as an allow, matching the warm path. But coldSummary only counted disagreements, so a failure on a step the worker also allowed was no disagreement and left no trace: the report showed no failure, a run with no measured step reported a cold latency of 0 ms because percentile of an empty list is 0, and the exit status passed. Trading a crash for a clean-looking success hid exactly the failure the crash had exposed. coldSummary now counts errors and samples, and reports no latency when nothing was measured rather than a 0 that reads as a fast hook. The report prints "no valid sample" in place of a latency and adds a line for steps that failed in a fresh process, beside the existing disagreement line. The exit check moves into gateFails, which fails on cold errors as it already did on warm errors and cold disagreements, and is exported with coldSummary so both can be tested. The render test builds its report from summarize([]), the real summary shape, so it keeps working as metrics are added. --- scripts/dev/gateguard-eval.js | 50 +++++++++++++++++++++------- tests/scripts/gateguard-eval.test.js | 47 +++++++++++++++++++++++++- 2 files changed, 84 insertions(+), 13 deletions(-) diff --git a/scripts/dev/gateguard-eval.js b/scripts/dev/gateguard-eval.js index 1500b8693..261595600 100644 --- a/scripts/dev/gateguard-eval.js +++ b/scripts/dev/gateguard-eval.js @@ -370,14 +370,21 @@ function measureColdLatency(hookFile, scenarios) { } function coldSummary(coldSteps, warmSteps) { - // A step whose process died has no latency; it still counts toward disagreements. + // A step whose process died has no latency; it still counts toward disagreements + // and errors. Failures are counted on their own: a failed step is recorded as an + // allow, so where the worker also allowed it, it is no disagreement and would + // otherwise vanish from the report and the exit status. const sorted = coldSteps.map(step => step.latencyMs).filter(Number.isFinite).sort((a, b) => a - b); const warmDecisions = new Map(warmSteps.map(step => [`${step.scenario}/${step.step}`, step.decision])); const disagreements = coldSteps.filter(step => warmDecisions.get(`${step.scenario}/${step.step}`) !== step.decision).length; + // With no measured step there is no latency to report; 0 would read as a fast hook. + const at = fraction => (sorted.length ? Number(percentile(sorted, fraction).toFixed(3)) : null); return { - p50Ms: Number(percentile(sorted, 0.5).toFixed(3)), - p90Ms: Number(percentile(sorted, 0.9).toFixed(3)), - p95Ms: Number(percentile(sorted, 0.95).toFixed(3)), + p50Ms: at(0.5), + p90Ms: at(0.9), + p95Ms: at(0.95), + samples: sorted.length, + errors: coldSteps.filter(step => step.error).length, disagreements }; } @@ -507,6 +514,12 @@ function summarize(runs, table = questionTextTable()) { // --- reporting --- +/** A fresh-process percentile, or why there is none: not run, or no step measured. */ +function coldMs(s, key) { + if (!s.cold) return 'not run'; + return s.cold[key] === null ? 'no valid sample' : s.cold[key].toFixed(2); +} + const METRIC_ROWS = [ ['Steps', s => s.totals.steps], ['Denials', s => s.totals.denials], @@ -520,9 +533,9 @@ const METRIC_ROWS = [ ['Allows with a credit note', s => s.totals.allowsByKind.credit], ['Allows with a sibling note', s => s.totals.allowsByKind.sibling], ['Allows with a trivial-edit note', s => s.totals.allowsByKind.trivial], - ['Hook latency p50, fresh process (ms)', s => (s.cold ? s.cold.p50Ms.toFixed(2) : 'not run')], - ['Hook latency p90, fresh process (ms)', s => (s.cold ? s.cold.p90Ms.toFixed(2) : 'not run')], - ['Hook latency p95, fresh process (ms)', s => (s.cold ? s.cold.p95Ms.toFixed(2) : 'not run')], + ['Hook latency p50, fresh process (ms)', s => coldMs(s, 'p50Ms')], + ['Hook latency p90, fresh process (ms)', s => coldMs(s, 'p90Ms')], + ['Hook latency p95, fresh process (ms)', s => coldMs(s, 'p95Ms')], ['run() latency p50, warm (ms)', s => s.latency.p50Ms.toFixed(2)], ['run() latency p90, warm (ms)', s => s.latency.p90Ms.toFixed(2)], ['run() latency p95, warm (ms)', s => s.latency.p95Ms.toFixed(2)] @@ -563,6 +576,9 @@ function renderMarkdown(report) { if (cold && cold.disagreements > 0) { lines.push('', `${report.hooks[0].label}: ${cold.disagreements} step(s) decided differently in a fresh process.`); } + if (cold && cold.errors > 0) { + lines.push('', `${report.hooks[0].label}: ${cold.errors} step(s) failed in a fresh process.`); + } return `${lines.join('\n')}\n`; } @@ -732,11 +748,18 @@ async function main() { const report = await evaluate(options); process.stdout.write(options.format === 'json' ? `${JSON.stringify(report, null, 2)}\n` : renderMarkdown(report)); if (options.sarif) fs.writeFileSync(options.sarif, renderSarif(report), 'utf8'); + if (gateFails(report)) process.exitCode = 1; +} + +/** + * True when the working tree must not pass: a bypass, a mismatch, an explicit allow, + * a hook error in either pass, or a step a fresh process decided differently. + */ +function gateFails(report) { const working = report.hooks[0].summary.totals; - const coldDisagreements = report.hooks[0].summary.cold ? report.hooks[0].summary.cold.disagreements : 0; - if (working.mustDenyBypasses > 0 || working.mismatches > 0 || working.explicitAllows > 0 || working.errors > 0 || coldDisagreements > 0) { - process.exitCode = 1; - } + const cold = report.hooks[0].summary.cold || {}; + return working.mustDenyBypasses > 0 || working.mismatches > 0 || working.explicitAllows > 0 || + working.errors > 0 || cold.disagreements > 0 || cold.errors > 0; } if (!isMainThread && workerData && workerData.gateguardEval) { @@ -748,4 +771,7 @@ if (!isMainThread && workerData && workerData.gateguardEval) { }); } -module.exports = { loadCorpus, materializeHook, runCorpus, measureColdLatency, summarize, questionsAsked, renderMarkdown, renderSarif, parseArgs, evaluate }; +module.exports = { + loadCorpus, materializeHook, runCorpus, measureColdLatency, coldSummary, summarize, questionsAsked, + renderMarkdown, renderSarif, parseArgs, evaluate, gateFails +}; diff --git a/tests/scripts/gateguard-eval.test.js b/tests/scripts/gateguard-eval.test.js index dce12aab7..690c888e8 100644 --- a/tests/scripts/gateguard-eval.test.js +++ b/tests/scripts/gateguard-eval.test.js @@ -9,7 +9,9 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); const { pathToFileURL } = require('url'); -const { parseArgs, renderSarif, loadCorpus, runCorpus, summarize, measureColdLatency } = require('../../scripts/dev/gateguard-eval'); +const { + parseArgs, renderSarif, renderMarkdown, loadCorpus, runCorpus, summarize, measureColdLatency, coldSummary, gateFails +} = require('../../scripts/dev/gateguard-eval'); let passed = 0; let failed = 0; @@ -170,6 +172,46 @@ test('--no-cold turns off the fresh-process pass', () => { assert.strictEqual(parseArgs(['--no-cold']).cold, false); }); +// A fresh-process failure is recorded as an allow. Where the worker also allowed the +// step it is no disagreement, so before failures were counted on their own it left +// no trace in the report, the latency or the exit status. +test('a fresh-process failure on a step the worker allows is still counted', () => { + const cold = coldSummary( + [{ scenario: 's', step: 'a', latencyMs: null, decision: 'allow', error: 'killed' }], + [{ scenario: 's', step: 'a', decision: 'allow' }] + ); + assert.strictEqual(cold.disagreements, 0); + assert.strictEqual(cold.errors, 1); + assert.strictEqual(cold.samples, 0); + assert.strictEqual(cold.p50Ms, null, 'no measured step must not read as a 0 ms latency'); +}); + +test('a fresh-process failure fails the gate when nothing else does', () => { + const clean = report([step()]); + clean.hooks[0].summary.cold = { p50Ms: 1, p90Ms: 1, p95Ms: 1, samples: 1, errors: 0, disagreements: 0 }; + assert.strictEqual(gateFails(clean), false); + const failed = report([step()]); + failed.hooks[0].summary.cold = { p50Ms: null, p90Ms: null, p95Ms: null, samples: 0, errors: 1, disagreements: 0 }; + assert.strictEqual(gateFails(failed), true); +}); + +test('the report names fresh-process failures and invents no latency', () => { + // summarize([]) is the real summary shape with every count at zero, so this keeps + // working as metrics are added, which a hand-built totals object would not. + const working = { ...summarize([]), cold: { p50Ms: null, p90Ms: null, p95Ms: null, samples: 0, errors: 1, disagreements: 0 } }; + const failed = { + corpus: { scenarios: 0, steps: 0 }, + hooks: [ + { label: 'working tree', ref: null, summary: working }, + { label: 'upstream/main', ref: 'upstream/main', summary: summarize([]) } + ] + }; + const text = renderMarkdown(failed); + assert.match(text, /1 step\(s\) failed in a fresh process/); + assert.match(text, /no valid sample/); + assert.ok(!/fresh process \(ms\)[^\n]*\b0\.00\b/.test(text), 'a latency was reported with no measured step'); +}); + async function coldTests() { const corpusDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gateguard-eval-corpus-')); try { @@ -211,6 +253,9 @@ async function coldTests() { test('a throwing step is decided as the worker decides it, so it is no disagreement', () => { assert.deepStrictEqual(coldThrow.map(step => step.decision), warmThrow.steps.map(step => step.decision)); }); + test('every throwing step is reported as a fresh-process failure', () => { + assert.strictEqual(coldSummary(coldThrow, warmThrow.steps).errors, coldThrow.length); + }); } finally { fs.rmSync(throwingDir, { recursive: true, force: true }); }