fix(tier 4): a NaN estimate did not bypass the AI budget, it poisoned it - #374
Merged
Conversation
Tier 4 of the 17.0.0 sweep, third of five slices. Every tier-4 package is now
ABSENT from scripts/lib/finite-bounds-pins.ts, which is the machine-checkable
claim that this slice closed all of it: 59 sites to 23, and the 23 are tier 5
plus three audited `ui` props.
The worst of the twelve is not a bypass. `@ultimat3/ai`'s gateway used its
pre-flight token estimate as the ceiling check AND then wrote that estimate onto
the ledger and the per-process BudgetStore. Measured: a NaN estimate passed a
1,000-token ceiling, and every later call was compared against a poisoned total,
so a 5,000,000-token call passed the same ceiling. Screened at the seam every
model call crosses, and at each declaration under the key name the DECLARATION
uses — a bound reported under the framework's internal name sends the reader to
a key their app never wrote.
Two sites were a synchronous infinite loop, past every AbortSignal and past the
job timeout, on the worker's only thread: `chunk({ size })` and `embedBatched`.
Both are pinned by tests that HANG when the screen is removed and have to be
killed, which is the honest shape of that assertion.
An ISR page with a non-finite TTL was never fresh, so it regenerated on EVERY
request — `now - generatedAt < NaN` is false, and the entry comes from a
pluggable store. Repaired totally rather than by throwing.
`cache-control` emitted `max-age=NaN`, which is not a shorter age: it is an
unparseable directive a conforming cache IGNORES, so the response fell back to
heuristic caching rather than to the declared age. Total, never a throw — this
is the response path. Every fallback goes the shorter direction, so nothing can
lengthen an age the caller did not ask for.
THREE PREMISES OF THE BRIEF WERE FALSE, and each was disproved rather than
followed. `gateway`'s baseDelayMs is deliberately NOT screened: a shipped test
asserts `baseDelayMs: -500` yields a 0 wait, so a screen there would have
refused green code — the same trap `kdf.maxConcurrent` set on the last slice.
The cache-hint fix does not belong in `@ultimat3/render`, which never
constructs a CacheHint at all (zero hits tree-wide); the boot-time half is
#373. And two of the four `ui` sites are already screened ONE HOP AWAY, so a
second screen would have given one value two error codes.
`ui` keeps a pin of 3 rather than a repair, with the reason written out:
`debounceMs` and `level` are screened elsewhere, and `rows` is the one prop
whose bad value produces neither silent zero-work nor a throw — an unparsed
`rows` attribute falls back to the element default, and throwing an X_INVARIANT
out of a render would blank a whole form over a cosmetic typo.
No new error code: every refusal is X_INVARIANT from core's finiteOption /
finiteCount, so no wiki row and no manifest change.
Issues FILED by this slice, not fixed by it: #371 (a bare parameter default is
the fourth blind spot, and randomToken(NaN) is the empty string), #372 (notify's
duration parser is a copy of time's that now DISAGREES with it, because the sweep
screened the copy and not the original), #373 (Route.cache has no
declaration-site screen).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
On-demand reviews are free for the next 25 days. After that, they cost $0.25 per reviewed file. Or wait 4 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 76 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (86)
Comment |
This was referenced Aug 26, 2026
sebyx07
added a commit
that referenced
this pull request
Aug 26, 2026
… 129 places (#378) One sweep, five slices, in tier order: #364 (tiers 0–1), #370 (2–3), #374 (4), #375 (5), #377 (the blind spots). `bun run finite-bounds` goes from 129 sites to 4, and both survivors are AUDITED pins carrying the sentence saying why screening them would be worse, not unexamined debt. THE DEFECT CLASS. `??` guards nullish and `NaN` is not nullish, so `Number(process.env.X)` on an unset variable, a parseInt of a typo and an untyped config value all walk past the default and land on the bound intact. `Math.max`, `Math.min` and `Math.floor` are not validators either — all three PROPAGATE NaN, and this repo was relying on all three as guards. What that produced, each measured rather than reasoned about: a NaN token estimate did not bypass the AI budget, it POISONED it — after one such call a 5,000,000-token request passed a 1,000-token ceiling; `awaitActionable({ timeoutMs: NaN })` ran 835,462 polls in 3 seconds against a real browser, past ctx.signal, past the watchdog, past the job timeout; `syncAuthenticator({ ttlMs: NaN })` held a revoked session across a full year of clock advance; `randomToken(NaN)` returned "" — the framework's secret generator, producing no secret and reporting success; `generateRecoveryCodes(Infinity)` wedged the process on the enrolment path, and `NaN` enrolled a user with zero recovery codes; `configureLifecycle({ deadlineMs: NaN })` made a deploy drop in-flight requests and abandon close hooks on the first tick; an ISR page with a non-finite TTL was never fresh, so it regenerated on EVERY request; `chunk({ size })` and `embedBatched` were synchronous infinite loops. THREE BREAKING ENTRIES, all the same shape: a numeric option that used to accept NaN refuses it, at boot or at the call boundary rather than mid-request. An app passing real numbers is unaffected. An app passing NaN was not working — the bound it declared was not being enforced, and nothing said so. `0` stays legal everywhere it means something: port 0 asks the OS for a free port, timeout 0 is one look, seed 0 is a seed, maxAgeSeconds 0 is "revalidate every time". THE RATCHET SHIPPED WITH THE SWEEP AND WAS WIDENED THREE TIMES BY DEFECTS THAT WALKED PAST IT — an optional chain on the object, a default read out of a table of numbers, and bare parameter defaults. All three were found the same way: by TESTING the guard rather than reading it. One clause added along the way measured inert and was deleted rather than left reading as a rule holding a line it was not holding. Its non-vacuity guard also broke because the tree got better: `total > 10` was calibrated at 129 sites, so it failed as the count approached zero. It now asserts that every PINNED package still reports exactly its pinned count. Docs made true rather than restated: CLAUDE.md said `@ultimat3/notify` had never been published and owed a hand publish before the next release run. True when written; the 16.0.0 run published it, and the audit answers 31/31 attested — so following that paragraph would have produced an E403. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tier 4 of the 17.0.0 sweep, third of five slices, in tier order. Tiers 0–1 landed in #364, tiers 2–3 in #370.
Every tier-4 package is now absent from
scripts/lib/finite-bounds-pins.ts, which is that file's own convention for "this slice closed all of it" — a count that falls is a claim; an absent row is a machine-checked one. 59 sites → 23, and the 23 are tier 5 plus three auditeduiprops.The defect class
A numeric bound whose own non-finite value makes its guard read false.
??guards nullish, andNaNis not nullish.Math.max/Math.min/Math.floorare not validators either — all three propagateNaN.The worst one is not a bypass — it is a poisoning
@ultimat3/ai's gateway used its pre-flight token estimate as the ceiling check and then wrote that estimate onto the ledger and the per-processBudgetStore.Measured, pre-fix: a
NaNestimate passed a 1,000-token ceiling — and every later call was then compared against a poisoned total, so a 5,000,000-token call passed the same ceiling. One bad request does not overspend once; it disables the budget for the process.Screened at the seam every model call crosses, and at each declaration under the key name the declaration itself uses. That second half is not cosmetic:
budget.tokensPerRunlands inBudgetLimitsasrequest, so a single screen in the ledger would emit afix:naming a key the app never wrote.Two sites were a synchronous infinite loop
chunk({ size })andembedBatched— past everyAbortSignal, past the job timeout, past the watchdog, on the worker's only thread.Both are now pinned by tests that hang when the screen is removed and have to be killed (
timeout 25→ exit 143). That is the honest shape of that assertion; a test that returns cannot express "this never returns."Four more worth naming
now - generatedAt < NaNis false, so the page is never fresh and regenerates on every request — silent unbounded origin load. The entry comes from a pluggable store, so this is reachablecache-controlemittedmax-age=NaNNaNprecache entryNaN, killing the budget warning for every other entry. Reproduced: anInfinityentry printedprecache is 0b (over 1b)capQueryRowswith a non-finite ceilingtruncated: false— an empty table reported as the complete answer — and the 256 KiB context guard switches offretentionPlan(deploys, NaN)Math.max(1, NaN)isNaN,slice(0, NaN)is[]→ evicts every deploy, including the running oneThree premises of the brief were FALSE, and each was disproved rather than followed
This is the part I want reviewed hardest, because in all three cases doing what I asked would have made the tree worse.
gateway'sbaseDelayMs/maxDelayMsare deliberately NOT screened.gateway-backoff.test.ts:74is a shipped test assertingbaseDelayMs: -500yields a0wait. AfiniteCountthere would have refused green, intentional code. Non-finiteness is already refused downstream by core'sbackoffDelay. Same trapkdf.maxConcurrent: 0set on the previous slice, avoided by grepping the tests before choosing a floor.@ultimat3/render. I routed it there on the reasoning that it belongs at the declaration site.grep -rn "CacheHint|maxAgeSeconds|sMaxAgeSeconds|staleWhileRevalidateSeconds" packages/*/srcreturns zero hits inpackages/render/—defineRoutecarriesrevalidate: { ttl, tags }and render composescache-controlitself. The boot-time half is http: Route.cache is a declaration site with no screen, so a bad cache hint is only caught on the response path where throwing is not allowed #373; looking for the declaration site is what turned up the ISR twin above.uisites are already screened one hop away —debounce()andheadingTag()both throwX_UI_INVALID_VALUE. Adding a second screen would have given one value two error codes. That is the ratchet's own documented blind spot 3 ("a repair in a different file"), working as designed.uikeeps a pin of 3, with the reason written outNot everything should be screened, and this is the case for it.
debounceMsandlevelare covered elsewhere.rowsis the only genuinely unscreened one, and it is the only prop of the four whose bad value produces neither silent zero-work nor a throw: arowsattribute the HTML parser rejects falls back to the element default — a two-row box instead of three. Throwing anX_INVARIANTout of a render would blank a whole form over a cosmetic typo. The pin carries that sentence and the one-line edit that overturns it.Floors are asserted, not assumed
Every
minis pinned by a mutation. Flipping any of them turns a test red — including three that turn a pre-existing shipped test red, which is how we know the floor is right and not merely plausible:retentionPlan(deploys, 0)— shipped testnever evicts everythingSkeleton lines: 0— shipped test asserts it renders one linehive concurrency: 0— has always meant one workertimeoutMsfloors at 1, because both drivers hand the value straight toAbortSignal.timeoutwith no zero branch, so0aborts on the next tick and is not "no deadline"Verification
bun run verify→ 14 of 20 passed, 6 skipped (drift, contract-diff, budgets, seo, i18n, policy — all gate onapp.config.tsand skip at the repo root).The first run was RED on 2 steps, and both were the predictable cost of forbidding workers to run
tsc -bon a shared tree: 5 test typecheck errors and one file over the 500-line ceiling. Both were fixed, not pinned — the gate offeredraise 'render' to 4 in scripts/lib/test-typecheck-pins.ts on purposeand taking it would have written this slice's own mistakes into the ratchet as permanent debt.bun run manifest→ no change. No new error code: every refusal isX_INVARIANTfrom core'sfiniteOption/finiteCount, so nowiki/Error-Codes.mdrow is needed.Issues filed by this slice, not fixed by it
randomToken(NaN)is"",generateRecoveryCodes(NaN)enrols a user with zero recovery codes. Public API; no in-repo caller is affected today.notify'stoDurationMsis a byte-for-byte copy oftime'stoMs, and this sweep screened the copy and not the original, so the two now give opposite answers to the sameNaN.Route.cacheis a real declaration site with no screen, so a bad cache hint is only caught on the response path where throwing is not allowed.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.