perf(cli): import core modules directly in another 114 files - #10956
perf(cli): import core modules directly in another 114 files#10956yiliang114 wants to merge 1 commit into
Conversation
The same mechanical change as the previous commit, over the files a corrected reading of the test suite showed were always safe to move. The earlier pass classified a test as replacing the core package if the text of such a call appeared anywhere in it, including inside a comment. One file only mentions the pattern in a doc comment explaining why it deliberately avoids it, and being counted as a blocker there ruled out 217 modules that nothing actually blocks. Ignoring comments when detecting the call raises the number of files movable without touching a single test from 141 to 260. Every generated specifier is checked against the exports its named module really has, following that module's own re-exports — 865 pairs here, none wrong. Outside import statements every line is byte-identical.
|
Thanks for the PR! Stacked on #10946 → #10917, base Template — all required sections present. One deviation worth naming rather than blocking on: the body carries Problem — observed and measured, not theoretical. #10908 carries hard numbers: the Direction — aligned, and it is a maintainer-filed plan rather than a drive-by. One genuine question: #10908 scopes Phase ① as "migrate barrel imports to subpaths + lint rule", and credits the lint rule with keeping the win from decaying as test files grow (~110/week). Neither this PR nor #10917/#10946 adds a rule. Presumably it cannot land until the remaining 191 files are done — worth saying where it lands so the migration does not stop regressing the moment it finishes. Size — core paths are touched under the gate's patterns: Approach — minimal and honest about it. I checked the claim mechanically rather than taking it on trust: all 710 changed lines are import/export-from statement lines, there is exactly one hunk per file (114 hunks, 114 files), and the deepest hunk starts at old-side line 51 with an Risk — Stage 1e matched: 14 files under Moving on to code review. 🔍 中文说明感谢贡献！本 PR 基于 #10946 → #10917，base 为 模板 —— 必需章节齐全。有一处偏差，点出来但不作为阻塞：正文用的是 问题 —— 已观测且有实测数据，不是理论性问题。#10908 给出了硬数字： 方向 —— 对齐，且这是维护者自己提出的计划，不是顺手改动。有一个真实的问题：#10908 把阶段 ① 定义为「迁移 barrel 导入到子路径 + lint 规则」，并认为 lint 规则是防止收益随测试文件增长（约每周 110 个）而衰减的关键。本 PR 及 #10917/#10946 都没有加入该规则。推测是要等剩下 191 个文件完成后才能落地——建议说明它将落在哪里，否则迁移一结束就会开始回退。 规模 —— 按本仓库 gate 的匹配规则确实触及核心路径： 方案 —— 改动最小，且描述诚实。我没有直接采信，而是机械校验了这条声明：710 行改动全部是 import/export-from 语句行；每个文件恰好一个 hunk（114 个 hunk、114 个文件）；最深的 hunk 起始于旧文件第 51 行，其上下文标题行本身就是一条 风险 —— Stage 1e 命中： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI reviewed this statically at What checks outThe two properties that make a 114-file diff reviewable are both true, and I verified them rather than taking them on trust:
Blocker: five files break their colocated unit testsThe description's core claim is that these 114 files are movable "without touching any test", and that what remains "genuinely does need its mocks moved in the same change". Eight of the 114 have a colocated test that calls
The mechanism is the base branch's own alias table. The other three look genuinely benign, for different reasons: I confirmed this against the reviewed commit rather than inferring it: none of #10917, #10946 or #10956 touches any The fix is small and mechanical — either drop these five files into the batch that moves mocks, or move the five mock calls onto the subpaths in the same PR. What I would not do is land it as-is and let the retarget find it, for the reason in the next section: no gate on this PR can see this defect. Test evidenceThis is an unattended CI run, so the evidence below is the PR's own check results read through the API. I built nothing, ran no test, and executed no PR-derived code. The important part is what is absent.
Read that table against the blocker above and the gap is the finding: the only gate on this PR that compiles Not verified, with reasons:
Sandboxed verification would settle the one thing static review cannot: 中文说明代码审查我在 通过的部分让 114 个文件的 diff 可审查的两条性质都是真的，我是校验过的，不是采信的：
阻塞项：五个文件会破坏其同目录单元测试描述的核心主张是这 114 个文件「无需改动任何测试」即可迁移，而剩下的「确实需要在同一次改动里搬迁 mock」。114 个文件中有 8 个，其同目录测试对根包调用了
机制就来自 base 分支自己的 alias 表。 另外三个看起来确实无害，原因各不相同： 我是针对被审查的 commit 确认的，不是推断的：#10917、#10946、#10956 都没有改动任何 修复很小且机械——要么把这五个文件挪到「搬迁 mock」的那一批，要么在同一个 PR 里把这五处 mock 调用改到子路径上。我不建议按现状合并、让重定向去发现问题，原因见下一节：本 PR 上没有任何 gate 能看见这个缺陷。 测试证据这是无人值守的 CI 运行，因此下面的证据是通过 API 读取的 PR 自身检查结果。我没有构建、没有跑测试、没有执行任何 PR 派生代码。 关键在于缺失的部分。 把上表和阻塞项对照着看，缺口就是结论本身：本 PR 上唯一会编译 未验证项及原因：
沙箱验证可以确认静态审查无法确认的那一件事： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 2/5 — the migration itself is verifiably clean and I could not break it, but five files silently un-mock their own tests and no gate on this base is capable of seeing that. Stepping back from the two stages above. Read from the title and #10908 alone, before I looked at the diff, the proposal I would have written is close to what this is: a generated codemod over the barrel imports, batched, with the batch boundary drawn by which tests interfere. The one place I would have drawn the boundary differently turns out to be the place this PR is wrong, which is worth stating plainly because it is the whole finding. I would have asked "does any test that reaches this file override a symbol this file imports from core?" — an intersection question. The description asks "does a test replace the core package?" — a presence question. Those two come apart exactly when a test mocks the root partially: it spreads What I want to be clear about, because a 2/5 can read as "the whole thing is bad" and it isn't: the generator's export verification is genuinely good. I rebuilt the pair set independently — from the new side of every hunk, so that retargeted multi-line imports whose brace lists survive as context lines are included, which is 60 pairs a naive The part I cannot get past is that the failure is invisible on this base. A mock that stops intercepting is not a type error, so Two follow-ups that are not blockers on this PR but would be a shame to lose:
If I were maintaining this in six months I would thank them for the migration and for the export verification, and I would want the five files and the rule. For reference on the guardrails, neither fired: this is a same-repo branch ( Requesting changes on the five files above. Re-running the intersection check against the remaining 191 before batch 3 would be cheaper than finding them one retarget at a time. 中文说明Confidence: 2/5 —— 迁移本身经得起校验，我没能把它证伪；但有五个文件悄悄地让自己的测试失去 mock，而这个 base 上没有任何 gate 能看见这件事。 从上面两个阶段退一步看。 只看标题和 #10908、还没看 diff 时，我会写出的方案与这个 PR 很接近：一个针对 barrel 导入的生成式 codemod，分批推进，批次边界由「哪些测试会干扰」来划。唯一我会划得不一样地方，恰好就是这个 PR 出错的地方，值得直说，因为这就是全部结论。我会问的是「有没有任何能触达这个文件的测试，覆盖了该文件从 core 导入的某个符号？」——这是一个交集问题。而描述问的是「有没有测试替换掉 core 包？」——这是一个存在性问题。当测试只部分 mock 根包时，两者就分道扬镳：它 spread 了 有一点我要讲清楚，因为 2/5 容易被读成「整个东西都不行」，而事实并非如此：生成器的导出校验是真的做得好。我独立重建了符号对集合——从每个 hunk 的新侧提取，这样那些被重定向的多行 import（其花括号列表以上下文行形式保留）也会被计入，而只读 我过不去的那一点是：这个失败在当前 base 上是不可见的。「mock 不再拦截」不是类型错误，所以 两条不属于本 PR 阻塞项、但丢了可惜的后续：
如果六个月后由我来维护这份代码，我会感谢他们做了这次迁移、也做了导出校验；同时我会想要那五个文件，和那条规则。关于两条 guardrail 供参考：都没有触发——这是同仓库分支（ 针对上面五个文件请求修改。在 batch 3 之前，对剩下的 191 个文件重跑一次交集检查，会比一次次靠重定向去发现更便宜。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Requesting changes on five specific files — full reasoning in the Stage 2 and Stage 3 notes above.
The migration itself is clean and I verified it independently: all 710 changed lines are import statements (one hunk per file, nothing outside the import region), all 107 subpaths resolve to real files under packages/core/src/, and all 235 distinct symbol/module pairs are exported directly by the exact module named — no re-export chasing, which is the defect that reached the first batch. Nothing was dropped.
The problem is the batch-selection predicate. These five files have a colocated test that calls vi.mock('@qwen-code/qwen-code-core', …) on the root package and overrides exactly the symbols this PR moves to a subpath:
serve/local-control/service.ts—sleepInhibitor(full-replacement mock; test drivesisRunning)serve/routes/workspace-github-prs.ts—fetchGitHubPullRequests,createGitHubPullRequest,getDefaultBranchserve/routes/workspace-git-diff.ts—fetchGitDiff,fetchGitDiffHunksForFileserve/routes/workspace-git-log.ts—fetchGitLog,fetchGitCommitDetailui/commands/curator-command.ts—getAutoSkillCuratorStatus,runAutoSkillCurator,restoreArchivedAutoSkill,setAutoSkillPinned
vi.mock is keyed on the resolved module id, and the base branch's own packages/cli/vitest.config.ts maps the root to packages/core/index.ts while /^@qwen-code\/qwen-code-core\/(.*)$/ maps to packages/core/src/$1. Two distinct ids, so the mock no longer intercepts: the module under test calls the real implementation while the test asserts on its mock.
No gate on this base can see it. A mock that stops intercepting is not a type error, so tui-parity's npm run build stays green, and Qwen Code CI (the unit suite) does not trigger because the base is not main or release/**.
Fix is mechanical: move these five into the batch that relocates mocks, or repoint the five vi.mock calls at the subpaths in this PR. Also worth re-running the same intersection over #10946 — 10 of its 130 files are candidates for the same shape.
中文说明
针对五个具体文件请求修改——完整推理见上面的 Stage 2 与 Stage 3 说明。
迁移本身是干净的，我做了独立校验：710 行改动全部是 import 语句（每文件一个 hunk，导入区域之外无任何改动），107 个子路径全部对应 packages/core/src/ 下真实存在的文件，235 个不同的符号/模块组合全部由所指名模块直接导出——无需沿再导出链追踪，而这正是第一批出现过的问题。没有符号被丢弃。
问题出在批次筛选判据上。以下五个文件的同目录测试对根包调用了 vi.mock('@qwen-code/qwen-code-core', …)，且覆盖的恰好就是本 PR 移到子路径的那些符号：
serve/local-control/service.ts——sleepInhibitor（完全替换式 mock；测试驱动isRunning）serve/routes/workspace-github-prs.ts——fetchGitHubPullRequests、createGitHubPullRequest、getDefaultBranchserve/routes/workspace-git-diff.ts——fetchGitDiff、fetchGitDiffHunksForFileserve/routes/workspace-git-log.ts——fetchGitLog、fetchGitCommitDetailui/commands/curator-command.ts——getAutoSkillCuratorStatus、runAutoSkillCurator、restoreArchivedAutoSkill、setAutoSkillPinned
vi.mock 以解析后的模块 id 为键，而 base 分支自己的 packages/cli/vitest.config.ts 把根映射到 packages/core/index.ts，把 /^@qwen-code\/qwen-code-core\/(.*)$/ 映射到 packages/core/src/$1。两个不同的 id，因此 mock 不再拦截：被测模块调用真实实现，而测试断言的是它的 mock。
这个 base 上没有任何 gate 能看见它。「mock 不再拦截」不是类型错误，所以 tui-parity 的 npm run build 依然是绿的；而 Qwen Code CI（单元测试套件）不会触发，因为 base 不是 main 或 release/**。
修复是机械的：把这五个文件挪到搬迁 mock 的那一批，或在本 PR 里把这五处 vi.mock 调用改指到子路径。另外建议对 #10946 重跑同一次交集检查——它 130 个文件里有 10 个属于同一形态的候选。
— Qwen Code · qwen3.8-max-2026-09-02
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — the 'tmux-testing' CI check was skipped at this commit and its interactive E2E suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：build-and-test — the 'tmux-testing' CI check was skipped at this commit and its interactive E2E suite did not run locally.
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
|
||
| import { Storage, type Config } from '@qwen-code/qwen-code-core'; | ||
| import type { Config } from '@qwen-code/qwen-code-core/config/config.js'; | ||
| import { Storage } from '@qwen-code/qwen-code-core/config/storage.js'; |
There was a problem hiding this comment.
[Critical] R1-1: Converting this file (and six others in this batch) to a subpath import silently breaks the tests that intercept it through package-root mocks. Vitest resolves vi.mock('@qwen-code/qwen-code-core') and the new @qwen-code/qwen-code-core/config/storage.js specifier to different module ids (root alias → core/index.ts, wildcard alias → core/src/), so the root-keyed mock no longer applies here: Storage is now the real implementation while acpAgent.test.ts seeds its fixtures under the fake getGlobalQwenDir(), and the failing run writes into the real user HOME. The same mechanism breaks six more suites — ui/commands/curator-command.test.ts (14/15, real skill-curator functions hit the filesystem), serve/routes/workspace-git-diff.test.ts (8/16), workspace-git-log.test.ts (11/16), workspace-github-prs.test.ts (13/19, spies record zero calls and real git/gh code runs against fake cwds), serve/local-control/service.test.ts (1/5, the real sleepInhibitor spawns systemd-inhibit during the test), services/McpPromptLoader.test.ts (21/47, root-namespace vi.spyOn never intercepts the subpath binding) — and leaves acpAgent.worktree.test.ts green while it silently exercises real core implementations (Storage, createDebugLogger, getMCPServerStatus, sessionIdContext, APPROVAL_MODES) instead of its factory overrides. All seven suites pass at the merge base and fail only on this commit, so this batch's selection criterion ("what remains genuinely needs its mocks moved") misclassified them: their mocks must move in the same change.
Witness:
test-delta (built merge base 0e476635 vs PR): 11 failing files on base, 18 on the PR
-- the 7 files above fail ONLY on the PR (netNew)
npm test --workspace=packages/cli at HEAD: exit 1, 18 failed files / 143 failed tests
targeted reruns of the 7 net-new files: 71 failing tests
curator-command.test.ts: EACCES: permission denied, mkdir '/project' (real fs reached)
Fix: either pull the seven coupled files (this one plus ui/commands/curator-command.ts, serve/routes/workspace-git-diff.ts, workspace-git-log.ts, workspace-github-prs.ts, serve/local-control/service.ts, services/McpPromptLoader.ts) out of this batch and let the change that moves their mocks convert them — the series' own selection rule — or move the mocks in this PR by mocking the exact subpath modules the sources now import, e.g. here:
vi.mock('@qwen-code/qwen-code-core/config/storage.js', () => ({
// return the SAME Storage fake object the root mock uses
}));…and …/utils/gitDiff.js, …/utils/github-prs.js, …/skills/skill-curator.js, …/prompts/mcp-prompts.js, …/services/sleepInhibitor.js in the other six suites. The mock specifier must spell the exact subpath with the .js suffix so it resolves to the same module id as the source import under the wildcard alias in packages/cli/vitest.config.ts, and the storage.js factory must reuse the SAME Storage fake object the root mock provides in acpAgent.test.ts — a second divergent fake reproduces the split that broke these tests. The seven failing suites pin this behavior themselves and must go green after the fix; removing the fix (re-migrating any of these files without its mock) turns them red again — e.g. rerun curator-command.test.ts after reverting the mock move and confirm it reds.
中文说明
把本文件（以及本批另外六个文件）转换为子路径导入后，通过包根 mock 拦截它们的测试会被悄悄绕过。Vitest 将 vi.mock('@qwen-code/qwen-code-core') 与新的 @qwen-code/qwen-code-core/config/storage.js 解析为不同的模块 id（根别名 → core/index.ts，通配别名 → core/src/），因此包根级的 mock 在这里不再生效：Storage 变成了真实实现，而 acpAgent.test.ts 仍把测试数据播种在假的 getGlobalQwenDir() 之下，失败的测试运行还会写入真实用户主目录。同一机制还破坏了另外六个套件：curator-command.test.ts（14/15，真实 skill-curator 函数直接操作文件系统）、workspace-git-diff.test.ts（8/16）、workspace-git-log.test.ts（11/16）、workspace-github-prs.test.ts（13/19，spy 记录到 0 次调用，真实 git/gh 代码在假的 cwd 上运行）、local-control/service.test.ts（1/5，真实 sleepInhibitor 在测试中拉起 systemd-inhibit）、McpPromptLoader.test.ts（21/47，对根命名空间的 vi.spyOn 拦不到子路径绑定）；此外 acpAgent.worktree.test.ts 虽然仍是绿色，却已悄悄改为运行真实 core 实现（Storage、createDebugLogger、getMCPServerStatus、sessionIdContext、APPROVAL_MODES）而非其工厂覆写。这七个套件在合并基线上全部通过、仅在本提交失败，说明本批的筛选条件（「剩下的确实需要连同 mock 一起搬迁」）把它们误分类了：它们的 mock 必须在同一次改动里一起搬迁。
修复：要么把这七个耦合文件移出本批，交给搬迁 mock 的那次改动去转换（即本系列自己的筛选规则）；要么在本 PR 里同时搬迁 mock —— 对源码现在导入的确切子路径模块打 mock（示例见英文部分代码块），其余六个套件分别 mock …/utils/gitDiff.js、…/utils/github-prs.js、…/skills/skill-curator.js、…/prompts/mcp-prompts.js、…/services/sleepInhibitor.js。mock 说明符必须写出带 .js 后缀的完整子路径，才能在 packages/cli/vitest.config.ts 的通配别名下解析到与源码导入相同的模块 id；storage.js 的工厂必须复用 acpAgent.test.ts 包根 mock 里的同一个 Storage 假对象——再造一个不同的假对象会重现导致这些测试坏掉的分裂。七个失败套件本身就是验收标准：修复后必须变绿；移除修复（在未搬 mock 的情况下重新迁移这些文件）会再次变红——例如回退 mock 搬迁后重跑 curator-command.test.ts 确认其变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Verified real at head 2e7b7eb: ran the seven suites together (acpAgent, curator-command, workspace-git-diff, workspace-git-log, workspace-github-prs, local-control/service, McpPromptLoader) — 7 files failed, 70 tests failed, matching the reported signatures (spies record 0 calls, real implementations reach fs/git, 200 vs expected 502). This PR changes no test files, so the source import rewrites are the cause.
Not auto-fixing because both remedies exceed three files: (a) revert these seven source files to barrel imports and let the mock-migration change convert them (the series' own selection rule), or (b) move vi.mock in all seven test files to the exact subpath modules, reusing the same Storage fake in acpAgent.test.ts. @yiliang114 which direction? Leaving open until decided.
|
Closing in favour of #10957, which carries the whole change and is green. This branch still holds the pre-revert set. Once the stack was retargeted at So the diff on this PR no longer describes anything that should land. Merging it on its own would reintroduce exactly the modules that were shown to break, and because its base is not #10957 is based on 中文说明关闭，改由 #10957 承载全部改动——它已全绿。 本分支仍是回退之前的内容。整个栈改到 因此本 PR 的 diff 已不代表任何应该落地的内容。单独合并它会把已被证实会挂的那批模块重新带进来；而且由于它的 base 不是 #10957 基于 |
2 participants
Stacked on #10946, which is stacked on #10917. Each will retarget as the one below it lands.
What this PR does
The same mechanical change as #10946, over 114 more files: they import the specific core modules they use instead of the package root. Only import statements change.
Why these files were not in the first batch
They should have been. The analysis that produced the first batch decided whether a test replaces the core package by looking for the text of such a call anywhere in the file — including inside comments. One test only mentions the pattern in a doc comment, explaining why it deliberately avoids the heavy mock its neighbour uses, and counting it as a blocker ruled out 217 modules that nothing actually blocks.
Ignoring comments when detecting the call changes the picture substantially:
The first batch took 130 of those; this takes 114 more. What remains genuinely does need its mocks moved in the same change.
Reviewer Test Plan
How to verify
Every generated specifier is checked against the exports its named module actually has, following that module's own re-exports — 865 symbol/module pairs here, none wrong. That check exists because the first batch shipped one bad pair: a symbol map built from re-export chains had named the module that re-exports a symbol rather than the one declaring it, and the build caught it.
Outside import statements every line is byte-identical to its parent, which is worth confirming independently since it is the property that makes this reviewable at this size.
Evidence (Before & After)
N/A — no user-visible behavior changes.
Tested on
Not run locally; relying on CI.
Risk & Scope
Linked Issues
Refs #10908
中文说明
本 PR 基于 #10946，后者基于 #10917。下层合并后会逐级自动重定向。
这个 PR 做了什么
与 #10946 相同的机械改动，覆盖另外 114 个文件：把「导入 core 包根」改为「导入实际用到的具体模块」。只有 import 语句变化。
为什么这些文件不在第一批里
它们本就应该在。产生第一批的分析在判断「某测试是否整体替换了 core 包」时，是在整个文件里搜索该调用的文本——包括注释内部。有一个测试仅在文档注释里提到这个写法（说明它为何刻意避开邻居那个笨重的 mock），把它当成阻塞点，就排除掉了 217 个实际上没有任何阻塞的模块。
检测该调用时忽略注释后，结论变化很大：
第一批取了其中 130 个，本批再取 114 个。剩下的确实需要在同一次改动里搬迁 mock。
审查者验证计划
每条生成的说明符都对照其目标模块的真实导出集合做了校验（跟随该模块自身的 re-export），本批 865 个符号/模块组合，无一错误。这道校验的由来是第一批漏过了一个错误组合：基于 re-export 链构建的符号表把某符号指向了再导出它的模块，而非声明它的模块，被构建捕获。
import 语句之外的每一行与父提交逐字节相同——在这个体量下，这条性质是可审查性的基础，值得独立确认。
风险与范围