Skip to content

fix(plan): an out-of-range alpha priced every case at the per-case cap - #45

Open
sferarc-hq[bot] wants to merge 2 commits into
mainfrom
fix/sprt-error-rate-guards
Open

sferarc-hq[bot] wants to merge 2 commits into
mainfrom
fix/sprt-error-rate-guards

Conversation

@sferarc-hq

@sferarc-hq sferarc-hq Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

What broke

sprtExpectedN is the SPRT costing primitive the whole budget layer runs on. It never checked its two error rates, and outside (0, 1) it does not return a wrong number, it returns a non-finite one: NaN for an alpha at or above 1, because log(beta / (1 - alpha)) is the log of a negative number, and Infinity for an alpha of 0.

Both of its callers read that the same way:

const raw = sprtExpectedN(p, p0, p1, cfg.alpha, cfg.beta);
const n = Number.isFinite(raw) && raw > 0
  ? Math.min(cfg.maxTrials, Math.max(cfg.minTrials, Math.ceil(raw)))
  : cfg.maxTrials;

NaN and Infinity both fail that test, so both fall back to the per-case cap. Neither makePlan nor evaluatePoint validated alpha or beta, though both validate mde, fdr, pairCoupling, cases, baselineRuns and maxTrials. The result is a complete plan, with no error anywhere, in which every case is priced at maxTrials. Measured on main with makePlan over three cases at 51/60, 55/60 and 48/60 and { ...DEFAULT_PLAN, alpha }:

default        totals.typicalRuns 192
alpha=2        totals.typicalRuns 576
alpha=0        totals.typicalRuns 576
beta=1         totals.typicalRuns 576
beta=-1        totals.typicalRuns 576
alpha=NaN      totals.typicalRuns 576

Every one of those returned a Plan object. Three times the expected bill, and a reader has no way to tell it apart from a suite that genuinely needs the cap. That is the same symptom as #26 ("sprtExpectedN returned negative run counts ... so affected cases were priced at the per-case cap") from a second cause.

Two siblings had the same gap:

  • sprtDecision put lower at NaN for an alpha at or above 1. Every comparison against a NaN is false, so the test reported CONTINUE for ever against a wall that is not a number. An alpha of 0 puts upper at Infinity, which no evidence ever crosses.
  • sampleSizeTwoProportion at an alpha of 1 sets zA to normalQuantile(0.5) = 0, which drops the type I error term out of the formula. sampleSizeTwoProportion(0.8, 0.15, 1) answered 29 runs for a design with no error control in it. Its own doc comment calls this "the 'naive approach' peeksafe is measured against", so a plausible number here understates what the naive design costs.

And one defect that the guards do not cover, found while writing them: sprtExpectedN returned NaN for alpha + beta === 1, which is inside the domain. Both walls then sit at a log likelihood ratio of 0, so Wald's operating characteristic (1 - B^h) / (A^h - B^h) is a genuine 0/0. Only the pairs where both logs round to exactly 0 in float64 were hit, which is why no guard on the arguments would have caught it: (0.5, 0.5), (0.25, 0.75) and (0.125, 0.875) returned NaN while (0.05, 0.95) escaped on a rounding residual.

Why it is a defect and not a judgement call

src/stats.ts's own module header states the contract these five functions broke:

Domain discipline. Every entry point that can be handed nonsense (successes > trials, a negative count, a probability outside [0,1]) throws a PeeksafeError with code PEEKSAFE_E_STAT_DOMAIN rather than returning NaN or a plausible-looking number. Returning Infinity is reserved for questions whose honest answer is "no finite sample size will do".

brain/architecture/overview.md records that this is the class most merged fixes have been, naming #21, #23, #25 and #27, and src/errors.ts already carries the guard these needed. makePlan and evaluatePoint disagreeing on the same options object is the drift requirePositiveConfig's own doc comment exists to describe.

The fix

Five guards and one closed form, no new helper:

  • requireOpenProbability on alpha and beta in sprtDecision, sprtExpectedN and sampleSizeTwoProportion (src/stats.ts), and on cfg.alpha and cfg.beta in makePlan (src/plan.ts) and evaluatePoint (src/frontier.ts), alongside the mde and fdr checks already there. Open at both ends: an error rate of 0 is a test that never decides and one of 1 is a test that always does.
  • if (A === B) return 0 in sprtExpectedN, before the tilt is computed. A === B is alpha + beta === 1, where the general form (L1 * (A - B) + B) / drift is 0 whatever L1 is. The neighbouring configurations already converge to 0 from both sides, which the test pins.

A 0 still trips the callers' Number.isFinite(raw) && raw > 0 test and falls back to the cap, which is the right conservative answer for a test that decides before it has seen any data. It now does so on a number rather than on a NaN.

No statistic changed for any in-domain input, so no paper.test.ts number and no README figure moved. No runtime dependency, no relaxed refusal, nothing published or tagged.

What proves it cannot break the same way

Four new cases, three files. All five assertion groups fail on main and pass here:

  • test/budget.test.ts, refuses an out-of-range alpha or beta rather than quoting every case at the cap: makePlan must throw on 0, 1, -1, 2, NaN and Infinity for each. It also asserts that the default config prices these cases strictly under cases * maxTrials, so the cap fallback is pinned as a materially different answer and the test cannot be satisfied by a plan that always quotes the cap.
  • test/budget.test.ts, refuses an out-of-range alpha or beta, the same as makePlan: the same six values through computeFrontier.
  • test/stats.test.ts, refuses an error rate outside (0,1) rather than deciding against a NaN wall: the same six through sprtDecision and sprtExpectedN, plus assertions that the defaults still give finite walls and a positive expected sample number, so the guard cannot have been satisfied by tightening the domain.
  • test/stats.test.ts, sampleSizeTwoProportion refuses an error rate outside (0,1), which also pins that an effect the case cannot suffer is still Infinity rather than a refusal: that is an answer about the design, not about the arguments.
  • The alpha + beta === 1 half is pinned by the three pairs that returned NaN, each now 0, plus two neighbours that must come in under 1e-3 so the limit is approached and not jumped to.

On main these report expected [Function] to throw an error and expected NaN to be +0.

brain/architecture/planner.md gains two lines under "Things that have been wrong before", next to the #26 entry with the same symptom.

Checks

Run on this runner, all green:

  • npm run typecheck
  • npm test (12 files, 240 tests, up from 236 on main)
  • npm run build

Not run here and left for CI: Node 22, since this runner is Node 24.21.0, and the packed-tarball dependency check in .github/workflows/ci.yml. There is no Docker daemon on this runner, though nothing in this repository's checks needs one.

Not in flight, and what it overlaps

I read the whole open pull request queue with gh api repos/sferarc/peeksafe/pulls --paginate and no row limit: 5 rows, #37 release/0.1.0, #39 chore/pnpm-node26, #40 chore/biome-lefthook, #42 fix/plan-affordability-grid-baseline, #44 ci/hq-stack-check. A scoped search for sprt returns only #42, and for alpha returns nothing. There are no open hq-queue issues and no issue describes this defect, so there is none to reference; the files changed are named above for the next shift's search.

Two overlaps a person resolving should know about:

Blocked reviews I could not answer, which outranked this

The shift brief puts a blocked review above anything I pick myself, and gh pr list --label hq-changes-requested returns two: #39 and #40 (draft, stacked on #39). I could not answer either. Every blocking point in the #39 review lands in .github/workflows/release.yml:

  1. the deleted pinned npm install and the unsourced claim that pnpm 12.8.1 does npm's OIDC trusted publishing,
  2. the token moving into a $HOME/.npmrc credential file,
  3. pnpm view making the "version already published" gate one that cannot fail.

My charter forbids pushing to .github/workflows/, and this token holds no workflows permission, so GitHub would refuse the push regardless. A doc-only commit would have dropped the hq-changes-requested label and sent #39 back for review with all three blockers still in place, which is worse than leaving it labelled, so I left both untouched. There is a request for a human in my closing message. Nothing else in the queue carries that label.

Follow-ups I found and did not take

Three more exported primitives in src/stats.ts fail the same module-header contract on an optional tuning argument. All are hardening rather than reachable budget errors, since every internal caller passes a valid value, which is why I left them out rather than widening this diff. Measured on this branch:

Call Answer
wilsonInterval(5, 10, -2) { low: 0.767, high: 0.233 }, an inverted interval
wilsonInterval(5, 10, NaN) { low: NaN, high: NaN }
betaCredibleInterval(betaPosterior(5, 5), 2) { low: 0, high: 1 }, the whole line at 200% credibility
betaCredibleInterval(betaPosterior(5, 5), -1) { low: 1, high: 0 }, inverted
probabilityMoved(51, 60, 20, 30, -0.5) 1, the probability of a drop of minus 50 points
probabilityMoved(51, 60, 20, 30, 0.15, 2.5) 1, from a Simpson's rule with 3.5 nodes

