fix(tier 2–3): at-least-once became never when a lease bound was NaN - #370
Conversation
…once became never when a lease bound was NaN Measured on createMemoryDriver: Number(process.env.JOB_VISIBILITY_MS) on an unset variable is NaN, ?? guards only nullish, and Math.max/Math.min/Math.floor propagate it — so visibleAt = at + NaN, the reclaim scan asks visibleAt <= at, and a job whose worker died is never claimable again. No error, and x jobs ls still prints the row as running. Its twin: slice(0, NaN) is [], so a concurrency: NaN worker claims nothing and reports healthy. Every numeric option is now read and refused in one place per subject — jobs/worker-options.ts, realtime/sync-node-bounds.ts, auth/policy-numbers.ts — each delegating to @ultimat3/core's finiteOption/finiteCount. sync-node-bounds.ts also takes sync-node.ts off the 500-line ceiling. Fixes #368. Both dbDrift() twins spliced a column name into a shell-double-quoted x db gen argument, where $(…) substitutes. The screen is promoted out of drift-findings.ts to shellInertIdentifier() in db/sql.ts rather than copied: identifier() answers about SQL and ACCEPTS a backtick and a dollar sign, so reusing it alone would have shipped a green suite over a live hole — that measurement is now a shipped test. db's constructor moved to drift-errors.ts because errors.ts cannot import sql.ts without cycling around the module that registers every code; the manifest row and the public surface are unchanged. Every benign rendered literal is byte-identical across seven names, so no doc quoting them moved. The ratchets this slice repaired were lowered in the same commit, which is X_FINITE_BOUND_PIN_STALE working: finite-bounds 129 -> 59 sites, seven packages unpinned to zero; proto-index's realtime row deleted. `bun run verify` green on this slice alone, built from merged main: 14 of 20 steps, 6 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QVodtwtGAKyVC1SxvjZ6Mp
|
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 50 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 77 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 (116)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
|
|
CI is green ✅ — no risk signals detected. Ready for maintainer merge. 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
|
CI is green and no risk signals detected. Looks ready to merge — consider applying a 🤖 AI review by developerz.ai[bot] 🤖 Posted by developerz.ai — the maintainer agent, not a human. |
…ards that claimed more than they checked
The review round on this slice. Every behavioural fix is mutation-proven — the fix is
broken, the test is watched to go red, and restored.
`search().page({ first })` refused page ONE. The screen fired on the framework's own
defaults: `limit` defaults to 20, `first` has none, so `search({…}) + .page(input,
{ first: 10 })` was a 500 with nothing misdeclared. A screen that fires on its own
defaults is not a screen. It now refuses only when rows would actually be CUT, and when
the page fits `hasNextPage` is false by construction, so no cursor is minted that a
second call is guaranteed to throw on.
`trustedProxyHops: 0` read as a configured value and trusted nothing — `forwarded.ts`
returns undefined for `hops < 1`. The floor is 1, and the unreachable `?? 0` beside it is
removed rather than left reading as a live default.
`kdf.maxConcurrent: NaN` parked every `hashPassword` in a queue nothing releases, so
login stopped answering rather than shedding. The floor is 0 and not 1: a shipped test
uses `maxConcurrent: 0` as a deliberate zero-width gate.
Two guards claimed a reach they did not have.
`finite-bounds` matched `?? DEFAULT_TTL_MS[input.purpose]` with its pattern and then threw
it away in the numeric filter, because the declaration spans lines and the single-line
value capture reads `{`. A table of numbers is now its own set — kept apart from the
scalar set deliberately, since folding it in would report `o.opts ?? DEFAULT_OPTS`, an
object default, and a false report is how a rule gets switched off. A `\]` clause added to
the same regex the same day measured INERT — 59 sites with it and without, because an
identifier after a `]` is reached through `.` either way — so it is deleted rather than
left reading as a rule holding a line it is not holding.
`changelog-check` summed `[Unreleased]` into a total whose own row says "every major
section". Every PR landing a breaking change turned it red, and the repair was a number
the next release invalidates. It counts released sections now, and the documented command
reproduces it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round: 6 defects, all fixed, all mutation-provenThe automated reviewer was rate-limited on this PR (77 reviews in 7 days → 1/hour) and the The one that mattered most
The screen fired on the framework's own defaults: The premise the screen was built on also does not hold — It now refuses only when rows would actually be cut, naming both edits. When the page The rest
Two guards that claimed more than they checkedBoth found by testing the guard rather than reading it.
A
Verification
Routed onward rather than widened into this slice
|
… 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>
1 participant
Tier 2–3 of the 17.0.0 sweep
Second of five, built from a
mainthat already carries #364 — not rebased, not stacked. Every package here adoptsfiniteOption()/finiteCount()from@ultimat3/corerather than carrying its own copy.The worst instance in the sweep: at-least-once became never
packages/jobs/src/worker-options.tsstates it in its own header, and it was measured rather than reasoned:Its quieter twin:
slice(0, NaN)is[], so aconcurrency: NaNworker claims nothing and reports healthy.One place per subject
jobsworker-options.tscreateWorkeraccepts, the slot table included — a queue name is data, a slot count is a bound, and both arrive from the same deployment configrealtimesync-node-bounds.tslistenSyncNodeaccepts.maxConnections: NaNis a node that accepts without limit and says nothing — theAcceptBudgetadmits the whole herdauthpolicy-numbers.tsdefineAuthresolves, plus the options arriving on a call rather than through config:jwks, both OAuth legs, TOTP drift, the limiter's key capsync-node-bounds.tsis also a split —sync-node.tswas at the 500-line ceiling thefilesizestep enforces.auth's refusal raisesX_CONFIG_INVALID, which only answers with a row because #364 added one. Landing these in the other order would have shipped a window where the refusal existed and the status did not. That is what "tier order is the split order" buys.packages/auth/src/auth-fixture.tscollapses three private copies of one credential-flow fixture — cheap KDF parameters, the shared password, theAuthErrorcatcher. Three copies is three chances for one to drift from what the flow actually enforces. Not re-exported from the barrel; it is not public API.The ratchets this slice is obliged to lower
Both fired on the first run, which is them working:
finite-bounds: 129 → 59 sites.X_FINITE_BOUND_PIN_STALEon seven packages the moment their repairs landed, because a pin above what the tree contains would let that many back in. Lowered with the--unpinthe error itself names:action,auth,entity,http,jobs,query,realtime, all to zero — so all seven leave the table entirely, since a row claiming a debt of zero reads as a rule still in force over nothing.proto-index: therealtimerow deleted, its fix being in this slice.scripts/finite-bounds.test.tsnames every swept package individually rather than counting them, and a name may only ever be added to that list. It now carries tiers 0 through 3. Tier 4 and tier 5 add their own bands as they land.Fixes #368 — the twin that could not be fixed until now
Two
dbDrift()declarations, pinned to each other by a "keep in sync" comment, both builtx db gen "add ${columnName}"— shell double quotes, so$(…)and backticks substitute, and a column named$(id)yields afix:line that executesidwhen pasted. Byte-identical in shape to the hole closed indrift-findings.tson #364.It could not go in #364: the twins live in
packages/db(tier 1) andpackages/entity(tier 2), and a one-sided fix makes the "keep in sync" comment a lie. This slice is the first tree where both are editable in one commit.db/errors.tscould not take the fix in place, and that was not foreseen:sql.tsimportsidentifierUnsafefromerrors.ts, so importing the screen there would close an import cycle around the module whose evaluation callsregisterErrorCodes().packages/db/CLAUDE.mdrefuses exactly that in as many words.dbDrift(table, column)is public API shipped since 1.0 so its signature cannot change, which rules out the escapedependent-view.tsuses (handerrors.tsa finished string). The constructor moved topackages/db/src/drift-errors.tsinstead — the patternmigration-errors.tsandinvariant-errors.tsalready follow.X_DB_DRIFTstays declared, titled and registered inerrors.ts, andcollectErrorCodes()still answersat: packages/db/src/errors.ts, so the manifest row is unchanged and@ultimat3/db's public surface is byte-identical.The screen is promoted, not copied — a second copy of a security screen is the
sql-literal-copiesstory, where three copies shipped and two were wrong the same way. It matters that the screen is not justidentifier(): that function answers about SQL and accepts a backtick and a dollar sign (SAFE_IDENTIFIEReven allows$on its fast path), which are exactly the two characters a shell substitutes inside double quotes.Every benign rendered literal is byte-identical, verified end-to-end across seven names (
publish_at,id,orgId,a1,_x,o'brien, 63×x) against the formulas taken verbatim fromHEAD, over all six affected sites. So none of the ~ten files quotingx db gen "add <name>"inpackages/cli,packages/core,wiki/anddocs/needed to move. Mutation-checked four ways, including rewording a benign literal — which goes red on both sides — and reverting entity's site alone, which goes red on the cross-package test asserting the two answer with the same text. "Keep in sync" is now asserted rather than asked.Latent rather than live — neither twin has a runtime caller carrying a catalog-supplied name; the only caller is
packages/cli/src/templates/entity.ts, which the CLI writes and never runs, with the literal'id'.Verification
bun run verifygreen on this slice alone, built from mergedmain.any, noas any, no@ts-expect-error, nobiome-ignore.🤖 Generated with Claude Code
https://claude.ai/code/session_01QVodtwtGAKyVC1SxvjZ6Mp
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.