Skip to content

fix(cli): narrow update-check error classification - #7431

Merged
yiliang114 merged 2 commits into
QwenLM:mainfrom
yiliang114:cx/7423-narrow-update-check-cause
Jul 22, 2026
Merged

fix(cli): narrow update-check error classification#7431
yiliang114 merged 2 commits into
QwenLM:mainfrom
yiliang114:cx/7423-narrow-update-check-cause

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

This narrows the update-check classifier change merged in #7428 to the behavior reproduced through the complete user path. It keeps direct error.cause inspection for Node 22 fetch failures, removes the killed/SIGTERM child-process special case, and restores ETIMEDOUT to the existing offline bucket.

Why it's needed

Post-merge tmux testing exercised a real temporary global npm installation against a local registry that accepted requests but never responded. Both the #7409 baseline and #7428 rendered registry did not respond within 5s. The outer fetchInfoWithTimeout and inner npm child-process timeout both use 5000 ms; the outer typed timeout surfaces before the killed child's rejection propagates. The direct child-process unit test bypassed that real wrapper and therefore did not demonstrate the claimed user-visible fix.

The nested fetch cause remains a real issue: Node 22 reports DNS failure as TypeError: fetch failed with ENOTFOUND on error.cause.code. A real startup TUI run changes from registry error on the #7409 baseline to registry unreachable with that fix.

Reviewer Test Plan

How to verify

  • Run cd packages/cli && npx vitest run src/ui/utils/updateCheck.test.ts; all 40 tests should pass.
  • Start the bundled interactive CLI with an isolated home and npm_config_registry=http://this-host-does-not-exist-zzz.invalid; the background warning should report registry unreachable.
  • As a regression control, run a temporary global npm installation against a registry that accepts but never responds; the warning should remain registry did not respond within 5s.

Evidence (Before & After)

The corrected real-startup tmux evidence and the no-differential global-npm timeout control are posted as a separate PR comment.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

macOS 15.1.1, Node.js v22.22.0, tmux 3.5a. Windows and Linux are left to CI.

Risk & Scope

Linked Issues

Follow-up to #7428

Related to #7423 and #7409

中文说明

本 PR 做了什么

本 PR 将 #7428 的更新检查分类改动收窄到完整用户路径中真实复现的部分:保留 Node 22 fetch 失败的直接 error.cause 检查,删除 killed/SIGTERM 子进程特判,并把 ETIMEDOUT 恢复到原有的离线分类。

为什么需要

合并后的 tmux 测试使用真实的临时全局 npm 安装,并连接到一个接受请求但永不响应的本地 registry。#7409 baseline 与 #7428 都显示 registry did not respond within 5s。外层 fetchInfoWithTimeout 与内层 npm 子进程超时都使用 5000 ms,外层类型化超时会在被杀子进程的 rejection 传播前返回。原来的直接子进程单元测试绕过了真实包装层,不能证明所声称的用户可见修复。

嵌套 fetch cause 则是真实问题:Node 22 将 DNS 失败报告为 TypeError: fetch failed,并把 ENOTFOUND 放在 error.cause.code。真实启动 TUI 从 #7409 baseline 的 registry error 正确变为 registry unreachable

验证

  • 目标单元测试 40/40 通过。
  • ESLint、Prettier、CLI typecheck 和 CLI build 通过。
  • 真实启动 tmux Before/After 与 global-npm timeout 无差异控制证据见独立评论。

风险与范围

恢复 #7409 原有 timeout 分类,已复现的 nested fetch 修复保持不变。递归/聚合 cause 遍历以及 Windows/Linux 手动 TUI 验证不在本 PR 范围内。

@yiliang114
yiliang114 marked this pull request as draft July 21, 2026 13:57
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Corrected tmux E2E Test Report

Important

This replaces the earlier report's evidence boundary. The earlier Before/After used explicit qwen update, not the #7409 startup warning, and the raw Node execFile screenshot did not prove a complete Qwen timeout path. The corrected tests below use the real interactive startup TUI for both scenarios.

Result

  • PASS — nested fetch cause: real startup changes from registry error to registry unreachable.
  • NO DIFFERENTIAL — global-npm timeout: both revisions render registry did not respond within 5s.
  • Follow-up fix(cli): narrow update-check error classification #7431 keeps the reproduced cause fix and removes the unsupported timeout classifications.

Environment: macOS 15.1.1, Node.js v22.22.0, tmux 3.5a, real interactive Qwen TUI at 200×50. Each run used an isolated empty QWEN_HOME plus --safe-mode; user settings and credentials were not read or changed.

Scenario 1: real startup background DNS failure

This was a normal interactive startup, not qwen update:

env QWEN_HOME=<isolated-dir> npm_config_registry=http://this-host-does-not-exist-zzz.invalid node dist/cli.js --safe-mode
Before — #7409 baseline 19ea6cba92 After — #7428 5c76c37f46
Before startup TUI: yellow warning reports registry error After startup TUI: yellow warning reports registry unreachable

