fix(storm-measure): exclude an unreadable empty_turns, and stop printing BOTH for an OR rule
Two defects in the adoption gate, both in the direction that flatters
adoption.
1. A non-numeric empty_turns counted as an eligible run.
Number('many') is NaN, and the test was `Number.isFinite(empty) &&
empty > 0`, so NaN fell through to the eligible branch. Measured
before: 0 -> eligible, 2 -> excluded, undefined -> eligible, null ->
eligible, 'many' -> ELIGIBLE, NaN -> ELIGIBLE. The exclusion is one of
the two properties docs/storm-measurement.md names as carrying this
gate's honesty, and the run whose bookkeeping broke is the run whose
numbers deserve the least trust. Now excluded. Absent and null stay
eligible via `?? 0` - a field never written is a genuine zero on a run
where the loop never armed.
2. The printed threshold line said "adopt >= 30.0% on BOTH - decline <
15.0% on BOTH" while decideVerdict evaluates OR on both sides. S82
restored the pre-registered OR rule in the logic (c37bf50d) and left
this line describing the stricter AND gate, one line above the verdict
that OR produced. The summary is the only form of the rule most readers
ever see, so it now states EITHER on both sides and that adopt wins
ties. Found while fixing (1); not a review finding.
A test pins the printed line against the string "on BOTH" so the two
cannot drift apart again silently.
Review finding 24a76c21ffc694cd782cd449212c9502d31aeda6 (MINOR).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LuGhWAbWyRFBFeemfhxoVv
This commit is contained in:
parent
32e20fc0dc
commit
7d5e4fcbcc
3 changed files with 61 additions and 3 deletions
|
|
@ -52,7 +52,11 @@ deliberate, reviewable act, not a tuning knob to be nudged toward a result.
|
|||
|
||||
Runs with `empty_turns > 0` are **excluded from the gain and reported as a
|
||||
count**. An empty turn is one that spent budget and returned no findings, or
|
||||
findings without citations. Including those runs decides adoption on a broken
|
||||
findings without citations. A run whose `empty_turns` cannot be **read** as a
|
||||
number is excluded on the same footing: a field that says `"many"` is not
|
||||
evidence of zero empty turns, and treating it as one would let the least
|
||||
trustworthy run back into the denominator in the direction that flatters
|
||||
adoption. Including those runs decides adoption on a broken
|
||||
denominator — the loop looks cheap because its failures are averaged into its
|
||||
successes. The harness prints the excluded count on every invocation; if that
|
||||
count is a large fraction of the treatment arm, the finding is about the loop's
|
||||
|
|
|
|||
|
|
@ -91,6 +91,15 @@ export function parseStats(text) {
|
|||
|
||||
/**
|
||||
* Split off the runs that must not count toward a gain.
|
||||
*
|
||||
* A value that cannot be READ as a turn count is excluded, not treated as zero.
|
||||
* `Number('many')` is NaN, and testing `Number.isFinite(empty) && empty > 0`
|
||||
* sent NaN down the eligible branch — so a garbage field silently re-entered the
|
||||
* denominator, in the direction that flatters adoption: the run whose bookkeeping
|
||||
* broke is the run whose numbers deserve the least trust. Absent and null stay
|
||||
* eligible via `?? 0`, because a field that was never written is a genuine zero
|
||||
* on any run where the loop did not arm.
|
||||
*
|
||||
* @param {object[]} records
|
||||
* @returns {{eligible: object[], excluded: number}}
|
||||
*/
|
||||
|
|
@ -99,7 +108,7 @@ export function partitionEligible(records) {
|
|||
let excluded = 0;
|
||||
for (const r of records) {
|
||||
const empty = Number(r.empty_turns ?? 0);
|
||||
if (Number.isFinite(empty) && empty > 0) excluded++;
|
||||
if (!Number.isFinite(empty) || empty > 0) excluded++;
|
||||
else eligible.push(r);
|
||||
}
|
||||
return { eligible, excluded };
|
||||
|
|
@ -281,7 +290,7 @@ function mainCli() {
|
|||
L.push(` median gain, unique_sources: ${pct(m.sources.gain)}`);
|
||||
L.push(` median gain, dimensions over baseline: ${pct(m.dimensions.gain)} (n=${m.dimensions.n})`);
|
||||
L.push('');
|
||||
L.push(` thresholds: adopt >= ${pct(ADOPT_THRESHOLD)} on BOTH · decline < ${pct(DECLINE_THRESHOLD)} on BOTH`);
|
||||
L.push(` thresholds: adopt >= ${pct(ADOPT_THRESHOLD)} on EITHER · decline < ${pct(DECLINE_THRESHOLD)} on EITHER · adopt wins ties`);
|
||||
L.push(` VERDICT: ${m.verdict}`);
|
||||
process.stdout.write(L.join('\n') + '\n');
|
||||
process.exit(0);
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@
|
|||
|
||||
import { test } from 'node:test';
|
||||
import { strict as assert } from 'node:assert';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import {
|
||||
median,
|
||||
parseStats,
|
||||
|
|
@ -127,6 +128,38 @@ test('partitionEligible: empty_turns === 0 is eligible; a missing field counts a
|
|||
assert.equal(excluded, 0);
|
||||
});
|
||||
|
||||
// A malformed empty_turns used to land in the ELIGIBLE arm: Number('many') is
|
||||
// NaN, NaN fails the isFinite test, and the `else` branch pushed it in. The
|
||||
// exclusion is one of the two properties carrying this gate's honesty, so a
|
||||
// garbage value silently re-entering the denominator defeats it — and it does so
|
||||
// in the direction that flatters adoption, since the run that broke is the run
|
||||
// whose numbers are least trustworthy.
|
||||
test('partitionEligible: a non-numeric empty_turns is EXCLUDED, never silently eligible', () => {
|
||||
const { records } = parseStats(jsonl(
|
||||
STANDARD[0],
|
||||
JSON.stringify({
|
||||
ts: 'm', effort: 'high', unique_sources: 999,
|
||||
dimensions: 8, dimensions_baseline: 5, empty_turns: 'many',
|
||||
}),
|
||||
));
|
||||
const { eligible, excluded } = partitionEligible(records);
|
||||
assert.equal(excluded, 1, 'a value that cannot be read as a turn count is not evidence of zero empty turns');
|
||||
assert.equal(eligible.length, 1);
|
||||
});
|
||||
|
||||
test('partitionEligible: an unparsable empty_turns cannot move the median either', () => {
|
||||
const m = measure(parseStats(jsonl(
|
||||
...STANDARD,
|
||||
...HIGH,
|
||||
JSON.stringify({
|
||||
ts: 'm', effort: 'high', unique_sources: 900,
|
||||
dimensions: 8, dimensions_baseline: 5, empty_turns: {},
|
||||
}),
|
||||
)).records);
|
||||
assert.equal(m.excluded, 1);
|
||||
assert.equal(m.sources.treatment, 13, 'the 900-source malformed run must not reach the median');
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// measure — the known-answer test
|
||||
// ---------------------------------------------------------------------------
|
||||
|
|
@ -209,3 +242,15 @@ test('thresholds are the pre-registered 30% / 15%', () => {
|
|||
assert.equal(ADOPT_THRESHOLD, 0.30);
|
||||
assert.equal(DECLINE_THRESHOLD, 0.15);
|
||||
});
|
||||
|
||||
// The human-readable summary is the only form of the rule most readers will
|
||||
// ever see. It said "on BOTH" on both sides while decideVerdict evaluated OR —
|
||||
// so the report described a stricter gate than the one that produced the verdict
|
||||
// printed one line below it.
|
||||
test('the printed threshold line states the OR rule that decideVerdict actually applies', () => {
|
||||
const src = readFileSync(new URL('../../scripts/storm-measure.mjs', import.meta.url), 'utf-8');
|
||||
const line = src.split('\n').find((l) => l.includes('thresholds: adopt'));
|
||||
assert.ok(line, 'the summary must still print its threshold rule');
|
||||
assert.doesNotMatch(line, /on BOTH/, 'the rule is OR on both sides — printing BOTH misstates the gate');
|
||||
assert.match(line, /EITHER/);
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue