Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaced Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Local CLI Code
participant GitHub as GitHub API
participant FS as Filesystem
participant Shell as Child Process
CLI->>GitHub: GET /repos/.../releases/latest (fetch)
GitHub-->>CLI: 200 { tag_name } / 4xx/5xx
alt tag_name valid
CLI->>CLI: parse semver (may strip leading "v")
CLI->>Shell: execFile("version") to get local version
Shell-->>CLI: local version stdout
else tag_name missing/invalid
CLI-->>CLI: latestVersion = undefined
end
CLI->>GitHub: GET archive_url (fetch)
GitHub-->>CLI: 200 arrayBuffer (binary)
CLI->>FS: writeFile(temp)
CLI->>FS: rm(temp, { force: true }) on failure
CLI->>FS: extract zip and chmod 0o755
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
jordanstephens
approved these changes
Apr 8, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkgs/npm/src/clilib.ts`:
- Around line 29-30: The code currently returns data.tag_name?.replace("v",
"").trim() which can produce empty or non-semver strings; update the logic in
clilib.ts so you first read data.tag_name, trim whitespace, strip only a single
leading "v" (not every "v" in the string), then validate the resulting value
against SEMVER_REGEX and return it only if it matches; otherwise return
undefined — this prevents run() from treating malformed values as installable
and avoids downstream errors in getAppArchiveFilename().
- Around line 22-23: The package now uses the native global fetch (seen in
pkgs/npm/src/clilib.ts where fetch is called for URL_LATEST_RELEASE), which
requires Node >=18; update pkgs/npm/package.json to include an "engines" entry
like "node": ">=18.0.0" to prevent installation on older Node versions, or
alternatively implement a runtime fallback in clilib.ts that detects
global.fetch and loads a polyfill (e.g., node-fetch) when absent; modify either
package.json or the fetch call fallback logic so the CLI won't fail with "fetch
is not defined" on Node <18.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9db523ba-8506-4e54-bc37-89779369b7f0
⛔ Files ignored due to path filters (1)
pkgs/npm/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
pkgs/npm/package.jsonpkgs/npm/src/clilib.tspkgs/npm/test/clilib.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkgs/npm/test/clilib.spec.ts (2)
97-99: Variable nameunlinkStubis misleading since it stubsfs.promises.rm.Consider renaming to
rmStubto match the actual method being stubbed.♻️ Suggested rename
- let unlinkStub: sinon.SinonStub; + let rmStub: sinon.SinonStub; ... - unlinkStub = sandbox + rmStub = sandbox .stub(fs.promises, "rm") .callsFake(() => Promise.resolve());And update all references from
unlinkStubtormStubin the assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/npm/test/clilib.spec.ts` around lines 97 - 99, The test uses a misleading variable name unlinkStub while stubbing fs.promises.rm; rename the variable to rmStub and update all references to it in this spec (the sandbox.stub call that currently assigns unlinkStub and any assertions/uses elsewhere) so the identifier matches the actual method being stubbed (fs.promises.rm), ensuring assertions reference rmStub instead of unlinkStub.
116-126: Test callsdownloadFiletwice unnecessarily.The test invokes
downloadFiletwice (lines 118 and 120), both returningnullon failure. The assertion effectively checksnull === null, which isn't meaningful. Consider simplifying:♻️ Simplified test
it("download fails path", async () => { fetchStub.returns(Promise.reject("failed")); - const targetFile = await clilib.downloadFile(url, downloadFileName); - await expect( - clilib.downloadFile(url, downloadFileName) - ).to.eventually.equal(targetFile); + await expect(clilib.downloadFile(url, downloadFileName)).to.eventually.be.null; sinon.assert.calledWith(fetchStub, url); sinon.assert.notCalled(writeStub); sinon.assert.calledWith(unlinkStub, downloadFileName); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/npm/test/clilib.spec.ts` around lines 116 - 126, The test calls clilib.downloadFile(url, downloadFileName) twice which is redundant and makes the assertion meaningless; replace the two calls with a single awaited call (assigning to targetFile), then assert the returned value is null or equals the expected failure value, and keep the existing verifications that fetchStub was called with url, writeStub was not called, and unlinkStub was called with downloadFileName; reference symbols: clilib.downloadFile, fetchStub, writeStub, unlinkStub, url, downloadFileName.pkgs/npm/src/clilib.ts (1)
44-70: Consider adding a timeout to prevent hanging on unresponsive servers.The native
fetchAPI has no default timeout, so if the download server becomes unresponsive, the CLI could hang indefinitely.💡 Optional: Add fetch timeout using AbortSignal
async function downloadFile( downloadUrl: string, downloadTargetFile: string ): Promise<string | null> { try { - const response = await fetch(downloadUrl); + const response = await fetch(downloadUrl, { + signal: AbortSignal.timeout(60_000), // 60 second timeout + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/npm/src/clilib.ts` around lines 44 - 70, The downloadFile function can hang because fetch has no timeout; add an AbortController and pass controller.signal to fetch, start a setTimeout (e.g., 30s or configurable) that calls controller.abort(), and clear the timer when the response is received or on success before returning; ensure the catch still deletes downloadTargetFile and returns null, and handle abort errors the same way (or log them distinctly). Update references: downloadFile and the fetch call to use the AbortController/signal and a timeout timer that is cleared on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkgs/npm/src/clilib.ts`:
- Around line 44-70: The downloadFile function can hang because fetch has no
timeout; add an AbortController and pass controller.signal to fetch, start a
setTimeout (e.g., 30s or configurable) that calls controller.abort(), and clear
the timer when the response is received or on success before returning; ensure
the catch still deletes downloadTargetFile and returns null, and handle abort
errors the same way (or log them distinctly). Update references: downloadFile
and the fetch call to use the AbortController/signal and a timeout timer that is
cleared on success.
In `@pkgs/npm/test/clilib.spec.ts`:
- Around line 97-99: The test uses a misleading variable name unlinkStub while
stubbing fs.promises.rm; rename the variable to rmStub and update all references
to it in this spec (the sandbox.stub call that currently assigns unlinkStub and
any assertions/uses elsewhere) so the identifier matches the actual method being
stubbed (fs.promises.rm), ensuring assertions reference rmStub instead of
unlinkStub.
- Around line 116-126: The test calls clilib.downloadFile(url, downloadFileName)
twice which is redundant and makes the assertion meaningless; replace the two
calls with a single awaited call (assigning to targetFile), then assert the
returned value is null or equals the expected failure value, and keep the
existing verifications that fetchStub was called with url, writeStub was not
called, and unlinkStub was called with downloadFileName; reference symbols:
clilib.downloadFile, fetchStub, writeStub, unlinkStub, url, downloadFileName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 529f9a81-0722-47f5-a9ed-8b31d6fd8a0c
📒 Files selected for processing (3)
pkgs/npm/package.jsonpkgs/npm/src/clilib.tspkgs/npm/test/clilib.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- pkgs/npm/package.json
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
2 participants
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.
Description
npm audit fix@types/nodechore: SHA-pin all GHA actions and add Dependabot #2034 (review)Linked Issues
Checklist
Summary by CodeRabbit
Chores
Tests