Repository navigation
ci: separate the end-user Node floor from the development Node floor - #339
Merged
Merged
Conversation
CI's `test (22.13.0)` job runs the whole vitest suite, so every dev dependency is held to the published packages' `engines.node` floor. jsdom 30 (^22.22.2 || ^24.15.0 || >=26.0.0) is the first to cross it; vite and oxlint sit 0.01 above. The mismatch is silent because pnpm's engine-strict is off, so the eventual failure would surface as an unexplained error in one matrix job. Design: keep the full suite on the release lines the toolchain supports (22 / 24.16.0 / 26) and add a `floor-smoke` job that runs the built dist under a bare 22.13.0 `node`. That job also covers `loadConfigFile`'s guided error for a `.ts` config on old Node — a branch vitest can never reach, since its module runner transforms in-process dynamic import(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Run scripts/floor-smoke.mjs on the test job's matrix Node too, since floor-smoke itself is pinned to 22.13.0 and only ever takes the old-Node branch of the .ts config contract check — nothing was covering the modern-Node side. Also harden the smoke script itself: preserve failure evidence (exit code, stderr, stack) instead of discarding it, add a `pnpm build` preflight, stop conflating a signal kill with exit 1, accept version prereleases, clean up its temp dirs, and rename a check to stop overclaiming mcp bin coverage. Qualify AGENTS.md's floor-separation claim to the dependencies the smoke actually executes, restore the pnpm version constraint as a CI comment, add the smoke's local-run caveat to AGENTS.md and pnpm smoke to CONTRIBUTING.md, and record the floating '22' tradeoff in the design doc's Risks section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change separates the published Node.js floor from the development matrix. It adds built-package smoke checks, a root command, dedicated Node 22.13.0 CI coverage, matrix execution, and documentation updates. ChangesNode floor validation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Build as Package build artifacts
participant Smoke as floor-smoke.mjs
participant CLI as Built CLI
participant Entry as Published entry points
CI->>Build: Restore or build dist artifacts
CI->>Smoke: Run smoke checks
Smoke->>CLI: Execute CLI scenarios
CLI-->>Smoke: Return output and exit codes
Smoke->>Entry: Import published entry points
Entry-->>Smoke: Return module exports
Smoke-->>CI: Return aggregate success or failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 16: Document that pnpm smoke requires built packages/cli/dist/bin.js:
update AGENTS.md line 16’s verification-table note to instruct users to run pnpm
build first or use pnpm build && pnpm smoke, and update CONTRIBUTING.md line 25
to show the same prerequisite beside the common command.
In `@scripts/floor-smoke.mjs`:
- Around line 116-122: Update the comment above the temporary SvelteKit fixture
setup in the smoke test to reflect that the CLI invokes loadConfigFile before
detectProject: the project only needs to resemble a SvelteKit app after the .ts
config loads so execution can proceed to project detection. Leave the test
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe4c1a27-425f-4d4a-bcd4-cfdb47bf230e
📒 Files selected for processing (8)
.github/workflows/ci.ymlAGENTS.mdCONTRIBUTING.mddocs/superpowers/plans/2026-07-31-node-floor-smoke.mddocs/superpowers/specs/2026-07-31-floor-smoke-design.mdpackage.jsonpackages/cli/test/config-file.test.tsscripts/floor-smoke.mjs
…d prerequisite CodeRabbit review on #339: loadConfigFile runs before detectProject (index.ts:174 vs :188), so the comment on the .ts-config check had the control flow backwards. Also record that pnpm smoke needs a prior pnpm build, since its preflight exits 1 on a fresh checkout. Co-Authored-By: Claude Opus 5 <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.
Why
The published packages declare
engines.node: >=22.13.0, and the dev toolchain ispinned separately by
devEngines.runtime. CI ignored that distinction: thetestmatrix pinned the published floor and ran the whole vitest suite on it, so every dev
dependency was held to the end users' Node version.
That coupling has been silently load-bearing for a while — vite 8, oxlint and oxfmt all
sit at
>=22.12.0, one patch below the floor. #332 (jsdom 29 -> 30) is the first devdependency to cross it: jsdom 30's only breaking change is raising its requirement to
^22.22.2 || ^24.15.0 || >=26.0.0. Nothing warns about the mismatch, because pnpm'sengine-strictis off by default.The failure this sets up is a dev dependency using a newer API and breaking exactly one
matrix job, with a runtime error that has no visible connection to the Node floor.
What changed
checktest22,24.16.0,26floor-smokedistunder a barenode, no test runner22.13.0testmatrix moves22.13.0->22(latest 22.x). It now tracks release linesthe dev toolchain supports, so the 22 line keeps full unit-test coverage while no
longer pinning the published floor.
scripts/floor-smoke.mjs(new,pnpm smoke) asserts the end-user contract againstthe built
dist: exit codes, a well-formed JSON report, and that every publishedentry point imports. Node builtins only — importing a test runner would recreate the
coupling this removes.
.tsconfig on old Node and branched so it passed either way. The smoke asserts the CLI's
guided error instead — a branch no vitest test can reach, because vitest's module
runner transforms in-process dynamic
import(), so a.tsconfig always loads insidevitest regardless of the host Node.
The smoke runs in both jobs:
floor-smokecovers the old-Node side of that contract,the
testmatrix covers the modern-Node side.What this does not claim
floor-smokestill runspnpm installon 22.13.0, so pnpm itself and the buildtoolchain remain floor-bound.
AGENTS.mdstates the rule with that qualification ratherthan as an absolute, and the workflow re-records the "22.13.0 is the minimum the pinned
pnpm can run on" constraint that the old matrix comment carried.
engines.nodeis unchanged, andengine-strictis deliberately not enabled — turning iton would fail the install on the floor Node because of a dev dependency, which is the
coupling being removed.
Verification
pnpm lint,pnpm build,pnpm typecheck,pnpm test(2152 tests),pnpm smokeallpass locally. The smoke's checks were each break-tested to confirm they can fail.
Design doc:
docs/superpowers/specs/2026-07-31-floor-smoke-design.mdPlan:
docs/superpowers/plans/2026-07-31-node-floor-smoke.mdNo changeset: CI and tooling only, no published-package code changes.
Relationship to #332
Independent — #332 can merge before or after. The floor mismatch predates jsdom 30; that
bump only made it visible.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation