Verify runtime version in fedify init - #981
Conversation
✅ Deploy Preview for fedify-json-schema canceled.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesRuntime verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The runtime verification change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant FedifyInit
participant fillPackageManager
participant checkAllRuntimes
participant RuntimeCommands
FedifyInit->>fillPackageManager: resolve package manager
fillPackageManager->>checkAllRuntimes: validate runtime requirements
checkAllRuntimes->>RuntimeCommands: execute version commands
RuntimeCommands-->>checkAllRuntimes: return runtime statuses
checkAllRuntimes-->>fillPackageManager: return compatible options
fillPackageManager-->>FedifyInit: continue or exit
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🤖 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 `@packages/init/src/ask/pm.ts`:
- Around line 40-64: Update the explicit package-manager branch before its
availability/runtime checks to validate isWfSupportsPm(options.webFramework,
pm), matching the interactive selection path. Reject unsupported combinations
using the existing error/exit behavior, while preserving the current checks and
successful return for compatible package managers.
- Around line 53-55: Update the result.status === "missing" branch to include
actionable runtime installation guidance, using runtimes[pmToRt(pm)].label and
an install URL. Extend the installer metadata or add a runtime helper to support
the getInstallUrl signature needed for this path, while preserving the existing
missing-runtime error message.
In `@packages/init/src/lib.ts`:
- Around line 221-224: Update the return type of checkRuntimeVersion to use the
imported RuntimeCheck type instead of redeclaring the equivalent union, matching
checkRuntimeRequirement and keeping the shared contract centralized.
- Around line 239-249: Update checkRuntimeVersion so unexpected runtime-check
errors are converted into the same result shape used for missing, unsupported,
and malformed runtimes instead of being rethrown. Ensure checkAllRuntimes and
askPackageManager handle that result by printing an actionable CLI message and
exiting cleanly, while preserving the existing not-found behavior.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e67122f-54e4-4ac4-a1fb-5b08cbe4b156
📒 Files selected for processing (9)
CHANGES.mdchanges.d/init/verify-runtime-version.mdpackages/init/src/ask/pm.tspackages/init/src/const.tspackages/init/src/json/rt.jsonpackages/init/src/lib.test.tspackages/init/src/lib.tspackages/init/src/types.tspackages/init/src/webframeworks/astro.ts
2chanhaeng
left a comment
2chanhaeng
left a comment
There was a problem hiding this comment.
Great work! Thanks for your contribution to Fedify. Before merging this PR, some changes are needed, and I have left suggestions to make your PR better. Please read them and consider applying them!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGES.md (1)
183-195: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the PR reference.
Line 183 escapes the closing citation bracket. Line 195 links PR
#981as an issue. Remove the escape and change the reference URL tohttps://github.com/fedify-dev/fedify/pull/981.Based on PR objectives: this change is PR
#981.🤖 Prompt for 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. In `@CHANGES.md` around lines 183 - 195, In CHANGES.md, correct the PR `#981` citation by removing the unnecessary escape from its closing bracket, and update the `#981` reference URL from the issues endpoint to the pull endpoint. Leave the other changelog references unchanged.
🤖 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.
Outside diff comments:
In `@CHANGES.md`:
- Around line 183-195: In CHANGES.md, correct the PR `#981` citation by removing
the unnecessary escape from its closing bracket, and update the `#981` reference
URL from the issues endpoint to the pull endpoint. Leave the other changelog
references unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 408742d3-7138-47d9-9b17-033403cf539d
📒 Files selected for processing (4)
CHANGES.mdchanges.d/init/verify-runtime-version.mdpackages/init/src/ask/pm.tspackages/init/src/lib.ts
|
I applied changes. Could you take another look? |
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
2chanhaeng
left a comment
2chanhaeng
left a comment
There was a problem hiding this comment.
OK, you've addressed my comments well, but it seems there are still some areas for improvement. Could you take a look at these points?
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/init/src/ask/pm.ts (1)
61-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReport the runtime failure details when all choices are disabled.
The error only states that no supported package manager is available. It omits the disabled reasons already computed for each choice. A user cannot identify the missing runtime, malformed version output, or required minimum version.
Include the relevant disabled reason or reasons in this exit path. The PR requires clear errors for missing, malformed, and unsupported runtimes.
🤖 Prompt for 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. In `@packages/init/src/ask/pm.ts` around lines 61 - 68, Update the all-disabled branch after the PACKAGE_MANAGER choices are computed to include each disabled choice’s existing reason in the printErrorMessage output. Preserve the current package-manager availability message while reporting missing, malformed, and unsupported runtime details from the choice objects before exiting.
🤖 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 `@packages/init/src/ask/pm.ts`:
- Around line 42-52: The explicit disabled package-manager path in the async
package-manager prompt must terminate after printing the error, regardless of
test mode. Update the branch handling choice.disabled in the packageManager
selection flow to exit unconditionally, while preserving the existing successful
return and interactive prompt behavior.
---
Outside diff comments:
In `@packages/init/src/ask/pm.ts`:
- Around line 61-68: Update the all-disabled branch after the PACKAGE_MANAGER
choices are computed to include each disabled choice’s existing reason in the
printErrorMessage output. Preserve the current package-manager availability
message while reporting missing, malformed, and unsupported runtime details from
the choice objects before exiting.
🪄 Autofix
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02b29fbf-5fc3-4845-abb8-1beb9e08cfb3
📒 Files selected for processing (2)
packages/init/src/ask/pm.tspackages/init/src/lib.ts
2chanhaeng
left a comment
2chanhaeng
left a comment
There was a problem hiding this comment.
Thanks for your hard work! I'm really sorry that this PR is dragging on, but I still see a few spots that need some fixing. Hang in there, we're almost there!
2chanhaeng
left a comment
2chanhaeng
left a comment
There was a problem hiding this comment.
Great work! Before merging this PR, please rebase from main branch.
Fedify has minimum runtime versions, and some frameworks require even newer ones. `fedify init` did not check this, so it could scaffold a project on a runtime below the minimum, which can then fail at runtime. Verify the selected runtime version before creating any files. The base minimums (Deno 2.0.0, Node.js 22.0.0, Bun 1.1.0) mirror the values declared by `@fedify/fedify` and are defined directly in `rt.json`. Frameworks with stricter requirements, such as Astro's Node.js 22.12, raise the minimum through `minRuntimeVersions`. Assisted-by: Claude Code:claude-opus-4-8
Cover `verifyRuntimeVersion` for versions below, equal to, and above the minimum, and `resolveRequiredVersion` for framework overrides that raise the base minimum. Assisted-by: Claude Code:claude-opus-4-8
Assisted-by: Claude Code:claude-opus-4-8
`askPackageManager` recomputed the runtime checks through `checkAllRuntimes` every time it built the prompt. Following the review, that computation was separated into `calculateChoices` to run once. When `fillPackageManager` already has a `packageManager`, it now looks the value up in `choices` instead of calling `checkRuntimeRequirement` again. `checkRuntimeRequirement` is no longer used outside `lib.ts`, so its export was removed. Assisted-by: Claude Code:claude-opus-4-8
When the chosen package manager was not installed, `fillPackageManager` recursed to prompt again, recomputing `calculateChoices` on every retry, so replace the recursion with a loop that reuses the `choices` computed once at the start. Assisted-by: Claude Code:claude-opus-4-8
`checkRuntimeRequirement` only forwarded to `checkRuntimeVersion`, so it was inlined into `checkAllRuntimes` to make the chain simpler. Assisted-by: Claude Code:claude-opus-4-8
`choices` recorded each runtime's version but not whether a package manager's binary was installed, so `fillPackageManager` checked the binary separately and looped to retry. Move that check into `choicePackageManager` so `choices` also reflects binary availability. `fillPackageManager` then trusts `choices` and drops the re-check, the retry `while` loop, and the `pm` alias. A disabled choice falls back to `askPackageManager` instead of exiting. Assisted-by: Claude Code:claude-opus-4-8
When a command is not installed, dax throws a ShellError with exit code 127. `isNotFoundError` only checked `ENOENT`, so it missed the error and init crashed. Recognize exit code 127 as well. Assisted-by: Claude Code:claude-opus-4-8
The `outputPattern` for bun and node was anchored to the end of the string with `$`, so it only matched a bare `major.minor.patch`. That missed pre-release versions such as bun's `1.2.14-canary.96` or Node.js nightly builds. Replace the trailing `$` with a word boundary (`\b`) so the pattern still captures the `major.minor.patch` core while allowing a pre-release or build-metadata suffix to follow. Assisted-by: Claude Code:claude-opus-4-8
b3c248c to
766524b
Compare
2 participants
Summary
Added runtime version verification to fedify init to enhance runtime safety by pre-checking minimum requirements.
Related issue
fedify init#964Changes
minVersion) tojson/rt.json, derived aRUNTIMElist inconst.ts, and addedRuntime/Runtimes/RuntimeChecktypes.verifyRuntimeVersion,checkRuntimeVersion,resolveRequiredVersion,checkRuntimeRequirement, andcheckAllRuntimestolib.ts.minRuntimeVersionsfield toWebFrameworkDescriptionand set Astro's Node.js 22.12 requirement.ask/pm.tsto exit with an actionable error for an unsupported runtime in non-interactive mode, and to disable the affected package managers in the interactive menu.verifyRuntimeVersionandresolveRequiredVersion.AI disclosure
Claude Code (
claude-opus-4-8) assisted with this change: it proposed the design and drafted much of the implementation, tests, and this description. I reviewed and edited the work, made the design decisions, and verified the behaviour myself by runningfedify initand the test suite.