src/cluster.ts already has the guard the second row wants, as its own requireLevel, with a hint saying "a two-sided confidence level such as 0.95, not a percentage and not an error budget". pairedDiscordance already has the guard the third row wants on its own mde, and probabilityMoved is the last exported entry point taking an mde without one. A z needs a guard no helper covers yet: finite and at or above 0, since a z of 0 is a coherent degenerate interval.

`sprtExpectedN` answers NaN for an error rate at or above 1 and Infinity
for one of 0. Both of its callers gate on
`Number.isFinite(raw) && raw > 0` and quote `maxTrials` when that fails,
and both answers fail it, so `makePlan` and `computeFrontier` returned a
complete plan with every case at the cap and no error anywhere. Neither
entry point validated `alpha` or `beta`, though both validated `mde`,
`fdr` and `pairCoupling`.

`sprtDecision` had the same gap with a louder symptom: an alpha at or
above 1 makes `log(beta / (1 - alpha))` the log of a negative number, so
`lower` came back NaN and the test reported CONTINUE for ever against a
wall that is not a number. `sampleSizeTwoProportion` at an alpha of 1
sets its own critical value to 0 and answers a run count for a design
with no error control in it.

Separately, `sprtExpectedN` returned NaN for `alpha + beta === 1`, which
is inside the domain: both walls sit at a log likelihood ratio of 0 and
Wald's operating characteristic is a genuine 0/0. The answer is 0 runs,
which is what the neighbouring configurations converge to.

No statistics moved, so no README or `paper.test.ts` figure moved.
@sferarc-hq

sferarc-hq Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Review: fix(plan) out-of-range alpha priced every case at the per-case cap

Read against main. I could not fetch this branch here (git fetch is not on the shepherd allowlist) and there is no Docker daemon on this runner, so npm test, npm run typecheck and npm run build on this head are CI's to confirm. Everything below is the diff read against the base tree, plus arithmetic I did by hand where a claim was checkable.

The two defects are real and the diagnosis of both is right

The guards. sprtExpectedN computes A = log((1 - beta) / alpha) and B = log(beta / (1 - alpha)). At alpha >= 1 the second is the log of a negative number, so B is NaN and the quotient is NaN; at alpha === 0 the first is Infinity. Both callers, src/plan.ts:378 and src/frontier.ts:279, read the answer through Number.isFinite(raw) && raw > 0 ? ... : cfg.maxTrials, and NaN and Infinity both fail that, so the plan came back complete with every case at 96 observations and nothing anywhere saying it was a fallback. That is the same silent-cap failure as #26, from a different cause, and the fix is the same shape: refuse at the entry point rather than let a non-finite answer be read as "cannot price this".

requireOpenProbability is the right guard and PEEKSAFE_E_STAT_DOMAIN is the right code: src/check.ts:54 and src/check.ts:166 already validate alpha exactly that way, so this closes a drift rather than inventing a convention. Strictly open at both ends is correct and the inline comment gives the reason in one line ("an error rate of 0 is a test that never decides and one of 1 is a test that always does").

The A === B NaN. The algebra checks out exactly: A === B iff (1 - beta)(1 - alpha) === alpha * beta iff alpha + beta === 1, and at that point both logs are exactly 0, not merely close to it. So the guard's condition is equivalent to the configuration it claims to catch, which is why testing A === B rather than alpha + beta === 1 is the better test here: it catches exactly the pairs where the arithmetic actually degenerates.

At A === B === 0, sprtAcceptH1 computes a = b = 0, m = 0, and returns (exp(0) - exp(0)) / (exp(0) - exp(0)), a true 0/0. NaN confirmed, for an in-domain configuration the new guards cannot catch, which is exactly why it needed its own branch.

I also checked that the guard does not leave a ring of NaN around itself, which is the obvious way a fix like this goes wrong. For a pair that sums to 1 in decimal but not in binary, B comes out exactly 0 and A comes out a few times 2^-53, so A !== B and the general path runs. Worked by hand at p = 0.5, p0 = 0.6, p1 = 0.45: at (0.1, 0.9), A is about -2.8e-16 and m === a, so the numerator of sprtAcceptH1 is a difference of one value with itself, L1 is 0, and the result is B / drift === 0. At (0.3, 0.7) the signs flip and it is -0 and 0 again. Both land on the same 0 the guard returns, so the function is continuous across the knife edge rather than discontinuous on the pairs the guard misses.

Returning 0 is also the defensible answer rather than a convenient one: both walls sit where the test starts, so it decides before it has seen anything, and the neighbours converge to it. I checked the two the test pins: at (0.5, 0.499) the general form gives about 4.2e-5 and at (0.5, 0.501) about 3.9e-5, both comfortably under the asserted 1e-3 and both positive.

Placement and blast radius

  • if (A === B) return 0; sits after if (second === 0) return Infinity;, which is what preserves test/exports.test.ts:212 and :213, where p0 === p1 must still answer Infinity. Had it gone first, those two would have flipped.
  • sampleSizeTwoProportion's guard sits before the delta <= 0 || delta >= p0 branch, deliberately, so the same error rate is refused whatever the effect. The README figure at README.md:311 ("sampleSizeTwoProportion(0.1, 0.15) is Infinity rather than a plausible-looking 44") uses the defaults, so it is unaffected, and the new test pins that exact call so it stays that way.
  • No new refusal can reach gate or shouldStop. sprtDecision, sprtExpectedN and sampleSizeTwoProportion are called only from plan.ts, frontier.ts and the tests, so nothing in the decision path gains a throw.
  • enumerateFrontier does not catch per point, so evaluatePoint's throw propagates out of computeFrontier, which is what the new frontier test depends on.
  • No test was relaxed, no refusal in gate.test.ts was touched, no runtime dependency was added, and no README figure moved.
  • The tests are not assertion-free and not satisfiable by deletion: the refusals are matched on the error code rather than bare toThrow(), the toBe(0) on three knife-edge pairs would fail on a -0 (vitest's toBe is Object.is), and budget.test.ts adds expect(plan.totals.typicalRuns).toBeLessThan(cases.length * DEFAULT_PLAN.maxTrials) so that "the fallback is materially different" is pinned rather than asserted in a comment.

Four follow-ups, none of them a reason to hold this

  1. The guard is per-rate, not joint, so alpha + beta > 1 is still accepted and still prices silently. At (0.6, 0.6) the two walls cross: A is -0.405 and B is +0.405, the upper wall below the lower one. sprtExpectedN then answers a small positive number rather than NaN, so it clears raw > 0, and both callers clamp it up to minTrials. A plan on crossed decision walls therefore reports 8 observations a case where the honest figure is around 30: the same class of silent wrong answer this pull request exists to remove, pointing at the floor instead of the cap. It is pre-existing and this change does not make it worse, which is why it is a follow-up and not a blocker, but it is the obvious next one and the brain note would be worth extending when it is done.
  2. second === 0 is checked before A === B, so p0 === p1 together with alpha + beta === 1 answers Infinity ("no number of runs ends the test") for a configuration whose walls say it decides at zero runs. No caller can tell the two apart, since both trip the > 0 fallback, so this is a note for whoever reads the function next rather than a defect.
  3. The public doc comment on sprtExpectedN does not mention that it can now return 0. sprtExpectedN is an exported part of the contract (README.md:429), the new behaviour is recorded in an inline comment, the brain note and the test, and the two in-tree callers gate on > 0, but a third-party caller doing Math.ceil(raw) would read 0 as free. One line in the doc comment above the function would close that.
  4. CHANGELOG.md is untouched and has an empty ## Unreleased section. Five exported functions gain a refusal they did not have in 0.1.0, which is a notable change by the file's own stated policy. CONTRIBUTING.md does not require an entry, and the same is true of fix(plan): the affordability grid priced a suite with no baseline #42, so I am raising it against the batch rather than against this diff: worth one entry covering both before the next release.

Separately, and nothing to do with this change: planCase computes fixedNPerArm as sampleSizeTwoProportion(Math.max(1e-6, rate), cfg.mde) (src/plan.ts:280), passing neither cfg.alpha nor cfg.beta, so the fixed-sample comparator in every plan is priced at 0.05 and 0.1 whatever the caller configured. After this change makePlan validates two numbers that one of its own figures then ignores. Worth its own issue.

VERDICT: GO both fixes are correct at the point I checked the arithmetic, they use the guard and error code the rest of the library already uses for alpha, they cannot reach gate or shouldStop, and the tests pin the refusals by error code plus the 0 that replaces the NaN and the fact that the cap fallback was a materially different answer.

@sferarc-hq sferarc-hq Bot added the hq-reviewed Read adversarially by the reviewer shift; the merge gate requires it label Oct 3, 2026
…uards

Keep both sets of planner.md bullets: the alpha and beta guards from this
branch and the baselineTrials refusal from main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HaKqfnv3eoTxHPZ8t5SZ2L
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hq-reviewed Read adversarially by the reviewer shift; the merge gate requires it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants