fix(cli): update npm installs safely in background - #7322
Conversation
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. |
|
Thanks for the PR! Template looks good ✓ Problem: this is a real, observed issue — the published CLI is code-split into content-hashed chunks, and replacing the global npm package while a process is running can delete a chunk that the old process imports later, producing Direction: aligned. Immutable version directories with a launcher pointer is the established pattern for this class of problem (similar to Nix, pnpm's content-addressable store). It keeps the running process safe without forcing an exit-time delay or automatic restart. CHANGELOG reference: the existing update mechanism already defers to exit; this replaces that with a background install that activates on next launch. Size: 633 production lines (additions + deletions excluding tests), 982 test lines, 60 docs lines. No core paths touched ( Approach: the scope feels right for the problem. You need the launcher logic (version selection, QWEN_HOME resolution, pin mechanism), the worker logic (npm install, validation, smoke test, atomic activation), and the concurrency handling (lockfile, highest-version-wins). The UI change (moving UpdateNotification above the composer) is a natural companion since the notification text changes from "restart required" to "next run". No unrelated changes spotted. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个真实的、已观测到的问题——发布后的 CLI 被拆分成带内容哈希的 chunk，在进程运行时替换全局 npm 包可能删除旧进程稍后需要导入的 chunk，产生 方向：对齐。不可变版本目录 + launcher 指针是这类问题的成熟模式（类似 Nix、pnpm 的内容寻址存储）。在不强制退出等待或自动重启的前提下保持运行进程安全。 规模：633 行生产代码（不含测试的增删），982 行测试，60 行文档。未触及核心路径——所有改动在 方案：范围与问题匹配。需要 launcher 逻辑（版本选择、QWEN_HOME 解析、pin 机制）、worker 逻辑（npm install、验证、冒烟测试、原子激活）和并发处理（lockfile、最高版本优先）。UI 改动（将 UpdateNotification 移到 composer 上方）是自然的配套改动。未发现无关改动。 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: for this problem I would have done the same thing — install the update to a versioned, immutable directory, write a pointer file the launcher reads on next invocation, validate before activating, and handle concurrency. The PR matches this approach and adds launcher-scoped isolation (different npm/nvm prefixes get separate pointers), a pin mechanism to keep nested sessions on the running build, and base-installation change detection. Findings: no critical blockers. The implementation is careful:
One minor observation (non-blocking): the Real-Scenario TestingCLI starts, responds to prompts, and the launcher script resolves correctly. The QWEN_HOME warning is expected in this CI environment (no settings.json at the configured path). Unit TestsAll changed test files pass:
TypeScript typecheck passes with no errors. 中文说明代码审查独立方案： 对于这个问题，我会做同样的事情——将更新安装到带版本号的不可变目录，写一个 launcher 在下次启动时读取的指针文件，激活前验证，处理并发。PR 与此方案一致，并增加了 launcher 级隔离、pin 机制和基础安装变更检测。 发现： 无关键阻塞问题。实现细致：使用 实际场景测试CLI 正常启动、响应提示，launcher 脚本正确解析版本。 单元测试所有改动的测试文件通过（共 168 个测试）。TypeScript 类型检查无错误。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 4/5 — solid implementation of a well-understood pattern; solves a real corruption bug with proper validation, concurrency safety, and comprehensive tests. Stepping back: this PR replaces a fundamentally broken update path (in-place npm global replace while the process is running) with the correct architecture — immutable version payloads, a launcher that selects the active version on next invocation, and a detached worker that stages and validates before activating. The approach matches what I would have proposed independently, and the execution is thorough: lockfile-protected activation, smoke tests before pointer writes, base-installation change detection, pin mechanism for nested sessions, and graceful fallback on any inconsistency. The code is straightforward for what it does. The 318-line Non-blocking nits: the After seeing it run: the CLI starts cleanly, the launcher resolves correctly, and 168 unit tests covering the new logic all pass. The before/after is clear — before, a running session could lose chunks to an in-place update; after, the running session is immutable and the next invocation picks up the verified update. LGTM, approving. ✅ 中文说明置信度：4/5 — 对成熟模式的扎实实现；用正确的验证、并发安全和全面测试解决了一个真实的文件损坏 bug。 总结：本 PR 用正确的架构（不可变版本 payload + launcher 选择 + detached worker 验证后激活）替换了根本上有问题的更新路径。实现细致，代码职责清晰，bootstrap 路径的防御性回退模式正确。168 个单元测试全部通过，CLI 正常启动和响应。 — 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. ✅
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. 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
Code review — background npm auto-updateOverviewFor writable global npm installs, the post-render update check now stages the exact resolved version into a launcher-scoped, immutable directory ( The design is well thought through — launcher isolation via a path hash, ctime-based invalidation on explicit reinstall, highest-version-wins on concurrent activation, stale-staging reclamation by PID liveness, and a lockfile around the pointer write. Nice work. I verified the change against the PR head (worktree at 1. Flaky test —
|
Code Review —
|
qwen-code-ci-bot
left a comment
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
|
Addressed the screen-reader layout review feedback in 8cbc9d5. The update notification now follows the same modal-dialog guard as the default layout, with focused coverage for visible and dialog-suppressed update notices. Verified with the focused ScreenReaderAppLayout test, Prettier, and git diff checks. |
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: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
— qwen3.7-max via Qwen Code /review
|
Addressed the latest review finding by rendering the background update notification in agent-tab views with the same dialog guard used elsewhere. Verified with |
qwen-code-ci-bot
left a comment
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
Review — background npm auto-updateRead the full diff plus the surrounding launcher/build code ( What's solid
Worth addressing
Nits
SecurityTrust boundary stays the user's own VerdictSolid, careful work with strong tests. My only substantive ask before this sees wide (esp. nightly) usage is a plan for disk hygiene (#1 + #2) — even a coarse age-based prune. Everything else is minor. Windows/Linux live-install caveats are already honestly flagged in the description. 中文摘要设计方向正确：把新版本装到 主要建议（都属于后续优化，非阻断）：
其余为小问题： Reviewed with Claude Code (Opus 4.8) |
Real tmux E2E report: failure safety and process isolationVerified the background npm update flow end-to-end against the final production code in Failure path
Success and old-process isolation
Focused verification also passed: 90/90 tests covering managed installation, failure events, launcher pinning, and update-notification layout. The first Ubuntu CI run exposed a test-only |
ReviewStrong PR. The core design — immutable per-launcher version payloads, an atomically-swapped What I verified
Findings1. Interrupted staging directories are not actually reclaimed (PR body says they are). 2. 3. 2-second timeout on 4. npm's stderr is discarded. The worker's npm spawn uses Minor
Risk noteWindows is marked 🤖 Generated with Claude Code — Claude Fable 5 |
|
Addressed the bounded findings in 71d9c5c:
I did not add a staging sweeper in this PR. I could not find a dedicated existing issue for that cleanup, and adding TTL/lease behavior would expand an already mature PR. The minor refactors were also left unchanged to avoid non-functional churn. Focused verification: 37/37 update tests, CLI typecheck, ESLint/Prettier, and diff checks passed. |
ReviewOverviewFor writable global npm installs, the post-render update check now spawns a detached worker ( What holds up well
Findings1. [Suggestion] Quitting the TUI mid-install can abort the update the design promises to protect — worker stderr is a pipe to the parent.
Suggested shape: spawn the worker with 2. [Suggestion]
3. [Nit] 4. [Nit] The failure log is misleading for the managed path. On worker failure, the debug message interpolates 5. [Question] Is the 10-minute cap on the staged Test coverage gaps
SecurityNo concerns beyond the same-user trust boundary that already exists: version strings are validated before path joins on both writer and reader (traversal-shaped versions can't be constructed), the pointer can't redirect a foreign launcher (realpath equality), the payload is validated before exec, and VerdictSolid, carefully-reasoned change with unusually good test discipline for this kind of bootstrap code; prior review feedback is visibly incorporated. Finding 1 is the one I'd want addressed (or explicitly accepted with rationale) before merge, since it undercuts the PR's core guarantee in its motivating scenario; the rest are minor. |
qwen-code-ci-bot
left a comment
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
|
Addressed the tilde |
|
Windows verification is complete for the current head, Tested on GitHub-hosted Windows Server 2022 (
The banner shows the source bundle's Final E2E job: https://github.com/QwenLM/qwen-code/actions/runs/29805563981/job/88555268953 I also ran the repository-wide Windows test matrix on an earlier PR head. It reached 14,210 passing tests but failed 58 tests across 17 files, mostly existing POSIX path and symlink assumptions. That run exposed two hard-coded POSIX path expectations in the new managed-update tests and the |
|
@qwen-code /triage |
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. ✅
Code Review —
|
|
Released in v0.20.1. |
3 participants
What this PR does
For writable global npm installations, the post-render update check now installs the exact available version into an immutable, launcher-scoped directory in the background. The active session continues using its original files, while the stable npm launcher atomically selects the verified update on the next invocation.
The staged install inherits npm's user/global registry configuration while explicitly forcing a predictable project layout. Before activation, Qwen Code validates the published package and loads its real CLI bundle through the production launcher. Concurrent activations keep the highest valid version, and failed installs are cleaned up when the worker can complete its failure handling. An explicit global reinstall invalidates the managed pointer even when the reinstalled version is unchanged.
The launcher resolves
QWEN_HOMEfrom the same home-scoped environment files and supported syntax as the CLI before selecting an update.Why it's needed
The published CLI is split into content-hashed chunks. Replacing a global npm package while its process is running can delete a chunk that the old process imports later, producing
ERR_MODULE_NOT_FOUND. Deferring installation until exit avoids that corruption but makes users wait when leaving and prevents long-running sessions from completing updates normally.This keeps the current process immutable without forcing an automatic restart or waiting for session exit: installation happens after first render, the current session remains uninterrupted, and the next ordinary invocation uses the new version.
Reviewer Test Plan
How to verify
Start a writable global npm installation with automatic updates enabled and an available release. Confirm that the TUI remains usable while npm stages the update, the global package is not replaced, and success reports that the new version will be used on the next run. Start a second invocation and confirm that it reports and runs the updated version.
Keep the old invocation running during activation and confirm that its later dynamic imports still work. Start concurrent activations and confirm that the highest completed version remains selected. Reinstall the original global version and confirm that the managed update is no longer selected.
Set a custom registry in user/global npm configuration and a conflicting registry in the current repository, then confirm that checking and staging use the user/global registry. Also set npm global mode in the environment and confirm that staging still produces the validated project layout. Configure
QWEN_HOMEin either supported home environment-file form and confirm that the next invocation finds the managed update.Evidence (Before & After)
Before: npm updates were announced as installing only after exit; directly updating in process could remove content-hashed chunks still needed by the active CLI.
After: npm updates install safely in the background after first render; the current session continues using immutable files; the next invocation selects the validated update without an automatic restart.
Local evidence: 146 focused tests passed, the launcher script tests passed, full lint and typecheck passed, and the full build plus packaged
--versionand--helpsmoke checks passed. A real registry installation from 0.19.12 to 0.20.0 also passed withNPM_CONFIG_GLOBAL=true: the next launcher selected 0.20.0 while the original 0.19.12 package remained unchanged.Tested on
Environment (optional)
macOS with Node.js 22 and npm 10. Cross-platform launcher paths, empty-home fallback, and Windows environment handling are covered by unit and subprocess tests; live Windows and Linux global installations were not exercised locally.
Risk & Scope
npm install -gremains authoritative and invalidates the managed pointer.Linked Issues
Supersedes #7250.
中文说明
本 PR 做了什么
对于可写的全局 npm 安装，首次渲染后的更新检查现在会在后台把确定的精确版本安装到按 launcher 隔离的不可变目录中。当前会话继续使用原来的文件，稳定的 npm launcher 会在下一次启动时原子地选择已经验证的新版本。
Staging 安装会继承 npm 的用户/全局 registry 配置，同时显式强制使用可预测的 project layout。激活前，Qwen Code 会验证发布包，并通过生产 launcher 实际加载 CLI bundle。并发激活会保留最高的有效版本；worker 能够完成失败处理时，失败安装的 staging 目录会被清理；即使用户重装的是相同旧版本，显式全局重装也会使托管指针失效。
Launcher 会先按照 CLI 相同的 home 环境文件及其支持语法解析
QWEN_HOME，再选择更新。为什么需要
发布后的 CLI 会拆分成带内容哈希的 chunk。运行中的进程如果直接替换全局 npm 包，旧进程稍后动态导入的 chunk 可能已经被删除，从而触发
ERR_MODULE_NOT_FOUND。把安装推迟到退出可以避免损坏，但会让用户退出时等待，也会阻止长时间运行的正常会话完成更新。本方案在不强制自动重启、也不等待会话退出的前提下，保持当前进程依赖的文件不可变：安装在首次渲染后进行，当前会话不中断，下一次正常启动直接使用新版本。
Reviewer 测试计划
如何验证
使用启用自动更新且存在可用版本的可写全局 npm 安装。确认 npm 在后台 staging 更新时 TUI 仍可使用、全局包没有被替换，并且成功提示说明新版本会在下一次启动时使用。再启动一个新进程，确认它报告并运行更新后的版本。
在激活期间保持旧进程运行，确认旧进程后续的动态导入仍然正常。同时发起多个更新激活，确认最终选择的是完成的最高版本。随后重装原来的全局版本，确认托管更新不再被选择。
在用户/全局 npm 配置中设置自定义 registry，并在当前仓库中设置冲突的 registry，确认版本检查和 staging 都使用用户/全局 registry。再通过环境变量开启 npm global mode，确认 staging 仍会生成可验证的 project layout。用 home 环境文件支持的任一语法配置
QWEN_HOME，确认下一次启动能够找到托管更新。前后证据
之前：npm 更新被提示为仅在退出后安装；进程内直接更新还可能删除当前 CLI 仍需要的内容哈希 chunk。
之后：npm 更新会在首次渲染后安全地后台安装；当前会话继续使用不可变文件；下一次启动直接选择已经验证的新版本，不会触发自动重启。
本地证据：146 个聚焦测试通过，launcher 脚本测试通过，完整 lint 和 typecheck 通过，完整 build 以及打包后的
--version、--help冒烟检查通过。还通过真实 registry 验证了从 0.19.12 更新到 0.20.0，并设置了NPM_CONFIG_GLOBAL=true：下一次启动选择 0.20.0，原 0.19.12 包保持不变。测试平台
环境（可选）
macOS，Node.js 22，npm 10。跨平台 launcher 路径、空 home 回退和 Windows 环境处理已由单元测试与子进程测试覆盖；没有在真实 Windows 和 Linux 全局安装上执行。
风险与范围
npm install -g仍然具有最高优先级，并会使托管指针失效。关联 Issue
替代 #7250。