fix(cli): Avoid ACP runtime preload on serve fast path - #5989
Merged
Conversation
Route serve request helper imports through lightweight ACP bridge subpaths so runQwenServe can keep the ACP runtime behind its existing dynamic boundary. Add source graph and esbuild metafile guards that fail if the serve fast path statically reaches the ACP runtime modules again. Refs #4748 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
marked this pull request as ready for review
June 29, 2026 06:45
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: aligned. Serve daemon startup latency is a real concern — there's prior work in this area (#5785 "Optimize serve daemon startup"), and #4748 tracks this class of regression. Preventing the fast path from accidentally pulling in the heavy ACP runtime through a re-export shim is a genuine, targeted fix. On approach: the source change is minimal and correct — 2 import lines in Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：对齐。serve daemon 启动延迟是真实关切——此前已有相关优化（#5785），且 #4748 跟踪此类回归。通过 re-export shim 意外拉入重量级 ACP runtime 是合理的修复目标。 方案：源码改动极小（ — Qwen Code · qwen3.7-max |
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
Code ReviewThe source change is a clean 2-line import rerouting in
Both subpath exports verified present in acp-bridge's The test infrastructure (~190 lines) adds two regression guards:
No correctness bugs, no security concerns, no AGENTS.md violations. The code is straightforward and well-scoped. Test ResultsTypecheck passes across all packages (acp-bridge, cli, core, sdk, webui, chrome-bridge, audio-capture). The bundle metafile guard test ran esbuild and verified the static closure in 869ms — confirming the 中文说明代码审查源码改动是
两个子路径导出均在 acp-bridge 的 测试结果58 个测试全部通过，包括新的 bundle metafile guard（869ms）。Typecheck 在所有包上均通过。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a textbook import-boundary fix: 2 lines of source change that solve the stated problem, plus thorough regression coverage that prevents it from recurring. The dual-layer approach (source graph + bundle metafile) is the right call — source-level analysis catches code changes, bundle-level analysis catches esbuild config changes. Neither alone would be complete. The PR matches my independent proposal exactly. The All 58 tests pass, typecheck clean across all packages. The test infrastructure adds ~870ms for the bundle metafile guard — a reasonable cost for protecting a startup-critical invariant. Approving. ✅ 中文说明这是一个教科书级的 import 边界修复：2 行源码改动解决问题，双层回归覆盖防止复发。方案与我的独立提案完全一致——将 fast path 中的 import 从重量级 re-export shim 重定向到轻量子路径。58 个测试全部通过，typecheck 全包通过。批准。 — Qwen Code · qwen3.7-max |
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. ✅
wenshao
reviewed
Jun 29, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| 'packages/acp-bridge/src/bridge.ts', | ||
| 'packages/acp-bridge/src/bridgeClient.ts', | ||
| 'packages/acp-bridge/src/spawnChannel.ts', | ||
| ]); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The forbiddenInputs set is missing 'packages/acp-bridge/src/bridgeErrors.ts', while the source graph test at line 513 explicitly forbids '@qwen-code/acp-bridge/bridgeErrors'. The two guards enforce the same boundary from different angles (source-level vs bundle-level), but their forbidden lists disagree.
If a future change transitively pulls bridgeErrors.ts into the bundle, the source graph test catches it but this bundle metafile test does not — weakening the regression guard.
Suggested change
| ]); | |
| const forbiddenInputs = new Set([ | |
| 'packages/cli/src/serve/acp-session-bridge.ts', | |
| 'packages/acp-bridge/src/bridge.ts', | |
| 'packages/acp-bridge/src/bridgeClient.ts', | |
| 'packages/acp-bridge/src/bridgeErrors.ts', | |
| 'packages/acp-bridge/src/spawnChannel.ts', | |
| ]); |
— qwen3.7-max via Qwen Code /review
| function collectBundledRunServeStaticRuntimeOffenders(): string[] { | ||
| const metafilePath = resolve(repoRoot, 'dist/esbuild.json'); | ||
| rmSync(metafilePath, { force: true }); | ||
| execFileSync(process.execPath, [resolve(repoRoot, 'esbuild.config.js')], { |
Collaborator
There was a problem hiding this comment.
[Suggestion] execFileSync with stdio: 'pipe' captures stderr/stdout into the error object's Buffer properties, but the exception is never caught to log them. If esbuild fails (non-zero exit), the test fails at expect(existsSync(metafilePath)).toBe(true) with only "expected true to be false" — the actual esbuild error message is silently discarded.
Consider wrapping in a try/catch that surfaces the build diagnostic:
Suggested change
| execFileSync(process.execPath, [resolve(repoRoot, 'esbuild.config.js')], { | |
| try { | |
| execFileSync(process.execPath, [resolve(repoRoot, 'esbuild.config.js')], { | |
| cwd: repoRoot, | |
| env: { ...process.env, DEV: 'true' }, | |
| stdio: 'pipe', | |
| timeout: 30_000, | |
| }); | |
| } catch (err) { | |
| const stderr = (err as { stderr?: Buffer }).stderr?.toString() ?? ''; | |
| throw new Error(`esbuild rebuild failed:\n${stderr}`); | |
| } |
— qwen3.7-max via Qwen Code /review
Collaborator
✅ Maintainer verification — local real build + tmux runtime testVerified PR head What the fix actually does
1. Baseline —
|
| Guard | On reverted source |
|---|---|
keeps the runQwenServe static source graph free of ACP runtime modules |
FAIL → src/serve/acp-session-bridge.ts in static graph |
keeps bundled runQwenServe static imports free of ACP runtime modules (real esbuild) |
FAIL → 4 offenders across 3 chunks |
chunk-PODGUZJI.js -> packages/cli/src/serve/acp-session-bridge.ts
chunk-BLWLPS2D.js -> packages/acp-bridge/src/bridge.ts
chunk-BLWLPS2D.js -> packages/acp-bridge/src/bridgeClient.ts
chunk-V7GIQELR.js -> packages/acp-bridge/src/spawnChannel.ts
Restore → Tests 2 passed. So the tests are not vacuous — they go red precisely when the regression is reintroduced.
3. Shipped-bundle metafile analysis (dist/esbuild.json, fixed source)
Independent walk of the esbuild metafile from the run-qwen-serve output chunk, following only import-statement (static/eager) edges:
run-qwen-serve chunk: dist/chunks/run-qwen-serve-SW2I4VI4.js static closure: 21 chunks
acp-session-bridge.ts -> server-X7SJLPWC.js in eager closure? no (lazy-only ✓)
bridge.ts -> chunk-RTHNVSRC.js in eager closure? no (lazy-only ✓)
bridgeClient.ts -> chunk-RTHNVSRC.js in eager closure? no (lazy-only ✓)
spawnChannel.ts -> chunk-XY2A4HS4.js in eager closure? no (lazy-only ✓)
ACP runtime inputs reachable via dynamic-import closure: 4 / 4 ✓ (functionality preserved)
0 ACP runtime inputs in the eager closure, yet all 4 remain reachable behind dynamic imports — the lazy boundary is intact, nothing is removed.
4. Runtime e2e — real qwen serve daemon in tmux
Booted the bundled daemon (dist/cli.js) from the fixed source and exercised the rerouted constant over real HTTP:
qwen serve listening on http://127.0.0.1:63109 (workspace bound)
startup timing: processToListenMs=62 runQwenServeToListenMs=22
| Probe | Result |
|---|---|
GET /health |
200 {"status":"ok"} |
POST /session cwd len 5001 |
400 "cwd exceeds the 4096-character limit" |
POST /session cwd len 4097 (limit+1) |
400 "…exceeds the 4096-character limit" |
POST /session cwd len 4096 (limit) |
passes the gate → next stage¹ |
POST /session short mismatched cwd |
400 workspace_mismatch (cleanly past the gate) |
GET /health (after all probes) |
200 — daemon survived, no crash |
The 4097-vs-4096 boundary pins MAX_WORKSPACE_PATH_LENGTH = 4096 live at runtime via the new import path. The daemon booting + serving also proves the rerouted imports resolve correctly (no missing export / init crash).
¹ At exactly 4096 the app gate passes and realpathSync then throws ENAMETOOLONG (500) because the OS path limit is below 4096 — a pre-existing edge in canonicalizeWorkspace, untouched by this PR and out of scope.
5. Build health
cli package tsc --noEmit → exit 0 · eslint on both changed files → exit 0. acp-session-bridge.ts still re-exports both symbols, so other consumers are unaffected.
⚠️ CI note — the red check is unrelated
Test (ubuntu-latest, Node 22.x) is red, but the only failures are 2 flaky race tests in src/ui/voice/voice-keyterms-race.test.ts (one Test timed out in 5000ms, one TOCTOU race assertion expected […] to not include 'EvilTerm'). That file is not in this PR (changed files: request-helpers.ts, fast-path.test.ts), and this PR's fast-path.test.ts passed in the same CI run (only 2 of 10,043 cli tests failed, both voice). An import reroute in serve cannot affect voice keyterms file races — a re-run should clear it.
Minor, non-blocking observations
- The bundle-metafile guard runs a full esbuild inside a unit test (~1.3s, writes ignored artifacts to
dist/, couples toesbuild.config.js). Already disclosed in the PR's Risk section — a reasonable trade-off, since it's the only way to assert the shipped static closure. - Both guards use hardcoded denylists (1 local + 5 external for the source graph; 4 inputs for the bundle). A future leak via a different ACP module name wouldn't be auto-caught — fine for this targeted regression, just worth knowing.
🇨🇳 中文版（完整对应）
✅ 维护者验证 —— 本地真实构建 + tmux 运行时测试
在独立 git worktree 中以完整 npm ci（真实依赖 + 真实 esbuild 打包）验证了 PR head 792eef641a。改动与描述完全一致；新增的两个 guard 确实能在源码图和最终产物 bundle两个层面捕获这类回归，且重路由后的常量在运行时接线正确。建议合并 —— 唯一红色的 CI 检查是一个与本 PR 无关的 flaky 语音测试（见下）。
修复到底做了什么
request-helpers.ts 从 serve fast-path 入口静态可达（run-qwen-serve.ts → rate-limit.ts → request-helpers.ts，全是值导入）。修复前的代码从 ../acp-session-bridge.js 导入值 MAX_WORKSPACE_PATH_LENGTH，而该模块内部 import { createAcpSessionBridge } from '@qwen-code/acp-bridge/bridge'，于是 fast path 把整个 ACP runtime 拖进了急加载静态闭包。修复把两个符号改到叶子模块：
MAX_WORKSPACE_PATH_LENGTH→@qwen-code/acp-bridge/workspacePaths（只 importnode:fs/node:path）AcpSessionBridge→ 从@qwen-code/acp-bridge/bridgeTypestype-only 导入（编译期擦除）
1. 基线 —— fast-path.test.ts
✓ src/serve/fast-path.test.ts (58 个测试) —— 含真实 esbuild 的 bundle guard (1357ms)
Test Files 1 passed (1) Tests 58 passed (58)
2. 变异测试（证明 guard 有效的决定性证据）
只把 request-helpers.ts 回退到修复前的 ../acp-session-bridge.js 导入 —— 两个新 guard 都如预期失败，恢复后又全部通过：
| Guard | 回退源码后 |
|---|---|
keeps the runQwenServe static source graph free of ACP runtime modules |
FAIL → 静态图里出现 src/serve/acp-session-bridge.ts |
keeps bundled runQwenServe static imports free of ACP runtime modules（真实 esbuild） |
FAIL → 3 个 chunk 中 4 个违规输入 |
chunk-PODGUZJI.js -> packages/cli/src/serve/acp-session-bridge.ts
chunk-BLWLPS2D.js -> packages/acp-bridge/src/bridge.ts
chunk-BLWLPS2D.js -> packages/acp-bridge/src/bridgeClient.ts
chunk-V7GIQELR.js -> packages/acp-bridge/src/spawnChannel.ts
恢复后 → Tests 2 passed。说明测试不是空过 —— 一旦重新引入回归就会变红。
3. 最终产物 metafile 分析（dist/esbuild.json，修复后源码）
从 run-qwen-serve 输出 chunk 出发，独立走 esbuild metafile，只跟随 import-statement（静态/急加载）边：
run-qwen-serve chunk: dist/chunks/run-qwen-serve-SW2I4VI4.js 静态闭包: 21 个 chunk
acp-session-bridge.ts -> server-X7SJLPWC.js 在急加载闭包内? 否 (仅 lazy ✓)
bridge.ts -> chunk-RTHNVSRC.js 在急加载闭包内? 否 (仅 lazy ✓)
bridgeClient.ts -> chunk-RTHNVSRC.js 在急加载闭包内? 否 (仅 lazy ✓)
spawnChannel.ts -> chunk-XY2A4HS4.js 在急加载闭包内? 否 (仅 lazy ✓)
经 dynamic-import 闭包可达的 ACP runtime 输入: 4 / 4 ✓ (功能保留)
急加载闭包中 0 个 ACP runtime 输入，但 4 个全部仍通过 dynamic import 可达 —— 懒加载边界完好，没有删任何东西。
4. 运行时 e2e —— tmux 里跑真实 qwen serve daemon
用修复后源码打出的 daemon（dist/cli.js）启动，通过真实 HTTP 验证重路由后的常量：
qwen serve listening on http://127.0.0.1:63109 (已绑定 workspace)
startup timing: processToListenMs=62 runQwenServeToListenMs=22
| 探针 | 结果 |
|---|---|
GET /health |
200 {"status":"ok"} |
POST /session cwd 长度 5001 |
400 "cwd exceeds the 4096-character limit" |
POST /session cwd 长度 4097（上限+1） |
400 "…exceeds the 4096-character limit" |
POST /session cwd 长度 4096（上限） |
通过该 gate → 进入下一阶段¹ |
POST /session 短的不匹配 cwd |
400 workspace_mismatch（干净地越过 gate） |
GET /health（所有探针之后） |
200 —— daemon 存活，无崩溃 |
4097 与 4096 的边界把 MAX_WORKSPACE_PATH_LENGTH = 4096 在运行时通过新导入路径钉死。daemon 能启动并服务也证明重路由后的导入解析正确（无缺失导出 / 初始化崩溃）。
¹ 恰好 4096 时 app gate 通过，随后 realpathSync 抛 ENAMETOOLONG（500），因为操作系统路径上限低于 4096 —— 这是 canonicalizeWorkspace 里既有的边界，本 PR 未触及，超出范围。
5. 构建健康度
cli 包 tsc --noEmit → 退出码 0 · 对两个改动文件 eslint → 退出码 0。acp-session-bridge.ts 仍然 re-export 这两个符号，因此其他消费方不受影响。
⚠️ CI 说明 —— 红色检查与本 PR 无关
Test (ubuntu-latest, Node 22.x) 是红的，但唯一的失败是 src/ui/voice/voice-keyterms-race.test.ts 里 2 个 flaky race 测试（一个 Test timed out in 5000ms，一个 TOCTOU race 断言 expected […] to not include 'EvilTerm'）。该文件不在本 PR 内（改动文件：request-helpers.ts、fast-path.test.ts），而本 PR 的 fast-path.test.ts 在同一次 CI 运行中通过（10043 个 cli 测试里只有这 2 个语音测试失败）。serve 里的一次 import 重路由不可能影响语音 keyterms 文件竞态 —— 重跑一次即可清掉。
次要、不阻塞的观察
- bundle metafile guard 在单测里跑了一次完整 esbuild（约 1.3s，向
dist/写入被忽略的产物，并耦合esbuild.config.js）。PR 的 Risk 部分已说明 —— 这是合理取舍，因为这是断言最终产物静态闭包的唯一办法。 - 两个 guard 都用硬编码的 denylist（源码图：1 个本地 + 5 个外部；bundle：4 个输入）。未来若经由一个不同名的 ACP 模块泄漏，不会被自动捕获 —— 对这个定向回归来说没问题，但值得知道。
Collaborator
CI failure analysis — flaky unrelated test, not caused by this PRTL;DR: The What actually failed (attempt 1)2 of 3 tests in
Both failed on Why it's unrelated to this PRThis PR only touches the serve fast-path: Evidence it's flaky (load-sensitive), not a regression
ConclusionNo action needed on this PR — CI is now passing. Separately, the 中文说明CI 失败分析 —— 与本 PR 无关的 flaky 测试结论先行： 实际失败的是什么（第 1 次尝试）失败的是
两者都挂在 为什么与本 PR 无关本 PR 只改动 serve fast-path： flaky（受负载影响）而非回归的证据
结论本 PR 无需任何处理 —— CI 现已通过。另外， |
wenshao
approved these changes
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 keeps
qwen serve's fast startup path from statically pulling in the ACP runtime before the daemon has reached its dynamic runtime boundary. It routes the request helper's bridge type and workspace path limit through lightweight ACP bridge subpaths, then adds regression coverage that checks both the source-level static import graph and the esbuild bundle metafile static closure.The guards intentionally ignore type-only imports and dynamic imports, so the existing lazy boundary for the server runtime, bridge factory, and spawn channel remains intact while accidental static runtime imports fail fast in tests.
Why it's needed
Issue #4748 tracks startup regressions where the serve fast path can preload heavy ACP runtime modules too early. A helper-level re-export import made the fast path transitively reachable to the compatibility bridge shim, which defeats the intended lazy runtime split and can regress daemon listen latency.
The new tests protect against this class of regression from both source changes and bundle chunking changes without changing CLI flags, HTTP/ACP protocol behavior, public types, or the compatibility shim.
Reviewer Test Plan
A reviewer should confirm that the serve fast path still keeps ACP runtime modules behind dynamic imports, and that the new regression tests fail if the static source graph or bundled static closure reaches the ACP runtime again.
How to verify
Run
cd packages/cli && npx vitest run src/serve/fast-path.test.tsand confirm all 58 tests pass, including the source graph and bundle metafile guards. Runnpm run build,npm run typecheck, andDEV=true npm run bundle; then inspectdist/esbuild.jsonand confirm the static import closure from therun-qwen-serveoutput does not include ACP runtime inputs while the lazy runtime chunks are still emitted through dynamic imports.Evidence (Before & After)
N/A — this is a non-UI startup import-boundary fix with regression coverage.
Tested on
Environment (optional)
Local macOS worktree with repository npm scripts; validation used the repo's configured Node/npm toolchain.
Risk & Scope
dist/.Linked Issues
Refs #4748
中文说明
What this PR does
本 PR 防止
qwen serve的 fast startup path 在 daemon 到达动态 runtime 边界前静态拉入 ACP runtime。它把请求 helper 需要的 bridge 类型和 workspace path 上限改为从轻量 ACP bridge 子路径获取，并新增回归覆盖，同时检查源码级静态 import 图和 esbuild bundle metafile 的静态闭包。这些 guard 会有意忽略 type-only import 和 dynamic import，因此现有的 server runtime、bridge factory、spawn channel 懒加载边界保持不变；如果后续误加静态 runtime import，测试会直接失败。
Why it's needed
Issue #4748 跟踪的是 serve fast path 过早预加载较重 ACP runtime 模块导致的启动回归。一个 helper 层的 re-export import 让 fast path 可以传递到兼容 bridge shim，从而破坏预期的 lazy runtime split，并可能回归 daemon 监听前延迟。
新的测试同时从源码变更和 bundle chunking 变更两个层面保护这类回归，并且不改变 CLI 参数、HTTP/ACP 协议行为、公共类型或兼容 shim。
Reviewer Test Plan
Reviewer 应确认 serve fast path 仍然把 ACP runtime 模块保留在 dynamic imports 之后，并确认当静态源码图或 bundle 静态闭包重新触达 ACP runtime 时，新的回归测试会失败。
How to verify
运行
cd packages/cli && npx vitest run src/serve/fast-path.test.ts，确认全部 58 个测试通过，包括 source graph 和 bundle metafile guard。运行npm run build、npm run typecheck和DEV=true npm run bundle；然后检查dist/esbuild.json，确认从run-qwen-serve输出出发的静态 import closure 不包含 ACP runtime inputs，同时 lazy runtime chunks 仍然通过 dynamic imports 产出。Evidence (Before & After)
N/A — 这是非 UI 的启动 import 边界修复和回归覆盖。
Tested on
Environment (optional)
本地 macOS worktree，使用仓库 npm scripts；验证使用仓库配置的 Node/npm 工具链。
Risk & Scope
dist/下写入 ignored bundle artifacts。Linked Issues
Refs #4748