Skip to content

feat(release): ship OpenTUI runtime via per-platform npm packages - #9885

Merged
chiga0 merged 4 commits into
feat/opentui-migratefrom
feat/opentui-npm-preview
Aug 31, 2026
Merged

feat(release): ship OpenTUI runtime via per-platform npm packages#9885
chiga0 merged 4 commits into
feat/opentui-migratefrom
feat/opentui-npm-preview

Conversation

@chiga0

@chiga0 chiga0 commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

After #8677 lands, npm install -g @qwen-code/qwen-code still runs the CLI under Node, where the OpenTUI renderer silently falls back to ink — only the standalone archives (which bundle a pinned Bun runtime) actually render OpenTUI. This follow-up closes that gap by publishing the standalone runtime as five per-platform npm packages and wiring the main package's bin to them, the same distribution pattern used by @lydell/node-pty, @teddyzhu/clipboard in this repo, and by esbuild/swc/opencode across the ecosystem:

  • New scripts/package-npm-platform-packages.js repackages the five standalone archives into npm platform packages @qwen-code/qwen-code-{darwin-arm64, darwin-x64, linux-arm64, linux-x64, win-x64}. Each carries the pinned Bun build, the native renderer libraries, and the bundled CLI; os/cpu manifest fields make npm install exactly one per host.
  • New scripts/npm-bin.js becomes the main package's bin: it resolves the installed platform package for the current OS/arch and spawns its bundled Bun on lib/cli-entry.js, setting QWEN_CODE_LAUNCHER_PATH exactly like the standalone bin/qwen wrapper so the in-CLI updater relaunches correctly. No postinstall — --ignore-scripts installs behave identically, and a missing platform package (--omit=optional, mirror gaps) produces a clear error pointing at the still-shipped node entry (cli-entry.js), which the launcher never hides.
  • prepare-package.js stamps the five platform packages into optionalDependencies (version-locked to the main package version, mirroring @qwen-code/audio-capture) and ships npm-bin.js as the bin.
  • release.yml packages and publishes the platform packages before the main package, reusing the existing dry-run / already-published / --tag guards.

Design doc with the full rationale (why launcher over postinstall copying, alternatives rejected): docs/design/npm-platform-runtime-packages.md.

Why it's needed

Without it, the npm channel and the standalone channel ship different renderers after #8677: npm users get ink (with its known flicker issues) while standalone users get OpenTUI. The whole point of the migration is that OpenTUI is the better default; npm is the largest distribution channel and should not be left behind.

Reviewer Test Plan

How to verify: the E2E chain below was run locally against this branch's artifacts (main package packed from dist/, platform package packed from dist/npm-platform/qwen-code-darwin-arm64/); reproducing needs nothing beyond npm.

  • npm pack both packages and install them into a scratch prefix.
  • qwen --version runs through the launcher (Bun path) and prints the version.
  • A PTY smoke over the npm-installed bin shows mouse-tracking sequences (CSI ?1000h/?1006h), the composer, and DEC 2026 sync — OpenTUI is active, not the ink fallback.
  • Remove the platform package: the bin exits 1 with a message whose suggested node <package-root>/cli-entry.js command was itself verified to run (ink fallback intact).
  • npm install of the main package alone (no platform package available) still succeeds — the dependency is optional, so CI and --omit=optional flows are unaffected.

Evidence: local run on macOS arm64 — launcher qwen --version0.21.11 (exit 0); PTY smoke 3/3 assertions PASS; fallback error path exit 1 with a working node command; yamllint clean on release.yml; eslint clean on the touched scripts.

Tested on:

OS Shell Terminal Result
macOS 15 (arm64) zsh Terminal.app via PTY ✅ launcher + OpenTUI active, fallback verified

Environment: node 22/24 npm 10/11, Bun 1.3.14 (bundled in the platform package), built from d6e42a6636.

Not validated: real npm publish (requires release secrets) — the publish steps mirror the battle-tested @qwen-code/audio-capture step and run behind the release workflow's dry-run default; a workflow_dispatch dry-run of the Release workflow on a branch is the natural pre-merge check. Windows launcher path is implemented per the standalone qwen.cmd conventions but only smoke-checked structurally.

Risk & Scope: additive to the release pipeline; no CLI runtime code changes. Main package bin changes from the node entry to the launcher, which is the intended behavior change (npm users get OpenTUI); the node entry remains in the tarball as the documented fallback. The platform packages add ~50MB (compressed) per published platform per release — npm storage only, the JS tarball is unchanged apart from the launcher.

Linked Issues: follow-up to #8677 (stacked on it); distribution aspect of the milestone tracked in #8662.

中文说明

本 PR 是 #8677 的后续(stacked PR,base 为 feat/opentui-migrate 分支,#8677 合并后请将 base 改为 main)。

#8677 合入后,npm 安装的 CLI 仍在 Node 下运行,OpenTUI 渲染器会静默回退到 ink——只有自带 Bun 运行时的 standalone 包才能真正跑 OpenTUI。本 PR 补齐这一缺口:把 standalone 运行时发布为五个平台 npm 包(@qwen-code/qwen-code-{darwin-arm64, darwin-x64, linux-arm64, linux-x64, win-x64}os/cpu 字段让 npm 每台主机只装一个),主包 bin 换成 npm-bin.js 启动器——运行时解析对应平台包并用其内置 Bun 启动 CLI,行为与 standalone 的 bin/qwen 包装一致(含 QWEN_CODE_LAUNCHER_PATH 供 CLI 内更新器使用)。

不使用 postinstall(对比 opencode 的做法):--ignore-scripts 安装行为一致、无安装期副作用、无半拷贝状态;平台包缺失时给出明确报错并指向仍在包内的 node 入口(ink 回退,已验证可跑)。发布流程在主包之前打包并发布平台包,复用现有 @qwen-code/audio-capture 发布步骤的 dry-run / 已发布跳过 / dist-tag 防护。

本地已验证:两包 npm pack + 安装 → qwen --version 走 Bun 链路;PTY 冒烟 3/3 断言(mouse tracking / composer / DEC 2026)确认 OpenTUI 激活;删平台包后报错路径与 node 回退命令均实测可用。设计决策详见 docs/design/npm-platform-runtime-packages.md

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — this closes a real gap in the OpenTUI rollout.

Template: complete ✓ — all required sections present, with a concrete reviewer test plan and local evidence.

Problem: real, not theoretical. After #8677 lands, the npm-installed CLI runs under Node and the OpenTUI renderer silently falls back to ink — only the standalone archives (which bundle a pinned Bun) actually render OpenTUI. That is a documented distribution gap of the migration (milestone #8662), and the PR body lays out the before/after precisely.

Direction: aligned — the point of the OpenTUI migration is that OpenTUI is the better default, and npm is the largest channel, so leaving it on the ink fallback would undercut the migration. Two things I'm flagging up front, neither a rejection:

Size: no core-module paths touched — release scripts, one workflow, a design doc, and one line in the root package.json. Production 497 lines / test 0 / generated 0, so the two-tier core gate has nothing to act on.

Approach: the per-platform optionalDependencies + bin-launcher pattern is the ecosystem-standard answer here (esbuild / swc / opencode, and in-repo precedent with @qwen-code/audio-capture), and the PR's rejection of postinstall copying (--ignore-scripts parity, no install-time side effects, no half-copied state) matches how I'd approach it. Scope feels right — every file maps to the stated goal, no drive-by changes, and the design doc records the rejected alternatives.

Risk: no high-risk paths from the revert-correlation list are touched. The release.yml change is itself the sensitive surface: publish ordering (platform packages before the main package) and idempotent re-publish behavior are where review should focus. Publishing five new npm package names is additive but effectively irreversible once published — the dry-run default is the right guard.

Moving on to code review, with the release-mechanics escalation noted. 🔍

中文说明

感谢贡献——这个 PR 补上了 OpenTUI 迁移中一个真实的缺口。

**模板:**完整 ✓——必填章节齐全,有具体的 reviewer 测试计划和本地验证证据。

**问题:**真实存在,不是理论问题。#8677 合入后,npm 安装的 CLI 跑在 Node 上,OpenTUI 渲染器会静默回退到 ink——只有自带 pinned Bun 的 standalone 包才真正渲染 OpenTUI。这是迁移过程中已记录的发行缺口(milestone #8662),PR 描述把 before/after 讲得很清楚。

**方向:**对齐——OpenTUI 迁移的意义就在于 OpenTUI 是更好的默认渲染器,而 npm 是最大的分发渠道,让它停留在 ink 回退会削弱迁移本身。有两点提前说明,都不是拒绝:

**规模:**未触及核心模块路径——只有发布脚本、一个 workflow、一篇设计文档和根 package.json 的一行。生产代码 497 行 / 测试 0 / 生成代码 0,两级核心门禁无需介入。

**方案:**按平台 optionalDependencies + bin launcher 是生态标准做法(esbuild / swc / opencode,仓库内已有 @qwen-code/audio-capture 先例),PR 拒绝 postinstall 复制方案的理由(--ignore-scripts 行为一致、无安装期副作用、无半拷贝状态)也与我的判断一致。范围合适——每个文件都对应目标,无顺手改动,设计文档记录了被否决的备选方案。

**风险:**未触及 revert 相关性列表中的高风险路径。release.yml 本身就是敏感面:review 应聚焦发布顺序(平台包先于主包)和重复发布的幂等行为。发布五个新的 npm 包名是增量操作,但一旦发布实际上不可撤销——dry-run 默认开启是正确的防护。

进入代码审查,并已记录发布机制升级事项。🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

