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 }); }