fix(cli): keep sudo-required npm installs on npm instead of migrating to standalone - #5207
Merged
Merged
Conversation
… to standalone When the npm global prefix is not writable, getInstallationInfo (added in #4629) silently migrated the install to the standalone installer, which bundles an official Node 22 build requiring glibc >= 2.28. On older hosts (e.g. CentOS 7 / glibc 2.17) the bundled node can't load and the update fails. Keep npm installs on npm and ask the user to update with sudo.
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts CLI installation detection to avoid silently switching npm installs (that require sudo) to the standalone updater, preventing failures on older glibc hosts where the bundled Node runtime can’t run.
Changes:
- When the npm global install location is not writable,
getInstallationInfo()now returns an npm-based sudo update message and does not mark the install as standalone. - Removes the prior “migrate to standalone” fallback for sudo-required npm installs.
- Adds a unit test asserting no standalone migration and no unattended
updateCommandwhen npm prefix isn’t writable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/cli/src/utils/installationInfo.ts | Stops auto-migrating sudo-required npm installs to standalone; returns a sudo update message without updateCommand. |
| packages/cli/src/utils/installationInfo.test.ts | Adds coverage for the “npm prefix not writable → prompt for sudo, no standalone migration” behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
Thanks for the PR, @yiliang114! Template headings don't exactly match Direction: This fixes a real regression introduced in #4629 (v0.18.0). Silently migrating sudo-requiring npm installs to the standalone installer swaps in a bundled Node 22 that requires glibc ≥ 2.28, breaking auto-update on CentOS 7, older RHEL, Alpine/musl, and similar hosts. The fix is well-aligned — install method should drive update method, not silently override it. CHANGELOG: no direct reference, but the Approach: Minimal and focused. Two files, 37 additions / 24 deletions. The diff removes the silent migration branch entirely and replaces it with a plain "use sudo" message. The Moving on to code review and testing. 🔍 中文说明感谢 @yiliang114 的贡献！ PR 模板的标题与 方向： 修复的是 #4629（v0.18.0）引入的真实回归。把需要 sudo 的 npm 安装静默迁移到 standalone，会引入一个需要 glibc ≥ 2.28 的捆绑 Node 22，导致 CentOS 7、老版 RHEL、Alpine/musl 等主机上的自动更新失败。修复方向正确——安装方式应决定更新方式，不应静默替换。CHANGELOG 中无直接引用，但 方案： 极简且聚焦。两个文件，+37 / -24。diff 移除了整个静默迁移分支，替换为简单的"请使用 sudo"提示。 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
Collaborator
2a. Code ReviewIndependent proposal (before reading the diff): The fix is straightforward — in the Comparison with PR: The PR matches this approach exactly. The diff is 37 additions / 24 deletions across 2 files — pure deletion of the migration logic plus a focused test. No issues found:
No blockers. 2b. TestingRan the PR's new test case against both main (before) and the PR branch (after) to confirm the fix resolves the regression. Before (main branch — old code, new test applied)On main, the old code returns After (this PR)On the PR branch, the test passes — All 24 tests in 中文说明2a. 代码审查独立方案（未看 diff 前）： 修复思路很明确——在 与 PR 对比： PR 与独立方案完全一致。diff 跨 2 个文件，+37 / -24——纯粹删除迁移逻辑加一个聚焦的测试。未发现问题：
无阻塞问题。 2b. 测试在 main（修复前）和 PR 分支（修复后）上分别运行了 PR 新增的测试用例，确认修复解决了回归。 修复前（main 分支）： 测试失败—— 修复后（PR 分支）： 测试通过——
— Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, minimal bug fix that does exactly what it should. The PR's approach matches my independent proposal — remove the silent standalone migration, keep npm installs on npm, tell the user to use sudo. The before/after test run confirms the regression is real on main ( The diff is 37 additions / 24 deletions — mostly deletion of the problematic migration logic. Every line changed serves the fix. No scope creep, no drive-by refactors, no abstractions for hypothetical futures. The The one gap is the PR template (missing Tested-on table, Risk & Scope), but the linked issue #5206 covers the reproduction in detail, and the Reviewer Test Plan in the PR body describes the verification approach. Not worth blocking on. One thing worth noting for follow-up (as the PR itself acknowledges): the standalone installer still bundles Node 22 which requires glibc ≥ 2.28. Users who deliberately run the standalone installer on old-glibc hosts will still hit this — a glibc preflight check or an alternative runtime would be the right follow-up. But that's out of scope for this PR, which only fixes the npm path. Approving. ✅ 中文说明这是一个干净、极简的 bug 修复，完全做了该做的事。 PR 的方案与我的独立方案一致——移除静默的 standalone 迁移，保持 npm 安装走 npm，提示用户使用 sudo。前后对比测试确认了 main 分支上的回归（ diff 为 +37 / -24，主要是删除有问题的迁移逻辑。每一行改动都服务于修复本身。没有范围蔓延，没有顺手重构，没有为假设性需求做的抽象。 一个不足是 PR 模板不完整（缺少 Tested-on 表格和 Risk & Scope），但关联的 issue #5206 详细覆盖了复现步骤，PR 正文中的 Reviewer Test Plan 描述了验证方法。不构成阻塞。 值得后续关注的一点（PR 自己也提到了）：standalone 安装器仍然捆绑了需要 glibc ≥ 2.28 的 Node 22。主动使用 standalone 安装器的老 glibc 用户仍会遇到这个问题——glibc 预检或替代运行时是合适的后续工作。但这不在本 PR 范围内，本 PR 只修复了 npm 这条路径。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 16, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
added
category/cli
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
| expect(info.packageManager).toBe(PackageManager.NPM); | ||
| expect(info.isGlobal).toBe(true); | ||
| // Must NOT silently migrate to the standalone installer (bundled Node can be | ||
| // incompatible with the host, e.g. an older glibc). |
Collaborator
There was a problem hiding this comment.
[Suggestion] This test only exercises getInstallationInfo(projectRoot, true), but the fix also changes behavior for isAutoUpdateEnabled=false + non-writable npm prefix. Previously that combination fell through to the default npm return (with updateCommand: 'npm install -g ...' and no sudo hint); now it returns early with no updateCommand and the sudo message. If a future change accidentally re-introduces && isAutoUpdateEnabled, that regression would go undetected for locked-down CI images and workstations.
Consider adding a companion assertion under the same EACCES mock:
// Same scenario but with auto-update disabled — should still
// recommend sudo and NOT silently migrate to standalone.
const infoDisabled = getInstallationInfo(projectRoot, false);
expect(infoDisabled.packageManager).toBe(PackageManager.NPM);
expect(infoDisabled.isGlobal).toBe(true);
expect(infoDisabled.isStandalone).toBeUndefined();
expect(infoDisabled.standaloneDir).toBeUndefined();
expect(infoDisabled.updateCommand).toBeUndefined();
expect(infoDisabled.updateMessage).toContain('sudo');— qwen3.7-max via Qwen Code /review
| 'lib', | ||
| 'qwen-code', | ||
| ); | ||
| if (!npmPrefixWritable) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The block comment explains why standalone migration was removed, but does not explain why isAutoUpdateEnabled was deliberately dropped from the guard (!npmPrefixWritable && isAutoUpdateEnabled → !npmPrefixWritable). A future maintainer reading this comment could reasonably re-add && isAutoUpdateEnabled as a "harmless refinement", which would silently reintroduce the wrong non-sudo updateCommand for users with auto-update disabled.
Consider extending the comment to document the guard broadening:
// The npm global prefix requires sudo. Do NOT silently migrate to the
// standalone installer here: that swaps in a bundled Node runtime which
// can be incompatible with the host (e.g. an older glibc), breaking users
// who were updating fine via npm. Keep npm installs on npm and ask the
// user to update with sudo instead. No updateCommand is returned so the
// auto-updater does not attempt an unattended sudo.
// NOTE: this guard intentionally omits `&& isAutoUpdateEnabled`. Even when
// auto-update is off, a non-writable npm prefix means `npm install -g`
// (without sudo) is not a valid update path for any consumer.— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jun 16, 2026
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
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.
TLDR
When a global npm install needs sudo, auto-update (since #4629) silently migrated it to the standalone installer, which bundles an official Node 22 build requiring
glibc >= 2.28. On older hosts (e.g. CentOS 7 / glibc 2.17) the bundled node can't load and the update fails withGLIBC/GLIBCXX not found. This PR keeps npm installs on npm and asks the user to update with sudo instead.What this changes
packages/cli/src/utils/installationInfo.ts:isStandalone: true, migrate to~/.local/lib/qwen-code(bundled Node 22).Update requires sudo. Please run: sudo npm install -g @qwen-code/qwen-code@latest). NoupdateCommandis returned, so the auto-updater won't attempt an unattended sudo. No silent runtime swap.Install method now drives update method: npm stays npm; genuine standalone installs are unaffected.
Reviewer Test Plan
Unit tests (run locally):
installationInfo.test.ts(24) +handleAutoUpdate.test.ts(24) → 48 passed. Added a test asserting a not-writable npm prefix returns the sudo message and does not setisStandalone/standaloneDir.packages/clisuite: 8352 passed / 9 skipped; the 15 failures are pre-existing flaky UI/timing tests unrelated to this change. A clean baseline run (no patch) failed a different set of flaky tests (serve/server.test.ts,ui/auth/AuthDialog.test.tsx), confirming this change introduces no new failures.End-to-end on glibc 2.17 (
centos:7, root-owned npm install, non-root runtime), running the realgetInstallationInfobefore vs after the fix:before / after output
Known Limitations
This only fixes the npm auto-update path. Users who deliberately run the standalone installer on an old-glibc host still get a bundled Node 22 that won't run — out of scope here; a glibc preflight (or a glibc-217 runtime) for the standalone installer can be a follow-up.
Linked issues
Closes #5206
中文说明
npm 全局目录需要 sudo 时,#4629 会静默迁移到 standalone,自带的 Node 22 在老 glibc 上跑不起来导致升级失败。本 PR 改为:npm 装的继续走 npm,需要 sudo 就提示用户 sudo,不再静默换运行时。只修 npm 这条路;主动跑 standalone 安装脚本的老 glibc 用户不在本 PR 范围,可后续加 glibc 预检。
验证:相关单测 48/48 通过(新增 1 个用例锁死"不迁 standalone");全量 cli 套件无本次改动引入的新失败(失败项为无关 flaky,已用干净基线对照);centos:7(glibc 2.17)真环境 before/after:修复前
isStandalone: true走迁移→崩溃,修复后isStandalone: undefined返回 sudo 提示→不再崩溃。