The approach is the right one — per-platform optionalDependencies + a Node bin launcher is the ecosystem-standard pattern (esbuild / swc / opencode), and the implementation follows it cleanly with no postinstall. I verified the parts of this diff that depend on #8677's standalone infrastructure against the base branch, not just against the diff's own claims:

  • Layout matches reality. package-npm-platform-packages.js asserts qwen-code/ archive root, lib/cli-entry.js, bun/bin/bun (unix) / bun/bun.exe (win-x64), and bin/qwen / bin/qwen.cmd — all exactly what create-standalone-package.js and build-standalone-release.js on the base branch produce, including the .zip (not tar.gz) for win-x64. Build fails loudly on drift, which is the right guard.
  • Resolution is sound. createRequire(import.meta.url) + require.resolve('<name>/package.json') walks node_modules correctly for both global and local installs, and works because the platform manifests deliberately carry no exports map (documented in both scripts). ESM is valid — the dist manifest sets type: module, and npm-bin.js is copied to the dist root alongside cli-entry.js and added to files.
  • Release wiring is consistent. The new publish step is a faithful copy of the established @qwen-code/audio-capture / main-package guards (dry-run flag, already-published skip, --tag), publishes the platform packages before the main package that references them, and runs on ubuntu-latest where tar/unzip exist. The for pkg_dir in dist/npm-platform/*/ loop is safe against the staging dirs (dot-prefixed, cleaned up in a finally).

Three findings, none a blocker:

  1. Signal-path exit status in npm-bin.js (worth a one-line fix). When the child dies by signal, the close handler does process.kill(process.pid, signal) — but the SIGINT/SIGTERM/SIGHUP listeners are still installed, so the re-raise re-enters the JS handler (which child.kills an already-dead child) instead of terminating the launcher by signal. Net effect: the launcher exits 0 where the comment says it mirrors the signal status (shell would see 0 instead of 130/143). The everyday path is unaffected — the TUI's Ctrl+C handling exits with a code — this only bites when the Bun child is killed externally. Removing the listener for that signal before re-raising fixes it.
  2. optional-dependencies.json is emitted but nothing consumes it. prepare-package.js hardcodes the five platform deps (correctly version-locked). Either wire the artifact into prepare-package.js or drop it — as-is the platform list lives in three places and this file can silently go stale.
  3. Doc/code mismatch on musl. The design doc says unsupported platforms (it names linux musl variants) get the launcher's friendly pointer to the standalone archives, but linux-x64 is in the resolution map, so a musl host resolves fine and then hits Bun's own loader failure rather than the friendly error. Either detect musl in the launcher or soften the doc's claim.

No correctness or security blockers found. No existing platform-resolution logic existed in the repo to reuse; scripts/ has no unit-test convention in this repo, so the absence of tests here is consistent (the build-time assertFile guards are the substitute).

The launch chain the PR introduces, for orientation:

sequenceDiagram
    participant P1 as npm user
    participant P2 as npm bin shim (node)
    participant P3 as npm-bin.js launcher
    participant P4 as Bun child (platform package)
    P1->>P2: qwen args
    P2->>P3: node npm-bin.js args
    P3->>P3: resolve platform package via createRequire
    P3->>P4: spawn bundled Bun on cli-entry.js, sets QWEN_CODE_LAUNCHER_PATH
    P4-->>P3: close event with code or signal
    P3-->>P1: mirror exit status
Loading

Test evidence — the PR's own CI

This commit has no code CI at all: the repo's CI workflow triggers only for bases main / release/** (ci.yml), and this PR's base is feat/opentui-migrate (stacked on #8677, still open). No unit, lint, typecheck, or integration run exists on d6e42a6, and none will fire until the base is re-pointed to main after #8677 merges. The only check-runs on the commit are bot orchestration. The finalize job has no PR CI to settle here; the table below records that state honestly.

Check Conclusion
Qwen Code CI (unit / lint / typecheck / integration) not triggered — base branch outside the CI filter (main, release/**)
Bot orchestration (triage, review-pr) in progress (this run)

Everything behavioral therefore rests on the author's local report (macOS arm64 only, per the PR body — their claim, not independently re-run here; unattended CI does not execute PR code): launcher qwen --version through Bun, a 3-assertion PTY smoke showing OpenTUI active, and the missing-platform-package error path with a working node fallback. Not verified by anyone's run: real npm publish (requires release secrets — the author says so too, and the dry-run default is the guard), and the Windows launcher path (author says structurally checked only).

Sandboxed verification would settle this: @qwen-code /verify — that the launcher actually resolves the installed platform package and execs the bundled Bun, and that the missing-package path exits non-zero with the node fallback intact, is not observable from the diff, and this commit has no CI at all. For the TUI surface, @qwen-code /tmux would confirm OpenTUI (mouse tracking, composer) is genuinely active when launched through the npm-installed bin rather than the ink fallback.

中文说明

代码审查:方案正确——按平台 optionalDependencies + Node bin launcher 是生态标准做法(esbuild / swc / opencode),实现干净、无 postinstall。依赖 #8677 standalone 基建的部分我逐一对照了 base 分支:归档布局断言(qwen-code/ 根、lib/cli-entry.jsbun/bin/bun / bun/bun.exebin/qwen / qwen.cmd、win-x64 用 .zip)与 create-standalone-package.js / build-standalone-release.js 的产物完全一致,构建期断言失败即报错;createRequire 解析在全局/本地安装下都正确(平台包故意不带 exports,两个脚本都有注释说明);dist 为 type: module,launcher 的 ESM 写法有效;发布步骤完整复用现有 audio-capture / 主包的防护(dry-run、已发布跳过、--tag),发布顺序(平台包先于主包)正确,ubuntu-latesttar/unzip 可用。

三个非阻塞发现:① npm-bin.js 子进程被信号杀死时,close 回调里 process.kill(process.pid, signal) 会重新进入已注册的信号处理器而不是以信号终止自身,最终退出码为 0 而非镜像信号状态(日常 Ctrl+C 路径走退出码不受影响)——重发前移除对应监听即可修复;② optional-dependencies.json 生成后无人消费(prepare-package.js 硬编码五个平台依赖),要么接上要么删掉,避免平台清单第三处副本悄悄过期;③ 设计文档称 musl 等不受支持平台会得到指向 standalone 的友好报错,但 linux-x64 在解析表里,musl 主机实际会撞到 Bun 自身的加载错误——要么在 launcher 里检测 musl,要么修正文档措辞。

测试证据:该提交没有任何代码 CI——仓库 CI 只对 base 为 main / release/** 的 PR 触发,而本 PR base 是 stacked 的 feat/opentui-migrate。单测、lint、typecheck、集成测试都没有跑过,#8677 合并、base 改回 main 之前也不会跑。提交上仅有的 check-run 是机器人编排任务。因此所有行为性结论目前只依据作者的本地报告(仅 macOS arm64,作者自述,未独立复跑;无人值守 CI 不执行 PR 代码):launcher qwen --version 走 Bun、PTY 冒烟 3 断言确认 OpenTUI 激活、缺平台包时报错路径与 node 回退可用。无人验证过:真实 npm publish(需发布密钥,作者亦说明,dry-run 默认是防护)与 Windows launcher 路径(作者称仅结构性检查)。

沙箱验证可以补齐:@qwen-code /verify 可证明 launcher 真实解析平台包并 exec 内置 Bun、缺包路径以非零码退出且 node 回退可用——这些从 diff 看不出来,且该提交完全没有 CI;@qwen-code /tmux 可确认从 npm 安装的 bin 启动时 OpenTUI(鼠标跟踪、composer)真实激活而非 ink 回退。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the code is genuinely good and the approach is the right pattern, but this is a release/public-contract change with no CI signal behind it, so it needs a human release-owner's call, not a bot approval.

Stepping back: this is the right solution to a real problem. After #8677 lands, npm installs run under Node and OpenTUI silently falls back to ink while standalone users get the new renderer — the two channels diverge, and npm is the biggest channel. The per-platform optionalDependencies + launcher-bin design is exactly the ecosystem-standard answer (esbuild / swc / opencode), the implementation is clean, and it's honest about what it did and didn't verify. I came in skeptical of whether five runtime packages was overkill; it isn't — that's the minimum to cover the platforms the standalone archives already ship, and cutting it would just leave some hosts on the fallback. No drive-by changes, nothing beyond the stated goal.

Why I'm deferring instead of approving:

  • This is release infrastructure and a public-contract change. The main package's bin moves to the launcher and five new packages get published. CODEOWNERS already gates release.yml on core-maintainer approval, and our policy escalates release/public-contract changes to a human regardless of how clean the diff reads. Publishing npm package names is effectively irreversible, so this deserves the release owner's eyes on the mechanics specifically.
  • There is no CI signal to stand on. Because the base is the still-unmerged feat/opentui-migrate branch (outside the main / release/** CI filter), no suite has run on d6e42a6 and none will until the base is re-pointed. I can't attest to green tests that don't exist, and the behavioral story rests on one author-reported environment (macOS arm64).
  • It's also simply not mergeable yet — it's stacked on feat(tui): OpenTUI renderer backend (react track) — flicker-free, first-class mouse [PR-1] #8677 and has to be re-based onto main after that lands.

To get this over the line: #8677 lands, base re-pointed to main, real CI goes green on the new head (that's the signal the finalize job can then act on), and a maintainer signs off on the publish ordering / package-name decisions. The minor items from the review — the signal-path exit status in the launcher, the unconsumed optional-dependencies.json, and the musl wording in the design doc — are worth addressing but are not blockers. Not verifying: real npm publish and the Windows launcher path (both flagged by the author too).

Deferring to @pomelo-nwu @wenshao — release/public-contract change with no CI coverage on a stacked branch; needs a human call on the release mechanics and a green CI run once it's re-pointed at main.

中文说明

信心:3/5 —— 代码质量确实好、方案也是正确的模式,但这是一个发布/公开契约层面的改动,且背后没有任何 CI 信号,需要人类发布负责人拍板,而不是机器人批准。

退一步看:这是对真实问题的正确解法。#8677 合入后,npm 安装在 Node 下运行,OpenTUI 会静默回退到 ink,而 standalone 用户能拿到新渲染器——两个渠道分叉,而 npm 是最大的渠道。按平台 optionalDependencies + launcher bin 正是生态标准答案(esbuild / swc / opencode),实现干净,且对"验证了什么、没验证什么"很诚实。我起初怀疑五个运行时包是否过度设计——并非如此:这正好覆盖 standalone 归档已支持的最低平台数,砍掉只会让某些主机留在回退上。没有顺手改动,没有超出目标的内容。

为什么是"暂缓"而非"批准":

  • 这是发布基础设施与公开契约改动。 主包 bin 换成 launcher,且新发布五个包。CODEOWNERS 本就要求核心维护者批准 release.yml,我们的政策也规定发布/公开契约改动无论 diff 多干净都要升级给人类。发布 npm 包名实际上不可撤销,所以发布机制尤其值得发布负责人过目。
  • 没有任何 CI 信号可依据。 因为 base 是尚未合并的 feat/opentui-migrate 分支(不在 main / release/** 的 CI 过滤内),d6e42a6 没有跑过、也不会跑任何套件,直到 base 改回。我无法为一组不存在的绿色测试背书,而行为层面的说法目前只依据作者自报的单一环境(macOS arm64)。
  • 现在也无法合并——stacked 在 feat(tui): OpenTUI renderer backend (react track) — flicker-free, first-class mouse [PR-1] #8677 之上,待其合入后需 rebase 到 main

推进路径:#8677 合入 → base 改回 main → 新 head 上跑出真实绿色 CI(这正是 finalize 任务可以据此行动的信号)→ 维护者对发布顺序/包名决策签字。审查中提到的次要项(launcher 的信号退出状态、未被消费的 optional-dependencies.json、设计文档中 musl 的措辞)值得处理但不是阻塞项。未验证:真实 npm publish 与 Windows launcher 路径(作者也已自行标注)。

暂缓并转交 @pomelo-nwu @wenshao —— 发布/公开契约改动,且 stacked 分支上无 CI 覆盖;需要人类对发布机制拍板,并在其改回 main 后拿到一次绿色 CI。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

⏸️ Deferring to @pomelo-nwu @wenshao — release/public-contract change (main package bin → launcher, five new public npm packages, release.yml publish steps) with zero CI coverage on its stacked base (feat/opentui-migrate, #8677 unmerged), and CODEOWNERS already requires core-maintainer approval for release.yml. Code review is clean apart from three minor notes (see the review comment above); needs a human call on the release mechanics, and a green CI run once the base is re-pointed to main after #8677 lands.

@chiga0
chiga0 force-pushed the feat/opentui-npm-preview branch from d6e42a6 to dd0ef60 Compare August 24, 2026 09:32
@github-actions

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

@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.

Partially reviewed — gaps disclosed.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R1-5 musl launcher error lacks the documented fallback pointer — already reported (triage stage-2 review, comment 5392897105)
  • R1-6 optional-dependencies.json unconsumed, platform list triplicated — already reported (triage stage-2 review, comment 5392897105)

Not reviewed: build-and-test — no code CI (unit / lint / typecheck / integration) fires on the stacked base feat/opentui-migrate (only bot orchestration checks ran); the existing scripts/tests suite was not run in full locally (package-assets.test.js ran during verification: 34/34).

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — no code CI (unit / lint / typecheck / integration) fires on the stacked base feat/opentui-migrate (only bot orchestration checks ran); the existing scripts/tests suite was not run in full locally (package-assets.test.js ran during verification: 34/34)。

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

— qwen3.8-max via Qwen Code /review (v0.22.0)

Comment thread scripts/npm-bin.js Outdated
Comment thread scripts/prepare-package.js
Comment thread scripts/package-npm-platform-packages.js
Comment thread scripts/prepare-package.js
Comment thread scripts/npm-bin.js Outdated
Comment thread scripts/package-npm-platform-packages.js Outdated
Comment thread scripts/package-npm-platform-packages.js Outdated
Comment thread scripts/npm-bin.js Outdated
Comment thread scripts/prepare-package.js
Comment thread .github/workflows/release.yml
@chiga0
chiga0 force-pushed the feat/opentui-npm-preview branch from dd0ef60 to 6a1ae8a Compare August 24, 2026 13:55
@chiga0
chiga0 force-pushed the feat/opentui-migrate branch from a2c3a48 to b25ee16 Compare August 24, 2026 13:57
@chiga0
chiga0 force-pushed the feat/opentui-npm-preview branch from 6a1ae8a to 0671500 Compare August 24, 2026 13:57

@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.

Partially reviewed — gaps disclosed.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • musl/Alpine fallback pointer — already reported (triage stage-2 review, comment 5392897105)
  • optional-dependencies.json unconsumed — already reported (triage stage-2 review, comment 5392897105)

Not reviewed: build-and-test — no code CI fires on the stacked base feat/opentui-migrate (CI filters bases main/release/**); build-test found no workspace affected by the diff (root-level scripts only). scripts/tests release-workflow + package-assets suites were run green (47/47) by Agent 7, but no lane covers the new scripts end-to-end.

Not explored to full depth (tool budget reached): "agent 5": none (no check was cut short)..

Not reviewed: reverse audit — stopped before round 8 by the review time budget.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — no code CI fires on the stacked base feat/opentui-migrate (CI filters bases main/release/**); build-test found no workspace affected by the diff (root-level scripts only). scripts/tests release-workflow + package-assets suites were run green (47/47) by Agent 7, but no lane covers the new scripts end-to-end。

未探索到全部深度(达到工具调用预算):"agent 5"none (no check was cut short).

未审查:反向审计——评审时间预算不足,未能开始第 8 轮。

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

— qwen3.8-max via Qwen Code /review (v0.22.0)

Comment thread scripts/npm-bin.js Outdated
Comment thread scripts/prepare-package.js
Comment thread scripts/package-npm-platform-packages.js
Comment thread scripts/npm-bin.js
Comment thread scripts/npm-bin.js
Comment thread .github/workflows/release.yml
Comment thread scripts/npm-bin.js Outdated
Comment thread scripts/npm-bin.js Outdated
Comment thread scripts/prepare-package.js Outdated
Comment thread scripts/package-npm-platform-packages.js
@chiga0
chiga0 force-pushed the feat/opentui-npm-preview branch from 0671500 to 5ad434d Compare August 25, 2026 02:03

@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.

Partially reviewed — gaps disclosed.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • musl/spawn-error branch lacks the documented fallback pointer (scripts/npm-bin.js:117) — already reported (triage stage-2 review, comment 5392897105)
  • optional-dependencies.json dead artifact (scripts/package-npm-platform-packages.js:186) — already reported (triage stage-2 review, comment 5392897105)

Not reviewed: build-and-test — diff touches only root-level files outside every workspace; build-test scoped to zero suites, and no code CI fires on the stacked base feat/opentui-migrate.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Convergence: round 3 posted 21 inline comment(s), 3 of them reported for the first time; the previous round posted 19 (7 new). Findings keep coming back to the same files: .github/workflows/release.yml (findings in rounds 1, 2; 1 more now); scripts/package-npm-platform-packages.js (findings in rounds 1, 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:build-and-test — diff touches only root-level files outside every workspace; build-test scoped to zero suites, and no code CI fires on the stacked base feat/opentui-migrate。

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

收敛情况:第 3 轮发布了 21 条行内评论,其中 3 条是首次提出;上一轮发布了 19 条(其中 7 条首次提出)。发现反复回到同一批文件:.github/workflows/release.yml(第 1、2 轮已出过发现,本轮又有 1 条);scripts/package-npm-platform-packages.js(第 1、2 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)

— qwen3.8-max via Qwen Code /review (v0.22.0)

Comment thread scripts/npm-bin.js Outdated
Comment thread scripts/prepare-package.js
Comment thread scripts/package-npm-platform-packages.js
Comment thread scripts/npm-bin.js
Comment thread scripts/npm-bin.js Outdated
Comment thread .github/workflows/release.yml
Comment thread scripts/npm-bin.js Outdated
Comment thread scripts/package-npm-platform-packages.js
Comment thread docs/design/npm-platform-runtime-packages.md Outdated
Comment thread scripts/package-npm-platform-packages.js Outdated
…ints, release guards (R1-1..R1-13, R2-1..R2-7)
@chiga0

chiga0 commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Review batch 3 pushed in ccc5f52. Every Critical and most Suggestions from all three rounds are addressed; per-thread replies follow.

Critical

  • R1-1 (signal re-raise swallowed): the close handler now removes its forwarding listeners before re-raising, so the re-raise takes the default disposition. Pinned by the new scripts/tests/npm-bin.test.js.
  • R1-2 (+ the dist/npm-platform sibling): npm-bin.js and npm-platform added to DIST_NPM_PACKAGE_ONLY_ENTRIES, so create-standalone-package.js skips them instead of dying on Unexpected dist asset.
  • R1-3 (standalone fingerprints): packagePlatform() now strips manifest.json, bin/, and node/ after lifting the archive root and asserts none of the isStandaloneInstallDir() fingerprints survive; end-to-end test drives the real script over fixture archives.
  • R1-4 / R1-5 (bin rewiring breaks no-platform consumers, exit-44 relaunch): the launcher no longer hard-fails — whenever the platform package is missing, damaged, or unmapped it prints a one-line notice and runs cli-entry.js under node, exactly the pre-PR behavior. The exit-44 relaunch re-entering through npm-bin.js therefore keeps working. QWEN_CODE_LAUNCHER_PATH stamping was dropped (the platform package no longer ships bin/qwen; cli-entry.js's PATH scan resolves the npm shim).
  • R2-5 (Windows SIGINT TerminateProcess): SIGINT forwarding is skipped on win32 — the console already delivers CTRL_C_EVENT to the child, and forwarding would defeat the double-Ctrl+C guard. Test covered.
  • R2-6 (Docker lane): the Dockerfile installs with --omit=optional, so the image runs the CLI bundle it was built from via the node fallback; combined with the launcher fallback the entrypoint cannot exit 1 even before the first platform release ships.
  • PUBLISHED_PACKAGES: the five platform packages are added to scripts/get-release-version.js; the pin test is updated.

Suggestions

  • R1-6: new scripts/tests/npm-bin.test.js (8 tests: mapping, missing/damaged/unmapped fallbacks, exit-code mirroring, re-raise after dropping forwarders, Windows SIGINT exemption).
  • R1-7: the publish-guard loop in package-scripts.test.js now covers Publish platform runtime packages, including subshell wrapping.
  • R1-8: package-assets.test.js pins bin.qwen=npm-bin.js, npm-bin.js in files (+ dist copy), and the five version-locked platform optionalDependencies.
  • R1-9: hand-rolled parser replaced by the shared parseArgs from release-script-utils.js; --key=value works (tested).
  • R1-10: PLATFORMS derived from RELEASE_TARGETS (build-standalone-release.js).
  • R1-11: the damaged branch now falls back to node with a reinstall hint.
  • R1-12: installManagedNpmUpdate removes the five staged platform packages after the staging install, before activation — deliberately not --omit=optional, since sharp/node-pty/clipboard/audio-capture are needed by the node-run payload.
  • R1-13: new scripts/tests/package-npm-platform-packages.test.js runs the real script end-to-end in test:citest:scripts.
  • R2-1 (unmapped platforms): fall back to node instead of hard-failing.
  • R2-2 (wrong comment): rewritten — no longer claims every optional dep is version-locked.
  • R2-4 (other terminating signals): unix forwarding extended to SIGHUP/SIGINT/SIGQUIT/SIGTERM.
  • R2-7 (node/ dead weight): the compat mirror is stripped, removing the win-x64 second Bun copy.
  • Design doc cost model: now states the session-long waiter (~65 MB RSS on Node 22); Fallbacks section rewritten for the automatic node fallback.
  • R2-3 (publish-guard copy-paste): deferred — extracting the guard into a shared step touches five existing workflow steps and is out of scope for this PR; noted as follow-up.

Verification: test:scripts green for all touched/new suites (npm-bin 8/8, package-npm-platform-packages 4/4, package-scripts 16/16, package-assets 34/34, get-release-version 48/48), managed-npm-update.test.ts 14/14, tsc --noEmit clean, ESLint/Prettier clean, plus an end-to-end probe of the repackager over a fixture archive (fingerprints stripped, layout asserted).

中文说明

第三批评审修复已推送(ccc5f52874)。三轮评审的全部 Critical 与大部分 Suggestion 已处理,逐 thread 回复见下。

Critical:R1-1 信号重抛前先移除转发监听器;R1-2(含 dist/npm-platform 兄弟条目)加入跳过白名单;R1-3 重新打包时剥离 manifest.json/bin//node/ 三类 standalone 指纹并断言;R1-4/R1-5 启动器不再硬失败——平台包缺失/损坏/未映射时打印一行提示并以 node 运行 cli-entry.js(exit-44 更新重启契约恢复),同时移除 QWEN_CODE_LAUNCHER_PATH 打点;R2-5 Windows 不再转发 SIGINT(控制台已投递 CTRL_C_EVENT,转发即 TerminateProcess,会击穿双击确认);R2-6 Dockerfile 以 --omit=optional 安装,镜像运行自己构建的 JS 而非上一个发布版的平台包;PUBLISHED_PACKAGES 补齐 5 个平台包。

Suggestion:新增 npm-bin.test.js(8 测试)与 package-npm-platform-packages.test.js(端到端驱动真实脚本);发布守卫测试覆盖平台包发布步骤;manifest 断言钉住 bin/files/optionalDependencies;改用共享 parseArgs(支持 --key=value);PLATFORMSRELEASE_TARGETS 派生;损坏分支回退 node;托管更新在激活前删除 5 个暂存平台包(不用一刀切 --omit=optional,sharp/node-pty 等 node 载荷需要);未映射平台回退 node;注释措辞修正;信号转发扩至 SIGHUP/SIGINT/SIGQUIT/SIGTERM;设计文档成本模型补记会话期常驻内存(约 65MB RSS)并重写 Fallbacks 一节。R2-3(发布守卫复制粘贴)推迟到后续 PR——抽共享步骤会触及五个现有工作流步骤,超出本 PR 范围。

验证:相关 scripts 测试套件全绿、managed-npm-update.test.ts 14/14、tsc --noEmit 0 错误、ESLint/Prettier 干净,另有夹具归档端到端探针确认指纹剥离与布局断言。

@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.

Partially reviewed — gaps disclosed.

2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • R2-3 publish-guard block duplication (.github/workflows/release.yml:529) — still stands; author deferred to a follow-up, settled in its thread
  • R2-4 launcher forwards only a terminating-signal subset (scripts/npm-bin.js) — partially addressed (SIGQUIT added); author's cross-platform rationale settled the thread

Not reviewed: the executable-script lint — Dockerfile: hadolint is not installed.

Not explored to full depth (tool budget reached): "agent 8a": none — all six invariant areas were walked to a conclusion..

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Test Plan (not a blocker): 3 assertions PASS — this review observed 22264 passed.

Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:

  • scripts/npm-bin.js:34 — [probe] launcher/manifest platform sets unpinned against RELEASE_TARGETS (drift mutant keeps 94 tests green)
  • scripts/package-npm-platform-packages.js:149 — [probe] the no-exports-map resolution contract is unpinned (adding exports silently disables the Bun channel)
  • docs/design/npm-platform-runtime-packages.md:39 — [review] design doc contradicts shipped code (QWEN_CODE_LAUNCHER_PATH stamping; exit-1 fallback claim)

Convergence: round 4 posted 20 inline comment(s), 19 of them reported for the first time; the previous round posted 21 (3 new). Findings keep coming back to the same files: scripts/npm-bin.js (findings in rounds 1, 2; 1 more now); scripts/prepare-package.js (findings in rounds 1, 2; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:the executable-script lint — Dockerfile: hadolint is not installed。

未探索到全部深度(达到工具调用预算):"agent 8a"none — all six invariant areas were walked to a conclusion.

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

Test Plan(非阻断):3 assertions PASS — this review observed 22264 passed

收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 4 轮发布了 20 条行内评论,其中 19 条是首次提出;上一轮发布了 21 条(其中 3 条首次提出)。发现反复回到同一批文件:scripts/npm-bin.js(第 1、2 轮已出过发现,本轮又有 1 条);scripts/prepare-package.js(第 1、2 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。(仅为观察——本轮评审未因此扣留任何内容。)

— qwen3.8-max via Qwen Code /review (v0.22.0)

Comment thread packages/cli/src/utils/managed-npm-update.ts Outdated
Comment thread scripts/npm-bin.js
Comment thread scripts/tests/package-npm-platform-packages.test.js
Comment thread scripts/tests/package-scripts.test.js
Comment thread packages/cli/src/utils/managed-npm-update.ts Outdated
Comment thread scripts/tests/package-assets.test.js Outdated
Comment thread scripts/tests/package-npm-platform-packages.test.js Outdated
Comment thread scripts/tests/package-npm-platform-packages.test.js Outdated
Comment thread scripts/tests/package-scripts.test.js
Comment thread scripts/prepare-package.js
@chiga0

chiga0 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Review batch 4 pushed in c63e912 — all 20 R4 comments addressed (per-thread replies posted, threads resolved).

Critical fixes

  • R4-1 Managed updates on the Bun/platform channel: getInstallationInfo now routes platform-channel installs (/node_modules/@qwen-code/qwen-code-*, checked after the local-install guard) to a manual npm install -g @qwen-code/qwen-code@latest message with no updateCommand, so handleAutoUpdate never starts a node-channel managed update from the Bun runtime. npm-bin.js stamps QWEN_CODE_HOST_NODE and updateCheck.runGlobalNpm prefers it, fixing the background update check under Bun. Two new tests.
  • R4-2 Runtime launch failure: linux platform packages declare libc: ["glibc"] (npm >= 10 skips musl hosts); npm-bin.js falls back to node cli-entry.js when the platform runtime fails to spawn (spawnFailed flag suppresses the post-ENOENT close(-2) mirror).
  • R4-3 zip hard dependency: platform-package test now probes zip/unzip and uses describe.skipIf, matching the install-script.test.js gate.

Suggestion fixes — hardcoded platform list replaced by manifest-derived purge (R4-5 + matching test R4-6); launcher test rewrite: exact runtime paths incl. win32 bun.exe (R4-8), it.each over all 5 platforms (R4-12), per-conjunct damaged-package cases (R4-15), close(0,null)exit(0) branch (R4-16), dual spawn-error tests (R4-11), invocationCallOrder signal sequence (R4-14), exact (signal, handler) removal (R4-17), forwarded-signal kill assertions (R4-7), fallback arg passthrough (R4-19); packaging tests: exact os/cpu/libc field table (R4-9), subshell regex pin (R4-13), Bun runtime survival (R4-18), dist/npm-bin.js content identity with source (R4-20); publish-order pin for the R1-7 remainder.

Deferred — R4-10 (cross-launcher-root cleanup of stale version directories) needs live-session pins for activated version directories beyond the staging window; impact is bounded (platform payloads are now purged before activation, so stale roots no longer grow). Tracked as a follow-up issue per the review-round policy.

Verification: scripts tests 70/70, cli update/install tests green, tsc --noEmit clean, eslint clean.

@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.

Partially reviewed — gaps disclosed.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • QWEN_CODE_LAUNCHER_PATH design-doc drift (docs/design/npm-platform-runtime-packages.md:38) — already recorded on the PR in the round-4 deferred list (docs/design/npm-platform-runtime-packages.md:39)

Not reviewed: the executable-script lint — Dockerfile: hadolint is not installed.

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:

  • scripts/package-npm-platform-packages.js:61 — [review] --help prints a misleading version error and exits 1
  • scripts/prepare-package.js:355 — [review] platform set hardcoded in two lists with no cross-check
  • .github/workflows/release.yml:522 — [review] nothing pins the packaging step feeding the publish loop
  • scripts/npm-bin.js:94 — [test] forwarder teardown before re-raise is unpinned (mutant survived)
  • scripts/npm-bin.js:98 — [test] the ?? 1 exit-code fallback is unpinned (mutant survived)
  • scripts/tests/npm-bin.test.js:70 — [review] call-through process.on spy leaks real signal handlers on the worker
  • scripts/tests/npm-bin.test.js:20 — [review] no-exports-map contract for platform packages is unpinned
  • scripts/tests/npm-bin.test.js:99 — [review] a shell:true spawn mutant survives the suite
  • scripts/tests/npm-bin.test.js:142 — [review] fallbackEntry's import.meta.url derivation is unwitnessed

Convergence: round 5 posted 17 inline comment(s), 17 of them reported for the first time; the previous round posted 20 (19 new). Findings keep coming back to the same files: scripts/tests/npm-bin.test.js (findings in round 4; 5 more now); scripts/npm-bin.js (findings in round 4; 4 more now); scripts/tests/package-npm-platform-packages.test.js (findings in round 4; 2 more now), and 3 more file(s). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)

Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:the executable-script lint — Dockerfile: hadolint is not installed。

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 9 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 5 轮发布了 17 条行内评论,其中 17 条是首次提出;上一轮发布了 20 条(其中 19 条首次提出)。发现反复回到同一批文件:scripts/tests/npm-bin.test.js(第 4 轮已出过发现,本轮又有 5 条);scripts/npm-bin.js(第 4 轮已出过发现,本轮又有 4 条);scripts/tests/package-npm-platform-packages.test.js(第 4 轮已出过发现,本轮又有 2 条),另有 3 个文件。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)

机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)

— qwen3.8-max via Qwen Code /review (v0.22.2)

Comment thread scripts/npm-bin.js
Comment thread scripts/tests/npm-bin.test.js
Comment thread scripts/tests/npm-bin.test.js Outdated
Comment thread scripts/npm-bin.js
Comment thread scripts/npm-bin.js
Comment thread Dockerfile
Comment thread packages/cli/src/utils/installationInfo.ts
Comment thread scripts/tests/npm-bin.test.js
Comment thread scripts/tests/package-npm-platform-packages.test.js
Comment thread scripts/tests/npm-bin.test.js
… watcher, serve signal dedupe (R5 criticals)

- npm-bin: drop the failed child's signal forwarders before launching the fallback, so a re-raised death signal is no longer swallowed (launcher exited 0 for signal-killed runs).
- npm-bin: install a presence-only SIGINT watcher on win32; without one libuv's console control handler returns FALSE for CTRL_C_EVENT and Windows terminates the launcher instantly while the CLI child keeps running. Never forward: child.kill('SIGINT') maps to TerminateProcess on Windows.
- npm-bin tests: pin platform/arch in the happy path, normalize path separators in the /bun/ predicate, add a teardown witness asserting forwarders are removed on the spawn-error path, and rewrite the win32 test around the watcher contract.
- run-qwen-serve: dedupe signals redelivered within 50ms of drain start (process-group forwarding from the npm launcher, cgroup-wide stops) instead of treating them as an operator double-press; same shape as gemini.tsx's SIGINT_RERAISE_IGNORE_MS.
- run-qwen-serve tests: rework the force-exit test past the dedupe window and add a duplicate-delivery test covering the process-group shape.
@chiga0

chiga0 commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

Review batch 5 pushed in 9eb74f6 — all 17 R5 comments addressed (per-thread replies posted, threads resolved).

Criticals — all five fixed:

  • R4-2 (fix-induced forwarder leak): the spawn-error path now removes the failed child's signal forwarders before launching the fallback, so a re-raised death signal is no longer swallowed (launcher exited 0 for signal-killed runs). Witness added: forwarder-pair teardown assertions plus a fallback-child signal close driving the re-raise.
  • R4-6 (fix-induced): happy-path runtime test now pins setPlatform('linux'); setArch('x64'), so the POSIX Bun layout assertion is stable on the merge_group-only Windows lane.
  • Damaged-runtime predicate: separator-agnostic '/bun/' matching (the suggested replaceAll('\\', '/') normalization applied verbatim), so the fallback test also triggers under Windows backslash paths.
  • Windows SIGINT watcher: presence-only SIGINT registration on win32 so libuv's console control handler dispatches CTRL_C_EVENT instead of letting Windows terminate the launcher instantly; deliberately never forwarded (child.kill('SIGINT') maps to TerminateProcess). The win32 test was rewritten around the new contract.
  • Serve signal dedupe: run-qwen-serve.ts's onSignal ignores a signal redelivered within 50 ms of drain start (process-group forwarding from the npm launcher, systemd KillMode=control-group), same shape as gemini.tsx's SIGINT_RERAISE_IGNORE_MS. A genuine operator double-press still force-exits. New duplicate-delivery test covers the process-group shape; the force-exit test was reworked past the window (its old in-flight killAllSync assertions were a timing artifact — the fast mock drain deregisters the worker from the manager's kill set within the window).

Two Suggestions were implemented alongside the Critical fixes (they witness the same paths): R4-11 fallback-child close assertions and the first-child forwarder-teardown witness.

Deferred to a follow-up (per AGENTS.md — this PR is past ~5 review rounds, so only Critical fixes land in this round; recording here so nothing is silently dropped):

  1. R4-5: KEEP-half witness for the purge predicate (non-platform optionalDependencies survive activation).
  2. R4-15: mode assertion for the copied dist/npm-bin.js (witness the chmodSync(0o755) in prepare-package.js).
  3. R4-17: exec-mode pin for the bundled Bun through the archive → extract → publish pipeline.
  4. R4-18: strengthen the subshell regex to relate npm publish to the skip branch's paren group.
  5. QWEN_CODE_HOST_NODE producer-half test (launcher's ??= stamp).
  6. Docker: explicit opt-out env knob to silence the fallback notice on --omit=optional installs.
  7. Docker: gate the image's refusal to resolve a platform runtime (image smoke check when one exists).
  8. installationInfo: sudo-aware update message on the platform-runtime branch.
  9. npm-bin test: pin the createRequire base argument (expect.stringMatching(/npm-bin\.js$/)).
  10. package-npm-platform-packages.test.js: throw (not warn-and-skip) when zip/unzip are missing on a CI host.

Verification: scripts/tests/npm-bin.test.js 16/16, run-qwen-serve.test.ts 264/264, Prettier clean, ESLint clean, npm run typecheck green.

@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.

Partially reviewed — gaps disclosed.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

  • optional-dependencies.json unconsumed / platform list triplicated — already reported (round 1 as R1-6, triage stage-2 review, comment 5392897105)

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Not reviewed: the executable-script lint — Dockerfile: hadolint is not installed.

Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted.

Test Plan (not a blocker): 3 assertions PASS — this review observed 22267 passed.

Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/serve/run-qwen-serve.test.ts:10321 — [review] force-exit kill wiring lost its only positive witness (fix-induced by the R5-3 fix)
  • packages/cli/src/utils/installationInfo.ts:291 — [probe] platform branch has no Windows-shaped test witness
  • scripts/tests/npm-bin.test.js:80 — [probe] suite leaks real signal listeners into the vitest worker
  • scripts/tests/npm-bin.test.js:330 — [probe] win32 SIGTERM forwarder never invoked in the test
  • scripts/tests/package-assets.test.js:779 — [probe] node-fallback cli-entry.js shipment is unpinned
  • scripts/tests/package-npm-platform-packages.test.js:161 — [probe] platform manifest shipment fields unpinned

Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had no anchor this round could use either — none at all, one with no certifier, one certified by an identity other than the one this round runs under, or one this round's fetch refused or resolved to the head — so the next review re-reads the whole diff unless recovery grafts an earlier own anchor that the round running it can use onto the complete work list this round leaves behind, and keeps doing so until a round's marker carries an anchor again or a graft lands that the round running it can use. (Stated, not acted on — this changes nothing about what the round posts.)

中文说明

仅完成部分审查,审查缺口已披露。

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

未审查:reverse audit — did not converge within the reverse-audit round cap of 5。

未审查:the executable-script lint — Dockerfile: hadolint is not installed。

未检查(工具限制,非阻断):the executable-script lint — .github/workflows/release.yml: actionlint embedded-shell source mapping is not yet supported — not linted。

Test Plan(非阻断):3 assertions PASS — this review observed 22267 passed

收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 6 条(原文未翻译,列表见上方英文部分)。

机制健康:本轮未能干净收尾,因而扣留了增量锚点,而它恢复到的那一轮也没有留下本轮可用的锚点——要么完全没有、要么没有认证者、要么由本轮运行身份之外的身份认证、要么被本轮的获取拒绝或解析为头提交——因此下一次评审将重读整个 diff,除非恢复流程把本轮能使用的更早自有锚点嫁接到本轮留下的完整工作清单上;并会一直如此,直到某一轮的标记重新带上锚点,或落地的嫁接能被运行该轮的评审使用。(仅陈述,不据此行动——这不改变本轮发布的任何内容。)

— qwen3.8-max via Qwen Code /review (v0.22.2)

@chiga0
chiga0 merged commit 9982398 into feat/opentui-migrate Aug 31, 2026
490 of 506 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.

2 participants