perf(cli): import core modules directly instead of the package root - #10957
perf(cli): import core modules directly instead of the package root#10957yiliang114 wants to merge 31 commits into
Conversation
…files importing the whole package Importing from the core package root pulls in its entire export graph — a bit over six hundred modules — however little of it a file actually uses. In a release run the cli workspace spent 2223s collecting modules against 1372s running tests, and a file that imports the package root costs about 11.5s before its first assertion where one importing a single module costs about 2s. cli's tsconfig already maps a wildcard subpath onto core's sources, so esbuild resolves per-module imports when it bundles. Vitest does not read tsconfig paths, and the alias list that stands in for them named only four subpaths, so those imports did not resolve under test at all. This adds the wildcard there. Expressing the alias list as an ordered array is what allows a pattern entry. The package root has to become an exact match in the process: as a string it would also match everything beneath it and rewrite each subpath into a path under index.ts. Two files move to per-module imports as a first check that the mapping holds end to end. Both were picked because nothing that depends on them replaces the core package with a mock factory — where a test does that, the mock stops intercepting once the code under test imports the module directly, so those call sites need their mocks moved in the same change and are left alone here.
The previous commit was assembled from a working copy that predated main by several weeks, so it silently reverted this file to that older state. Four named core subpaths added since — envVarResolver, noFollowOpen, subSessionConstants and toolWriteOrigin — disappeared with it, and the new wildcard then claimed those specifiers and pointed them at files that do not exist. 257 test files failed to load as a result. All eight named subpaths are restored and kept ahead of the wildcard, with a comment saying why that order matters and what a contributor adding a ninth has to do. None of the eight can be derived from its specifier, so none of them can be folded into the pattern. The two migrated source files are rebuilt on their current contents for the same reason; one of them had also been reverted by a line.
Importing from the core package root evaluates its whole export graph — a bit over six hundred modules — however little of it a file uses. On the release lane the cli workspace spends more time collecting modules than running tests, and on the main lane it now takes 84 minutes on its own, most of it collection. These 130 files ask for named modules instead. They were chosen by checking, for every test whose module graph reaches them, whether that test replaces the core package with a mock: a test that swaps the package wholesale stops intercepting once the code under test imports a module directly, and a test that spreads the real package and overrides a few names only matters if one of those names is what the file imports. Files with either kind of coupling are left for a later change that moves the mocks at the same time. Only import statements move; every other line is byte-identical.
…nd wrap long imports Two problems with the previous commit, both found by CI. The symbol map resolved a re-exported name to the module that re-exports it rather than the one that declares it, so `ProviderModelConfig` was asked of `models/types` when it is declared in `providers/types`, and the build failed to typecheck. A checker now confirms, for every generated specifier, that the named module really does export that symbol — following its own re-exports — and it reports one bad pair out of 492. The formatting pass that was supposed to run over these files had silently done nothing: invoked from the repository root against paths outside it, Prettier skips the files and still reports success, so long import statements went out unwrapped. Rerunning it properly reflows 47 files. Two files are left with an over-long line Prettier would wrap, because that line is over-long on the base commit too and the lint gate does not flag it; reformatting it here would be unrelated noise. Every other line outside an import statement stays byte-identical.
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.
Where a test replaces the whole core package with a factory, the code under test cannot move to per-module imports on its own: the mock would stop intercepting and the real implementation would load instead, quietly changing what the test exercises while leaving it green. The mock has to move in the same commit. These three are the cases where that is unambiguous — every name the factory stubs is declared in one module, and the code under test imports exactly those names. Each pair moves together onto that module. The pattern generalises: about sixty tests each hold back one or two modules this way, and roughly ninety more modules are held by several tests at once and need them changed together. Establishing the shape on the clean cases first keeps the ambiguous ones honest.
|
Retargeted to The repository's CI runs on With Reviewing is still easier one layer at a time:
中文说明改 base 到 仓库的 CI 在 base 改成 审阅仍建议逐层来看，各层范围见上表。 |
|
Qwen Code review was cancelled before a review could be posted. Nothing failed and nothing is retried automatically: the run was cancelled — by an operator, an upstream event, or the job exceeding its execution time limit. If you still want a review of this PR, request one with |
|
Thanks for the PR — and for publishing the measurement that undercuts your own headline. That comment is the most useful thing on this thread. Template looks good ✓ — every required heading is present, including Problem: real and measured, not theoretical. Collection dominating assertion time (2223s collecting vs 1372s running on the release lane, ~84 minutes on the main lane), the suite up 87% in ten weeks, and sharding ruled out with an actual argument — the three release shards finish within a minute of each other, so the split is already balanced and more shards only divide a fixed per-file cost. That is an observed problem with a tracked plan (#10908). But the problem this PR solves is smaller than the problem it is titled for. Your own measurement two days after opening: 109 modules moved, 24 of 1003 test files got cheaper, ~228s of CPU ≈ a minute of wall across four workers against an 84-minute lane — under 2%, inside run-to-run spread, your words being "Do not expect this PR to move the number." Nothing in the CI evidence on this head contradicts that, and nothing confirms a win either. So: problem exists ✓, this slice's contribution is not observable. Direction: in scope — this is test-infrastructure cost, and the mechanism you land (resolver mapping, an exports entry with a check standing on it, the lint rule taught the new shape) is the part that has to exist before any later batch can. Two things push it to a maintainer rather than a gate, and I want to name both rather than wave them through:
Size: 129 files, 1853 changed lines (+1347 / −506). Breakdown, because the shape matters more than the total:
So ~1184 lines of machinery and tests carry a 669-line sweep averaging 6 lines a file. Stage 0: Approach: the honest question is whether the sweep belongs in this PR at all. Your own history answers it — the first full run broke 16 files and 127 tests, and you restored 146 of the 255 migrated modules rather than sharpen a heuristic that local reading could not validate. You then wrote that selecting batches by "which modules are safe to move" is the wrong unit, and that "which test graphs can be made entirely clean" is the right one. If that is true, this PR is a batch chosen by the unit you have since rejected, and it is the batch that produced the breakage. Cut to 20% and the mechanism still stands on its own: the ordered alias array with the wildcard, the Two smaller notes, neither a blocker — the body says "two mock moves" and the diff has three, and Risk: one Stage 1e match ( ⏸️ Escalating the direction question to a maintainer rather than deciding it here — Stage 3 carries the reasoning and the mention. 中文说明感谢这个 PR——也感谢你公开了那个削弱自己标题的测量数据。那条评论是整个 thread 里最有价值的东西。 模板完整 ✓ —— 所有必需标题都在，包括 **问题：**真实且经过测量，不是理论性的。收集时间压过断言时间（release lane 上收集 2223s vs 运行 1372s，main lane 约 84 分钟），测试套件十周内增长 87%，并且用实际论证排除了分片方案——三个 release 分片彼此在一分钟内完成，说明切分本身是均衡的，增加分片只是把一个固定的「每文件成本」再除一次。这是一个有观测证据、有跟踪计划（#10908）的问题。 但本 PR 实际解决的问题，比它标题所声称的要小。你自己在开 PR 两天后的测量：迁移了 109 个模块，1003 个测试文件里只有 24 个变便宜，约 228s CPU ≈ 四个 worker 下一分钟墙钟，对着 84 分钟的 lane——不到 2%，落在运行间波动范围内，你的原话是「不要指望本 PR 让数字变好看」。当前 head 的 CI 证据既没有反驳这一点，也没有证明任何收益。所以：问题存在 ✓，但这一片的贡献不可观测。 **方向：**在范围内——这是测试基础设施成本，而你落地的机制（解析映射、有检查兜底的 exports 条目、认识新形态的 lint 规则）正是后续任何批次都必须先存在的部分。有两点让它应该交给 maintainer 而不是由 gate 决定，我想明确点出来而不是含糊放过：
**规模：**129 个文件，1853 行改动（+1347 / −506）。拆分如下，因为形状比总数更重要：
即约 1184 行机制与测试，承载一次平均每文件 6 行、共 669 行的迁移。Stage 0： 方案：真正该问的是这次迁移是否应该留在这个 PR 里。你自己的历史已经回答了——第一次完整运行弄坏了 16 个文件、127 个测试，你选择把 255 个已迁移模块中的 146 个还原，而不是去打磨一个本地阅读无法验证的启发式。你随后写道，以「哪些模块可以安全迁移」来选批次是错误的单位，正确的单位是「哪些测试图能被彻底清干净」。如果这是对的，那本 PR 就是一个用你已经否定的单位选出来的批次，而且正是产生了那次破坏的批次。 砍到 20%，机制本身依然站得住：带通配的有序 alias 数组、 两个更小的点，都不是阻塞项——正文写「两处 mock 搬迁」而 diff 里有三处； **风险：**一个 Stage 1e 命中（ ⏸️ 方向问题上交 maintainer，而不是在这里下结论——推理与 @mention 在 Stage 3。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo Critical findings. I went looking for the two ways this class of change breaks — a resolver that mis-orders, and an import that names a module which does not declare the symbol — and neither is present. What follows is what I verified, then the two things I would actually change. Verified correct, not assumed:
Suggestion — the new check is more load-bearing than your description says. Suggestion — gating asymmetry. You built three gates and left two maps ungated, and said so yourself in the code:
The ungated one is the most-exercised map in the repo: every cli test run goes through it. To be fair about the failure mode, it is loud rather than silent — a new named key whose target stem differs from the key falls through the wildcard to Note — three mock moves, not two. Files changed (13 of 129 shown)
TestingThis was an unattended CI run ( The signal is strong on Linux and thin everywhere else.
The gaps, stated plainly:
Nothing red to attribute, so no log excerpt to quote — the only non-success conclusions on this commit are Sandboxed verification would settle the one claim CI cannot: 中文说明代码审查没有 Critical 问题。我专门去找这类改动的两种典型崩坏方式——解析器顺序排错、以及导入指向一个并未声明该符号的模块——两种都不存在。下面是我核实过的内容，然后是我真正会改的两点。 已核实为正确（不是想当然）：
建议——新检查比你的描述所说的更承重。 **建议——门禁不对称。**你建了三道门禁，却留下两张映射表没有门禁，而且在代码里自己说明了：
没有门禁的那一张，是仓库里被使用最频繁的映射：每一次 cli 测试运行都要经过它。关于失败模式，公平地说它是响的而不是静默的——一个新的具名键若其目标 stem 与键名不同，会穿过通配落到 注——三处 mock 搬迁，不是两处。 测试这是一次无人值守的 CI 运行（ 信号在 Linux 上很强，在其他平台上很薄。 上方表格为本次审查 commit 的 CI 结论；三个缺口如下：
没有红色需要归因，所以没有日志摘录可引——本 commit 上唯一的非 success 结论就是 沙箱验证可以判定 CI 判不了的那个主张： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the engineering is clean and I could not break it, but a What I would have proposed. Before reading the diff, from the title and the "Why it's needed" section alone: confirm with one measurement that the barrel evaluation is actually the collection cost, then land only the mechanism — the resolver mapping, the exports entry, a check that pins it, the lint rule taught the new shape — with zero module migrations, and migrate afterwards by whole test graphs, one graph per PR, measuring each. Same mechanism, none of the 669-line sweep, and no revert cycle. Your history is the evidence for that ordering, which is why I keep coming back to it. The sweep is what broke 16 files and 127 tests. The mechanism is not implicated in a single one of those failures — every one was a stub that stopped intercepting, i.e. a consequence of which modules moved, never of how they resolve. You then wrote that "which modules are safe to move" is the wrong unit and "which test graphs can be made entirely clean" is the right one. This PR is a batch selected by the unit you have since rejected, and it is the batch that produced the breakage. Your own analysis is a better argument for splitting than anything I could construct. What I would thank you for. The gates are the genuinely valuable part and they are well built. Reservations, in the order they'd stop me.
Why I am not requesting changes. I found no correctness defect, no security issue, and no regression. Every load-bearing claim in the code that I could check independently checked out, including the two I most expected to break: alias precedence under Why I am not approving either. The question this PR leaves open is not "is it correct" — it is. It is "should a sub-2% perf slice, carrying a widening of the published core exports map and roughly 1200 lines of permanent machinery, land now, when its author's own measurement concludes the remaining work should be re-sequenced by test graph rather than by module?" I cannot answer that from the diff, the tests, or the description, and the description is the thing arguing against it. Approving would mean I ran out of reasons to say no. ⏸️ Deferring to @chiga0 — needs a human call on this one. Specifically, three things I could not resolve:
You reviewed this at 中文说明Confidence: 3/5 —— 工程实现是干净的，我没能把它弄坏；但一个 **我原本会怎么做。**在读 diff 之前，仅凭标题和「Why it's needed」：先用一次测量确认 barrel 求值确实是收集成本的来源，然后只落地机制——解析映射、exports 条目、一个把它钉住的检查、学会新形态的 lint 规则——一个模块都不迁移；之后再按「整张测试图」为单位迁移，一个 PR 一张图，每批都测量。机制相同，没有那 669 行迁移，也没有还原循环。 你的历史正是这个顺序的证据，这也是我反复回到这一点的原因。弄坏 16 个文件、127 个测试的是迁移，不是机制——机制没有牵涉其中任何一次失败，每一次都是 stub 停止拦截，也就是「迁移了哪些模块」的后果，从来不是「它们如何解析」的后果。你随后写道「哪些模块可以安全迁移」是错误的单位，「哪些测试图能被彻底清干净」才是正确的单位。本 PR 就是一个用你此后已否定的单位选出的批次，而且正是产生了那次破坏的批次。你自己的分析，比我能构造出的任何论证都更有力地支持拆分。 **我要感谢的部分。**门禁是真正有价值的部分，而且建得很好。 保留意见，按会让我停下来的顺序。
**为什么我不 request changes。**我没有发现正确性缺陷、安全问题或回归。代码里每一个我能独立核实的承重主张都核实通过了，包括我最预期会坏的两个： **为什么我也不 approve。**这个 PR 留下的问题不是「它正确吗」——它正确。而是「一个不到 2% 的性能切片，携带对已发布 core exports 映射的扩大和约 1200 行永久机制，是否应该现在落地——而它作者自己的测量已经得出结论：剩余工作应当按测试图重新编排，而不是按模块」。我无法从 diff、测试或描述中回答这个问题，而描述本身正在反对它。approve 就意味着我只是找不出说不的理由了。 ⏸️ 上交 @chiga0 —— 这件事需要人来定。具体是我无法解决的三点：
你在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Retargeting this PR at main got the unit suite to run for the first time, and it failed: 16 files, 127 tests, none of them failing on the jsdom PR that shares the same base. So they are this stack's doing. Every one is the same shape — a stub the suite installs no longer intercepts once the code under test names a module instead of the package root. The static analysis that picked these files models three ways of installing such a stub and misses at least two more: a spy planted on a namespace import of the package, and whatever six of the sixteen suites do, which it cannot parse at all. Sharpening the heuristic further is not the answer; it was already wrong in a way no amount of local reading would have caught. So this restores every migrated module that any failing suite reaches, 146 of them, and keeps the 110 that nothing failing depends on. That is blunt — some of the 146 are certainly fine — but it is the version that can be shown to pass, and picking the survivors apart is work for a run that is green to begin with.
|
The full run found real breakage, and I have cut the change back to what passes. What the run said. 16 files, 127 tests. The jsdom PR (#10958), which branches from the same base and shares none of these commits, failed 1 file and 2 tests — a Why. Every failure is the same shape: a stub the suite installs stops intercepting once the code under test names a module instead of the package root. Three examples, three different mechanisms:
Why the analysis missed them. It models What I did. Restored every migrated module that any failing suite reaches — 146 files — and kept the 110 that nothing failing depends on. Deliberately blunt: some of the 146 are certainly fine, but separating them belongs on top of a green run rather than in place of one. The two batches below still read as originally written. Once this is green, they should be squashed to match what actually lands, or this can carry the whole change and they can close. 中文说明完整运行抓到了真实的破坏，我已把改动收缩到能通过的范围。 运行结果：16 个文件、127 个测试失败。而同样基于该基线、与本栈无任何共同提交的 jsdom PR（#10958）只挂了 1 个文件 2 个测试，且是两个 PR 都没碰过的 原因：所有失败都是同一形态——被测代码从「包根」改为「具名模块」后，用例安装的 stub 不再拦截。三个例子对应三种不同机制： 分析为何漏掉：它建模了 处理：回退所有被失败用例触达的已迁移模块（146 个），保留没有任何失败用例依赖的 110 个。这刀切得钝——其中必然有一些本来没问题——但把它们摘出来应该建立在一次绿色运行之上，而不是取而代之。 下面两个 PR 的描述仍是最初写法。本 PR 转绿后，它们应压缩成实际落地的内容，或者由本 PR 承载全部改动、它们关闭。 |
…e by path The integration gate failed to compile against the migrated files: error TS2307: Cannot find module '@qwen-code/qwen-code-core/utils/debugLogger.js' error TS2307: Cannot find module '@qwen-code/qwen-code-core/utils/editor.js' Only packages/cli maps these specifiers, through a wildcard in its own tsconfig. The integration suite lists the eight named subpaths and no wildcard, and the package's exports map has entries for those same eight plus the dist and src trees — so anything resolving the normal way, this suite and any consumer of the published package alike, cannot name a core module. Both gaps close here: the wildcard is added to the integration suite's path mappings, and a catch-all maps a bare module path onto the build output. The catch-all exposes nothing new; `./dist/*` already reaches the same files. This is the part of the change with consequences beyond the test run. The shipped CLI is a single bundle and never resolves these specifiers at runtime, but the package is published, and until now a migrated import was only resolvable from inside the one workspace that happens to map it.
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
yiliang114
changed the title
The exports check validated resolved targets against the repo tree, but core's exports map also carries "./src/*": "./src/*" while package.json publishes only dist, vendor and scripts/postinstall.js. A specifier routed through that entry resolved to a real in-repo packages/core/src file, passed the bare existsSync and exited 0 even though the published artifact ships nothing — the installed CLI would die at startup with ERR_MODULE_NOT_FOUND while the gate stayed green. Require the resolved target to lie inside packages/core/dist/, where every legitimate runtime specifier lands. Add scripts/tests/check-core-subpath-exports.test.js: a fixture-tree suite (per the scripts/tests convention) that runs a copy of the real script in a temp workspace. It pins the dep-only scan from a18ffeb (goalWire named by no cli source resolves; removing its exports entry exits 1 — the mutation witness requested in review, made hermetic) and pins this guard (a ./src/*-routed specifier is reported as not published; removing the guard turns that test red). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtn1pqbuir
Keep the subpath import style while absorbing main's output-style dialog changes: route OutputStyleDefinition/BUILT_IN_OUTPUT_STYLES through core/output-styles.js, drop the now-unused BUILT_IN_OUTPUT_STYLES import from dialogs-modes, and register the new conversationsRuntimeMarker alias ahead of the wildcard. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtn4km2niw
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
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. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit lanes were skipped in CI; OS-specific portability (junction symlinks, realpath spellings, exec-bit skipIf) verified by reading only.
Not reviewed: build-and-test — packages/cli and packages/core unit suites timed out under runner load (infrastructure); packages/web-shell failed only in files this PR does not touch (base A/B unavailable — path-rule attribution); packages/vscode-ide-companion and packages/webui suites never ran (continuation cap).
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
scripts/tests/no-core-utils-upward-import.test.js:251 — [review] D5-1 'keeps exact export keys ahead of the wildcard' cannot discriminate its mutation (goalWire reports 1 under either ordering); re-specify it with a key whose orderings dive…scripts/tests/dev.test.js:164 — [review] D5-2 dev-loader fall-through stub is argument-blind — a nextResolve(sub, context) mutation ships green; echo the specifier in the stub and assert iteslint-rules/no-core-root-barrel-import.js:35 — [review] D5-3 CORE_BARREL_SPECIFIERS misses the /dist/src/index.js barrel spelling reachable via the ./dist/* exports entry — add it plus a rejects-table casescripts/check-core-subpath-exports.mjs:33 — [review] D5-4 8 of the 9 named exports entries are never probe-imported; a repoint to another existing dist file passes the gate — extend EXPORT_PROBES
Convergence: round 5 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: scripts/check-core-subpath-exports.mjs (findings in round 4; 2 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.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit lanes were skipped in CI; OS-specific portability (junction symlinks, realpath spellings, exec-bit skipIf) verified by reading only.
未审查（原文为英文）：build-and-test — packages/cli and packages/core unit suites timed out under runner load (infrastructure); packages/web-shell failed only in files this PR does not touch (base A/B unavailable — path-rule attribution); packages/vscode-ide-companion and packages/webui suites never ran (continuation cap).
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 5 轮发布了 5 条行内评论，其中 5 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：scripts/check-core-subpath-exports.mjs（第 4 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
…r probe stages
The round-4 dist-containment guard rejected
@qwen-code/qwen-code-core/package.json, which core's exports map
deliberately publishes ("./package.json": "./package.json") and npm ships
regardless of "files". Allow that one target; ./src/* targets stay
rejected (existing test remains red without the guard).
Also extend the fixture suite to the two failure stages it did not
witness: the named-export probe stage (target exists but lacks the probed
export) and the resolve-failure branch (exports map without the ./*
catch-all). Mutant-verified: removing the probe import/export-name check
or the resolve-catch failed++ turns the matching new test red and nothing
else.
Co-authored-by: Qwen-Coder <qwen-coder@alibaba-cloud.com>
Patrol-Run: qwen-pr-closeout/jmtngpuscjl
…er map The loader's named map listed 8 of the 9 named entries in core's exports map: conversationsRuntimeMarker (source at utils/conversations-runtime-marker.ts, a path the specifier does not mirror) was missing. The specifier is reachable in the harness graph today via shared-env-keys.ts, so resolution fell through to the exports map — ERR_MODULE_NOT_FOUND mid-capture on a fresh clone, or a stale dist silently mixed into a source-backed capture otherwise. Add the entry, matching packages/core/package.json and the cli vitest alias. Also add a sync guard (scripts/tests) asserting every named key of core's exports map has a loader entry pointing at an existing core source file, so the next omitted entry goes red in CI. Co-authored-by: Qwen-Coder <qwen-coder@alibaba-cloud.com> Patrol-Run: qwen-pr-closeout/jmtngpuscjl
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 48 passed · 2 failed · 50 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：48 通过 · 2 失败 · 50 总计 抖动门： Verification reportPR 10957 — deep verification (follow-up round)Verdict: Nothing regressed. The two failed assertions are my census model's calibration limits, not defects in the PR (named in Findings §5 and in Not covered); the central claim was re-measured with 32 real vitest runs whose oracle is noise-free. Since the last round the PR grew six commits, all of them in the exports gate, its new test file, and the skill-review harness loader — and that delta is where this round's evidence is: the loader change is load-bearing (base 3/4 harness targets load and 中文摘要结论： A/B 结论：上一轮的 16 个单元在新 head/base 上全部重跑（32 次 vitest 真实运行）。机制形态与上轮一致——4/16 个单元从「加载 barrel」翻转为「不加载」（585→16、585→15、585→16、587→18 个模块），其余 12 个单元模块数逐字节相同（662=662、718=718、641=641…）；无任一单元在 head 加载更多模块；32 次运行中跨树污染为 0。唯一变红的单元（ 上一轮发现的处置：F2（门禁普查止于 本轮新发现：
未覆盖：cli 全量单元套件（84 分钟 × 2 臂）、仓库级 lint/typecheck、 Previous-round findings — status at the new headLast round verified head
ScopeCentral claim (perf): importing individual core modules instead of the package root removes core's ~578-module barrel from a cli test file's module graph. Secondary claims: (a) the vitest alias ordering and core's This round's emphasis is the delta: the exports gate's two new guards and their new fixture test, and the skill-review-harness loader's new subpath handling — the one branch the previous round listed as unreachable from CI. Central claim + A/BControl: Oracle: a counting vite plugin wrapping each arm's own Cells are the same 16 the previous round measured (all still colocated with a module this PR migrated), so the two rounds are directly comparable.
Both shapes from last round reproduce exactly:
The one red cell is an A/A control, not a regression. Closure census — partially calibrated this roundThe static walk (
Labelled an estimate, not a measurement: with one cell disagreeing in the under-predicting direction the −21 could be a few files larger, and the −81 production cross-check does not close against the 110 migrated files (that assertion is one of the two fails in CorrectionsC1 still stands, verbatim, at the new head.
The PR body's own Evidence section — "The shipped CLI is a single bundle and never resolves these specifiers at runtime" — remains the correct statement, and the CI comment still contradicts it. This is a correction to the description and the comment, not a request to change the code: the gate is genuinely load-bearing, but for CI lanes and non-bundled runs, and a maintainer reading "breaks Findings1. The new loader-sync test pins the map's presence, not its correctness — Suggestion
Measured (mutation M13,
The survivor is real, not a dead harness: the positive control M14 in the same file (point Blast radius is bounded and worth stating plainly: this loader is exercised only by Suggested fix (not applied — measured only in a scratch copy)Derive the expected target from the exports map instead of only checking existence — the map already carries it: it('points every entry at the module the exports map names', () => {
const exportsMap = JSON.parse(
readFileSync(join(coreDir, 'package.json'), 'utf8'),
).exports;
for (const [name, target] of named) {
const importTarget = exportsMap[`./${name}`]?.import;
expect(importTarget, `exports map has no import target for ./${name}`).toBeTruthy();
// "./dist/src/goals/goal-wire.js" -> "goals/goal-wire.ts"
expect(target).toBe(
importTarget.replace(/^\.\//, '').replace(/^dist\/src\//, '').replace(/\.js$/, '.ts'),
);
}
});Verified against the current map: all nine named entries satisfy it ( Reproduce: 2. The harness loader is now load-bearing, and this round proves it — pass, reported because last round could not reach itThe previous round listed this branch as untestable from CI. Constructing the configuration that reaches it was cheap, and the result is the cleanest A/B of the round ( The loader body was extracted byte-verbatim from each arm's
That third row reproduces both failure modes the new code comment claims, which is worth recording because the second one (silent load from compiled output) is invisible to any behavioural assertion. It also independently corroborates the PR body's report that the integration gate failed on The real harness was also run end to end: 3. Three unrelated files carry pure formatting churn — Nice to have
Measured, and it is benign rather than damaging: the base versions fail the repo's pinned formatter ( The cost is review-surface, not correctness: a 4. The dist-containment guard is narrower than the manifest it cites — Nice to haveThe new guard rejects any resolved target that is not under Measured as correct today: all 96 real specifiers the gate collects are Two properties of the guard that hold and are worth recording: 5. The two failed assertions are my census model, not the PR
Per the verdict contract a nonzero Mutation matrixWitness
7/8 killed. M13's survivor is classified a coverage gap rather than dead code or redundant defence: the mapping is live, the loader uses it, and driving the mutated loader through a real Not covered
MethodologyEnvironment: the CI verify container, Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
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. Suggestions are inline.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- PR description mock-move count (three landed, description says two; 110 vs 109) — already reported (issue-level triage comments 5529582370 and 5529583464 at the current head)
- Dev-loader named-subpath fall-through to stale dist (scripts/dev.js:95) — already reported (issue-level triage comment 5529582959, non-blocking follow-up)
- Gate source scan covers 3 of the cli's runtime file: workspace dependencies (scripts/check-core-subpath-exports.mjs:47) — already reported as R5-2 (comment 3937838491); author declined to fix at this head
- integration-tests files outside every npm workspace (integration-tests/tsconfig.json:63) — already reported as R1-6 (comment 3931108394), still open
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit lanes were skipped in CI; OS-specific portability of the new scripts tests verified by reading only.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": none — all checks above completed. (Note: the tail of the ToolGroup.tsx diff section past diff line 2701 belongs to the next chunk and was left to its agent.).
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
scripts/check-core-subpath-exports.mjs:47 — [review] R5-2: still stands (author declined to fix at this head) — the gate's source scan hardcodes 3 of the cli's runtime file: workspace dependencies
Convergence: round 6 posted 8 inline comment(s), 8 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: integration-tests/terminal-capture/skill-review-harness/text-capture.tsx (findings in round 5; 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. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit lanes were skipped in CI; OS-specific portability of the new scripts tests verified by reading only.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 4)"：none — all checks above completed. (Note: the tail of the ToolGroup.tsx diff section past diff line 2701 belongs to the next chunk and was left to its agent.)。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 6 轮发布了 8 条行内评论，其中 8 条是首次提出；上一轮发布了 5 条（其中 5 条首次提出）。发现反复回到同一批文件：integration-tests/terminal-capture/skill-review-harness/text-capture.tsx（第 5 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
… in tsconfig paths The ninth named exports entry was missing from the paths block, so `@qwen-code/qwen-code-core/conversationsRuntimeMarker` (imported by packages/cli/src/config/shared-env-keys.ts) fell through the wildcard to a nonexistent substitution and resolved against packages/core/dist — or failed with TS2307 on an unbuilt tree — the exact stale-dist mode this block's comment warns about. Add a sync gate asserting every named exports key whose target stem differs from the key has a paths entry pointing at the matching source file. Co-authored-by: Qwen-Coder <qwen-coder@alibaba-cloud.com> Patrol-Run: qwen-pr-closeout/jmtntktgsk8
…ore exports The harness loader sync gate only checked key coverage (exports key -> loader entry) and file existence, so two drifts passed it green: a retargeted exports entry left the loader serving the old module (the named map short-circuits ahead of the stem probe), and a stale loader-only entry short-circuits captures on a module graph the shipped package can no longer resolve. Assert each loader entry equals the exports import target mapped into source space, and every loader entry has a matching exports key. Mutating either direction now fails the suite. Co-authored-by: Qwen-Coder <qwen-coder@alibaba-cloud.com> Patrol-Run: qwen-pr-closeout/jmtntktgsk8
The rule verdicts by directory layer and never checks file existence, so the goalWire fixture reported one violation whether the exact exports key or the ./* wildcard resolved it — removing the exact-key branch of resolveExportTarget left the test green. Switch the fixture to transcriptRecords, whose exact resolution lands inside utils/ (allowed) while the wildcard resolution lands outside (violation), so dropping the exact-key branch now flips the assertion from 0 to 1. Co-authored-by: Qwen-Coder <qwen-coder@alibaba-cloud.com> Patrol-Run: qwen-pr-closeout/jmtntktgsk8
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — the four changed files sit outside every npm workspace; the scoped harness built and tested zero suites (their CI lanes are test:scripts and typecheck:integration).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit lanes were skipped in CI; OS-specific portability of the new scripts tests verified by reading only.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
scripts/tests/integration-tsconfig-core-paths-sync.test.js:50 (+1 locations) — [review] D7-1 the new paths sync gate pins only the named-exports-key tier; the load-bearing wildcard entry (integration-tests/tsconfig.json:66) is asserted by n…scripts/tests/integration-tsconfig-core-paths-sync.test.js:37 — [review] D7-2 the two new sync gates paste the same core-exports interpretation verbatim (namedKeys filter + dist/src stem derivation); extract a shared helper per the workflow…
中文说明
未审查（原文为英文）：build-and-test — the four changed files sit outside every npm workspace; the scoped harness built and tested zero suites (their CI lanes are test:scripts and typecheck:integration).
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit lanes were skipped in CI; OS-specific portability of the new scripts tests verified by reading only.
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
packages/cli/vitest.config.ts named the cli tsconfig `paths` block as the trigger for adding a named core subpath alias, but only three of the nine entries (noFollowOpen, subSessionConstants, transcriptRecords) appear there; the source of truth is the named keys of the `exports` map in packages/core/package.json. The comment now says so, and records that this map — unlike the skill-review-harness loader's, which scripts/tests/text-capture-core-loader-sync.test.js checks against core's exports — has no gate. StandaloneSessionPicker.test.tsx's wiring test claimed to pin the wrapper's import, but StandaloneSessionPicker.tsx is not in that suite's module graph: the tests render SessionPicker, which takes `currentBranch` as a prop, so the stub's only consumer is the test file's own import. Renamed the test and corrected both comments to state what it pins and what it does not cover. Comment and test-name only; no behavior change. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmto4amd1ku
One conflict, in integration-tests/concurrent-runner/export-html-from-chatrecord-jsonl.js. main removed the legacy-exported-JSONL render path (it now throws and asks for source ChatRecord JSONL); this branch still carried buildLegacySessionData plus the let/else shape around it. This branch's only change to that file was a prettier reflow of the buildProductSessionData call, which main's version already contains, so taking main's side drops nothing this PR wanted and keeps main's safety decision. Verified: no conflict markers left, node --check clean, no remaining buildLegacySessionData reference anywhere in the tree, packages/webui absent from the merged index (main retired it in #9812), and "tsc -p integration-tests/tsconfig.json" reports no error in the resolved file. The tsc errors that do appear are unbuilt-workspace phantoms (@qwen-code/acp-bridge/* subpaths, generated/git-commit.js, generated web-templates modules) which need npm run build rather than merge repair; the full typecheck lane runs on push. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmto8kxx1l2
One overlapping file: packages/cli/src/ui/components/agent-view/AgentComposer.tsx. main (#10315) removed the isTerminalStatus() call site, so the merged file keeps this branch's subpath import split (AgentStatus from agents/runtime/agent-types.js, ApprovalMode/APPROVAL_MODES from config/approval-mode.js) and drops the now-unused isTerminalStatus import. package.json and ci.yml auto-merged with only this branch's own additions (check:core-subpath-exports script and its CI step). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtof0f82lb
|
Merge conflict cleared —
The other two files both sides touched ( Verification: no conflict markers repo-wide; CI attribution for the previous head |
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.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- D8-23 StandaloneSessionPicker.test.tsx added mock-wiring block — already reported as R6-6 (comment 3939170878)
- D8-24 scripts/dev.js named-subpath fall-through to stale dist — already reported (issue-level triage comment 5529582959, non-blocking follow-up)
- D8-25 no-core-root-barrel-import.js CORE_BARREL_SPECIFIERS /dist/src/index.js spelling — already reported as D5-3 (round-5 deferral), tracked under open thread R1-7 (comment 3931108340)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit lanes were skipped in CI; OS-specific portability of the new scripts tests verified by reading only.
Not reviewed: finding D8-29 — the verifier never ruled on it: the reverse-audit loop stopped on the review time budget before a further verification round could be built, so it is reported terminal-only at low confidence.
Not explored to full depth (tool budget reached): chunk 5: none — but for transparency, two checks I deliberately did not run because they are outside my chunk's authorship and were settled by precedent instead: npm ru…; "agent reverse-audit (round 1)": bundle *evaluation* (module-init-order / TDZ after the reorder) was not completed — my ad-hoc esbuild invocation cannot reproduce the repo's banner + per-file….
Not reviewed: reverse audit — stopped before round 5 by the review time budget.
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
.github/workflows/ci.yml:1215 — [review] New gate cold-rebuilds packages/core a second time in the…packages/cli/src/ui/components/StandaloneSessionPicker.test.tsx:24 — [probe] Both factual claims in the added scope note are measurably…scripts/check-core-subpath-exports.mjs:18 — [probe] Gate's fileoverview and failure text claim a published-CLI…scripts/tests/check-core-subpath-exports.test.js:120 — [probe] Fixture never exercises the glob's tsx or recursion arms,…scripts/check-core-subpath-exports.mjs:49 — [probe] A renamed scan root silently degrades the gate to its four…scripts/tests/check-core-subpath-exports.test.js:123 — [probe] Untokenized harvest counts test-file mock strings and…eslint-rules/no-core-utils-upward-import.js:70 — [probe] Condition tier reads only import ; a default -only key…scripts/tests/text-capture-core-loader-sync.test.js:59 — [probe] Sync gate never pins the loader's base URL, whose trailing…scripts/check-core-subpath-exports.mjs:75 — [probe] Containment prefix is not canonicalized, so a…packages/cli/src/ui/utils/modelsBySource.ts:11 — [probe] The converted modules' own colocated tests still…packages/cli/vitest.config.ts:31 — [probe] The reachability clause this diff adds is false for one of…scripts/check-core-subpath-exports.mjs:89 — [probe] existsSync passes for a directory, so a directory…scripts/tests/check-core-subpath-exports.test.js:82 — [probe] No fixture reaches the !existsSync branch, so its accurate…scripts/tests/dev.test.js:158 — [probe] The data: URL import escapes the node:fs mock, so…scripts/tests/integration-tsconfig-core-paths-sync.test.js:51 (+3 locations) — [probe] CLASS: both new sync gates assume every named exports key…scripts/tests/integration-tsconfig-core-paths-sync.test.js:40 (+1 locations) — [probe] CLASS: the tsconfig sync gate's assertions are incomplete…integration-tests/terminal-capture/skill-review-harness/text-capture.tsx:227 — [probe] The comment this diff adds names a command that cannot…scripts/check-core-subpath-exports.mjs:31 — [probe] Hardcoded probe contract reddens a required gate on a…scripts/check-core-subpath-exports.mjs:96 — [probe] Containment allows dist/ only while its own text recites…scripts/tests/no-core-utils-upward-import.test.js:32 — [probe] The tie-break test is written in the key order a…- …and 1 more (see the run report)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) unit lanes were skipped in CI; OS-specific portability of the new scripts tests verified by reading only.
未审查（原文为英文）：finding D8-29 — the verifier never ruled on it: the reverse-audit loop stopped on the review time budget before a further verification round could be built, so it is reported terminal-only at low confidence.
未探索到全部深度（达到工具调用预算）：chunk 5：none — but for transparency, two checks I deliberately did not run because they are outside my chunk's authorship and were settled by precedent instead: npm ru…；"agent reverse-audit (round 1)"：bundle *evaluation* (module-init-order / TDZ after the reorder) was not completed — my ad-hoc esbuild invocation cannot reproduce the repo's banner + per-file…。
未审查：反向审计——评审时间预算不足，未能开始第 5 轮。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 21 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Catch the branch up with main so CI validates the current tree against the current gates instead of a 27-commit-old checkout. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtosl3ualt
`Check core subpath exports resolve` was added to the lint_and_static job without being added to the pin that asserts that job's full-gated payload by name and order, so the guard failed with 19 received against 18 expected. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtosl3ualt
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.
12 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- integration-tsconfig-core-paths-sync.test.js gate key-filter coverage — root key :35-38, wildcard key :37, reverse direction :41-42 — already reported (round-8 CLASS deferrals at :40 and :51)
- .github/workflows/ci.yml:1215 duplicate cold core rebuild in the new gate step — already reported (round-8 deferral, same line)
- scripts/dev.js:96 named-subpath fall-through to stale dist — already reported (round-8 duplicate drop D8-24; issue-level triage comment 5529582959)
- StandaloneSessionPicker.test.tsx:94 mock-wiring block has no consumer in the suite graph — already reported as R6-6 (comment 3939170878)
- check-core-subpath-exports.mjs:18 fileoverview and CI-comment blast-radius claim — already reported (round-8 deferral, same line)
- check-core-subpath-exports.mjs:48 untokenized specifier harvest — already reported (round-8 deferral)
- check-core-subpath-exports.mjs:89-95 unreached !existsSync branch — already reported (round-8 deferral at check-core-subpath-exports.test.js:82)
- check-core-subpath-exports.test.js:120-123 fixture is .ts-only so the harvest glob's .tsx arm is unpinned — already reported (round-8 deferral, same line)
- dev.test.js:172-174 data: URL loader import escapes the node:fs mock — already reported (round-8 deferral at dev.test.js:158)
- text-capture.tsx:260 stem-probe fallback executed by no test — already reported and settled as R4-2 (comment 3933497731)
- text-capture.tsx:227-228 added comment names a command that does not exercise the loader — already reported (round-8 deferral at :227)
- no-core-root-barrel-import.js CORE_BARREL_SPECIFIERS omits the dist/src/index.js spelling — already reported as D5-3, tracked under open thread R1-7 (comment 3931108340)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) is skipped in CI at this commit and its suite did not run locally, while this PR changes integration-tests/tsconfig.json and integration-tests/terminal-capture/skill-review-harness/text-capture.tsx; only npm run typecheck:integration was executed here (clean).
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) are merge_group/schedule/workflow_dispatch-gated and report skipped, so the new scripts tests were never executed on those hosts; their portability was verified by reading plus targeted POSIX probes (symlinked TMPDIR runs, pathToFileURL separator semantics, fs.globSync brace expansion, junction analysis, win32 exclude-list checks) rather than by a run on either lane.
Not reviewed: build-and-test — the test-efficacy probe established no mutation coverage for this diff: harnessValidated is null because no probe file was green in the unmutated baseline (skippedForBaseline: 2), and 111 hunks were never reverse-applied (cap of 6), so no claim is made that the PR's tests fail when its code is broken.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": tsc -p integration-tests/tsconfig.json was not executed, so the tsconfig comment's claim that nodenext substitutes .js for .ts on a *paths-substituted* ta…; "agent reverse-audit (round 4)": the packages/cli vitest suite was not run, so the mock-bypass layer above is established by static cross-referencing of mock factories against post-migration …; "agent reverse-audit (round 3)": Windows-lane confirmation that fileURLToPath(import.meta.resolve(…)) and path.join(root, 'packages', 'core', 'dist') + path.sep agree on drive-letter casing…; "agent 8b": I did not mutation-test each of the 22 root-mocking suites individually (only the three relocated-mock suites); their clearance rests on the static symbol-inter…; "agent 8b": the static closure follows only relative specifiers inside packages/cli/src ; modules reached through other workspace packages ( acp-bridge , sdk-typescript ,…, and 1 more.
Not linted (tool limitation, not a blocker): .github/workflows/ci.yml — actionlint embedded-shell source mapping is not yet supported.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/cli/vitest.config.ts:82 — [probe] Wildcard alias capture also swallows the non-src core spellings the exports map blesses (package.json, src/*, dist/*)packages/cli/vitest.config.ts:89 — [probe] RegExp alias form is invisible to the build guard's text-scraping alias set (real function: 35 -> 34, losing the core root)scripts/check-core-subpath-exports.mjs:37 — [probe] CLASS: the gate's checked set cannot reach any exports entry no harvested specifier names — measured on the root "." entry and on "./dist/*"scripts/tests/integration-tsconfig-core-paths-sync.test.js:50 — [probe] Sync gate's mirroring exemption skips exactly the class the tsconfig wildcard cannot serve (all 9 named keys are extensionless)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 12 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) is skipped in CI at this commit and its suite did not run locally, while this PR changes integration-tests/tsconfig.json and integration-tests/terminal-capture/skill-review-harness/text-capture.tsx; only npm run typecheck:integration was executed here (clean).
未审查（原文为英文）：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) are merge_group/schedule/workflow_dispatch-gated and report skipped, so the new scripts tests were never executed on those hosts; their portability was verified by reading plus targeted POSIX probes (symlinked TMPDIR runs, pathToFileURL separator semantics, fs.globSync brace expansion, junction analysis, win32 exclude-list checks) rather than by a run on either lane.
未审查（原文为英文）：build-and-test — the test-efficacy probe established no mutation coverage for this diff: harnessValidated is null because no probe file was green in the unmutated baseline (skippedForBaseline: 2), and 111 hunks were never reverse-applied (cap of 6), so no claim is made that the PR's tests fail when its code is broken.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 4)"：tsc -p integration-tests/tsconfig.json was not executed, so the tsconfig comment's claim that nodenext substitutes .js for .ts on a *paths-substituted* ta…；"agent reverse-audit (round 4)"：the packages/cli vitest suite was not run, so the mock-bypass layer above is established by static cross-referencing of mock factories against post-migration …；"agent reverse-audit (round 3)"：Windows-lane confirmation that fileURLToPath(import.meta.resolve(…)) and path.join(root, 'packages', 'core', 'dist') + path.sep agree on drive-letter casing…；"agent 8b"：I did not mutation-test each of the 22 root-mocking suites individually (only the three relocated-mock suites); their clearance rests on the static symbol-inter…；"agent 8b"：the static closure follows only relative specifiers inside packages/cli/src ; modules reached through other workspace packages ( acp-bridge , sdk-typescript ,…，另有 1 条。
未检查（工具限制，非阻断）：.github/workflows/ci.yml——actionlint 对 workflow 内嵌 shell 的源映射尚未支持。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 206 passed · 3 failed · 209 total Flakiness gate: ✅ 10 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：206 通过 · 3 失败 · 209 总计 抖动门：✅ 10 changed test file(s) x 5 identical rounds, no divergence Verification report (report.md, truncated)Flakiness gate logEvidence imagesHarness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
⏸️ Deferring to @chiga0 — needs a human call on this one. Triage ran all three stages on Three things I could not resolve from the diff, the tests, or the description:
Assigned to @chiga0 so this lands in your filter. You reviewed at 中文说明⏸️ 上交 @chiga0 —— 这件事需要人来定。 Triage 在 三点我无法从 diff、测试或描述中解决：
已指派给 @chiga0，让它进入你的过滤器。你在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed at head e82b6716 (delta since the round-9 zero-findings sha cdabd7dad3 is two mechanical main-merges; no PR-specific commits). Every historical Critical is verified resolved at this head, and my Critical-only pass — focused on the import-mechanism impact across the cli / daemon / standalone surfaces — finds no blocking defect.
Historical blockers — status at this head:
- R8-1 (the last standing Critical, round 8): the new
Check core subpath exports resolvestep inci.ymlis now registered in the pinnedlint_and_staticstep list — verified:cdabd7dad3adds the exact step name toscripts/tests/ci-platform-lanes.test.js:474, and round 9 re-reviewed at that sha with zero findings. The merge delta afterwards touches no PR-owned logic. - Rounds 1–2 Criticals (half-migrated mock pairs, dev.js mode): resolved and confirmed closed by the intervening approvals/rounds; the current suite carries the mock-wiring blocks and the dev-loader sync tests.
Impact assessment (cli / daemon / standalone), verified at this head:
- Runtime bundle: the strongest hazard of subpath imports — a specifier that resolves in source/vitest but not against the published dist layout, leaving every suite green while
qwenbreaks at startup — is gated by the newscripts/check-core-subpath-exports.mjsCI step, which buildspackages/coreand validates every core subpath used by the CLI against the built package. The gate and its 245-line test suite are in this diff. - Daemon / serve / standalone-session: these live inside
packages/cliand receive the same mechanical rewrite; their behavior lanes (Serve A/B,Real daemon E2E,Integration Tests (no-AK)) are running at this head with no failures recorded so far. - Dev mode and integration typecheck:
scripts/dev.js's subpath→source loader map andintegration-tests/tsconfig.jsonpaths are each pinned to core's exports map by dedicated sync tests (text-capture-core-loader-sync.test.js,integration-tsconfig-core-paths-sync.test.js,dev.test.js), so the three surfaces cannot drift silently. - Specifier hygiene: the strengthened eslint rules (
no-core-root-barrel-import,no-core-utils-upward-import) keep root-barrel and subpath spellings from mixing, which is what protects module-instance identity in the bundle.
CI at this head: no failing or cancelled checks at review time; Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK), Serve A/B, Real daemon E2E / Java 11 and the Java matrix legs are still pending, which does not gate this review per policy.
6 participants
Carries the whole change: the resolver mapping, the import migration that survived a full test run, two mock moves, and the two resolution fixes the integration gate turned out to need. Supersedes #10946 and #10956, both closed.
What this PR does
Why it's needed
Importing from the package root evaluates core's whole export graph — a bit over six hundred modules — however little of it a file uses. The cost shows up as collection rather than assertions: the cli workspace reported 2223s collecting against 1372s running tests on the release lane, and 84 minutes on its own on the main lane with collection at roughly three times the test time.
Sharding cannot reach this. The three release shards finish within a minute of each other, so the split is balanced and adding shards only divides a fixed per-file cost — one that grows with every test file, and the suite has grown 87% in ten weeks.
Measurements and the wider plan are in #10908.
What the first full run changed
This started as four stacked PRs. CI here runs on pull requests against
mainandrelease/**only, so the three stacked ones were reporting seven or eight passing checks that were just the TUI gates and the bot jobs — no unit suite, no lint. Retargeting this one atmainran the suite for the first time and it failed: 16 files, 127 tests, against 1 file and 2 tests on a jsdom PR sharing the same base.Every failure was a stub that stopped intercepting. Three mechanisms, only one of which the analysis behind the original batches modelled:
vi.spyOn(cliCore, 'getMCPServerPrompts')does nothing for a module that imported the function directly;Six of the sixteen install their stub in a way the analysis could not identify at all. So rather than sharpen a heuristic that was wrong in a way local reading would never catch, every migrated module any failing suite reaches was restored — 146 of them — leaving the 110 here. Some of those 146 are certainly fine; separating them belongs on top of a green run.
The integration gate then failed to compile:
Cannot find module '@qwen-code/qwen-code-core/utils/debugLogger.js'. Only packages/cli maps these specifiers. The integration suite lists the eight named subpaths and no wildcard, and the package's exports map covers those same eight plus the dist and src trees — so anything resolving the ordinary way, that suite and any consumer of the published package alike, could not name a core module. Both gaps are closed here, and Serve A/B, which was failing for the same reason, went green with them.Reviewer Test Plan
How to verify
Green across the unit suite, lint, the integration gate and Serve A/B — all of which were red at some point in this PR's history and each of which caught something real.
Three things worth a reviewer's judgement rather than a check:
./*maps a bare module path onto the build output. It exposes nothing new;./dist/*already reaches the same files.Evidence (Before & After)
N/A — no user-visible behavior changes. The shipped CLI is a single bundle and never resolves these specifiers at runtime.
Tested on
Via CI.
Risk & Scope
Linked Issues
Refs #10908
中文说明
本 PR 承载全部改动：解析映射、经完整测试验证后保留下来的导入迁移、两处 mock 搬迁，以及集成门禁暴露出的两处解析修复。取代 #10946 与 #10956（均已关闭）。
这个 PR 做了什么
为什么需要
从包根导入会求值 core 的整个导出图（六百多个模块），无论调用方实际用到多少。代价体现在模块收集而非断言：release lane 上 cli 收集耗时 2223s、跑测试 1372s；main lane 上 cli 单独就要 84 分钟，收集约为测试时间的三倍。
分片解决不了：三个 release 分片耗时相差不到一分钟，说明切分已均衡，加分片只是摊薄一个固定的单文件成本——而它随每个新增测试文件增长，套件十周内长了 87%。
数据与整体方案见 #10908。
第一次完整运行改变了什么
本改动最初是四个叠加 PR。此仓库 CI 只对 base 为
main和release/**的 PR 触发，因此那三个叠加 PR 显示的七八项通过只是 TUI 门禁和机器人任务——没有单元测试，没有 lint。把本 PR 改到main后套件第一次真正运行，结果失败：16 个文件、127 个测试；而共享同一基线的 jsdom PR 只挂 1 个文件 2 个测试。所有失败都是「stub 不再拦截」，涉及三种机制，而原批次背后的分析只建模了其中一种：打在命名空间导入上的 spy（
vi.spyOn(cliCore, 'getMCPServerPrompts')对直接导入该函数的模块无效）；断言 debug logger 被调用、而代码拿到真实实现；期望 stub 的 git 输出、却拿到真实输出。十六个用例中有六个，其 stub 安装方式该分析完全无法识别。与其继续打磨一个「本地读代码根本发现不了其错误」的启发式，不如回退所有被失败用例触达的已迁移模块（146 个），保留此处的 110 个。那 146 个中必然有一些本无问题，但把它们摘出来应建立在一次绿色运行之上。
随后集成门禁编译失败：
Cannot find module '@qwen-code/qwen-code-core/utils/debugLogger.js'。只有 packages/cli 映射了这类说明符；集成用例只列了八个具名 subpath 而无通配，包的 exports 也只覆盖那八个加 dist 与 src 两棵树——因此任何按常规方式解析的一方，无论是该用例还是已发布包的消费者，都无法指名 core 的某个模块。两处缺口在此补齐；因同一原因失败的 Serve A/B 也随之转绿。风险与范围