fix(cli): align npm update checks with global registry - #7224
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: Observed bug with a clear reproduction. Issue #7151 reports a failure during upgrade, and the PR describes the exact scenario: the update check reads the project's Direction: Aligned — this fixes a concrete user-facing bug in the update flow. The update check and installation must use the same registry configuration; offering an update that can't be installed is broken behavior. Size: Not applicable — all changes are in Approach: The scope feels right. The PR addresses the registry mismatch at both the check path (query versions through the same npm CLI used for installation) and the install path (invoke npm through the active Node.js runtime instead of a shell). The Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，有明确复现。Issue #7151 报告了升级时的错误，PR 描述了具体场景：更新检查读取项目的 方向：对齐——修复更新流程中具体的用户可见 bug。更新检查和安装必须使用相同的 registry 配置；提示一个配置源无法安装的版本是错误行为。 规模：不适用——所有改动在 方案：范围合理。PR 在检查路径（通过与安装相同的 npm CLI 查询版本）和安装路径（通过当前 Node.js 运行时执行 npm 而非 shell）两处修复了 registry 不一致。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: Given "update check uses project .npmrc but installation uses global npm config", I would: (1) detect global npm installation by comparing the resolved CLI path against Comparison with the diff: The PR's approach matches this proposal closely. The implementation is clean and well-structured:
No critical blockers or AGENTS.md violations found. One non-blocking observation: Real-Scenario TestingUnit tests: 125 tests pass across all 5 changed test files (updateCheck: 29, handleAutoUpdate: 26, update-relaunch: 6, relaunch: 15, gemini: 49). Typecheck clean. CLI smoke test (built from PR code): Direct function verification: All three new functions work correctly in a real environment: 中文说明代码审查独立方案： 针对"更新检查使用项目 .npmrc 但安装使用全局 npm 配置"的问题，我的方案是：(1) 通过比较解析后的 CLI 路径与 与 diff 的比较： PR 方案与我的独立提案高度一致。实现干净、结构良好：
未发现关键阻塞问题或 AGENTS.md 违规。一个非阻塞观察： 真实场景测试单元测试：125 个测试全部通过（updateCheck: 29, handleAutoUpdate: 26, update-relaunch: 6, relaunch: 15, gemini: 49）。类型检查通过。 CLI 冒烟测试（从 PR 代码构建）：正常启动并响应。三个新函数在真实环境中均正确工作。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 4/5 — solid fix for a real bug; clean implementation with thorough tests, only non-blocking nit on the arg-splitting pattern. This PR solves a problem I can immediately recognize as real: the update check and the update installation disagree on which registry to use, so users get offered a version they can't install. The reproduction in the PR description is concrete (0.19.12 sees 0.20.0 via project npmjs, but The implementation matches my independent proposal almost exactly. The detection logic ( The code is straightforward to maintain. Nothing feels over-engineered — the DI parameters exist because the tests need them, the comments explain non-obvious decisions (why The only nit: 中文说明置信度：4/5 — 对真实 bug 的扎实修复；实现干净、测试充分，仅有参数拆分模式的非阻塞小问题。 这个 PR 解决了一个我立刻能认定为真实的问题：更新检查和更新安装对使用哪个 registry 不一致，导致用户被提示一个无法安装的版本。PR 描述中的复现具体（0.19.12 通过项目 npmjs 看到 0.20.0，但 实现与我的独立方案几乎完全一致。检测逻辑（ 代码易于维护。没有过度工程——DI 参数因测试需要而存在，注释解释了非显而易见的决策，测试覆盖充分（125 个测试全部通过）。 唯一的小问题： — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
The main flow looks right to me — routing both the version check and the install through the same npm --global CLI invocation is the right fix for the ETARGET mismatch: check and install now resolve the same registry by construction, so they can no longer disagree.
I traced the return false → true changes in update-relaunch.ts against the caller in gemini.tsx (return shouldRelaunch ? UPDATE_COMPLETE_EXIT_CODE : 0). The intent is consistent — relaunch the existing version on update/check failure instead of dropping the session — and the standalone-deferred path still correctly returns false, so the boolean stays meaningful.
One non-blocking nit: in fetchGlobalNpmUpdateInfo, type: 'latest' is hardcoded even when distTag is 'nightly'. Harmless today since the downstream only reads .latest, but it's misleading if anything starts consuming .type later — might be worth deriving it from distTag.
Nothing blocks merge.
Code Review —
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
yiliang114
changed the title
|
Validation update for
The three latest automated review notes are coverage-only Suggestions rather than defects in the current behavior: the no-update-command return permutations, the unchanged non-npm shell invocation branch, and the negative global-path containment branch. This PR has already exceeded the repository's review-round threshold, so I am deferring those additional test-only expansions to a focused follow-up instead of widening this update-safety change. No Critical finding remains. |
This reverts commit 5c5b529.
yiliang114
changed the title
|
Follow-up hardening on this PR:
Added a regression test that a |
Re-review (updated PR)Re-reviewed after the latest push (head Prior concerns — addressed 👍
New concern (introduced by this revision) — please look at this one
return fs.realpathSync(path.join(path.dirname(nodePath), 'npm')); // non-win32and
Also note the Windows/Unix asymmetry: Residual minor points
VerdictThe relaunch-semantics rework is a real improvement and resolves my main earlier feedback. The one thing I'd change before merge is the Unix 中文小结本次更新总体是进步：
|
|
Closed the current review round with commit e7f0e81. Fixed:
The remaining unresolved-looking update-relaunch thread was outdated on the current diff, so I resolved it without code changes. Verification:
Note: npm -w packages/cli run typecheck is blocked in this temporary worktree by broader generated package/type resolution issues outside these two test files. |
…paths - isGlobalNpmInstallation now realpath-resolves argv[1] before matching the npm package path (consistent with getInstallationInfo), so a bin-symlink launch no longer silently skips the global-npm update path. - getNpmCliPath falls back to <prefix>/lib/node_modules/npm/bin/npm-cli.js on POSIX instead of throwing when npm is not adjacent to node. - fetchGlobalNpmUpdateInfo treats an empty `npm view dist-tags.<tag>` response as no-update instead of failing the whole check, so a missing nightly tag on a private mirror no longer poisons the latest check.
|
One more edge, pushed in ee56a77: |
Re-review (head
|
The QwenLM#6857 timeout tests called checkForUpdatesDetailed() with the default detectGlobalNpm (real isGlobalNpmInstallation), which runs a real async realpath() before the timeout setTimeout is armed. Under fake timers this races with vi.advanceTimersByTimeAsync: on a slow/loaded runner the advance completes before the timer is scheduled, so the timeout never fires and the test hangs until vitest's 15s limit. Seen on ubuntu-latest CI (2 of 4 hung). Inject a synchronous `async () => false` stub so the timer is armed deterministically and the tests isolate the timeout logic from global-npm detection.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the PR adds Windows-specific spawn/path code (path.win32, cmd.exe branch) and the suite ran on Linux only.
— qwen3.8-max-preview via Qwen Code /review
| } catch { | ||
| return path.join( | ||
| path.dirname(nodePath), | ||
| '..', |
There was a problem hiding this comment.
[Suggestion] The non-win32 fallback branch of getNpmCliPath (taken when fs.realpathSync throws because npm isn't symlinked adjacent to node) is not exercised by any test — installationInfo.test.ts doesn't import getNpmCliPath, and the handleAutoUpdate/updateCheck tests run where npm IS adjacent to node, so only the realpathSync-success path is hit. Concrete cost: if a future refactor alters this fallback path (e.g. drops the '..' segment), no test fails — the update spawn would ENOENT on every split-layout system (node at /usr/bin/node, npm elsewhere), surfacing only as a user-facing update failure. Consider adding a unit test that mocks fs.realpathSync to throw and asserts the returned path matches the conventional <prefix>/lib/node_modules/npm/bin/npm-cli.js shape:
it('falls back to the conventional prefix layout when npm is not adjacent to node', () => {
vi.spyOn(fs, 'realpathSync').mockImplementation(() => { throw new Error('ENOENT'); });
expect(getNpmCliPath('/usr/bin/node', 'linux')).toBe(
path.join('/usr', 'lib', 'node_modules', 'npm', 'bin', 'npm-cli.js'),
);
});— qwen3.8-max-preview via Qwen Code /review
| getNpmCliPath(process.execPath, platform), | ||
| ...updateCommand.split(' ').slice(1), |
There was a problem hiding this comment.
[Suggestion] updateCommand.split(' ').slice(1) rebuilds the npm argument array by space-splitting a string that was designed for shell execution. The same updateCommand string is also shown verbatim to the user via formatUpdateInstructions, so the human-readable command and this machine-parsed arg array share a format contract (exactly four space-free tokens) that is enforced nowhere at the point of use. Today the interpolated version is semver.gt-validated upstream so it can't contain spaces and the split is safe; but a future edit to the template in getInstallationInfo (e.g. a --prefix /some path flag, a npx npm wrapper, or any space-containing value) would silently produce wrong npm arguments — npm would fail or install the wrong target, surfaced only as the generic "Automatic update failed". The non-npm branch passes the whole string to a shell, so the two branches already diverge in how they treat the same value. Consider building the npm args directly (e.g. [getNpmCliPath(process.execPath, platform), 'install', '-g', packageName + '@' + resolvedVersion]) or returning a structured updateArgs from getInstallationInfo; at minimum, document the assumed format and its single producer here.
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /triage |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
| setWorkflowKeywordActive(false); | ||
| } | ||
| }, [isIdle, buffer.text, isProcessing, messageQueue.length]); |
There was a problem hiding this comment.
[Critical] Widening this effect's dependency array from [streamingState] to [isIdle, buffer.text, isProcessing, messageQueue.length] causes setWorkflowKeywordActive(false) to fire at submission time instead of when the steered turn finishes. — Failure scenario: user submits a workflow-keyword prompt → handleSubmitAndClear clears buffer.text and enqueues the message (changing messageQueue.length) in the same batch → the next render still has isIdle === true (streamingState hasn't changed yet) → the effect re-runs on the changed deps, hits the if (isIdle) branch, and clears the indicator immediately. The old [streamingState] dep did not re-run on that render, so the indicator persisted through the turn.
| setWorkflowKeywordActive(false); | |
| } | |
| }, [isIdle, buffer.text, isProcessing, messageQueue.length]); | |
| setWorkflowKeywordActive(false); | |
| } | |
| }, [isIdle]); |
Decouple the two responsibilities: keep flush() on the widened deps (it is idempotent), but clear workflowKeywordActive in a separate effect keyed only on isIdle (or on streamingState transitioning to Idle), matching the prior behavior.
— qwen3.8-max-preview via Qwen Code /review
| if (process.env[SKIP_INITIAL_PROMPT_ENV_VAR] === 'true') { | ||
| if (process.env['QWEN_CODE_NO_RELAUNCH'] || process.env['SANDBOX']) { | ||
| delete process.env[SKIP_INITIAL_PROMPT_ENV_VAR]; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The main() function's consumption of SKIP_INITIAL_PROMPT_ENV_VAR (clearing prompt, promptInteractive, query) has no test. Entry-point propagation is tested in cli.test.ts and sandbox passthrough in sandbox.test.ts, but the actual prompt-clearing behaviour in main() is unexercised. — Concrete cost: if a future refactor drops query: undefined (or the entire block), a relaunched session would re-execute the original --query or --prompt argument despite the supervisor signalling it was already consumed. No test would catch this regression.
— qwen3.8-max-preview via Qwen Code /review
| function withResumeSession(args, sessionId) { | ||
| const result = []; | ||
| for (let i = 0; i < args.length; i++) { | ||
| const arg = args[i]; |
There was a problem hiding this comment.
[Suggestion] withResumeSession strips six conflicting session flag forms (--continue, -c, --resume, -r, --session-id, --sandbox-session-id, plus =-suffixed variants) before appending --resume <id>. The only test exercising this function passes ['--prompt', 'a&b'] — args with no conflicting flags — so none of the stripping logic is exercised. — Concrete cost: if the stripping of --continue or --sandbox-session-id is broken, a relaunched session would receive both --continue and --resume <id>, causing the CLI to resume the wrong session or error on conflicting flags. No test would detect this.
— qwen3.8-max-preview via Qwen Code /review
| export function getNpmCliPath( | ||
| nodePath = process.execPath, | ||
| platform = process.platform, | ||
| ): string { |
There was a problem hiding this comment.
[Suggestion] getNpmCliPath has no direct unit test. The Windows branch (path.win32.join) is only exercised through a mock in updateCheck.test.ts that replaces the function entirely. The Unix branch (fs.realpathSync) is exercised indirectly in handleAutoUpdate.test.ts via vi.importActual, but only as a side effect of the spawn-args assertion. — Concrete cost: if the Windows path construction is wrong (e.g., npm changes its layout from node_modules/npm/bin/npm-cli.js), the mocked test would still pass while the real function produces a nonexistent path, causing handleAutoUpdate to fail to spawn the update process on Windows.
— qwen3.8-max-preview via Qwen Code /review
| stdio: 'inherit', | ||
| env: newEnv, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] relaunch.test.ts is inert — all 14 remaining tests pass with the IPC-based update-on-exit code reverted, so no test guards the removal of the IPC channel and message handler from relaunchAppInChildProcess. — Concrete cost: the PR removed the 'ipc' stdio entry and the child.on('message', …) handler. The surviving test only asserts onUpdateRelaunch is not called on exit 0, which is trivially true because no IPC message is ever emitted. If a future change accidentally re-introduces the IPC channel, no test will fail.
— qwen3.8-max-preview via Qwen Code /review
| if ( | ||
| installInfo.isStandalone && | ||
| installInfo.standaloneDir && | ||
| os.platform() === 'win32' | ||
| ) { | ||
| return updateStandalone(); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The Windows standalone interactive update path bypasses the isAutoUpdateEnabled check, downloading and staging an update even when the user has set enableAutoUpdate: false. — Failure scenario: a user on a Windows standalone installation sets enableAutoUpdate: false, runs /update. The old code routed through isAutoUpdateEnabled && canAutoUpdate, falling through to manualInstructions() when disabled. The new code places the os.platform() === 'win32' standalone check before the isAutoUpdateEnabled guard (line 166), so updateStandalone() runs unconditionally.
| if ( | |
| installInfo.isStandalone && | |
| installInfo.standaloneDir && | |
| os.platform() === 'win32' | |
| ) { | |
| return updateStandalone(); | |
| } | |
| if ( | |
| isAutoUpdateEnabled && | |
| installInfo.isStandalone && | |
| installInfo.standaloneDir && | |
| os.platform() === 'win32' | |
| ) { | |
| return updateStandalone(); | |
| } |
— qwen3.8-max-preview via Qwen Code /review
| const [resolvedCliPath, unresolvedGlobalRoot] = await Promise.all([ | ||
| canonicalize(cliPath), | ||
| runGlobalNpm(['root', '--global'], run), | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] isGlobalNpmInstallation does not guard the Promise.all against ENOENT from runGlobalNpm → getNpmCliPath, even though it already guards the subsequent canonicalize(unresolvedGlobalRoot) call against the same error class. — Failure scenario: on a non-Windows host where the CLI path passes looksLikeNpmPackagePath but no npm symlink exists next to process.execPath (minimal Node.js install, broken symlink), getNpmCliPath throws ENOENT. The error propagates uncaught into checkForUpdatesDetailed's outer catch, returning { status: 'error' } — the user sees "Failed to check for updates" on every startup instead of falling back to update-notifier.
| const [resolvedCliPath, unresolvedGlobalRoot] = await Promise.all([ | |
| canonicalize(cliPath), | |
| runGlobalNpm(['root', '--global'], run), | |
| ]); | |
| let resolvedCliPath: string; | |
| let unresolvedGlobalRoot: string; | |
| try { | |
| [resolvedCliPath, unresolvedGlobalRoot] = await Promise.all([ | |
| canonicalize(cliPath), | |
| runGlobalNpm(['root', '--global'], run), | |
| ]); | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') return false; | |
| throw error; | |
| } |
— qwen3.8-max-preview via Qwen Code /review
| fs.writeFileSync( | ||
| statePath, | ||
| JSON.stringify({ sessionId, skipInitialPrompt }), |
There was a problem hiding this comment.
[Suggestion] Unguarded fs.writeFileSync in relaunchForUpdate aborts the entire update relaunch (skipping runExitCleanup() and process.exit(UPDATE_RELAUNCH_EXIT_CODE)) if the state-file write fails. The IPC-based predecessor wrapped process.send in a try/catch and degraded gracefully. — Failure scenario: the temp directory holding the state file is cleaned by an OS tmp-reaper between the supervisor's mkdtempSync and this write, or the tmp filesystem is full (ENOSPC). writeFileSync throws, process.exit(43) is never reached, the parent never sees exit code 43, and the already-staged update is not installed.
| fs.writeFileSync( | |
| statePath, | |
| JSON.stringify({ sessionId, skipInitialPrompt }), | |
| try { | |
| fs.writeFileSync( | |
| statePath, | |
| JSON.stringify({ sessionId, skipInitialPrompt }), | |
| { encoding: 'utf8', mode: 0o600 }, | |
| ); | |
| } catch { | |
| // State handoff failed — proceed without session resume. | |
| } |
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Post-merge smoke test on Windows: the fix is partial — update check still throws Setup: clean What I measured (PowerShell
Peeking at the installed bundle, the 2-second budget is still in the code with no user-side override path:
As a workaround test, switching Happy to test a follow-up PR if useful. |
|
Thanks for the detailed repro — the timeout gap is tracked in #7049 (raise budget + soften the error styling). The registry/auth fix here was orthogonal; the follow-up for the 2s→5s bump and warning UX is already assigned. |
|
Released in v0.20.1. |
4 participants
What this PR does
This PR makes global npm installations use the same npm configuration scope for update checks and installation.
Global installations query versions through the trusted npm CLI, matching the registry and authentication configuration used by
npm install --global. Both the check and installation paths invoke the npm CLI through the active Node.js runtime without going through the shell or the current project'sPATH.If an explicit update check or installation fails, the stable launcher restarts the existing working version instead of ending the session flow. A background update requested for normal session exit preserves the existing exit behavior.
Why it's needed
Before this change, the update check could read the current project's
.npmrc, while the global installation command used a different npm executable or configuration path.This is reproducible locally: Qwen Code 0.19.12 sees 0.20.0 through the project's npmjs registry, then
npm install --globaluses anpm, where 0.20.0 is not available, and fails withETARGET No matching version found.The result is an update notification for a version that the configured installation source cannot install.
Reviewer Test Plan
How to verify
.npmrcand user npm configuration with different registries.Evidence (Before & After)
Before: The locally installed Qwen Code 0.19.12 reports 0.20.0 from the project npmjs registry, while
npm install --global @qwen-code/qwen-code@0.20.0 --dry-runrequests anpm and fails withETARGET.After: The patched check identifies the installation as global npm and queries through the same trusted npm CLI used for installation. In the reproduced environment, both operations resolve 0.19.12, so Qwen Code no longer offers an update that its configured registry cannot install.
Tested on
Environment (optional)
macOS with Node.js 22 and npm 10. Windows and Linux invocation paths are covered by automated tests, but no live Windows or Linux global update was performed.
Risk & Scope
Linked Issues
Fixes #7151. Follow-up to #6874 and #6889.
中文说明
此 PR 做了什么
此 PR 确保全局 npm 安装的 Qwen Code 在检查和安装更新时使用同一个 npm 配置作用域。
全局安装现在通过可信 npm CLI 查询版本，与后续
npm install --global使用相同的 registry 和认证配置。检查与安装都通过当前 Node.js 运行时直接执行 npm CLI，不再经过 shell 或当前项目的PATH。如果显式更新检查或安装失败，稳定 launcher 会重新启动当前可用的旧版本，避免用户流程直接结束。正常会话退出时触发的后台更新保持原有退出语义。
为什么需要
此前更新检查可能读取当前项目的
.npmrc，而全局安装使用不同的 npm 可执行文件或配置路径。本机可以稳定复现：Qwen Code 0.19.12 通过项目 npmjs registry 看到 0.20.0，随后npm install --global实际访问 anpm，而 anpm 中没有 0.20.0，最终报ETARGET No matching version found。这会产生“提示存在更新，但配置的安装源无法安装该版本”的不一致行为。
Reviewer 测试计划
如何验证
.npmrc和用户 npm 配置为不同 registry。证据（Before & After）
Before：本机 Qwen Code 0.19.12 从项目 npmjs registry 提示 0.20.0，但
npm install --global @qwen-code/qwen-code@0.20.0 --dry-run实际请求 anpm 并报ETARGET。After：补丁识别当前安装为 global npm，并通过与安装相同的可信 npm CLI 查询版本。在复现环境中，两次操作都解析到 0.19.12，因此不会再提示配置安装源无法安装的更新。
测试平台
环境
macOS、Node.js 22、npm 10。Windows 和 Linux 调用路径由自动化测试覆盖，但没有执行真实 Windows/Linux 全局更新。
风险与范围
关联 Issue
关联 #6874 和 #6889 的后续修复。