fix(cli): resolve npm wrappers to npm-cli.js - #7544
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with a clear reproduction — issue #7543 documents mise placing a bash wrapper at Direction: aligned — update checking is a core feature and this fixes it for mise (and potentially asdf/proto) users. The issue was already triaged and a community PR was explicitly welcomed. Size: not applicable (no core paths touched — Approach: the scope is exactly right. Validate the resolved path ends with Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,有明确复现——issue #7543 记录了 mise 在 方向:对齐——更新检查是核心功能,此修复惠及 mise(以及可能的 asdf/proto)用户。issue 已被分类,明确欢迎社区 PR。 规模:不适用(未触及核心路径——仅 方案:范围恰好。验证解析后的路径以 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the problem (mise's bash wrapper at Comparison with the diff: the PR does exactly this — no more, no less. The No critical blockers. No convention violations. The existing comment block explaining the non-async call site is preserved. The new test covers the mise wrapper scenario; the 32 existing tests cover the unchanged paths. Real-Scenario Testingtmux is not available on this CI runner, so I ran a standalone before/after test with a simulated mise-style layout ( Unit tests with PR code: 33/33 passed (32 existing + 1 new regression test). 中文说明代码审查独立方案: 针对问题(mise 的 bash 包装脚本通过 与 diff 对比: PR 完全按照这个方案实现——不多不少。 无关键阻塞项。无规范违反。现有的解释非异步调用点的注释块被保留。新测试覆盖了 mise 包装脚本场景;32 个现有测试覆盖了未变更的路径。 真实场景测试此 CI 运行器无 tmux,因此使用模拟的 mise 风格布局运行了独立的 before/after 测试( PR 代码单元测试:33/33 通过(32 个现有 + 1 个新回归测试)。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — textbook minimal bugfix: observed bug, clear reproduction, one-line guard, no scope creep. This is exactly the kind of PR that's easy to review and easy to maintain. The problem is real (mise users get a bogus "registry error" on every startup), the fix is the smallest thing that works ( My independent proposal was identical to what the PR does — no simpler path exists. If I had to maintain this in six months, I'd thank the author: one guard condition, one hoisted variable for DRY, done. 中文说明置信度:5/5 — 教科书式的最小修复:已观测到的 bug、明确的复现、一行守卫、无范围蔓延。 这正是容易审查、容易维护的 PR。问题是真实的(mise 用户每次启动都看到虚假的"仓库错误"),修复是最小的可行方案( 我的独立方案与 PR 完全一致——不存在更简路径。如果六个月后维护这段代码,我会感谢作者:一个守卫条件、一个提升的变量消除重复、完事。 — 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.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| describe('getNpmCliPath', () => { | ||
| it('falls back to npm-cli.js when adjacent npm is a wrapper', () => { | ||
| mockedRealPathSync.mockReturnValue('/prefix/bin/npm'); |
There was a problem hiding this comment.
[Suggestion] The .endsWith('.js') ternary has two branches, but only the fallback (wrapper) branch is tested. There is no test for the happy path where realpathSync resolves to a .js file — the standard npm install case.
Failure scenario: if a future change removes the .endsWith('.js') guard (e.g., simplifies to return npmCliPath), this test still passes because it asserts the fallback value. On a standard npm install where the symlink resolves to a real .js file at a non-standard location, the function would silently return the wrong path with no test to catch it.
| describe('getNpmCliPath', () => { | |
| it('falls back to npm-cli.js when adjacent npm is a wrapper', () => { | |
| mockedRealPathSync.mockReturnValue('/prefix/bin/npm'); | |
| describe('getNpmCliPath', () => { | |
| it('returns the resolved path when adjacent npm is a real symlink', () => { | |
| mockedRealPathSync.mockReturnValue( | |
| '/prefix/lib/node_modules/npm/bin/npm-cli.js', | |
| ); | |
| expect(getNpmCliPath('/prefix/bin/node', 'linux')).toBe( | |
| '/prefix/lib/node_modules/npm/bin/npm-cli.js', | |
| ); | |
| }); | |
| it('falls back to npm-cli.js when adjacent npm is a wrapper', () => { | |
| mockedRealPathSync.mockReturnValue('/prefix/bin/npm'); | |
| expect(getNpmCliPath('/prefix/bin/node', 'linux')).toBe( | |
| '/prefix/lib/node_modules/npm/bin/npm-cli.js', | |
| ); | |
| }); | |
| }); |
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.8-max-preview via Qwen Code /review
| it('falls back to npm-cli.js when adjacent npm is a wrapper', () => { | ||
| mockedRealPathSync.mockReturnValue('/prefix/bin/npm'); | ||
|
|
||
| expect(getNpmCliPath('/prefix/bin/node', 'linux')).toBe( |
There was a problem hiding this comment.
[P1] This Linux-path test is host-path-dependent and will fail when the test suite runs on Windows. getNpmCliPath('/prefix/bin/node', 'linux') uses the host's path.join, so on Windows it returns a backslash-separated path rather than the asserted POSIX path. Use platform-specific path handling in the implementation or make the expectation host-aware.
— codex-cli 0.144.6 (gpt-5.6-sol) via Qwen Code /review
yiliang114
left a comment
There was a problem hiding this comment.
Codex review complete. One P1 finding posted inline — Linux-path test is host-path-dependent and will fail on Windows. Implementation looks correct otherwise.
— codex-cli 0.144.6 (gpt-5.6-sol) via Qwen Code /review
|
Thanks for jumping on this so quickly! The fix is spot-on — clean one-liner guard, hoisted fallback variable, exactly the minimal change needed. Really appreciate you getting it out within hours of the issue landing. We ended up going with #7545 since it covers the same logic with a few extra regression tests (symlink happy path, ENOENT, Windows), but your PR was equally correct and the before/after mise simulation was a nice touch. Hope to see more contributions from you! 🙏 |
|
Thanks for the kind feedback! I completely understand going with #7545 for the broader test coverage. I really appreciate you taking the time to review it, and I'll keep an eye out for similar issues. |
What this PR does
The update checker now ignores a non-JavaScript
npmwrapper next to the Node executable and uses the conventionalnpm-cli.jspath instead. Normal Node installations whose adjacentnpmsymlink resolves to JavaScript keep the existing behavior.Why it's needed
Version managers such as mise can place a shell wrapper at
bin/npm. Passing that wrapper to Node produces a syntax error, so startup checks and/updateincorrectly report a registry failure even when npm is reachable.Reviewer Test Plan
How to verify
Resolve the npm CLI path with a Node executable whose adjacent
npmentry resolves to a shell wrapper. It should return the conventionallib/node_modules/npm/bin/npm-cli.jspath. An adjacent entry that resolves tonpm-cli.jsshould still be preferred.Evidence (Before & After)
Before: the reproduced mise-style layout selected
/usr/bin/bashas the npm CLI. After: the focused regression and all 33 installation-info tests pass, and the resolver selectsnpm-cli.js.Tested on
Environment (optional)
Node.js 22.22.1 with the repository lockfile dependencies.
Risk & Scope
.jssuffix now use the standard fallback path.Linked Issues
Fixes #7543
中文说明
本 PR 的修改
更新检查器现在会忽略 Node 可执行文件旁边的非 JavaScript
npm包装脚本,改用常规的npm-cli.js路径。对于相邻npm符号链接解析到 JavaScript 文件的普通 Node 安装,现有行为保持不变。修改原因
mise 等版本管理器可能会在
bin/npm放置 shell 包装脚本。将该脚本传给 Node 会触发语法错误,因此即使 npm 仓库可访问,启动检查和/update也会错误地报告仓库故障。审阅者测试计划
验证方法
使用一个相邻
npm条目解析到 shell 包装脚本的 Node 可执行文件来解析 npm CLI 路径。结果应为常规的lib/node_modules/npm/bin/npm-cli.js路径。解析到npm-cli.js的相邻条目仍应优先使用。修改前后证据
修改前:复现的 mise 风格布局错误地选择
/usr/bin/bash作为 npm CLI。修改后:针对性回归测试和全部 33 个安装信息测试通过,解析器选择npm-cli.js。测试环境
Node.js 22.22.1,使用仓库锁文件依赖。
风险与范围
.js后缀的自定义 npm 入口现在会使用标准回退路径。关联 Issue
Fixes #7543