Before:

△ Update check skipped (registry error) — run /update to retry.

After:

△ Update check skipped (registry unreachable) — run /update to retry.

This is the reproduced user-visible improvement: Node 22's nested ENOTFOUND is classified as offline in the real #7409 startup path.

Scenario 2: real global-npm timeout control

Each revision was installed into an isolated temporary npm global layout and launched through the real production launcher. A local registry accepted the real package GET request and intentionally never responded.

Before — #7409 baseline 19ea6cba92 After — #7428 5c76c37f46
Before global npm timeout: registry did not respond within 5s After global npm timeout: the same registry did not respond within 5s result

Observed on both revisions:

△ Update check skipped (registry did not respond within 5s) — run /update to retry.

The local server recorded one package request from each run. The requested baseline registry error → PR timeout differential was not reproducible. The outer fetchInfoWithTimeout and inner npm child timeout both use 5000 ms; the outer typed UpdateCheckTimeoutError surfaces before the killed child's rejection propagates. Baseline already classifies that typed error as timeout.

Verification

  • Both tested revisions independently passed npm run build -- --cli-only and npm run bundle before their tmux runs.
  • Follow-up fix(cli): narrow update-check error classification #7431: focused tests 40/40, changed-file ESLint, Prettier, CLI typecheck, and CLI build passed.
  • Final Ponytail review: Lean already. Ship.

Screenshot provenance

macOS screen-capture permission was unavailable to the test process. Each PNG is visibly labeled and was rendered without content changes from its corresponding real tmux capture-pane output. The terminal text is included inline above for independent review.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: Observed bug with evidence. Post-merge tmux testing against a real temporary global npm installation showed the killed/SIGTERM child-process special case is dead code — the outer fetchInfoWithTimeout timer surfaces before the killed child's rejection propagates (both use 5000 ms). The ETIMEDOUT classification as "timeout" was also wrong: it's a TCP-level connection timeout, not the application-level UpdateCheckTimeoutError, so "registry unreachable" is the correct bucket. The nested error.cause fix from #7428 (ENOTFOUND on error.cause.code) is preserved and confirmed working via real startup TUI.

Direction: Aligned — this narrows #7428 to what was actually reproduced, removing speculative branches. No CHANGELOG reference needed; this is a follow-up fix to an already-merged PR.

Size: Not applicable — no core paths touched. 2 files, 12 additions / 34 deletions (net -22 lines).

Approach: Scope feels right. Every edit serves the stated goal: ETIMEDOUT moves to NETWORK_ERROR_CODES, the UpdateCheckTimeoutError check is hoisted to a direct instanceof before the Error block (correct since it's always the top-level error from fetchInfoWithTimeout's Promise.race), and the dead killed/SIGTERM branch is removed. No unrelated changes.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题: 已观测到的 bug,有证据。合并后的 tmux 测试使用真实的临时全局 npm 安装,证明 killed/SIGTERM 子进程特判是死代码——外层 fetchInfoWithTimeout 计时器在被杀子进程的 rejection 传播前就已返回(两者都用 5000 ms)。ETIMEDOUT 归类为 "timeout" 也是错误的:它是 TCP 级连接超时,不是应用层的 UpdateCheckTimeoutError,应归入 "registry unreachable"。#7428 的嵌套 error.cause 修复(ENOTFOUND 在 error.cause.code 上)被保留并通过真实启动 TUI 确认有效。

方向: 对齐——将 #7428 收窄到实际复现的部分,删除推测性分支。

规模: 不适用——未触及核心路径。2 个文件,+12 / -34(净减 22 行)。

方案: 范围合理。每处改动都服务于目标:ETIMEDOUT 移入 NETWORK_ERROR_CODESUpdateCheckTimeoutError 检查提升为 instanceof 直接判断(正确,因为它总是 fetchInfoWithTimeoutPromise.race 抛出的顶层错误),删除死代码 killed/SIGTERM 分支。无无关改动。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 1134c3764111cde7e743888feeba71af85202d0d · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: given the problem (killed/SIGTERM is dead code because the outer fetchInfoWithTimeout timer wins the race; ETIMEDOUT is a TCP-level timeout, not the application-level one), I would: add ETIMEDOUT to NETWORK_ERROR_CODES, hoist the UpdateCheckTimeoutError check to a direct instanceof at the top, remove the dead killed/SIGTERM and matchesCode('ETIMEDOUT') branches, and update tests. The PR does exactly this — no simpler path missed.

Correctness:

  • The UpdateCheckTimeoutError hoist is safe. It's always the top-level error from fetchInfoWithTimeout's Promise.race rejection — never nested as a cause. The old errors.some(… instanceof UpdateCheckTimeoutError) was unnecessarily broad.
  • The killed/SIGTERM removal is correct. The outer setTimeout(5000) rejection is a single macrotask callback; the killed-child path (kill → child exit → error event → promise rejection) takes multiple ticks, so the Promise.race always settles with UpdateCheckTimeoutError first.
  • ETIMEDOUT → offline is the right bucket. It's a TCP connection timeout ("unreachable"), not the application-level "did not respond within 5s" message that refers to UpdateCheckTimeoutError.
  • The error.cause traversal from fix(cli): classify nested update-check network errors #7428 is preserved — the errors array still includes error.cause, so the Node 22 TypeError: fetch failed + ENOTFOUND on cause.code path still classifies as offline.

No critical blockers. No convention violations.

Testing

Unit tests: 40/40 pass (packages/cli/src/ui/utils/updateCheck.test.ts).

Direct module verification (built PR code, exercising classifyUpdateCheckError + describeUpdateCheckFailure):

UpdateCheckTimeoutError: timeout → registry did not respond within 5s
ENOTFOUND in cause:     offline → registry unreachable
ETIMEDOUT:              offline → registry unreachable
killed/SIGTERM:         registry → registry error
ECONNREFUSED:           offline → registry unreachable
generic:                registry → registry error

All classifications match the PR's stated intent. The killed/SIGTERM → registry fallback is the dead-code path that never fires in practice (outer timeout wins the race).

tmux: not available on this CI runner (tmux: command not found). The direct module verification above exercises the exact classification logic end-to-end; the TUI rendering of the warning string is unchanged by this PR (same describeUpdateCheckFailure switch).

中文说明

代码审查

独立方案: 鉴于问题(killed/SIGTERM 是死代码,因为外层 fetchInfoWithTimeout 计时器赢得竞争;ETIMEDOUT 是 TCP 级超时,不是应用层超时),我会:将 ETIMEDOUT 加入 NETWORK_ERROR_CODES,将 UpdateCheckTimeoutError 检查提升为顶部的直接 instanceof,删除死代码 killed/SIGTERMmatchesCode('ETIMEDOUT') 分支,更新测试。PR 完全这样做了——没有遗漏更简路径。

正确性:

  • UpdateCheckTimeoutError 提升是安全的。它始终是 fetchInfoWithTimeoutPromise.race 拒绝的顶层错误——不会嵌套为 cause
  • killed/SIGTERM 删除正确。外层 setTimeout(5000) 拒绝是单个宏任务回调;被杀子进程路径需要多个 tick,所以 Promise.race 总是先以 UpdateCheckTimeoutError 结算。
  • ETIMEDOUT → offline 是正确的分类。它是 TCP 连接超时("不可达"),不是指 UpdateCheckTimeoutError 的应用层 "5 秒内未响应" 消息。
  • fix(cli): classify nested update-check network errors #7428error.cause 遍历被保留——errors 数组仍包含 error.cause

无关键阻塞。无规范违反。

测试

单元测试: 40/40 通过。

直接模块验证(构建 PR 代码):所有分类符合 PR 声明的意图。

tmux: 此 CI 运行器不可用。直接模块验证已端到端测试了分类逻辑。

Qwen Code · qwen3.7-max

Reviewed at 1134c3764111cde7e743888feeba71af85202d0d · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — clean across every stage; would merge without hesitation.

This is a textbook follow-up: #7428 added speculative branches (killed/SIGTERM, ETIMEDOUT-as-timeout) that post-merge tmux testing proved unnecessary. This PR narrows to what was actually reproduced, and the result is 22 fewer lines of code that are easier to reason about. The UpdateCheckTimeoutError hoist is correct (it's always the top-level error from the Promise.race), the killed/SIGTERM removal is correct (the outer timer wins the race — single macrotask vs. multi-tick child-process teardown), and ETIMEDOUT → offline is the right semantic bucket. The nested error.cause fix from #7428 is preserved intact.

Unit tests 40/40, direct module verification confirms all six classification paths. No concerns.

中文说明

置信度:5/5 — 每个阶段都干净;毫不犹豫地合并。

这是一个标准的后续修正:#7428 添加了推测性分支(killed/SIGTERM、ETIMEDOUT 归为 timeout),合并后的 tmux 测试证明它们不必要。本 PR 收窄到实际复现的部分,结果是少了 22 行更易理解的代码。UpdateCheckTimeoutError 提升正确(它始终是 Promise.race 的顶层错误),killed/SIGTERM 删除正确(外层计时器赢得竞争),ETIMEDOUT → offline 是正确的语义分类。#7428 的嵌套 error.cause 修复完整保留。

单元测试 40/40,直接模块验证确认所有六个分类路径。无顾虑。

Qwen Code · qwen3.7-max

Reviewed at 1134c3764111cde7e743888feeba71af85202d0d · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

Comment thread packages/cli/src/ui/utils/updateCheck.ts

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@yiliang114
yiliang114 added this pull request to the merge queue Jul 22, 2026
Merged via the queue into QwenLM:main with commit b054d83 Jul 22, 2026
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants