Skip to content

chore: pin the dev Node floor at 22.12, distinct from the consumer floor - #47

Open
dangrondahl wants to merge 1 commit into
mainfrom
chore/dev-node-floor-22-12
Open

chore: pin the dev Node floor at 22.12, distinct from the consumer floor#47
dangrondahl wants to merge 1 commit into
mainfrom
chore/dev-node-floor-22-12

Conversation

@dangrondahl

@dangrondahl dangrondahl commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Follows #31 (vitest 4), now merged — so the dev floor described below is the one in effect on main today.

Addresses the two open points from the review on #31.

The CI matrix wasn't testing the floor

This is the substantive one. node-version: '22' resolves to the latest 22.x, so the band that actually breaks — 22.0–22.11, which Vite 8 rejects — was never exercised. The matrix looked like it covered the bottom of the supported range and didn't.

Pinned to '22.12', so the tested floor is the real floor.

Two floors, on purpose

value why
Consumer floor engines.node >= 22 what the published server needs
Dev floor >= 22.12 vitest 4 → Vite 8 (^20.19.0 || >=22.12.0)

Deliberately not raising engines.node: it constrains consumers, and nothing under devDependencies ships (files: ["dist"]). Agreeing with the review on this — tightening the published floor for a test-only dependency would misrepresent what users need.

Made discoverable rather than found via a mystery EBADENGINE:

  • .nvmrc pinning 22.12 (nvm use picks it up)
  • A prerequisites line in CONTRIBUTING.md, right above the npm install block, stating both floors and what goes wrong below 22.12

@types/node^22.12.0

Vite 8 declares "@types/node": "^20.19.0 || >=22.12.0" as a peer. ^22.0.0 permitted versions outside that. Resolved version is unchanged at 22.19.17 — this only tightens the declared range, keeping it consistent with CLAUDE.md's rule that @types/node tracks the floor.

CLAUDE.md

Records the two-floor distinction explicitly, so nobody "fixes" the apparent inconsistency by raising engines.node. The "Raising the Node floor" checklist gains .nvmrc and the CONTRIBUTING note.

Verification

npm run build clean, npm test green (67 tests), lockfile regenerated with npm install --package-lock-only — one line changed, no dependency re-resolved.

vitest 4 (#31) pulls Vite 8, which requires ^20.19.0 || >=22.12.0. The
consumer floor stays engines.node >=22 — that field constrains users, and
nothing under devDependencies ships — but development now needs 22.12.

The CI matrix said '22', which resolves to the latest 22.x, so the bottom
of the range was never actually tested: the one band that breaks
(22.0-22.11) was invisible to CI. Pinned to '22.12' so the tested floor is
the real floor.

@types/node moves to ^22.12.0 to match Vite 8's peer range
(^20.19.0 || >=22.12.0); ^22.0.0 permitted versions outside it. The
resolved version is unchanged at 22.19.17.

.nvmrc and a CONTRIBUTING prerequisites note make the dev floor
discoverable rather than something found via a mystery EBADENGINE.
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Claude finished @dangrondahl's task in 2m 50s —— View job


Review complete

  • Gather context (diff, CLAUDE.md, related files)
  • Verify the two-floor claim against package.json / manifest.json / README.md / CI
  • Check CI matrix pin and .nvmrc correctness
  • Post review

No src/ changes, so none of the architectural invariants are in play — three tools, generated catalog, non-throwing KosliClient, compact JSON.stringify, org fallback, User-Agent, ESM .js imports, strict: true, no HTTP client, and the readOnlyHint/destructiveHint split are all untouched. Commit message and PR title are Conventional Commits, manifest.json's version placeholder is untouched, and the lockfile change is a single declared-range line with nothing re-resolved, as claimed.

What's genuinely good here

The core insight is correct and non-obvious: node-version: '22' resolves to the latest 22.x, so a matrix that looked like it covered the bottom of the supported range covered only the top. That's exactly the kind of thing that stays invisible until a user reports it. Keeping engines.node at >=22 instead of raising it for a test-only dependency is the right call and correctly reasoned (files: ["dist"] — devDependencies don't ship), and README.md / manifest.json were correctly left alone as a result. Writing the two-floor distinction into CLAUDE.md so nobody "fixes" the apparent inconsistency later is good instinct, and .nvmrc + the CONTRIBUTING note turn a mystery EBADENGINE into something discoverable.

Findings

Three inline comments, none blocking:

  1. .github/workflows/ci.yml:20-23 — the unchanged job-level comment on lines 13–14 ("22 is the floor declared in package.json engines") is now stale and contradicts the new comment two lines below it. Also flags that this makes the dev floor tested; the consumer floor (22.0–22.11, still allowed by engines.node) stays untested and can't be tested by this job.
  2. CONTRIBUTING.md:7 — the paragraph is factually false until chore: bump vitest from 3.2.4 to 4.1.10 #31 lands (vitest is still ^3.1.0 here, so no Vite 8 and no >=22.12 requirement). The PR body flags this; the comment records the concrete consequence and notes update-catalog.yml still uses '22', which CLAUDE.md's own checklist lists.
  3. package.json:45 — the @types/node bump changes which floor it tracks, in the opposite direction from the rest of the PR. CLAUDE.md's stated rationale ("tsc accepts APIs that don't exist on the minimum runtime") is about the consumer floor; Vite's @types/node peer is optional, the resolved version is unchanged, and a caret range never pinned the floor's type surface anyway. The CLAUDE.md sentence should say which floor it follows now that there are two.

One suggestion beyond the diff

