Uh oh!
There was an error while loading. Please reload this page.
ci: gate the declared Node floor against the dependency tree - #139
Merged
Conversation
The three Node floor declarations — root `engines.node`, `apps/docs` `engines.node`, and `.node-version` — are read by nothing in the install path. `.npmrc` sets no `engine-strict`, pnpm does not enforce `engines` by default, and all three workflows pin `node-version: 22` explicitly rather than consulting them. A wrong floor was therefore structurally silent: this repo's stayed wrong across the whole Node 20 era and was caught by someone reading a file, not by a red job. `check-node-floor.mjs` reduces every `engines.node` range in `pnpm-lock.yaml` to the lowest Node version satisfying it, takes the maximum across the tree, and requires each declared floor to clear it. The direction is the rule: a dependency asking for less than the declaration is the normal case and stays green. It also asserts the three declarations agree with each other. Zero-dependency, so it runs without an install. Per PR #74, `--self-test` builds fixtures and asserts each rule is demonstrably able to fail, plus that the silence-bearing rules can stay silent. It ran before the gate ever did and caught two real parser bugs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DXBoKN4MauvPdbMemPMqpr
Two things the repo's own guards surfaced after the first commit. `tools/ci-scripts/run-self-tests.mjs` fails when a script under `.github/scripts/` declares a `--self-test` dispatch but is not listed in `SELF_TESTED` — so the new gate turned the required `build` job red until it was registered. Measured: the runner exits 1 naming the file, and 0 with the script listed. That guard is exactly what it was written for. The workspace also holds a FOURTH Node declaration, not the three the card names: `tools/ci-scripts/package.json` declares `engines.node ">=20.0.0"`, disagreeing with the root and `apps/docs`. An explicit `DECLARATION_FILES` list that silently covers two of three declarations is the same structurally-silent shape this gate exists to end, so the gate now discovers every workspace `package.json` and REPORTS the ones it does not govern. The `ungoverned` rule is advisory, never blocking. "Not governed" is not a claim the value is wrong: `tools/ci-scripts` has no dependencies of its own, so `>=20.0.0` may be correct in isolation. Whether the workspace holds one floor or several is a decision for the seat, not something this script should force by going red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DXBoKN4MauvPdbMemPMqpr
os-warren
marked this pull request as ready for review
August 19, 2026 03:46
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes#127
Implements option 3 from the card: a CI check asserting the declared Node floor is consistent with what the repo's own dependency tree demands. Options 1 (
engine-strict=true) and 2 (treat the declarations as documentation) were ruled out — this PR adds noengine-strictand changes none of the declared values.All evidence below is from
7666a44, the head of this branch.The rule, and its direction
check-node-floor.mjsreduces everyengines.noderange inpnpm-lock.yamlto the lowest Node version that satisfies it, takes the maximum across the tree, and requires each declared floor to clear it.The direction is the whole rule. The defect being caught is a declared floor below what a dependency requires. A dependency asking for less than the declaration is the normal case — almost every one of the 428 entries does — and must stay green. There is a fixture asserting exactly that silence, because a rule that fired on the common case would be reverted within a day.
It also asserts the declarations agree with each other, semantically rather than byte-wise:
">=22"and">=22.0.0"are different bytes and the same floor, and a gate demanding cosmetic uniformity would be noise.Green on
maintoday, with zero headroom:Worth noting against the card's framing: the highest floor in the tree is 22.0.0, not the 20.19.0 a reading of the lockfile suggested. The declared 22 clears it exactly, with nothing to spare — the next dependency that raises its floor turns this job red, which is the point.
It is demonstrably able to go red
Per the precedent set by #74,
--self-testbuilds fixtures and asserts each case fires exactly the rules it declares, that every rule has a fixture able to trip it, and that the silence-bearing rules have a fixture proving they stay quiet. It runs as its own step before the real scan, so a rule that stopped being able to fail fails the job on its own.That was not ceremony. The self-test ran before the gate ever did and caught two real parser bugs:
>= 0.10and>= 10.*— the space-after-operator form, which this lockfile contains 40-odd times — were being read as unparseable. Unfixed, the gate would have been red onmainfor a reason that had nothing to do with the floor.Measured red paths, both from the committed tree, both restored to a byte-identical tree afterwards:
mainsurface>=20lockfilefindingswranglerfloor raised to>=24.0.0, declarations untouchedlockfilefindings--self-testThe 18 range cases pin the reduction on the exact shapes this lockfile contains, disjunctions included.
The pipefail trap, measured on this gate
The default step shell is
bash -e {0}with no pipefail, so a piped gate hands the steptee's exit status. Measured here with a deliberately-failing gate:Both
run:steps in the new job therefore nameshell: bash, as #74 did for the two existing steps.Beyond the declared file surface
The claim named
.github/scripts/check-node-floor.mjsplus a step inci.yml. One more file changed, and the reason is worth reading:tools/ci-scripts/run-self-tests.mjs— this repo already fails the requiredbuildjob when a script under.github/scripts/declares a--self-testdispatch but is not listed inSELF_TESTED. The new gate declares one, so until it was registered the runner exited 1 naming the file. That guard did exactly what it was written for; registering the script is a one-entry, mechanically-pinned change in the same gate family. Measured before: exit 1. After: exit 0, 2 self-tests passing.This also means the self-test runs twice — once in the fast
node-floorjob, once throughpnpm turbo run testinbuild. That redundancy is deliberate: thenode-floorjob has to stand on its own if it is ever made a required check, and a gate whose self-test lived only in another job could go green while unable to fail.Two findings this surfaced, filed rather than folded in
engines.node>=22claims support for 22.0.0–22.11.x, a window yargs does not support #137 — rootengines.node: ">=22"claims support for Node 22.0.0 through 22.11.x, andyargs@18.0.0(viawrangler) declares^20.19.0 || ^22.12.0 || >=23, which none of those versions satisfy. Max-of-minimums is structurally blind to a gap inside a disjunctive range: that range's minimum is 20.19.0, so it never moves the maximum. A stricter rule would catch it and is red onmaintoday, so it cannot land without also changing a declared value — out of scope here by ruling. The limitation is documented in the script header rather than left implicit.tools/ci-scriptssays>=20.0.0while the repo says 22 #138 — there is a fourth declaration, not the three the card names:tools/ci-scripts/package.jsondeclaresengines.node: ">=20.0.0". It may well be correct in isolation (that package has no dependencies of its own), so the gate does not judge it. It does report it: the gate discovers every workspacepackage.jsonand emits an advisory, never-blockingungovernednote for any declaring a floor it does not govern. An explicit file list that silently covered two of three declarations would be the same structurally-silent shape this card exists to end.Follow-up for the maintainer
Making Node floor a required status check is a repo-settings action only the maintainer can take. This PR does not and cannot do it, and nothing here should be read as claiming the check is enforced on merge. Until then the job runs and reports on every PR, push to
main, and merge-group entry.Notes
No changeset — this repo has no changeset flow.
pnpm turbo run testwas not run through turbo (that needs an install); the task's script,tools/ci-scripts/run-self-tests.mjs, was run directly with node and is reported above.Generated by Claude Code
Generated by Claude Code