Fix(cli): use npm view for update check instead of update-notifier (#7515) - #7528
Conversation
…M#7515) npm 11+ prints `npm view <pkg> dist-tags.<tag> --json` as ["0.20.1"] instead of "0.20.1", so the strict string check re-broke the update check with "Invalid npm latest version response". Accept both shapes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the PR, @dtometzki! Template looks good ✓ Problem: Observed bug with solid evidence. Issue #7515 includes curl reproduction showing registry.npmjs.org returns HTTP 406 for the Direction: Aligned. The update check is core functionality, and the fix reuses the existing Size: Not applicable — changes are in Approach: The scope feels exactly right. Two files, one production + one test, doing the minimum: remove the Moving on to code review. 🔍 中文说明感谢贡献,@dtometzki! 模板完整 ✓ 问题: 已观测到的 bug,证据充分。Issue #7515 包含 curl 复现,显示 registry.npmjs.org 对 方向: 对齐。更新检查是核心功能,修复复用了已有的 规模: 不适用——改动在 方案: 范围恰到好处。两个文件,一个生产代码 + 一个测试,做最小改动:移除 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: Given the 406 breakage, I would remove the Findings: No critical blockers. The change is clean and minimal:
One observation (non-blocking): Real-Scenario TestingRoot cause confirmationBefore (main branch) — npm 11+ array format breaksAfter (this PR) — npm 11+ array format handledUpdate check end-to-end (both versions, npm 10 environment)Both succeed on npm 10.9.8 because Unit tests中文说明代码审查独立方案: 鉴于 406 错误,我会移除 发现: 无关键阻塞。改动干净且最小:
一个观察(非阻塞): 实际场景测试
— Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — Clean fix for a real bug; minimal, correct, well-tested. Would merge without hesitation. This is exactly the kind of PR I like to see: the author hit a real bug, filed a clear issue with curl reproduction, then submitted a focused fix that reuses an existing working mechanism instead of adding something new. Two files, one production + one test, no scope creep. The code is simpler after this change than before — one less branch, one less runtime dependency for version checking. The npm 11+ array format handling is a small but important addition that I verified independently: the main branch throws "Invalid npm latest version response" on The one thing I couldn't reproduce on this machine (npm 10.9.8) is the exact 406 failure with 140 tests pass across all 6 related test files. No regressions. 中文说明置信度:5/5 — 干净地修复了真实 bug;最小、正确、充分测试。毫不犹豫地合并。 这正是我喜欢的 PR 类型:作者遇到了真实 bug,提交了带 curl 复现的清晰 issue,然后提交了一个聚焦的修复,复用已有的工作机制而非添加新东西。两个文件,一个生产 + 一个测试,无范围蔓延。 改动后代码比之前更简单——少一个分支,少一个运行时依赖。npm 11+ 数组格式处理是小但重要的补充,我独立验证了:main 分支对 在此机器(npm 10.9.8)上无法复现 140 个测试全部通过,无回归。 — 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. ✅
|
Thanks for this — the core approach (routing all installs through 1. #7520's global-mode rejection is not handled Issue #7520 has two root causes. The array shape ( A targeted fix: catch the rejection (both as a process error and as a JSON error object in stdout) and retry without let output: string;
try {
output = await runGlobalNpm([...viewArgs, '--global'], run);
} catch (error) {
if (!failedWithGlobalModeViewRejection(error)) throw error;
output = await runGlobalNpm(viewArgs, run);
}
if (output === '') return noNewerVersion;
let parsed: unknown = JSON.parse(output);
if (isGlobalModeViewRejection(parsed)) {
output = await runGlobalNpm(viewArgs, run);
if (output === '') return noNewerVersion;
parsed = JSON.parse(output);
}This preserves 2. The runtime import is removed but the dependency remains, so 3. Dead code: These are still exported and tested but no longer called by 4. Array handling is good The Overall this is a solid, minimal fix for #7515. With the global-mode retry added, it would also fully resolve #7520. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
…n code (QwenLM#7515) Version checking now goes through npm view for every install type, so: - replace the update-notifier UpdateInfo type import with a local interface and remove update-notifier / @types/update-notifier from dependencies - remove isGlobalNpmInstallation and looksLikeNpmPackagePath, which no longer have any production callers, along with their tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/review |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
|
Hi @wenshao, ready for approval :-) |
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 用 动机: 变更:
亮点: 依赖注入使测试更直接,不再需要 结论LGTM。 减少 bundle 体积,消除不必要的第三方依赖,测试更清晰。 |
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
| export interface UpdateInfo { | ||
| latest: string; | ||
| current: string; | ||
| type: string; | ||
| name: string; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The type and name fields on the UpdateInfo interface are never read by any production consumer — they are dead fields inherited from the old update-notifier type shape. — Concrete cost: fetchGlobalNpmUpdateInfo hardcodes type: 'latest' and echoes packageName as name on every return path. All downstream consumers (handleAutoUpdate.ts, update-relaunch.ts, commands/update.ts, ui/commands/update-command.ts) read only .latest. Tests assert the presence of these fields, so future developers must populate and verify values that serve no purpose.
| export interface UpdateInfo { | |
| latest: string; | |
| current: string; | |
| type: string; | |
| name: string; | |
| } | |
| export interface UpdateInfo { | |
| latest: string; | |
| current: string; | |
| } |
— qwen3.7-max via Qwen Code /review
| const createNotifier = (distTag: 'latest' | 'nightly') => ({ | ||
| fetchInfo: () => fetchGlobalNpm(name, version, distTag), | ||
| }); |
There was a problem hiding this comment.
[Suggestion] After removing the update-notifier branch, createNotifier is a factory wrapping a single function call in a throwaway object. The { fetchInfo() } shape was designed to abstract over two implementations; with one remaining, the abstraction adds indirection without value. — Concrete cost: a developer modifying the fetch logic must update three sites (the function, the factory, and the fetchInfoWithTimeout parameter type) instead of one.
| const createNotifier = (distTag: 'latest' | 'nightly') => ({ | |
| fetchInfo: () => fetchGlobalNpm(name, version, distTag), | |
| }); | |
| const fetchGlobalNpmWithArgs = (distTag: 'latest' | 'nightly') => | |
| fetchGlobalNpm(name, version, distTag); |
Then change fetchInfoWithTimeout to accept () => UpdateInfo | Promise<UpdateInfo> directly, and pass () => fetchGlobalNpm(name, version, 'latest') at each call site.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
Maintainer verification — real local build & testsI built this PR from a clean Verdict: safe to merge, and it does fix #7515 — but the fix that actually resolves the reporter's error is the npm‑12 array handling, not the Environment
1) The reporter's actual bug is reproduced and fixed: npm‑12 array outputThe issue quotes the error
A/B of the real
This is the load-bearing fix (commit 2) The
|
|
Hi @wenshao, Updated the PR description and code comment per your review — root cause reframed, stale wording fixed, npm 12+ corrected. |
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
|
Thanks @dtometzki for tracking this down and following through on the review feedback. The npm 12 singleton-array handling is correct, and removing |


What this PR does
runGlobalNpmandfetchGlobalNpmUpdateInforemain and are stillexported for their own use/tests.
isGlobalNpmInstallationandlooksLikeNpmPackagePathare removed as dead code in a follow-upcommit, since
checkForUpdatesDetailedwas their only productionconsumer once the branch they served no longer exists.
Why it's needed
Two independent issues currently break the update check:
npm 12 array output (the actual cause of the reported error).
npm view <pkg> dist-tags.<tag> --jsonreturns a bare string onnpm 10/11 (
"0.20.1"), but wraps it in an array on npm 12+(
["0.20.1"]).fetchGlobalNpmUpdateInfo's strict string checkthrows on the array shape, producing the reported
Failed to check for updates: Error: Invalid npm latest version response. This is the root cause confirmed against the reporter'snpm 12.0.1 environment.
update-notifier's corgi Accept header can 406.update-notifier'sfetchInfo()sendsAccept: application/vnd.npm.install-v1+jsonandregistry.npmjs.orgcan answer that with an emptyHTTP 406onsome paths (see npm/cli#7529).
With the
package-jsonversion currently locked in this repo,the request instead sends a multi-value Accept header with
fallbacks and gets
HTTP 200— so this path is not currentlyfailing here, but removing it eliminates the dependency on that
content-negotiation behavior entirely.
This PR fixes (1) directly (accepting single-element array output)
and addresses (2) as hardening by routing all installs through the
already-tested
npm view-based path (fetchGlobalNpmUpdateInfo)instead of maintaining two fetch mechanisms.
Reviewer Test Plan
How to verify
Reproduce the registry behavior directly (independent of this repo):
curl -s -i -H "Accept: application/vnd.npm.install-v1+json" https://registry.npmjs.org/@qwen-code/qwen-code/latestreturnsHTTP/2 406with an empty body, whilecurl -s https://registry.npmjs.org/@qwen-code/qwen-code/latest(no special header) returns a full200JSON response. Before this change, a non-global install hitting/updateor starting up would log[UPDATE_CHECK] Failed to check for updates: Error: Invalid npm latest version response. After this change, the check instead performs annpm view @qwen-code/qwen-code dist-tags.latest --jsoncall, which is unaffected by the 406 and returns the correct version.Evidence (Before & After)
N/A — non-UI change, the update check's user-visible output (the "update available" message /
/updatecommand output) is unchanged in shape; only the underlying fetch mechanism changed. Verified via automated tests instead (see below):npx eslintandnpx prettier --checkon both changed files: clean, no errors.Tested on
Environment (optional)
npm install --ignore-scriptsat repo root, thennpx vitest run/npx eslint/npx prettier --checkdirectly inpackages/cli. No sandbox/Docker needed — unit tests only, no live network calls in the test suite itself (registry behavior was confirmed separately viacurl, outside the test run).Risk & Scope
npm view(a child process) instead of making an in-process HTTP request for non-global installs; this is slightly heavier per check but bounded by the existingFETCH_TIMEOUT_MS(5s) and matches what was already done for global installs.fetchGlobalNpmUpdateInfoalready has existing test coverage for empty dist-tag responses from mirrors.checkForUpdatesDetailed()andcheckForUpdates()no longer accept adetectGlobalNpmparameter — every current call site in the codebase calls them with zero arguments, so nothing breaks; only test code that explicitly injected a customdetectGlobalNpmneeded updating (done as part of this PR).Linked Issues
Fixes #7515