tests(#484): 48 unit tests for cli/updater.js self-update module#331
Open
javimosch wants to merge 1 commit into
Open
tests(#484): 48 unit tests for cli/updater.js self-update module#331javimosch wants to merge 1 commit into
javimosch wants to merge 1 commit into
Conversation
Create cli/updater.js with: fetchLatestVersion (npm registry), downloadTarball (fetch + write), computeFileHash (crypto sha256/md5), verifyIntegrity (hash guard), backupCurrentInstall/restoreBackup (dir copy + rollback), applyUpdate (tar extract), and selfUpdate orchestrator (version check → download → backup → apply → rollback on failure). Add __tests__/updater.test.js with 48 tests covering: version fetch ok/fail/ missing-field/network-abort, tarball download ok/fail/no-write-on-error, hash computation determinism and algorithm selection, integrity mismatch throws, backup removes-existing/recurses-dirs, restore throws-when-missing/removes- installDir, tar arg forwarding, rollback on apply failure, full orchestrator happy/up-to-date/tarball-fail/version-fail paths. Closes #484 _(mago task #484)_
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds Self-update module
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Owner
Author
|
Review — Head of Org Engineering: The diff adds a new test file (approved — review-only mode; merge when ready.) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@__tests__/updater.test.js`:
- Around line 97-100: The abort/network error tests in
updater.fetchLatestVersion currently assert raw thrown messages, but they should
follow the same structured error contract as HTTP failures. Update the affected
cases in the updater tests to expect an error object with code, type, and
recoverable fields for the network/abort paths, and align the assertions in the
relevant fetchLatestVersion test blocks accordingly.
- Around line 577-580: The module constants test is asserting the wrong npm
package name, which locks in the typo instead of the intended registry target.
Update the PACKAGE_NAME expectation in the module constants test to match the
correct package name used by updater.PACKAGE_NAME, and keep the assertion
aligned with the PR objective so the test protects the intended name.
In `@cli/updater.js`:
- Line 10: The PACKAGE_NAME constant in updater logic is pointing to the wrong
published package, which will make self-update resolve the wrong
registry/tarball. Update the PACKAGE_NAME value in the updater module to match
the actual CLI package name used by the rest of the project, and verify any
request-building code that consumes PACKAGE_NAME continues to target the
intended package.
- Around line 24-50: The updater’s network calls in fetchLatestVersion and
downloadTarball only wrap non-OK HTTP responses, but rejected fetchWithTimeout
calls still escape without the structured { code, type, recoverable } shape. Add
try/catch around the fetchWithTimeout calls in these functions and convert
timeout/network rejections into Object.assign-wrapped Errors using the same
integration_error contract and the existing codes (110 for latest version, 111
for tarball) so all updater failures are handled consistently.
- Around line 150-151: The update flow in applyUpdate currently extracts the new
tarball directly into the existing installDir, which can leave stale files from
previous versions behind. Update the install logic to back up the current
install tree first, clear or replace the target directory before extraction,
then restore/rename into place atomically enough to avoid mixed-version
installs; use the applyUpdate and installDir flow as the entry point for the
change.
- Around line 73-82: The copyDirSync helper currently dereferences symlinks by
using fs.statSync, which causes linked directories/files to be copied as real
content instead of preserving the link. Update copyDirSync to use fs.lstatSync
so you can detect symlinks before recursing, and when a symlink is found,
recreate it at the destination rather than traversing it; keep the existing
recursive behavior only for true directories.
- Around line 144-146: The selfUpdate flow currently skips tarball verification
unless expectedHash is explicitly provided, so the default update path installs
without integrity checks. Update the selfUpdate logic in updater.js to retrieve
the tarball integrity from npm metadata and pass it into verifyIntegrity for the
downloaded tarballPath, only bypassing it when a trusted override is
intentionally supplied. Use the existing selfUpdate, verifyIntegrity,
expectedHash, and hashAlgorithm symbols to locate and wire the check.
- Around line 139-142: The updater artifacts are being written to predictable
paths under os.tmpdir(), which can collide across concurrent runs and be
tampered with. In the updater flow where tarballPath and backupDir are created
before downloadTarball, switch to a unique private work directory for each run
and place both the tarball and backup inside it. Keep the existing
downloadTarball usage and related cleanup logic aligned with the new workdir so
all updater artifacts stay isolated.
🪄 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: e6f6f9bf-400a-49ab-ad56-6cae10df59fa
📒 Files selected for processing (2)
__tests__/updater.test.jscli/updater.js
Comment on lines
+97
to
+100
| test("propagates network/abort error", async () => { | ||
| mockFetchAbort() | ||
| await expect(updater.fetchLatestVersion()).rejects.toThrow("aborted") | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert structured network errors.
These tests currently expect raw abort/network failures. Update them to expect the same { code, type, recoverable } contract used for non-OK HTTP responses.
Also applies to: 153-158
🤖 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 `@__tests__/updater.test.js` around lines 97 - 100, The abort/network error
tests in updater.fetchLatestVersion currently assert raw thrown messages, but
they should follow the same structured error contract as HTTP failures. Update
the affected cases in the updater tests to expect an error object with code,
type, and recoverable fields for the network/abort paths, and align the
assertions in the relevant fetchLatestVersion test blocks accordingly.
Comment on lines
+577
to
+580
| describe("module constants", () => { | ||
| test("PACKAGE_NAME is superacli", () => { | ||
| expect(updater.PACKAGE_NAME).toBe("superacli") | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don’t lock in the package-name typo.
This assertion should match the intended npm package name from the PR objective; otherwise the test suite protects the wrong registry target.
Proposed fix
- test("PACKAGE_NAME is superacli", () => {
- expect(updater.PACKAGE_NAME).toBe("superacli")
+ test("PACKAGE_NAME is supercli", () => {
+ expect(updater.PACKAGE_NAME).toBe("supercli")
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe("module constants", () => { | |
| test("PACKAGE_NAME is superacli", () => { | |
| expect(updater.PACKAGE_NAME).toBe("superacli") | |
| }) | |
| describe("module constants", () => { | |
| test("PACKAGE_NAME is supercli", () => { | |
| expect(updater.PACKAGE_NAME).toBe("supercli") | |
| }) |
🤖 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 `@__tests__/updater.test.js` around lines 577 - 580, The module constants test
is asserting the wrong npm package name, which locks in the typo instead of the
intended registry target. Update the PACKAGE_NAME expectation in the module
constants test to match the correct package name used by updater.PACKAGE_NAME,
and keep the assertion aligned with the PR objective so the test protects the
intended name.
| const { spawnSync } = require("child_process") | ||
|
|
||
| const NPM_REGISTRY_URL = "https://registry.npmjs.org" | ||
| const PACKAGE_NAME = "superacli" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align PACKAGE_NAME with the actual CLI package.
The PR objective names the CLI as supercli, but this constant points registry and tarball requests at superacli, so self-update will fetch the wrong package or 404.
Proposed fix
-const PACKAGE_NAME = "superacli"
+const PACKAGE_NAME = "supercli"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const PACKAGE_NAME = "superacli" | |
| const PACKAGE_NAME = "supercli" |
🤖 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 `@cli/updater.js` at line 10, The PACKAGE_NAME constant in updater logic is
pointing to the wrong published package, which will make self-update resolve the
wrong registry/tarball. Update the PACKAGE_NAME value in the updater module to
match the actual CLI package name used by the rest of the project, and verify
any request-building code that consumes PACKAGE_NAME continues to target the
intended package.
Comment on lines
+24
to
+50
| async function fetchLatestVersion() { | ||
| const res = await fetchWithTimeout(`${NPM_REGISTRY_URL}/${PACKAGE_NAME}/latest`) | ||
| if (!res.ok) { | ||
| throw Object.assign( | ||
| new Error(`Failed to fetch latest version: ${res.status} ${res.statusText}`), | ||
| { code: 110, type: "integration_error", recoverable: true } | ||
| ) | ||
| } | ||
| const data = await res.json() | ||
| if (!data || typeof data.version !== "string" || !data.version) { | ||
| throw Object.assign( | ||
| new Error("Invalid npm registry response: missing version field"), | ||
| { code: 110, type: "integration_error", recoverable: true } | ||
| ) | ||
| } | ||
| return data.version | ||
| } | ||
|
|
||
| async function downloadTarball(version, destPath) { | ||
| const url = `${NPM_REGISTRY_URL}/${PACKAGE_NAME}/-/${PACKAGE_NAME}-${version}.tgz` | ||
| const res = await fetchWithTimeout(url) | ||
| if (!res.ok) { | ||
| throw Object.assign( | ||
| new Error(`Failed to download tarball: ${res.status} ${res.statusText}`), | ||
| { code: 111, type: "integration_error", recoverable: true } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wrap network and timeout failures in structured errors too.
Only HTTP non-OK responses get { code, type, recoverable }; rejected fetch calls currently bypass the updater’s structured error contract.
Proposed fix
async function fetchLatestVersion() {
- const res = await fetchWithTimeout(`${NPM_REGISTRY_URL}/${PACKAGE_NAME}/latest`)
+ let res
+ try {
+ res = await fetchWithTimeout(`${NPM_REGISTRY_URL}/${PACKAGE_NAME}/latest`)
+ } catch (err) {
+ throw Object.assign(
+ new Error(`Failed to fetch latest version: ${err.message}`),
+ { code: 110, type: "integration_error", recoverable: true }
+ )
+ }
if (!res.ok) {
@@
async function downloadTarball(version, destPath) {
const url = `${NPM_REGISTRY_URL}/${PACKAGE_NAME}/-/${PACKAGE_NAME}-${version}.tgz`
- const res = await fetchWithTimeout(url)
+ let res
+ try {
+ res = await fetchWithTimeout(url)
+ } catch (err) {
+ throw Object.assign(
+ new Error(`Failed to download tarball: ${err.message}`),
+ { code: 111, type: "integration_error", recoverable: true }
+ )
+ }
if (!res.ok) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function fetchLatestVersion() { | |
| const res = await fetchWithTimeout(`${NPM_REGISTRY_URL}/${PACKAGE_NAME}/latest`) | |
| if (!res.ok) { | |
| throw Object.assign( | |
| new Error(`Failed to fetch latest version: ${res.status} ${res.statusText}`), | |
| { code: 110, type: "integration_error", recoverable: true } | |
| ) | |
| } | |
| const data = await res.json() | |
| if (!data || typeof data.version !== "string" || !data.version) { | |
| throw Object.assign( | |
| new Error("Invalid npm registry response: missing version field"), | |
| { code: 110, type: "integration_error", recoverable: true } | |
| ) | |
| } | |
| return data.version | |
| } | |
| async function downloadTarball(version, destPath) { | |
| const url = `${NPM_REGISTRY_URL}/${PACKAGE_NAME}/-/${PACKAGE_NAME}-${version}.tgz` | |
| const res = await fetchWithTimeout(url) | |
| if (!res.ok) { | |
| throw Object.assign( | |
| new Error(`Failed to download tarball: ${res.status} ${res.statusText}`), | |
| { code: 111, type: "integration_error", recoverable: true } | |
| ) | |
| } | |
| async function fetchLatestVersion() { | |
| let res | |
| try { | |
| res = await fetchWithTimeout(`${NPM_REGISTRY_URL}/${PACKAGE_NAME}/latest`) | |
| } catch (err) { | |
| throw Object.assign( | |
| new Error(`Failed to fetch latest version: ${err.message}`), | |
| { code: 110, type: "integration_error", recoverable: true } | |
| ) | |
| } | |
| if (!res.ok) { | |
| throw Object.assign( | |
| new Error(`Failed to fetch latest version: ${res.status} ${res.statusText}`), | |
| { code: 110, type: "integration_error", recoverable: true } | |
| ) | |
| } | |
| const data = await res.json() | |
| if (!data || typeof data.version !== "string" || !data.version) { | |
| throw Object.assign( | |
| new Error("Invalid npm registry response: missing version field"), | |
| { code: 110, type: "integration_error", recoverable: true } | |
| ) | |
| } | |
| return data.version | |
| } | |
| async function downloadTarball(version, destPath) { | |
| const url = `${NPM_REGISTRY_URL}/${PACKAGE_NAME}/-/${PACKAGE_NAME}-${version}.tgz` | |
| let res | |
| try { | |
| res = await fetchWithTimeout(url) | |
| } catch (err) { | |
| throw Object.assign( | |
| new Error(`Failed to download tarball: ${err.message}`), | |
| { code: 111, type: "integration_error", recoverable: true } | |
| ) | |
| } | |
| if (!res.ok) { | |
| throw Object.assign( | |
| new Error(`Failed to download tarball: ${res.status} ${res.statusText}`), | |
| { code: 111, type: "integration_error", recoverable: true } | |
| ) | |
| } |
🤖 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 `@cli/updater.js` around lines 24 - 50, The updater’s network calls in
fetchLatestVersion and downloadTarball only wrap non-OK HTTP responses, but
rejected fetchWithTimeout calls still escape without the structured { code,
type, recoverable } shape. Add try/catch around the fetchWithTimeout calls in
these functions and convert timeout/network rejections into
Object.assign-wrapped Errors using the same integration_error contract and the
existing codes (110 for latest version, 111 for tarball) so all updater failures
are handled consistently.
Comment on lines
+73
to
+82
| function copyDirSync(src, dest) { | ||
| if (!fs.existsSync(dest)) fs.mkdirSync(dest, { recursive: true }) | ||
| for (const entry of fs.readdirSync(src)) { | ||
| const srcPath = path.join(src, entry) | ||
| const destPath = path.join(dest, entry) | ||
| if (fs.statSync(srcPath).isDirectory()) { | ||
| copyDirSync(srcPath, destPath) | ||
| } else { | ||
| fs.copyFileSync(srcPath, destPath) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant region.
ast-grep outline cli/updater.js --view expanded
echo '--- lines 1-220 ---'
cat -n cli/updater.js | sed -n '1,220p'
echo '--- search for copyDirSync usage ---'
rg -n "copyDirSync|statSync|lstatSync|symlinkSync|readlinkSync|copyFileSync|readdirSync" cli/updater.jsRepository: javimosch/supercli
Length of output: 7586
🏁 Script executed:
node - <<'JS'
const fs = require('fs');
const os = require('os');
const path = require('path');
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'symlink-copy-'));
const targetDir = path.join(root, 'target');
const linkDir = path.join(root, 'linkdir');
const targetFile = path.join(root, 'file.txt');
const linkFile = path.join(root, 'linkfile');
fs.mkdirSync(targetDir);
fs.writeFileSync(path.join(targetDir, 'nested.txt'), 'nested');
fs.writeFileSync(targetFile, 'hello');
fs.symlinkSync(targetDir, linkDir, 'dir');
fs.symlinkSync(targetFile, linkFile);
console.log({
stat_link_dir_isDirectory: fs.statSync(linkDir).isDirectory(),
lstat_link_dir_isSymlink: fs.lstatSync(linkDir).isSymbolicLink(),
stat_link_file_isFile: fs.statSync(linkFile).isFile(),
lstat_link_file_isSymlink: fs.lstatSync(linkFile).isSymbolicLink(),
});
JSRepository: javimosch/supercli
Length of output: 294
Preserve symlinks in copyDirSync. fs.statSync dereferences links, so a symlinked directory gets traversed and copied as real files. That breaks faithful backup/restore and can walk outside the install tree; use lstatSync and recreate the link instead.
🤖 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 `@cli/updater.js` around lines 73 - 82, The copyDirSync helper currently
dereferences symlinks by using fs.statSync, which causes linked
directories/files to be copied as real content instead of preserving the link.
Update copyDirSync to use fs.lstatSync so you can detect symlinks before
recursing, and when a symlink is found, recreate it at the destination rather
than traversing it; keep the existing recursive behavior only for true
directories.
Comment on lines
+139
to
+142
| const tarballPath = path.join(tmpDir, `${PACKAGE_NAME}-${latestVersion}.tgz`) | ||
| const backupDir = path.join(tmpDir, `${PACKAGE_NAME}-backup-${currentVersion}`) | ||
|
|
||
| await downloadTarball(latestVersion, tarballPath) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect the updater implementation.
git ls-files cli/updater.js
wc -l cli/updater.js
cat -n cli/updater.js | sed -n '1,260p'
# Look for how tmpDir is sourced and whether cleanup happens later.
rg -n "tmpDir|mkdtemp|mkdtempSync|tmpdir|backupDir|tarballPath|downloadTarball|extract|rename|rmSync|rmdirSync" cli/updater.jsRepository: javimosch/supercli
Length of output: 7776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the updater entrypoint and any docs about how it is run.
rg -n "selfUpdate\(|fetchLatestVersion\(|downloadTarball\(|applyUpdate\(" .
rg -n "superacli|update|self-update|upgrade" README.md docs package.json cli -g '!node_modules'
# Inspect the CLI command wiring if present.
fd -a . cli src .Repository: javimosch/supercli
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow the search to repo files that can actually wire this updater in.
printf '\n== tracked files ==\n'
git ls-files | rg '^(package\.json|README\.md|cli/|src/)'
printf '\n== updater references ==\n'
rg -n "selfUpdate|downloadTarball|applyUpdate|backupCurrentInstall|restoreBackup|fetchLatestVersion|updater" package.json README.md cli src
printf '\n== package.json ==\n'
cat -n package.json | sed -n '1,220p'Repository: javimosch/supercli
Length of output: 2308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how the updater is exposed and compare the temp-file pattern in the plugin updater.
cat -n package.json | sed -n '1,220p'
printf '\n== updater file ==\n'
cat -n cli/updater.js | sed -n '125,160p'
printf '\n== plugin updater temp handling ==\n'
cat -n cli/plugins-update.js | sed -n '70,110p'Repository: javimosch/supercli
Length of output: 4022
Use a private temp workdir for updater artifacts. Predictable names under os.tmpdir() can collide across concurrent runs and allow local tampering with the tarball/backup paths. Create a unique work dir and place both files inside it.
Proposed fix
- const tarballPath = path.join(tmpDir, `${PACKAGE_NAME}-${latestVersion}.tgz`)
- const backupDir = path.join(tmpDir, `${PACKAGE_NAME}-backup-${currentVersion}`)
+ const workDir = fs.mkdtempSync(path.join(tmpDir, `${PACKAGE_NAME}-update-`))
+ const tarballPath = path.join(workDir, `${PACKAGE_NAME}-${latestVersion}.tgz`)
+ const backupDir = path.join(workDir, `${PACKAGE_NAME}-backup-${currentVersion}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const tarballPath = path.join(tmpDir, `${PACKAGE_NAME}-${latestVersion}.tgz`) | |
| const backupDir = path.join(tmpDir, `${PACKAGE_NAME}-backup-${currentVersion}`) | |
| await downloadTarball(latestVersion, tarballPath) | |
| const workDir = fs.mkdtempSync(path.join(tmpDir, `${PACKAGE_NAME}-update-`)) | |
| const tarballPath = path.join(workDir, `${PACKAGE_NAME}-${latestVersion}.tgz`) | |
| const backupDir = path.join(workDir, `${PACKAGE_NAME}-backup-${currentVersion}`) | |
| await downloadTarball(latestVersion, tarballPath) |
🤖 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 `@cli/updater.js` around lines 139 - 142, The updater artifacts are being
written to predictable paths under os.tmpdir(), which can collide across
concurrent runs and be tampered with. In the updater flow where tarballPath and
backupDir are created before downloadTarball, switch to a unique private work
directory for each run and place both the tarball and backup inside it. Keep the
existing downloadTarball usage and related cleanup logic aligned with the new
workdir so all updater artifacts stay isolated.
Comment on lines
+144
to
+146
| if (expectedHash) { | ||
| verifyIntegrity(tarballPath, expectedHash, hashAlgorithm) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the updater and related metadata flow.
git ls-files | rg '(^|/)cli/updater\.js$|(^|/)package\.json$|(^|/)npm|(^|/)update|(^|/)self[-]?update'Repository: javimosch/supercli
Length of output: 9758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline cli/updater.js --view expanded
printf '\n--- tests ---\n'
ast-grep outline __tests__/updater.test.js --view expanded
printf '\n--- package.json ---\n'
ast-grep outline package.json --view expandedRepository: javimosch/supercli
Length of output: 1230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' cli/updater.js
printf '\n--- tests ---\n'
sed -n '1,260p' __tests__/updater.test.jsRepository: javimosch/supercli
Length of output: 13343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "selfUpdate\\(|expectedHash|hashAlgorithm|verifyIntegrity" cli __tests__ . -g '!node_modules'
printf '\n--- remaining updater tests ---\n'
sed -n '260,520p' __tests__/updater.test.jsRepository: javimosch/supercli
Length of output: 13134
Verify tarball integrity in selfUpdate by default. selfUpdate only calls verifyIntegrity when expectedHash is passed, so the normal update path still installs the downloaded tarball without any integrity check. Pull the tarball integrity from npm metadata and enforce it unless a trusted override is explicitly supplied.
🤖 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 `@cli/updater.js` around lines 144 - 146, The selfUpdate flow currently skips
tarball verification unless expectedHash is explicitly provided, so the default
update path installs without integrity checks. Update the selfUpdate logic in
updater.js to retrieve the tarball integrity from npm metadata and pass it into
verifyIntegrity for the downloaded tarballPath, only bypassing it when a trusted
override is intentionally supplied. Use the existing selfUpdate,
verifyIntegrity, expectedHash, and hashAlgorithm symbols to locate and wire the
check.
Comment on lines
+150
to
+151
| try { | ||
| applyUpdate(tarballPath, installDir) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear the install directory before extracting the new package.
Extracting over the existing tree leaves stale files from older versions behind, which can keep removed commands/modules active after an update. Backup first, then replace the install tree atomically enough to avoid mixed-version installs.
Proposed fix
try {
+ fs.rmSync(installDir, { recursive: true, force: true })
+ fs.mkdirSync(installDir, { recursive: true })
applyUpdate(tarballPath, installDir)
} catch (err) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| applyUpdate(tarballPath, installDir) | |
| try { | |
| fs.rmSync(installDir, { recursive: true, force: true }) | |
| fs.mkdirSync(installDir, { recursive: true }) | |
| applyUpdate(tarballPath, installDir) |
🤖 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 `@cli/updater.js` around lines 150 - 151, The update flow in applyUpdate
currently extracts the new tarball directly into the existing installDir, which
can leave stale files from previous versions behind. Update the install logic to
back up the current install tree first, clear or replace the target directory
before extraction, then restore/rename into place atomically enough to avoid
mixed-version installs; use the applyUpdate and installDir flow as the entry
point for the change.
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.
What changed
Created
cli/updater.js— a new self-update module for the supercli CLI — with the following exports:fetchLatestVersion()/superacli/latest)downloadTarball(version, dest)computeFileHash(filePath, algo)verifyIntegrity(filePath, expected, algo)integrity_error(code 112) on hash mismatchbackupCurrentInstall(installDir, backupDir)restoreBackup(backupDir, installDir)rollback_error(code 113) if backup missingapplyUpdate(tarball, installDir)tar -xzfto extract update; throwsinstall_error(code 114) on failureselfUpdate(options)All helpers use structured errors with
code,type, andrecoverablefields consistent with the rest of the codebase.Tests (
__tests__/updater.test.js) — 48 tests{updated:false}, no second fetch when current, full success result, tarball URL contains version, skips integrity when no hash, rollback+re-throw on apply failure, rollback calls mkdirSync, version fetch failure, tarball download failureVerification
Closes #484 (mago task #484)
Summary by CodeRabbit
New Features
Tests