22.12 is now hardcoded in four places — .nvmrc, the CI matrix, CONTRIBUTING.md, CLAUDE.md — guarded only by CLAUDE.md prose. That's the same failure mode as the version drift that shipped 0.1.0 through release 0.5.0, and this repo already solved that with a test (test/version.test.ts). A small test/node-floor.test.ts reading .nvmrc, package.json#engines.node, and the node-version line out of ci.yml, asserting .nvmrc equals the matrix low end and satisfies engines.node, would make the invariant this PR documents actually enforced. Given the PR's whole purpose is preventing silent drift, it seems worth the ~15 lines. Fix this →

Two process notes: .github/workflows/ci.yml is in the diff, so per repo convention the claude-review.yaml self-validation rule doesn't apply here — but I can't modify workflow files myself, so the ci.yml suggestion above needs a human to apply. I also could not run npm ci / npm test in this run (install wasn't permitted), so I'm taking the "67 tests green" verification in the PR body at face value; nothing under src/ or test/ changed, so there's no new behaviour for the suite to cover.

Verdict: the substance is right and I'd merge it — after #31, as the PR body says, since the docs describe a floor that isn't real until then.
· branch chore/dev-node-floor-22-12

Comment thread package.json
},
"devDependencies": {
"@types/node": "^22.0.0",
"@types/node": "^22.12.0",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bump quietly changes which floor @types/node tracks, and it's the opposite direction from the rest of the PR.

CLAUDE.md's rule is "@types/node tracks the floor, not the newest release — otherwise tsc accepts APIs that don't exist on the minimum runtime." That rationale is about the consumer floor (22.0): a published server type-checked against 22.12 types can call something absent on a consumer's 22.0.x. Everywhere else in this PR you argue devDependency constraints shouldn't tighten what consumers see; here a devDependency's optional peer range (Vite declares @types/node under peerDependenciesMeta as optional, so a mismatch is a warning, not a failure) is what moves the declared range up.

Two things worth deciding explicitly rather than leaving implicit:

  1. Is the tightening buying anything? The resolved version is unchanged at 22.19.17 and satisfies both ranges, so this only narrows a range that was never the binding constraint.
  2. A caret range doesn't pin the floor anyway. ^22.0.0 already resolved to 22.19.17 — tsc has been seeing 22.19 types all along. If the CLAUDE.md rule is meant literally, only ~22.0.0 (or an exact pin) enforces it; if it isn't, the line deserves a hedge.

Whichever way you go, the CLAUDE.md sentence should now say which of the two floors @types/node follows, since the PR makes that ambiguous for the first time.

Comment thread .github/workflows/ci.yml
Comment on lines +20 to +23
# '22.12' rather than '22': the dev toolchain (Vite 8, via vitest) needs
# >=22.12, and a bare '22' resolves to the latest 22.x, so the bottom of
# the supported range would never actually be tested.
node-version: ['22.12', '24']

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unchanged job-level comment on lines 13–14 is now stale and contradicts this one: "22 is the floor declared in package.json engines" was true when the low end was '22', but engines.node is >=22 while the matrix low end is now 22.12. Reading top-down you learn the low end tracks engines, then two lines later learn it doesn't. Worth folding both into a single comment that states the real invariant:

Suggested change
# '22.12' rather than '22': the dev toolchain (Vite 8, via vitest) needs
# >=22.12, and a bare '22' resolves to the latest 22.x, so the bottom of
# the supported range would never actually be tested.
node-version: ['22.12', '24']
# Low end is the dev floor (22.12), not the consumer floor
# (engines.node >=22): the dev toolchain (Vite 8, via vitest) needs
# >=22.12. Pinned rather than a bare '22' because '22' resolves to the
# latest 22.x, so the bottom of the range would never be tested.
node-version: ['22.12', '24']

(and drop the now-redundant "22 is the floor declared in package.json engines" sentence above).

Separately — worth being precise about what this buys, since the PR frames it as "the tested floor is the real floor". It makes the dev floor tested. The consumer floor (22.0–22.11, still allowed by engines.node) remains untested, and can't be tested by this job, because the dev toolchain won't run there. That's not a regression (a bare '22' never tested 22.0 either), but the gap this PR names as the problem still exists one level down for consumers.

Comment thread CONTRIBUTING.md

## Getting started

**Node ≥ 22.12** (`.nvmrc` pins it — `nvm use` picks it up). The published server only needs Node ≥ 22, but the dev toolchain is stricter: vitest pulls Vite 8, which requires `^20.19.0 || >=22.12.0`. On Node 22.0–22.11 `npm install` reports `EBADENGINE` and the test run is likely broken.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph is not true on this branch yet — package.json still has "vitest": "^3.1.0", so there is no Vite 8 and no >=22.12 requirement. On today's main + this commit, npm install on Node 22.5 emits no EBADENGINE and the tests run fine. The PR body already flags this ("merge after #31"), so this is just the concrete consequence: the docs are wrong for exactly as long as this sits ahead of #31.

Two follow-ons worth confirming at merge time rather than now:

  • If chore: bump vitest from 3.2.4 to 4.1.10 #31 lands with a different vitest/Vite pair than assumed, 22.12 is hardcoded in four places (.nvmrc, the CI matrix, this paragraph, CLAUDE.md) and all four need re-checking against the range Vite actually declares.
  • .github/workflows/update-catalog.yml still uses node-version: '22', which CLAUDE.md's own "Raising the Node floor" checklist lists. It happens to keep working (bare '22' resolves to the latest 22.x, which satisfies >=22.12), so this isn't a break — but it is the one remaining place where the dev floor is satisfied by accident rather than on purpose. Either pin it to 22.12 for consistency or add a word to the checklist saying that workflow intentionally floats.

@dangrondahl
dangrondahl marked this pull request as ready for review August 13, 2026 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant