fix(cli): stop treating version-manager npm shims as npm-cli.js - #7545
Conversation
mise replaces bin/npm with a bash wrapper. getNpmCliPath resolved it without checking whether it was actually a JS file, so runGlobalNpm spawned `node /path/to/bash-script` and the resulting SyntaxError surfaced as a misleading "registry error" on every update check. Closes QwenLM#7543
|
Both PRs fix the same root cause with the same approach: validate the resolved path before handing it to Node. The code changes are functionally identical. The difference is test coverage. #7544 adds one test — the wrapper case that triggers the bug. This PR adds four, covering the paths that the fix touches but the bug doesn't exercise:
#7544 was tested on Linux. This PR was tested on macOS, where the bug was found and where mise is the primary Node version manager. Either PR fixes the user-facing issue. The question for maintainers is whether the additional regression coverage justifies merging this one instead, or cherry-picking the extra tests onto #7544. |
|
Thanks for the PR! Template looks good ✓ Problem: confirmed observed bug — issue #7543 documents mise users hitting "registry error" on every startup, with the exact Direction: aligned — the update check is core functionality and this fixes a real breakage for version-manager users. No CHANGELOG reference needed; the bug is self-evident from the issue. Size: not applicable (no core paths touched — Approach: the scope is exactly right. One guard clause, one shared fallback variable, four focused tests. Nothing to cut. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已确认的实际 bug——issue #7543 记录了 mise 用户每次启动都遇到 "registry error",根因是 方向:对齐——更新检查是核心功能,此修复解决了版本管理器用户的实际故障。 规模:不适用(未触及核心路径——仅 方案:范围恰到好处。一个守卫子句、一个共享回退变量、四个聚焦测试。无需删减。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewClean, minimal fix. The approach is exactly what I'd propose independently: hoist the fallback Both downstream callers ( The four new tests cover the right cases: normal symlink → No critical blockers. No convention violations. Real-Scenario TestingNon-UI change — verified via unit tests and headless CLI smoke test. Unit tests (36/36 pass, including 4 new)Headless CLI smoke test (PR build)CLI starts cleanly, no update-check crash. The QWEN_HOME warning is a CI environment artifact, unrelated to this PR. Note: cannot reproduce the mise scenario on this machine (no mise installed), so the before/after evidence is the unit tests mocking 中文说明代码审查干净、最小化的修复。方案与我独立提出的完全一致:将回退的 两个下游调用方( 四个新测试覆盖了正确的场景:正常符号链接 → 无关键阻塞项。无规范违反。 实际场景测试非 UI 变更——通过单元测试和无头 CLI 冒烟测试验证。 单元测试 36/36 全部通过(含 4 个新增)。无头 CLI 启动正常,无更新检查崩溃。本机未安装 mise,无法复现真实场景,issue #7543 提供了真实世界的复现。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — confirmed bug, minimal fix, clean tests, no reservations. This is the textbook small bugfix: one guard clause, one shared const, four tests. The issue (#7543) has a clear reproduction with the exact The diff carries nothing beyond what the goal needs. No drive-by refactors, no scope creep. If I had to maintain this in six months, I'd thank the author. 中文说明置信度:5/5 — 已确认的 bug,最小化修复,测试完整,无保留意见。 这是一个教科书式的小修复:一个守卫子句、一个共享常量、四个测试。issue #7543 有清晰的复现( — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
Review & Local Verification Report代码审查设计评价:完备的防御性修复。 本 PR 解决了版本管理器(mise/asdf/proto)将 修复策略: const resolved = fs.realpathSync(adjacentNpm);
if (resolved.endsWith('.js')) return resolved; // 真正的 npm-cli.js
return npmCliJs; // 回退到标准路径亮点:
注意: 本 PR 与 #7544 解决同一问题,但本 PR 测试更完备(4 cases vs 1 case),注释更详细。 结论LGTM。 推荐本 PR 而非 #7544(测试覆盖更全面)。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
What this PR does
The update check resolves the npm CLI path before spawning a Node process to query the registry. It now validates that the resolved path points to a JavaScript file. When a version manager has replaced the npm binary with a shell wrapper, the conventional
npm-cli.jslocation is used instead. Four unit tests cover the symlink, shell-wrapper, missing-file, and Windows cases.Why it's needed
Users who manage Node through mise see "registry error" on every startup and every
/updateattempt. The registry is reachable and the package resolves correctly from the command line. The error is misleading because the actual failure is a SyntaxError from Node trying to parse a bash script as JavaScript — the error classifier doesn't recognize it as a network or timeout issue, so it falls through to the default "registry" label. The same mechanism affects any version manager that wrapsbin/npmwith a non-JS script (asdf, proto).Reviewer Test Plan
How to verify
Run the unit tests for the affected module:
All 36 tests pass (32 existing + 4 new). The new tests mock
realpathSyncto return a non-.jspath (simulating mise's bash shim) and assert the fallbacknpm-cli.jspath is returned instead.Evidence (Before & After)
N/A — non-UI change.
Tested on
Environment
Unit tests only (
vitest run).Risk & Scope
<prefix>/lib/node_modules/npm/bin/npm-cli.js) is the same one already used whenrealpathSyncthrows. The only behavioral change is that a non-.jsresolved path now triggers the fallback instead of being returned as-is.Linked Issues
Closes #7543
中文说明
此 PR 做了什么
更新检查在生成 Node 进程查询 registry 之前会解析 npm CLI 路径。现在它会验证解析后的路径是否指向 JavaScript 文件。当版本管理器将 npm 二进制文件替换为 shell 包装脚本时,将改用标准的
npm-cli.js路径。新增四个单元测试,分别覆盖符号链接、shell 包装脚本、文件不存在和 Windows 四种情况。为什么需要
通过 mise 管理 Node 的用户在每次启动和每次执行
/update时都会看到 "registry error"。registry 可以正常访问,包也能通过命令行正确解析。这个错误具有误导性,因为实际的失败是 Node 试图将 bash 脚本当作 JavaScript 解析时抛出的 SyntaxError——错误分类器无法将其识别为网络或超时问题,因此回退到默认的 "registry" 标签。同样的机制会影响任何用非 JS 脚本包装bin/npm的版本管理器(asdf、proto)。审查者测试计划
如何验证
运行受影响模块的单元测试:
全部 36 个测试通过(32 个已有 + 4 个新增)。新测试 mock
realpathSync返回非.js路径(模拟 mise 的 bash shim),并断言返回的是回退的npm-cli.js路径。证据(修改前后对比)
N/A — 非 UI 变更。
测试平台
环境
仅单元测试(
vitest run)。风险与范围
<prefix>/lib/node_modules/npm/bin/npm-cli.js)与realpathSync抛出异常时已使用的路径相同。唯一的行为变化是:当解析后的路径不是.js文件时,现在会触发回退,而不是原样返回。关联 Issue
Closes #7543