fix(cli): Guard serve fast-path bundle closure - #5995
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
marked this pull request as ready for review
June 29, 2026 08:31
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. |
Collaborator
|
(Rerun — updating prior Stage 1 comment in place.) Thanks for the PR! Template looks good ✓ — all required headings present, bilingual description, risk/scope section, and a detailed Reviewer Test Plan. On direction: this is squarely aligned with the daemon startup optimization (#4748 / #5785). The serve fast-path closure must stay free of heavy ACP runtime and transitive core/vendor imports for the listener path to be fast. Source-level import guards alone can't catch esbuild shared chunks and hashed output names, so a metafile-based reachability check is the natural next step. No concerns here — this is infrastructure that protects performance. On approach: the scope is tight and every file in the diff is needed. The new Moving on to code review. 🔍 中文说明（重新跑 triage — 更新之前的 Stage 1 评论。） 感谢贡献！ 模板完整 ✓ — 所有必需标题都在，双语描述，risk/scope 部分，以及详细的 Reviewer Test Plan。 方向：与 daemon 启动优化 (#4748 / #5785) 完全对齐。serve fast-path closure 必须保持不引入重 ACP runtime 和 transitive core/vendor imports，listener 路径才能快。纯源码级 import guard 无法捕获 esbuild shared chunks 和 hash 后的输出文件名，所以基于 metafile 的 reachability check 是自然的下一步。没有顾虑 — 这是保护性能的基础设施。 方案：范围紧凑，diff 里每个文件都是必要的。新的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal (before reading the diff): To guard the serve fast-path bundle closure, I'd parse the esbuild metafile, BFS the static import graph from the Comparison with the PR: The PR's approach matches my proposal exactly — metafile BFS, static-only closure (dynamic imports correctly skipped via No blockers found. The code is correct, the scope is tight, and the approach is sound. Specific observations:
TestingAll three verification targets pass: Script unit tests (synthetic metafiles)4 tests covering: forbidden source detection via static imports, dynamic import allowance, vendor package detection, and Windows path normalization (no false positives on Real bundle closure checkThe actual esbuild metafile from a CLI fast-path unit testsAll 58 tests pass, including the new 中文说明代码审查独立方案（读 diff 前）： 为守护 serve fast-path bundle closure，我会解析 esbuild metafile，从 与 PR 对比： PR 方案与我的完全一致 — metafile BFS、仅静态 closure（通过 未发现阻塞问题。 代码正确，范围紧凑，方案可靠。 测试三个验证目标全部通过：
— Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, well-scoped infrastructure PR that does exactly what it says. The daemon startup optimization (#5785) depends on the serve fast-path closure staying free of heavy runtime modules. Source-level import guards alone can't catch esbuild shared chunks and hashed output names — this PR closes that gap with a metafile-based reachability check. The approach is the right one: BFS over the static import graph, skip dynamic imports, check inputs against forbidden lists. The diagnostic output (import path chains) makes violations actionable. The decision to move the heavy bundle assertion out of unit tests and into a dedicated CI check is the right call. Running esbuild inside a vitest test was slow and fragile — the new script runs once in CI with a pre-built metafile. The vendor package sentinels (glob, chokidar, @iarna/toml, fzf) add coverage that the old test didn't have. All verification passes: script tests (4/4), CLI fast-path tests (58/58), and the real bundle closure check against the actual esbuild output. The code is straightforward, every file in the diff is needed, and I'd be comfortable maintaining this. Approving. ✅ 中文说明这是一个干净、范围合理的 infrastructure PR，完全兑现了描述中的承诺。 daemon 启动优化 (#5785) 依赖 serve fast-path closure 不引入重 runtime 模块。纯源码级 import guard 无法捕获 esbuild shared chunks 和 hash 后的输出文件名 — 本 PR 用基于 metafile 的 reachability check 补上了这个缺口。方案正确：BFS 遍历静态 import 图，跳过 dynamic imports，检查 inputs 是否命中 forbidden 列表。诊断输出（import 路径链）让违规可追溯。 将重 bundle 断言从单测移到独立 CI 检查是正确决定。在 vitest 测试中跑 esbuild 既慢又脆弱 — 新脚本在 CI 中用预构建的 metafile 跑一次即可。vendor package sentinel（glob、chokidar、@iarna/toml、fzf）增加了旧测试没有的覆盖。 所有验证通过：脚本测试 (4/4)、CLI fast-path 测试 (58/58)、以及针对真实 esbuild 输出的 bundle closure 检查。代码直观，diff 中每个文件都是必要的，后续维护无压力。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jun 29, 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. ✅
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Verification report — PR #5995 (Guard serve fast-path bundle closure)I checked out this PR head ( TL;DRLooks good — no blocking issues. The new bundle guard is correct, well-tested, and provably load-bearing: it passes on the clean bundle and fails with a precise diagnostic the moment a forbidden ACP runtime module is statically pulled into the serve pre-listen closure. The refactor (moving the expensive real-bundle assertion out of the per-platform vitest path into a dedicated ubuntu CI check) is a net improvement and strictly broader in coverage than the test it replaces.
Setup
The decisive mutation test (A → B → A)The whole value of a guard is that it fails when it should. I converted the legitimate dynamic ACP load into a static one by adding That single static import pulled the entire ~179 KB ACP runtime cluster (bridge → compactionEngine, bridgeClient, permissionMediator, …) into one statically-reachable chunk — exactly the "heavy runtime entering the listener path through a shared chunk" regression this guard exists to block. The metafile-reachability approach (follow static Notes / trade-offs (non-blocking)
RecommendationNo changes requested. The guard does what it claims, is well-tested, and the import boundary holds on the real bundle. Recommend merge once the in-progress CI goes green — at report time the new "Check serve fast-path bundle closure" step was still running on the ubuntu leg (it passes locally); mac/windows are the expected named-job skips. 中文版（完整对应）验证报告 — PR #5995（给 serve fast-path bundle closure 加 guard）我 checkout 了本 PR head（ 结论速览没问题 —— 无阻塞项。 新的 bundle guard 正确、测试充分，且确实承重：在干净 bundle 上通过，一旦有 forbidden ACP runtime 模块被静态拉进 serve pre-listen closure，它立刻以精确诊断失败。这次重构（把昂贵的真实 bundle 断言从逐平台 vitest 路径移到独立的 ubuntu CI 检查）是净改进，且覆盖范围严格大于它替换掉的那个测试。
环境
决定性的变异测试（A → B → A）guard 的全部价值在于"该失败时能失败"。我把合法的动态 ACP 加载改成静态（往 pre-listen root 那一个静态 import 就把整个约 179 KB 的 ACP runtime 簇（bridge → compactionEngine、bridgeClient、permissionMediator……）拉进了一个静态可达 chunk —— 正是这个 guard 要拦的"重 runtime 通过 shared chunk 进入 listener 路径"的回归。基于 metafile 可达性的方法（跟随静态 说明 / 取舍（非阻塞）
建议无需改动。 guard 名副其实、测试充分，import 边界在真实 bundle 上成立。建议在进行中的 CI 变绿后合并 —— 报告时新的 "Check serve fast-path bundle closure" 步骤还在 ubuntu 腿上运行（本地通过）；mac/windows 是预期的 named-job skip。 Verified on macOS against PR head |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
The bundle-level guard script is well-structured — BFS closure traversal, suffix/package matching, and dynamic import exclusion are all correct. Build passes, all 12 script tests and 58 unit tests pass. The move from inline vitest assertion to a dedicated CI check is a clean improvement.
— qwen3.7-max via Qwen Code /review
wenshao
previously approved these changes
Jun 29, 2026
Collaborator
|
@qwen-code /triage |
Collaborator
|
(Rerun — updating prior Stage 2 comment with fresh independent verification.) Code ReviewIndependent proposal (before reading the diff): to guard the serve fast-path bundle closure I'd (1) parse the esbuild metafile, (2) BFS the static import graph from each pre-listen root output (skipping dynamic imports and externals), (3) match every reached output's inputs against a forbidden list of ACP bridge modules, core shell runtime, and heavy vendor packages, (4) surface the exact static import path for each violation so debugging is actionable, and (5) move the whole thing out of per-run unit tests into a dedicated CI script so the slow esbuild build happens once per CI run, not on every Comparison with the PR: the PR's approach matches this independent proposal essentially exactly — metafile BFS via The The vitest alias addition ( No blockers found. Code is correct, scope is tight, approach is sound. Testing (independent verification in worktree)Worktree checked out at PR head, 1. Script unit tests (synthetic metafiles): ✅ 14/142. Real bundle closure check: ✅ PASS3. CLI fast-path source-level tests: ✅ 58/58The 4. Workspace typecheck: ✅ 0 errors5. CI status: ✅ All passingReal-scenario (tmux) noteThis PR is non-UI — it's a build/import-boundary guard. There's no TUI to drive. The "real scenario" here is the real bundle + guard against the actual esbuild metafile; that's the load-bearing evidence, not a tmux session. 中文说明（重新跑 triage — 用新的独立验证更新之前的 Stage 2 评论。） 代码审查独立方案（读 diff 前）： 解析 esbuild metafile，从每个 pre-listen root output BFS 静态 import 图（跳过 dynamic imports 和 externals），匹配每个可达 output 的 inputs 到 forbidden 列表，给出精确静态 import 路径，搬到专门 CI 脚本。保留源码级 guard。 与 PR 对比： PR 方案与独立方案基本一致 — metafile BFS、仅静态 closure、source-file 后缀和 vendor-package sentinel 都覆盖、独立 CI 脚本、源码级 guard 保留并扩展。诊断输出包含调试违规所需的全部信息。 未发现阻塞问题。 代码正确，范围紧凑，方案可靠。 测试（worktree 中独立验证）
真实场景（tmux）说明本 PR 是非 UI 的构建/import 边界 guard。没有 TUI 可驱动。"真实场景"是真实 bundle + guard 对实际 esbuild metafile 的检查；这是承重证据，不是 tmux 会话。 — Qwen Code · qwen3.7-max |
Collaborator
|
(Rerun — updating prior Stage 3 comment based on fresh independent verification.) This is a clean, well-scoped infrastructure PR that does exactly what it says, and the guard is provably load-bearing. The motivation is real: the daemon startup optimization depends on the serve fast-path closure staying free of heavy runtime modules, and source-level import guards alone cannot catch esbuild shared chunks and hashed output names. The metafile-based reachability check closes that gap. My independent proposal for solving this matched the PR's approach essentially exactly — there isn't an obviously simpler path that was missed. The implementation is straightforward: BFS over the static import graph, skip dynamic imports and externals, match suffixes for source inputs and package names for vendor packages, report the exact import chain for each violation. The script is readable (~315 lines), the tests are thorough (14 synthetic metafile tests covering static/dynamic/external/vendor/missing-root/error paths), and the diagnostic output is actionable. All independent verification passed in a fresh worktree at PR head: script tests 14/14, real bundle closure check PASS, CLI fast-path tests 58/58, workspace typecheck 0 errors across all 7 packages. CI is green on ubuntu (the macos/windows skips are expected named-job behavior). The decision to move the heavy bundle assertion out of per-run vitest into a dedicated CI check is the right call — net CI win, strictly broader coverage (3 roots, 7 runtime modules, 4 vendor sentinels vs the old 1 root / 4 inputs), and source-level guards still run locally for fast feedback. Every file in the diff is needed for the stated goal. No drive-by refactors, no scope creep. Approving. ✅ 中文说明（重新跑 triage — 基于新的独立验证更新之前的 Stage 3 评论。） 这是一个干净、范围合理的 infrastructure PR，完全兑现了描述中的承诺，且 guard 被证明真的承重。 动机真实：daemon 启动优化依赖 serve fast-path closure 不引入重 runtime 模块，而纯源码级 import guard 无法捕获 esbuild shared chunks 和 hash 后的输出文件名。基于 metafile 的 reachability check 补上了这个缺口。独立方案与 PR 方案基本一致 — 没有更简单的路径被错过。 实现直接：BFS 静态 import 图、跳过 dynamic imports 和 externals、source input 用后缀匹配、vendor package 用包名匹配、报告每个违规的精确 import 链。脚本可读（约 315 行），测试充分（14 个 synthetic metafile 测试覆盖静态/动态/外部/vendor/缺失 root/错误路径），诊断输出可操作。 在全新 worktree 里所有独立验证都通过：脚本测试 14/14、真实 bundle closure 检查 PASS、CLI fast-path 测试 58/58、workspace typecheck 全部 7 个包 0 errors。CI 在 ubuntu 上绿色（macos/windows 跳过是预期的 named-job 行为）。 把重 bundle 断言从逐平台 vitest 搬到专门 CI 检查是正确决定 — CI 净收益，覆盖严格更广，源码级 guard 仍在本地跑。 diff 里每个文件对声明的目标都是必要的。没有顺手重构，没有范围蔓延。批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jun 29, 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. ✅ Independent verification in a fresh worktree confirmed everything: 12/12 script tests, real bundle closure PASS, 58/58 fast-path tests, workspace typecheck clean, and the decisive mutation test (static-import the ACP bridge into run-qwen-serve.ts, rebuild, re-run) caught the deliberate regression on all 10 offender categories with precise diagnostic import paths. Guard is provably load-bearing.
wenshao
reviewed
Jun 29, 2026
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
approved these changes
Jun 29, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jun 29, 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
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
The bundle-level guard is well-structured — BFS closure traversal, suffix/package matching, and dynamic import exclusion are all correct. All 14 script tests and 58 fast-path tests pass. Typecheck and ESLint clean.
— qwen3.7-max via Qwen Code /review
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Jun 29, 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.
What this PR does
This PR adds a dedicated bundle-level guard for the
qwen servefast path so the pre-listen static bundle closure is checked against ACP bridge runtime modules, core shell runtime, and the heavy vendor packages that previously entered through shared chunks. It also keeps the lightweight source-level guard for the request-helper import boundary and moves the expensive real-bundle assertion out of the CLI unit test path into an explicit CI check.Why it's needed
The daemon startup optimization depends on keeping the listener path free of eager ACP runtime and transitive core/vendor imports. Source-level import checks alone can miss esbuild shared chunks and hashed output names, so this adds a metafile-based reachability check that follows static output imports while allowing dynamic imports used by the runtime loader.
Reviewer Test Plan
How to verify
Run
npm run test:scripts -- scripts/tests/serve-fast-path-bundle-check.test.jsand confirm the synthetic metafile coverage catches static forbidden runtime imports, allows dynamic imports, reports the vendor package sentinels, and prints the static import path diagnostic. Runcd packages/cli && npx vitest run src/serve/fast-path.test.tsand confirm the source fast-path tests pass. Runnpm run check:serve-fast-path-bundleand confirm the real bundled serve fast-path closure passes. Runnpm run build && npm run typecheckand confirm the workspace still builds and typechecks.Evidence (Before & After)
N/A. This is a non-UI guard and import-boundary hardening change.
Tested on
Environment (optional)
macOS local checkout, Node v26.0.0, npm 11.12.1.
Risk & Scope
getCliVersion()timing, raw HTTP/Express bootstrap behavior, settings async reads, or packagesideEffectsmetadata.Linked Issues
Refs #4748
中文说明
What this PR does
这个 PR 为
qwen servefast path 增加了独立的 bundle-level guard，用来检查 pre-listen static bundle closure 是否包含 ACP bridge runtime、core shell runtime，以及此前通过 shared chunks 进入的重 vendor packages。同时保留 request-helper import 边界的轻量源码级 guard，并把真实 bundle 的重断言从 CLI 单测路径移到显式 CI 检查里。Why it's needed
daemon 启动优化依赖 listener 路径不 eager 加载 ACP runtime 和 transitive core/vendor imports。单纯的源码 import 检查会漏掉 esbuild shared chunks 和 hash 后的输出文件名，所以这里新增基于 metafile 的 reachability check：跟踪静态 output imports，同时允许 runtime loader 使用的 dynamic imports。
Reviewer Test Plan
How to verify
运行
npm run test:scripts -- scripts/tests/serve-fast-path-bundle-check.test.js，确认 synthetic metafile 覆盖可以捕获静态 forbidden runtime imports、允许 dynamic imports、报告 vendor package sentinels，并输出 static import path 诊断。运行cd packages/cli && npx vitest run src/serve/fast-path.test.ts，确认源码 fast-path 测试通过。运行npm run check:serve-fast-path-bundle，确认真实 bundled serve fast-path closure 通过。运行npm run build && npm run typecheck，确认 workspace 仍可 build 和 typecheck。Evidence (Before & After)
N/A。这是非 UI 的 guard 和 import-boundary hardening 变更。
Tested on
Environment (optional)
macOS 本地 checkout，Node v26.0.0，npm 11.12.1。
Risk & Scope
getCliVersion()timing、raw HTTP/Express bootstrap behavior、settings async reads，也不修改 packagesideEffectsmetadata。Linked Issues
Refs #4748