perf(core): Lazy-load first-use dependencies#7686
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E and performance reportFinal commit
The 2C4G acceptance run applies to prototype artifact SHA-256
The remote host had 2 vCPUs, 3.5 GiB RAM, no swap, and Node.js 22.23.1. It had no Git executable, so remote first-use verification covered the The prototype artifact reduced the ACP static closure from 13,405,027 to 12,314,617 bytes and attributed zero static bytes to all three target packages. The final rebased commit retains the zero-byte condition through the production bundle guard; its latency numbers are not being represented as a fresh 30-pair rerun. |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Fixed the Ubuntu test failure in ac96c57. The lazy Validation:
|
doudouOUC
left a comment
There was a problem hiding this comment.
Review — perf(core): lazy-load first-use dependencies
Overall this is a clean, well-scoped change. The lazy-loader pattern (module-scoped single-flight promise + CommonJS interop unwrap) is consistent across all three packages, the sync encoding API is preserved via the sync-file-encoding compatibility module, and the bundle guard + metafile-based test make the invariant hard to regress. Static value imports of iconv-lite, @xterm/headless, and simple-git are all gone from the source (only import type remains), and the async read/write GBK paths and loader edge cases are covered by tests. Nice work.
A few non-blocking observations below; none of them block landing.
Correctness / behavior
- The read-side fallback correctly folds a failed
loadIconvLite()into the existing UTF-8-replacement path (with the warn), and the write side rejects rather than corrupting bytes — matches the stated failure contract. - Loader rejections are cached for the process lifetime (
??=on a rejected promise). This is intentional per the design doc (a missing bundled chunk can't recover), so no change requested — just calling it out for reviewers.
Minor / maintainability
- Some inline notes below on duplication and a couple of small error-handling scope changes.
Nit
- On the non-UTF-8 write path,
prepareTextFileContentAsynccallsprepareTextFileContenttwice, so CRLF normalization runs twice before the iconv retry. Negligible (rare path), just noting.
|
|
||
| const debugLogger = createDebugLogger('SYNC_FILE_ENCODING'); | ||
|
|
||
| export function decodeBufferWithEncodingInfo(full: Buffer): FileReadResult { |
There was a problem hiding this comment.
The decode logic here is now a near-verbatim copy of decodeBufferWithEncodingInfoAsync in fileUtils.ts (BOM → valid-UTF-8 → chardet+iconv → UTF-8 fallback). Two copies of the same branching will drift over time — a future fix/edge-case in one won't reach the other.
Given the sync/async split is inherent (dynamic import() is async), full dedup is awkward, but you could at least factor out the shared non-iconv parts (detectBOM/decodeBOMBuffer/bomEncodingToName/isValidUtf8/detectEncodingFromBuffer sequencing) into a helper that takes the resolved iconv module as a parameter, so only the load step differs. Not blocking, but worth a follow-up.
| try { | ||
| const { simpleGit } = await loadSimpleGit(); | ||
| const worktreeGit = simpleGit(worktreePath); | ||
| const base = |
There was a problem hiding this comment.
Behavior change worth confirming is intentional: resolveBaseline() / getCurrentBranch() were previously computed outside the try, so an error there propagated to the caller. They're now inside the try, so such an error is swallowed into the "Error getting diff: …" string return. Same pattern applies to applyWorktreeChanges (baseline/merge-base resolution moved inside the try). This is probably fine / arguably an improvement, but it does change the contract from "throws" to "returns a diagnostic string" for those sub-steps.
| // against the *correct* directory. | ||
| const probe = new GitWorktreeService(this.targetDir); | ||
| const root = (await probe.getRepoTopLevel()) ?? this.targetDir; | ||
| const root = findGitRoot(this.targetDir) ?? this.targetDir; |
There was a problem hiding this comment.
Swapping new GitWorktreeService(...).getRepoTopLevel() (git rev-parse --show-toplevel) for findGitRoot() removes an eager simple-git construction here, which is good. Note the two aren't strictly equivalent: findGitRoot walks up for a .git entry and returns that directory as-is, whereas --show-toplevel returns the real (symlink-resolved) working-tree root. Under a symlinked repo path the resulting .qwen/worktrees directory could differ from before. For the stale-sweep fast-bail this is almost certainly harmless (the fs.access just no-ops on mismatch), but flagging in case symlink-normalization was relied upon.
|
Thanks for the PR — this is a well-put-together change with a thorough design doc. Template ✓ — all sections present, including the full Chinese translation. Problem: real and measured. This isn't theoretical — it's tied to #7264, and the numbers are concrete: the ACP static closure drops from 13.4 MB to 12.3 MB with Direction: aligned. Cold-start latency and RSS are core to the serve/ACP experience, and #7264 explicitly tracks these three packages as candidate 5. CHANGELOG has no direct reference, but the area is clearly relevant. Size & scope — flagging for a maintainer. This is ~559 production-logic lines (≈489 in Approach: sound, and matches how I'd tackle it. Per-package single-flight loaders, keeping the public sync encoding API via a tree-shakeable compat module, deferring Moving on to code review. 🔍 中文说明感谢贡献——这是一个组织良好、附带完整设计文档的 PR。 模板 ✓ —— 各节齐全,包含完整中文翻译。 问题:真实且有实测数据。 这不是理论性问题——它关联 #7264,且数据具体:ACP 静态闭包从 13.4 MB 降到 12.3 MB, 方向:对齐。 冷启动延迟和 RSS 是 serve/ACP 体验的核心,#7264 也明确将这三个包列为 candidate 5。CHANGELOG 无直接引用,但该领域明显相关。 规模与范围——提请注意维护者。 约 559 行生产逻辑( 方案:合理,且与我的思路一致。 按包单飞(single-flight)loader、通过可 tree-shake 的兼容模块保留公共同步编码 API、将 进入代码审查。🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewI read the diff against an independent proposal (per-package single-flight loaders + a sync compat module for the public encoding API + defer xterm/simple-git to first use + a metafile bundle guard). The PR matches that approach and I found no correctness blockers. Behavior preservation checks out:
Non-blocking observations (not asking for changes):
Test evidence (PR's own CI, read via the API — I did not run any PR code)At the reviewed commit
Not verified: the latency/RSS numbers — these apply to the prototype artifact (SHA-256 Real-scenario (tmux) testing: N/A — no user-visible/TUI changes, and this is an unattended CI run. 中文说明代码审查我先独立给出方案(按包单飞 loader + 为公共编码 API 保留同步兼容模块 + 将 xterm/simple-git 延迟到首次使用 + 基于 metafile 的 bundle 门禁),再对照 diff。PR 与该方案一致,未发现正确性阻断项。行为保持已核对:
非阻断观察(不要求修改):
测试证据(PR 自身的 CI,通过 API 读取——我未运行任何 PR 代码)在受审提交
未验证:延迟/RSS 数据——这些对应 prototype artifact(SHA-256 真实场景(tmux)测试:N/A —— 无用户可见/TUI 变化,且本次为无人值守 CI 运行。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — clean review and a genuinely high-quality PR, but it's a large structural refactor of core dependency-loading from a fork (titled Stepping back: this is the right fix for a real, measured problem. My independent proposal landed on the same shape — per-package single-flight loaders, a tree-shakeable sync compat module for the public encoding API, xterm/simple-git deferred to first use, and a metafile bundle guard to stop regressions — and the PR executes it cleanly. The encoding edge cases are preserved, the PTY abort/fallback contract holds, Git construction is side-effect-free, and CI is green with a no-change Serve A/B. The design doc is unusually good, and the author is candid that the latency numbers belong to the prototype artifact rather than the final rebased commit. Why I'm not approving: the diff is ~559 production lines that restructure how core services obtain their dependencies — three new loader modules, a sync/async split in the file service, an esbuild tree-shake plugin, narrow CLI runtime entries, and ~20 call-site conversions in Two things I'd want before merge that the static review can't settle: a Windows CI pass (skipped here and not manually exercised), and a maintainer's judgment on whether the final rebased commit warrants a fresh 30-pair benchmark or whether the enforced zero-byte bundle guard plus the prototype numbers are sufficient. 中文说明置信度:3/5 —— 审查干净、PR 质量确实高,但这是一次来自 fork、标题为 退一步看:这是对一个真实且有实测数据的问题的正确修复。我独立得出的方案与其形态一致——按包单飞 loader、为公共编码 API 保留可 tree-shake 的同步兼容模块、将 xterm/simple-git 延迟到首次使用、以及基于 metafile 的 bundle 门禁防止回归——而 PR 干净地实现了它。编码边界情况得以保留,PTY 的 abort/回退契约成立,Git 构造无副作用,CI 为绿色且 Serve A/B 无变化。设计文档异常出色,作者也坦诚延迟数据属于 prototype artifact 而非最终 rebase 后的提交。 我不批准的原因:diff 约 559 行生产代码,重构了核心服务获取依赖的方式——三个新 loader 模块、文件服务的 sync/async 拆分、一个 esbuild tree-shake 插件、窄化的 CLI runtime 入口,以及 合并前我希望补上、而静态审查无法定论的两点:过一次 Windows CI(本次被跳过且未手动验证),以及由维护者判断最终 rebase 后的提交是否需要重新跑一次 30 组配对 benchmark,还是强制执行的零字节 bundle 门禁加 prototype 数据就已足够。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
⏸️ Deferring to maintainers — cc @tanzhenxin @wenshao @yiliang114 @LaZzyMan The review is clean (see above) and the change is high quality with a real, measured cold-start benefit, but it's a ~559-production-line structural refactor of core dependency-loading from a fork. Our core-module gate routes a change of this profile to a human maintainer rather than auto-approving. Needs a human call on:
🔄 转交维护者 —— 审查干净(见上),改动质量高且有实测的冷启动收益,但这是来自 fork、约 559 行生产代码的核心依赖加载结构性重构。核心模块门禁将此类改动转交人类维护者而非自动批准。需要人类判断:(1) 该结构范围是否可作为单个 — Qwen Code · qwen3.8-max-preview |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the latest automated review at 2208817.
Validation:
|
Review —
|
| Metric | base 2051a4173 |
head 2208817ac |
Δ |
|---|---|---|---|
| ACP static-closure outputs | 145 | 143 | −2 |
| ACP static-closure bytes | 13,214,217 | 12,120,272 | −1,093,945 |
iconv-lite attributed |
552,742 | 0 | −552,742 |
@xterm/headless attributed |
213,120 | 0 | −213,120 |
simple-git attributed |
146,526 | 0 | −146,526 |
node scripts/check-serve-fast-path-bundle.js passes on that real bundle. sync-file-encoding.ts and iconvHelper.ts appear in zero outputs, so the esbuild plugin genuinely performs the tree-shake rather than merely relocating the code; iconv-lite survives only in chunks reachable by dynamic import. The bundle claim reproduces on the rebased base at the reported magnitude (−1.09 MB), so re-running the bundle gate on the final commit is not needed. The 2C4G latency numbers remain tied to the prototype artifact — that's a separate call.
2. Sync/async encoding equivalence. The compat module duplicates ~35 lines of decode logic, so I ran both implementations side by side over 11 decode cases (empty, ASCII, UTF-8 CJK, UTF-8/UTF-16LE/UTF-16BE/UTF-32LE BOM, GBK, Big5, Shift_JIS, invalid bytes) and 9 encode cases (incl. gbk+BOM, utf-16le+BOM, CRLF metadata, unsupported codec). Byte-identical in every case — decodeBufferWithEncodingInfo ≡ decodeBufferWithEncodingInfoAsync, encodeTextFileContent ≡ encodeTextFileContentAsync.
3. Mutation check on the new abort test. Deleting the post-import if (abortSignal.aborted) recheck in shellExecutionService.ts fails exactly the new test (1 failed / 132 passed). Discriminating, not incidentally green.
4. Tests / format. 577 focused Core tests pass (load-*, fileUtils, fileSystemService, read-text-range, shellExecutionService, gitWorktreeService, worktreeCleanup, extensionManager, github); 29 bundle-guard tests pass; Prettier clean across all 34 changed files.
Findings
1 · The esbuild plugin's dist branch can never match the file that holds the import. esbuild.config.js:126 gates on packages/core/(src|dist)/index.(ts|js). Only the src branch ever fires, because packages/cli/tsconfig.json:10 maps @qwen-code/qwen-code-core → ../core/src/index.ts and esbuild honours tsconfig paths — I confirmed packages/core/src/index.ts is the input in the real metafile. If that mapping is ever dropped, resolution falls to package.json exports → dist/index.js, which contains only export * from './src/index.js'; the actual export { … } from './utils/sync-file-encoding.js' lives in dist/src/index.js, which the regex does not match. So the dist half is dead today and would not work if reached. Either widen it to also match dist/src/index.js, or drop it and add a comment that the plugin covers the tsconfig-paths-resolved source path only. The bundle guard fails loudly in that scenario, so this is robustness/maintainability, not a silent regression.
2 · iconvHelper.ts:26 re-export is a re-entry point for the exact regression this PR guards against. export { isUtf8CompatibleEncoding } from './encoding.js'; has zero consumers repo-wide — the only remaining importers of iconvHelper.js are sync-file-encoding.ts and two test files, all for iconvEncode/iconvDecode/iconvEncodingExists. Keeping it means a contributor writing the historically-correct import { isUtf8CompatibleEncoding } from './iconvHelper.js' silently re-establishes a static edge to iconv-lite. Deleting it makes the compiler point them at encoding.js.
3 · Orphaned JSDoc. iconvHelper.ts now ends at line 63 with a doc block (57–63) describing isUtf8CompatibleEncoding, which moved to encoding.ts. Leftover from the extraction.
4 · loadIconvLite is the only loader without a shape guard, and its test covers only one shape. load-simple-git.ts and load-xterm-headless.ts both probe named exports before falling back to default, and each has a test per shape. load-iconv-lite.ts:17 does an unconditional module.default as unknown as IconvLite. I verified this is correct today — Node yields { default } only for iconv-lite, and esbuild's __toESM also sets default — so this is consistency, not a live bug. The asymmetric failure mode is what makes it worth three lines: if default were ever absent, loadIconvLite() resolves to undefined, and decodeBufferWithEncodingInfoAsync's try/catch swallows the resulting TypeError and silently returns UTF-8-replacement mojibake with nothing surfaced. Adding the guard plus the missing named-export test case makes all three loaders uniform.
5 · getWorktreeDiff quietly widened its catch. On base, resolveBaseline() / getCurrentBranch() ran before the try and threw to the caller. They are now inside it (gitWorktreeService.ts:824), so a baseline/branch resolution failure returns the string `Error getting diff: …` as the diff body instead of rejecting. Arguably an improvement, but it's a contract change unrelated to lazy loading and isn't in Risk & Scope. applyWorktreeChanges got the same treatment for its two simpleGit() constructions (much lower impact). If intentional, worth one line in the PR body.
6 · findGitRoot() substitution — agree it's safe, one nuance worth a comment. I agree with your earlier reply: git rev-parse --show-toplevel also returns the linked worktree root, so that case is preserved. Two remaining differences: findGitRoot is purely lexical (path.resolve + existsSync, no symlink resolution), so a repo reached through a symlinked path anchors at the symlinked path rather than the realpath; and it ignores GIT_DIR/GIT_WORK_TREE. Both only change which <root>/.qwen/worktrees the hygiene sweep scans, so the worst case is a no-op sweep — not data loss. Fine as-is, but config.ts:2806 sits under a comment block entirely about having fixed a "sweep was permanently a no-op" bug, so a one-liner noting the lexical-vs-rev-parse difference would help the next reader.
7 · Two unreachable throws. fileSystemService.ts:280 and sync-file-encoding.ts:78 both throw new Error('iconv-lite did not prepare non-UTF-8 text content'). Once iconvLite is supplied, prepareTextFileContent always returns a value — an unsupported codec falls through to the UTF-8 return rather than undefined — so neither can fire. Harmless, but reads like a real failure mode.
8 · The direct unit tests migrated onto the compat path. fileUtils.test.ts:547,558 and fileSystemService.test.ts:212,219 now exercise sync-file-encoding.ts, the copy with zero production callers in this repo. The live async variants keep only indirect coverage via readFileWithEncodingInfo / StandardFileSystemService.writeTextFile, and nothing asserts the two implementations agree. They do agree today (I checked — §2 above), but a small table-driven equivalence test is the cheap way to stop them drifting, since the whole point of the compat module is that it can never be deleted casually.
9 · Maintainer call: the sync compat exports have no in-repo consumers. decodeBufferWithEncodingInfo and encodeTextFileContent are referenced only by packages/core/src/index.ts and tests. Everything that buys them — the esbuild plugin, the widened fileUtils surface (isValidUtf8, decodeBOMBuffer, bomEncodingToName, BOMInfo, UnicodeEncoding), the exported prepareTextFileContent with its undefined sentinel, and the duplicated decode logic — exists purely to keep the published @qwen-code/qwen-code-core surface stable for external consumers. That may well be the right trade, but it is the single largest source of complexity in the diff and deserves an explicit decision rather than being inherited. (Not something to change in this PR.)
Quality, performance, security
- Style/conventions: consistent with the repo — Apache headers on new files,
createDebugLoggertags,??=single-flight matching existing loader idioms, narrow re-export modules type-checked by use (core-runtime.ts's 26 symbols exactly match the 26core.*references inrun-qwen-serve.ts). - Module identity:
core-runtime.ts/deferred-core-runtime.tsre-export from the same bundled Core instance, soinstanceofand class identity are preserved — confirmed via the metafile (singlepackages/core/src/index.tsinput). - Performance: win is real and reproduced above. The added per-call cost is one already-resolved promise
awaiton non-UTF-8 encode/decode, PTY start, and Git operations — negligible against the work that follows.prepareTextFileContentruns twice on the non-UTF-8 write path (once to detect the missing codec, once with it), butneedsCrlfLineEndingsis pure and the extra work is one string normalisation. - Security: no new surface. All three
import()calls take string literals; no dynamic specifier, nocreateRequire, nonode_modulesruntime dependency.allowUnsafeHooksPathusage is unchanged. - Rejection stickiness: the loader promises cache rejections for the process lifetime. Correct for a missing/corrupt bundled chunk (it cannot heal), and documented in the design doc — noting it only so it isn't rediscovered later.
- Windows: still the main unexercised axis, since PTY +
@xterm/headlessdeferral is exactly the Windows-sensitive path. Worth a green Windows CI run before merge, as already noted.
Verdict: the mechanism is sound, the numbers hold on the current base, and the behavioural contracts I probed (encoding equivalence, PTY abort/fallback, Git structured failures) are preserved. Findings 1–4 are the ones I'd fix before merge; they're all a few lines each.
中文说明
在 2208817ac 上审查。我没有直接采信 PR 中的数据,而是重新构建并复测,包括针对当前 PR base(2051a4173)做了一次全新的 bundle A/B —— 这正是此前 triage 留下的开放问题。无阻塞项,以下均为 Low 或提示性意见。
概述
将 iconv-lite、@xterm/headless、simple-git 移出 ACP 子进程的启动期静态导入闭包,通过三个 single-flight 包级 loader 在首次真实使用时各加载一次。同步的公开编码辅助函数通过兼容模块 utils/sync-file-encoding.ts 保留,并由 esbuild sideEffects: false resolver 插件从 Core 根导出中 tree-shake 掉;内部文件服务路径改用 …Async 变体。两个窄化的 CLI runtime 入口模块替代了对 Core 根的延迟 namespace import,check-serve-fast-path-bundle.js 新增这三个包为 ACP 静态闭包禁止项。
我实际执行的验证(macOS 26 arm64,Node 22.23.1)
1. 针对真实 PR base 的 bundle A/B。 对 2051a4173(base)与 2208817ac(head)分别执行 DEV=true node esbuild.config.js,然后从 ACP entry output 出发,仅沿静态边遍历 esbuild metafile:
| 指标 | base 2051a4173 |
head 2208817ac |
Δ |
|---|---|---|---|
| ACP 静态闭包 outputs | 145 | 143 | −2 |
| ACP 静态闭包字节 | 13,214,217 | 12,120,272 | −1,093,945 |
iconv-lite 归因 |
552,742 | 0 | −552,742 |
@xterm/headless 归因 |
213,120 | 0 | −213,120 |
simple-git 归因 |
146,526 | 0 | −146,526 |
node scripts/check-serve-fast-path-bundle.js 在该真实 bundle 上通过。sync-file-encoding.ts 与 iconvHelper.ts 在任何 output 中都不出现,说明 esbuild 插件确实完成了 tree-shake 而非仅仅搬运代码;iconv-lite 只存在于动态 import 可达的 chunk 中。bundle 结论在 rebase 后的 base 上以相同量级复现(−1.09 MB),因此最终提交无需重跑 bundle 门禁。2C4G 的延迟数据仍绑定 prototype artifact —— 那是另一个判断。
2. 同步/异步编码等价性。 兼容模块复制了约 35 行解码逻辑,因此我并排运行了两套实现:11 个解码用例(空、ASCII、UTF-8 中文、UTF-8/UTF-16LE/UTF-16BE/UTF-32LE BOM、GBK、Big5、Shift_JIS、非法字节)与 9 个编码用例(含 gbk+BOM、utf-16le+BOM、CRLF 元数据、不支持的 codec)。全部逐字节一致。
3. 对新增 abort 测试的变异检验。 删除 shellExecutionService.ts 中 import 之后的 if (abortSignal.aborted) 复检后,恰好只有新增的那条测试失败(1 失败 / 132 通过)。说明该测试具备判别力,而非偶然通过。
4. 测试 / 格式。 577 个聚焦 Core 测试通过;29 个 bundle guard 测试通过;34 个改动文件 Prettier 全部干净。
发现
1 · esbuild 插件的 dist 分支永远匹配不到真正含有该 import 的文件。 esbuild.config.js:126 以 packages/core/(src|dist)/index.(ts|js) 作为判定条件。实际只有 src 分支会触发,因为 packages/cli/tsconfig.json:10 将 @qwen-code/qwen-code-core 映射到 ../core/src/index.ts,而 esbuild 会遵循 tsconfig paths —— 我在真实 metafile 中确认输入即为 packages/core/src/index.ts。若该映射被移除,解析会回落到 package.json exports → dist/index.js,而该文件只有 export * from './src/index.js';真正的 export { … } from './utils/sync-file-encoding.js' 位于 dist/src/index.js,正则匹配不到。因此 dist 分支今天是死代码,且即使被走到也不生效。建议要么扩展为同时匹配 dist/src/index.js,要么删除它并注释说明该插件只覆盖 tsconfig-paths 解析出的源码路径。该场景下 bundle guard 会明确失败,所以这属于健壮性/可维护性问题,不是静默回归。
2 · iconvHelper.ts:26 的 re-export 正是本 PR 所防范回归的再入口。 export { isUtf8CompatibleEncoding } from './encoding.js'; 全仓零消费者 —— iconvHelper.js 仅剩 sync-file-encoding.ts 与两个测试文件引用,且都是为了 iconvEncode/iconvDecode/iconvEncodingExists。保留它意味着后续贡献者按历史习惯写 import { isUtf8CompatibleEncoding } from './iconvHelper.js' 会静默重建到 iconv-lite 的静态边。删除后编译器会把他们导向 encoding.js。
3 · 悬空 JSDoc。 iconvHelper.ts 现在停在第 63 行,末尾(57–63)是描述已迁往 encoding.ts 的 isUtf8CompatibleEncoding 的文档块,属提取遗留。
4 · loadIconvLite 是唯一没有形状守卫的 loader,其测试也只覆盖一种形状。 load-simple-git.ts 与 load-xterm-headless.ts 都会先探测具名导出再回退 default,且每种形状各有测试。load-iconv-lite.ts:17 则无条件 module.default as unknown as IconvLite。我验证过当前是正确的 —— Node 对 iconv-lite 只给出 { default },esbuild 的 __toESM 同样会设置 default —— 所以这是一致性问题而非现存 bug。值得花三行的原因在于其失败模式不对称:若 default 缺失,loadIconvLite() 解析为 undefined,而 decodeBufferWithEncodingInfoAsync 的 try/catch 会吞掉由此产生的 TypeError,静默返回 UTF-8 替换字符的乱码且不向上暴露任何信息。补上守卫与缺失的具名导出测试用例,可让三个 loader 保持一致。
5 · getWorktreeDiff 悄悄扩大了 catch 范围。 base 上 resolveBaseline() / getCurrentBranch() 在 try 之前执行并向调用方抛出。现在它们被移入 try(gitWorktreeService.ts:824),因此 baseline/分支解析失败会把 `Error getting diff: …` 字符串作为 diff 内容返回而非 reject。这也许是改进,但属于与 lazy load 无关的契约变化,且未列入 Risk & Scope。applyWorktreeChanges 的两处 simpleGit() 构造也做了同样处理(影响小得多)。若为有意为之,建议在 PR 描述中补一行。
6 · findGitRoot() 替换 —— 认同其安全性,但有一点值得加注释。 同意你此前的回复:git rev-parse --show-toplevel 在 linked worktree 中同样返回该 worktree 根,因此该场景行为保持不变。仍存在两点差异:findGitRoot 是纯词法的(path.resolve + existsSync,不解析符号链接),所以通过符号链接路径进入的仓库会锚定在符号链接路径而非真实路径;且它忽略 GIT_DIR/GIT_WORK_TREE。二者只影响清理任务扫描哪个 <root>/.qwen/worktrees,最坏情况是 sweep 变成 no-op,不会丢数据。维持现状即可,但 config.ts:2806 所处的注释块通篇都在讲修复"sweep 永久 no-op"的 bug,加一行说明词法遍历与 rev-parse 的差异会对后来者更友好。
7 · 两处不可达的 throw。 fileSystemService.ts:280 与 sync-file-encoding.ts:78 都 throw new Error('iconv-lite did not prepare non-UTF-8 text content')。一旦传入 iconvLite,prepareTextFileContent 必然返回值 —— 不支持的 codec 会落到 UTF-8 返回分支而非 undefined —— 因此两处都不可能触发。无害,但读起来像是真实故障模式。
8 · 直接单测被迁移到了兼容路径上。 fileUtils.test.ts:547,558 与 fileSystemService.test.ts:212,219 现在测的是 sync-file-encoding.ts,即本仓库中零生产调用方的那份副本。真正在用的 async 变体仅保留经由 readFileWithEncodingInfo / StandardFileSystemService.writeTextFile 的间接覆盖,且没有任何断言保证两套实现一致。它们今天确实一致(见上文 §2 我的验证),但补一个表驱动的等价性测试是防止二者漂移的低成本手段 —— 毕竟兼容模块的意义就在于它不能被随手删除。
9 · 需维护者决策:同步兼容导出在仓库内没有消费者。 decodeBufferWithEncodingInfo 与 encodeTextFileContent 仅被 packages/core/src/index.ts 和测试引用。为它们付出的全部代价 —— esbuild 插件、被放宽的 fileUtils 导出面(isValidUtf8、decodeBOMBuffer、bomEncodingToName、BOMInfo、UnicodeEncoding)、导出的带 undefined 哨兵值的 prepareTextFileContent、以及重复的解码逻辑 —— 完全是为了让已发布的 @qwen-code/qwen-code-core 对外部消费者保持稳定。这可能确实是正确取舍,但它是本 diff 中最大的复杂度来源,应当被显式决策而非默认继承。(不建议在本 PR 中改动。)
质量、性能与安全
- 风格/约定: 与仓库一致 —— 新文件带 Apache 头、
createDebugLoggertag、??=single-flight 沿用既有 loader 惯例、窄化 re-export 模块由使用处做类型校验(core-runtime.ts的 26 个符号与run-qwen-serve.ts中 26 处core.*引用完全对应)。 - 模块同一性:
core-runtime.ts/deferred-core-runtime.ts从同一个被 bundle 的 Core 实例 re-export,instanceof与类同一性得以保持 —— 已通过 metafile 确认(唯一packages/core/src/index.ts输入)。 - 性能: 收益真实且已复现。新增的单次调用成本是在非 UTF-8 编解码、PTY 启动与 Git 操作上多
await一个已 resolve 的 promise,相对其后的工作可忽略。非 UTF-8 写入路径会执行两次prepareTextFileContent(一次探测缺失 codec,一次带上它),但needsCrlfLineEndings是纯函数,额外开销仅一次字符串归一化。 - 安全: 无新增攻击面。三处
import()均为字符串字面量;无动态 specifier、无createRequire、无运行期node_modules依赖。allowUnsafeHooksPath用法未变。 - rejection 粘性: loader promise 会在进程生命周期内缓存 rejection。对于缺失/损坏的 bundled chunk 这是正确的(无法自愈),设计文档也已说明 —— 此处只是记录,避免日后被重新"发现"。
- Windows: 仍是主要未验证维度,因为 PTY +
@xterm/headless延迟加载恰是 Windows 敏感路径。如已指出,合并前值得跑一次绿色的 Windows CI。
结论: 机制可靠,数据在当前 base 上成立,我探测过的行为契约(编码等价性、PTY abort/fallback、Git 结构化失败返回)均得以保持。发现 1–4 是我建议合并前修掉的,每项都只需几行。
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks @wenshao — I checked each point against the current head and pushed Addressed before merge:
Documentation/disposition:
Validation passed: full workspace typecheck, 219 focused Core tests, 29 bundle-guard tests, targeted Prettier/ESLint, the CLI-only production build and bundle, and the real startup bundle closure guard. The review was top-level rather than inline, so there were no new review threads to resolve (0/0). |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the latest automated review in
Validation passed: full workspace typecheck, 139 focused Core tests, the CLI-only production build and bundle, and the real startup bundle closure guard. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
|
Latest review disposition (no code change):
Both inline threads contain the exact rationale. Per the repository's roughly-five-round rule, further changes on this PR are limited to Critical correctness, security, data-loss, or regression fixes. |
What this PR does
This PR moves
iconv-lite,@xterm/headless, andsimple-gitout of the ACP child's eager static import closure and loads each dependency once at its first real use. It preserves the synchronous package-root encoding helpers through a compatibility entry, uses asynchronous lazy variants on internal file-service paths, defers terminal construction until the PTY path is selected, and keeps Git service construction side-effect-free until an operation actually needs Git.Narrow deferred Core runtime entries prevent namespace imports from retaining the synchronous encoding compatibility path. The ACP bundle guard now rejects any static path from the ACP runtime to these three packages while allowing dynamic-only chunks.
Why it's needed
Issue #7264 identified these dependencies as approximately 890 KiB of direct package input in every cold ACP child even though most sessions do not use non-UTF-8 codecs, PTY terminal replay, or Git worktree operations during bootstrap. On the measured prototype artifact, the ACP static closure decreased from 13,405,027 to 12,314,617 bytes and all three packages reached zero bytes in the static closure.
On the 2-vCPU reference host, 30 alternating cold pairs measured process-to-first-session P50 improving from 1877.7 ms to 1733.3 ms,
channel.initializeP50 improving from 896.2 ms to 831.5 ms, and peak process-tree RSS P50 decreasing from 417.0 MB to 408.1 MB. These performance numbers apply to the recorded prototype artifact SHA-256f0ac7edc7665752efac7b7bfbb4fb055ce2d8ef1a8ae5dd1af630305a2c84d28; the final commit was rebased onto currentmainand locally rebuilt and guarded, but was not presented as a fresh 30-pair remote rerun.Reviewer Test Plan
How to verify
Build and bundle the CLI, then run the serve fast-path bundle check. The check should pass and a metafile traversal from the ACP runtime should attribute zero static-closure bytes to
iconv-lite,@xterm/headless, andsimple-git.Read and write ordinary UTF-8 text, then read and write a GBK file while preserving its encoding metadata. UTF-8 should complete without the codec chunk; the non-UTF-8 operation should load the codec once and preserve the decoded text and encoded bytes, including existing BOM behavior.
Run a shell command through the PTY path and through the child-process fallback. PTY output and terminal replay should remain unchanged, an abort during first use should not spawn a process, and a terminal chunk failure should retain the existing child-process fallback policy.
Exercise repository detection, worktree create/diff/apply/cleanup, and a Git-backed extension operation. The Git package should load once on the first real operation, structured failure-returning methods should retain their failure contracts, and construction alone should not load Git.
Evidence (Before & After)
N/A — no user-visible or TUI changes.
Tested on
Environment (optional)
Local verification used macOS 26.4.1 arm64, Node.js 22.22.3, and npm 10.9.8. It passed 461 focused Core tests, 374 focused CLI tests with one existing skip, 29 bundle-guard tests, targeted ESLint and Prettier checks, the CLI-only production build and bundle, the real startup bundle closure guard, and the full workspace typecheck.
Remote performance verification used Linux on a 2-vCPU host with 3.5 GiB RAM, no swap, and Node.js 22.23.1. It completed a smoke run, 30 alternating serial cold pairs, 30 alternating preheated pairs, concurrent first-session checks, telemetry-disabled startup, legacy single-session startup, and residual-process checks. The remote host did not have a Git executable, so it verified the
simple-gitdynamic chunk and factory there; real Git initialization and service behavior were verified locally.Risk & Scope
iconv-lite, while the targeted bundle plugin prevents that entry from entering the ACP startup closure.Linked Issues
Relates to #7264
中文说明
本 PR 做了什么
本 PR 将
iconv-lite、@xterm/headless和simple-git移出 ACP 子进程的启动期静态导入闭包,并在首次真实使用时分别只加载一次。它通过兼容入口保留包根级同步编码辅助函数,通过异步懒加载变体处理内部文件服务路径,在选中 PTY 路径后才加载终端实现,并确保 Git 服务构造本身无副作用,直到实际 Git 操作才加载依赖。窄化的延迟 Core runtime 入口避免 namespace import 保留同步编码兼容路径。ACP bundle guard 现在会拒绝 ACP runtime 到这三个包的任何静态路径,同时允许仅通过动态 import 到达的 chunk。
为什么需要
#7264 识别出这些依赖在每个冷启动 ACP 子进程中合计占用约 890 KiB 的直接包输入,尽管大多数会话在启动阶段不会使用非 UTF-8 编解码、PTY 终端回放或 Git worktree 操作。在已测 prototype artifact 上,ACP 静态闭包从 13,405,027 bytes 降至 12,314,617 bytes,三个包在静态闭包中的归因字节均降为 0。
在 2 vCPU 参考机器上,30 组交替冷启动配对测试显示:process-to-first-session P50 从 1877.7 ms 改善到 1733.3 ms,
channel.initializeP50 从 896.2 ms 改善到 831.5 ms,进程树峰值 RSS P50 从 417.0 MB 降至 408.1 MB。这些性能数据仅对应已记录的 prototype artifact SHA-256f0ac7edc7665752efac7b7bfbb4fb055ce2d8ef1a8ae5dd1af630305a2c84d28;最终提交已 rebase 到当前main并在本地重新构建和执行门禁,但没有被描述为一次新的远程 30 组配对复测。Reviewer 测试计划
如何验证
构建并 bundle CLI,然后运行 serve fast-path bundle 检查。检查应通过,并且从 ACP runtime 出发遍历 metafile 后,
iconv-lite、@xterm/headless和simple-git在静态闭包中的归因字节应全部为 0。先读写普通 UTF-8 文本,再读写一个保留编码元数据的 GBK 文件。UTF-8 路径应无需加载 codec chunk;非 UTF-8 操作应只加载一次 codec,并保持解码文本和编码字节不变,包括既有 BOM 行为。
分别通过 PTY 路径和 child-process fallback 运行 shell 命令。PTY 输出和终端回放应保持不变,首次使用期间发生 abort 时不应启动进程,终端 chunk 加载失败时应保留既有 child-process fallback 策略。
验证仓库识别、worktree 创建/diff/apply/cleanup 以及 Git 支持的扩展操作。Git 包应在首次真实操作时只加载一次,仅构造服务不应加载 Git,具有结构化失败返回值的方法应保留原有失败契约。
证据(修改前后)
N/A — 没有用户可见或 TUI 变化。
已测试平台
环境(可选)
本地验证环境为 macOS 26.4.1 arm64、Node.js 22.22.3 和 npm 10.9.8。已通过 461 个聚焦 Core 测试、374 个聚焦 CLI 测试(另有 1 个既有 skip)、29 个 bundle guard 测试、目标 ESLint 与 Prettier 检查、CLI-only 生产构建与 bundle、真实启动 bundle 闭包门禁,以及全 workspace typecheck。
远程性能验证使用 Linux 2 vCPU、3.5 GiB RAM、无 swap、Node.js 22.23.1 的机器。已完成 smoke、30 组交替串行冷启动配对、30 组交替 preheated 配对、并发首次 session、关闭 telemetry、legacy 单 session 和残留进程检查。远程机器没有 Git 可执行文件,因此远程只验证了
simple-git动态 chunk 和 factory;真实 Git 初始化与服务行为已在本地验证。风险与范围
iconv-lite支持的小型同步兼容入口,而定向 bundle 插件会阻止该入口进入 ACP 启动闭包。关联 Issue
Relates to #7264