refactor(cli): enforce utils leaf-layer dependency direction (#9146) - #9737
Merged
Conversation
Move domain-coupled modules out of packages/cli/src/utils into the directories that own them: config/ (dialogScopeUtils, settingsUtils), i18n/ (languageUtils), ui/ (handleAutoUpdate, standalone-update, systemInfo, systemInfoFields, update-relaunch, commands, doctorChecks), nonInteractive/ (nonInteractiveHelpers, chat-recording-failure, tool-result-boundary-diagnostics, permission-suggestions), serve/ (sandbox), services/housekeeping/ (scheduler, non-interactive-scheduler), and commands/review/ (findings). Extract the generic normalizePartList helper into utils/normalize-part-list.ts so utils consumers keep importing downward, and move the MergeStrategy enum into utils/deepMerge.ts (its owner). Add an eslint architecture rule (no-utils-upward-import) that forbids value imports from utils/ back up into a domain directory. Type-only imports stay exempt: they are erased at compile time and cannot create a runtime cycle (Settings in modelConfigUtils, CommandContext in sessionPaths). No behavior change: typecheck, build, and the affected unit tests pass.
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
yiliang114
enabled auto-merge
August 22, 2026 15:46
Collaborator
|
Re-run after the review round (head moved from Template: all substantive sections present — summary/motivation, reviewer test plan with commands and outputs, before/after evidence, tested-on table, risk & scope, linked issue, and the bilingual body. Headings are renamed slightly from the template ("Summary" / "What this changes"); noted, not blocking. ✓ Problem: observed and measured, not theoretical. #9146 measured 43 upward value imports out of Direction: aligned. Size (current head): 1083 changed lines across 127 files → 512 production logic lines (83 files) + 571 test lines (44 files). The production churn is dominated by pure Approach: scope remains right — move each domain-coupled module into the directory that owns it, rewrite the importing sites, and enforce with a small custom ESLint rule rather than a boundaries-plugin dependency. Since the first pass the branch merged Risk: Moving on to code review. 🔍 中文说明评审轮之后的重跑（head 从 模板：所有实质性章节齐全——概述/动机、带命令和输出的评审测试计划、before/after 证据、测试环境表、风险与范围、关联 issue、双语正文。标题命名与模板略有不同，记录但不阻塞。✓ 问题：已观测并量化，非理论性问题。#9146 测量出 方向：对齐。 规模（当前 head）：127 个文件共 1083 行变更 → 512 行生产逻辑（83 个文件）+ 571 行测试（44 个文件）。生产变更的主体是纯 方案：范围仍然合理——把每个域耦合模块移到归属目录、重写导入方、用小而自定义的 ESLint 规则强制约束。首次评审之后，分支合并了 风险： 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewRe-run focus: the branch moved from Review-round fixes — all four verified at this head:
Post-merge fix commits — checked: the stale One non-blocking nit from the first pass (stale One open question, not blocking: under the repo's Module moves map (17 modules + 2 adjacent changes)
All paths relative to Test evidence (this PR's own CI, via API — PR code never executed here)The full suite was already green on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified (and why it doesn't block): the author's reported local runs ( 中文说明代码审查重跑重点：分支从 评审轮修复——四项全部在本 head 验证：
合并后修复提交——已检查： 过期的 首轮的一个非阻塞小问题（web-shell 中过期路径注释）已由 一个待确认的疑问，不阻塞： 在仓库的 测试证据（此 PR 自身的 CI，经 API 获取——此处从未执行 PR 代码）完整套件在 未验证（及为何不阻塞）：作者报告的本地运行结果（ — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean architecture work that came back from the review round with all four findings genuinely fixed; the only reservations are CI still in flight on the reviewed commit and one semantic question the maintainer can settle in two minutes. Stepping back: the gate should wave this through, and now it can. The problem was measured, not hypothesized (#9146's 43-import baseline); the fix is still the minimum that solves it — moves, specifier rewrites, and the rule that keeps the fix from rotting. What changed since the first pass is that the PR survived its own review: the Critical was real (source-mode macOS sandbox startup would have thrown The honest reservation: the inline type-specifier exemption rests on "erased at compile time", and under this repo's Verdict: approve — with CI still running on the reviewed commit, the approval is deferred: it lands automatically once every check on 中文说明置信度：4/5 —— 干净的架构工作，评审轮的四个发现全部得到真实修复后回归；仅有的保留意见是被评审提交上的 CI 仍在运行，以及一个维护者两分钟即可确认的语义疑问。 退一步看：门禁应当放行这个 PR，现在可以了。问题是测量出来的，不是假设（#9146 的 43 个导入基线）；修复仍然是解决问题所需的最小集——移动、导入路径重写，以及防止修复腐化的规则。与首轮相比的变化是这个 PR 经受住了自己的评审：Critical 是真实的（重命名后源码模式的 macOS 沙箱启动会抛出 诚实的保留意见：内联类型说明符豁免建立在"编译期擦除"之上，而在仓库的 结论：批准 —— 由于被评审提交上的 CI 仍在运行，批准暂缓：当该提交上的所有检查全绿时自动落地；若有红色结果或 head 移动，则不予批准。批准落地时也将取代本 bot 在 19:16 评审轮中提出的 changes-requested 评审——其四个发现已在本 head 全部验证修复。 — Qwen Code · qwen3.8-max Reviewed at |
yiliang114
requested review from
qwen-code-review-bot
and
a lite review from Copilot
August 22, 2026 15:57
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the packages/cli dependency graph to make src/utils/ a true leaf layer (no runtime imports upward into domain directories) and adds mechanical enforcement via a custom ESLint rule, alongside file moves/import rewrites to match ownership.
Changes:
- Adds
architecture/no-utils-upward-importESLint rule + targeted tests to preventutils/**upward runtime imports. - Moves domain-coupled helpers out of
utils/into owning directories and rewrites imports throughout CLI, UI, serve, nonInteractive, and review command code. - Extracts/centralizes generic helpers (
normalizePartList) and relocatesMergeStrategytoutils/deepMerge.ts.
Reviewed changes
Copilot reviewed 104 out of 107 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/tests/no-utils-upward-import.test.js | Unit-tests custom ESLint rule logic. |
| scripts/tests/no-utils-upward-import-config.test.js | Verifies rule wiring in flat ESLint config. |
| packages/cli/tsconfig.json | Updates test include path after moves. |
| packages/cli/src/utils/normalize-part-list.ts | Adds shared normalizePartList helper. |
| packages/cli/src/utils/normalize-part-list.test.ts | Adds unit tests for normalizePartList. |
| packages/cli/src/utils/midTurnUserMessage.ts | Switches to new normalize-part-list helper. |
| packages/cli/src/utils/deepMerge.ts | Moves MergeStrategy enum into utils owner. |
| packages/cli/src/utils/deepMerge.test.ts | Updates imports for relocated MergeStrategy. |
| packages/cli/src/ui/utils/commandUtils.ts | Repoints command parsing imports to UI-owned module. |
| packages/cli/src/ui/update-relaunch.ts | Adjusts imports/dynamic imports after moves. |
| packages/cli/src/ui/update-relaunch.test.ts | Updates mocks/paths after update module moves. |
| packages/cli/src/ui/systemInfoFields.ts | Adds/moves system info field definitions into UI. |
| packages/cli/src/ui/systemInfoFields.test.ts | Adds tests for system info field ordering/formatting. |
| packages/cli/src/ui/systemInfo.ts | Updates CommandContext/version imports after moves. |
| packages/cli/src/ui/systemInfo.test.ts | Updates mocks/imports after systemInfo move. |
| packages/cli/src/ui/standalone-update.ts | Repoints helper imports to utils after move. |
| packages/cli/src/ui/standalone-update.test.ts | Updates source-path assertions after move. |
| packages/cli/src/ui/hooks/useProviderUpdates.test.ts | Updates settings utils mock path. |
| packages/cli/src/ui/hooks/useGeminiStream.ts | Uses extracted normalize-part-list helper. |
| packages/cli/src/ui/hooks/useCommandCompletion.tsx | Repoints command parsing imports into UI commands. |
| packages/cli/src/ui/hooks/slashCommandProcessor.ts | Repoints command parsing imports into UI commands. |
| packages/cli/src/ui/handleAutoUpdate.ts | Updates imports to new locations (types/utils). |
| packages/cli/src/ui/handleAutoUpdate.test.ts | Updates mocks/imports after move. |
| packages/cli/src/ui/components/ThemeDialog.tsx | Updates dialog scope utils import location. |
| packages/cli/src/ui/components/shared/ScopeSelector.tsx | Updates dialog scope utils import location. |
| packages/cli/src/ui/components/SettingsDialog.tsx | Updates settings/language/system info imports. |
| packages/cli/src/ui/components/SettingsDialog.test.tsx | Updates mocks/imports after moves. |
| packages/cli/src/ui/components/ApprovalModeDialog.tsx | Updates dialog scope utils import location. |
| packages/cli/src/ui/components/AboutBox.tsx | Updates system info imports after move. |
| packages/cli/src/ui/commands/update-command.ts | Updates dynamic import path for standalone update. |
| packages/cli/src/ui/commands/update-command.test.ts | Updates mocks after standalone update move. |
| packages/cli/src/ui/commands/languageCommand.ts | Moves language utilities import to i18n. |
| packages/cli/src/ui/commands/languageCommand.test.ts | Updates imports after language utils move. |
| packages/cli/src/ui/commands/doctorCommand.ts | Repoints doctor checks + standalone update import. |
| packages/cli/src/ui/commands/doctorCommand.test.ts | Updates mocks/imports after doctor checks move. |
| packages/cli/src/ui/commands/doctorChecks.ts | Moves doctor checks into UI commands ownership. |
| packages/cli/src/ui/commands/doctorChecks.test.ts | Updates imports/mocks after move. |
| packages/cli/src/ui/commands/config-command.ts | Updates settings utils import location. |
| packages/cli/src/ui/commands/commands.ts | Moves commands registry into UI commands folder. |
| packages/cli/src/ui/commands/commands.test.ts | Updates imports after commands move. |
| packages/cli/src/ui/commands/bugCommand.ts | Updates system info imports after move. |
| packages/cli/src/ui/commands/bugCommand.test.ts | Updates mocks/imports after move. |
| packages/cli/src/ui/commands/aboutCommand.ts | Updates system info import after move. |
| packages/cli/src/ui/commands/aboutCommand.test.ts | Updates mocks/imports after move. |
| packages/cli/src/ui/auth/useAuth.test.ts | Updates settings utils mock path. |
| packages/cli/src/ui/AppContainer.tsx | Repoints command parsing, update handler, and nonInteractive helpers. |
| packages/cli/src/ui/AppContainer.test.tsx | Updates mock path after update handler move. |
| packages/cli/src/startup/startup-prefetch.ts | Updates dynamic import paths after moves. |
| packages/cli/src/startup/startup-prefetch.test.ts | Updates mocks for moved modules. |
| packages/cli/src/services/housekeeping/scheduler.ts | Moves scheduler into services and reimports housekeeping utils. |
| packages/cli/src/services/housekeeping/scheduler.test.ts | Updates housekeeping imports after move. |
| packages/cli/src/services/housekeeping/non-interactive-scheduler.test.ts | Updates mocks/import paths after housekeeping move. |
| packages/cli/src/serve/sandbox.ts | Moves sandbox into serve and reimports shared utils. |
| packages/cli/src/serve/sandbox.test.ts | Updates imports after sandbox move. |
| packages/cli/src/serve/routes/workspace-settings.ts | Updates settings utils import location. |
| packages/cli/src/serve/routes/workspace-models.ts | Updates settings utils import location. |
| packages/cli/src/serve/daemon-status-provider.ts | Updates systemInfo import location after move. |
| packages/cli/src/nonInteractiveCliCommands.ts | Repoints command parsing imports. |
| packages/cli/src/nonInteractiveCli.ts | Repoints chat-recording + helper imports; uses extracted normalizePartList. |
| packages/cli/src/nonInteractive/tool-result-boundary-diagnostics.ts | Adjusts types import path within nonInteractive. |
| packages/cli/src/nonInteractive/tool-result-boundary-diagnostics.test.ts | Updates types imports after move. |
| packages/cli/src/nonInteractive/session.ts | Updates chat-recording import after move. |
| packages/cli/src/nonInteractive/permission-suggestions.ts | Adjusts types import path within nonInteractive. |
| packages/cli/src/nonInteractive/nonInteractiveHelpers.ts | Removes duplicated normalizePartList implementation. |
| packages/cli/src/nonInteractive/nonInteractiveHelpers.test.ts | Removes duplicated normalizePartList tests and updates type imports. |
| packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.ts | Updates diagnostics import after move. |
| packages/cli/src/nonInteractive/io/JsonOutputAdapter.ts | Updates diagnostics import after move. |
| packages/cli/src/nonInteractive/io/BaseJsonOutputAdapter.ts | Updates helper/diagnostics imports after move. |
| packages/cli/src/nonInteractive/control/controllers/permissionController.ts | Updates permission suggestions import after move. |
| packages/cli/src/nonInteractive/chat-recording-failure.ts | Moves chat recording failure helper into nonInteractive and updates imports. |
| packages/cli/src/nonInteractive/chat-recording-failure.test.ts | Updates mocks/imports after move. |
| packages/cli/src/i18n/languageUtils.ts | Moves language utils into i18n ownership (self-relative imports). |
| packages/cli/src/i18n/languageUtils.test.ts | Updates mocks/imports after move. |
| packages/cli/src/gemini.tsx | Repoints sandbox/language/update/housekeeping imports after moves. |
| packages/cli/src/gemini.test.tsx | Updates mocks/imports for moved modules. |
| packages/cli/src/dualOutput/DualOutputBridge.ts | Updates chat-recording-failure import after move. |
| packages/cli/src/config/settingsWatcher.ts | Updates settings utils import location. |
| packages/cli/src/config/settingsUtils.ts | Normalizes config-local imports + i18n language util import. |
| packages/cli/src/config/settingsUtils.test.ts | Updates mocks/imports after settingsSchema path change. |
| packages/cli/src/config/settingsSchema.ts | Imports MergeStrategy from new owner (utils/deepMerge). |
| packages/cli/src/config/settingsSchema.test.ts | Updates MergeStrategy import after move. |
| packages/cli/src/config/settings.ts | Updates settings utils import; uses deepMerge-owned MergeStrategy type. |
| packages/cli/src/config/migration/versions/v3-to-v4.ts | Updates settings utils import after move. |
| packages/cli/src/config/migration/versions/v2-to-v3.ts | Updates settings utils import after move. |
| packages/cli/src/config/migration/versions/v1-to-v2.ts | Updates settings utils import after move. |
| packages/cli/src/config/loadedSettingsAdapter.ts | Updates settings utils import after move. |
| packages/cli/src/config/loadedSettingsAdapter.test.ts | Updates mocks/imports after move. |
| packages/cli/src/config/dialogScopeUtils.ts | Normalizes config-local imports after move. |
| packages/cli/src/commands/update.ts | Updates standalone update import after move. |
| packages/cli/src/commands/update.test.ts | Updates mocks/imports after move. |
| packages/cli/src/commands/review/save-artifact.ts | Repoints findings import into review domain. |
| packages/cli/src/commands/review/save-artifact.test.ts | Repoints findings import into review domain. |
| packages/cli/src/commands/review/publish-assets.ts | Repoints findings import into review domain. |
| packages/cli/src/commands/review/lib/shell-quote.ts | Updates comment reference to moved standalone-update file. |
| packages/cli/src/commands/review/lib/review-settings.test.ts | Updates settings utils import location. |
| packages/cli/src/commands/review/findings.ts | Moves findings module into review domain; adjusts imports. |
| packages/cli/src/commands/review/findings.test.ts | Adds extensive tests for findings contract/behavior. |
| packages/cli/src/commands/review/compose-review.ts | Repoints findings imports after move. |
| packages/cli/src/commands/review.ts | Repoints findings command import after move. |
| packages/cli/src/acp-integration/session/Session.ts | Repoints helper imports after moves (normalizePartList/diagnostics). |
| packages/cli/src/acp-integration/session/history-replay-page.ts | Updates diagnostics import after move. |
| packages/cli/src/acp-integration/session/history-replay-page.test.ts | Updates diagnostics mocks/import paths after move. |
| packages/cli/src/acp-integration/session/emitters/tool-call-emitter.ts | Updates diagnostics import after move. |
| packages/cli/src/acp-integration/acpAgent.ts | Updates diagnostics/language/housekeeping imports after moves. |
| packages/cli/src/acp-integration/acpAgent.test.ts | Updates mocks/imports after moves. |
| eslint.config.js | Registers rule in flat config for packages/cli/src/utils/**. |
| eslint-rules/no-utils-upward-import.js | Implements utils/ upward-import boundary rule. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
🩺 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 |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
🖼️ 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 |
Collaborator
Author
|
Closeout round on top of latest
|
#9677 recorded qwen-autofix.yml at 392111 bytes while the file it committed was already 397656, so every PR that merged main after it tripped the growth ratchet. Re-record the actual size; the file itself is unchanged by this PR.
…eaf-layer move (#9146) The #9146 move returned findings.ts to commands/review/, but the digest root lists merged from main still pinned it under utils/, where the file no longer exists — the absent root darkened every review's staleness check and failed review-source-digest.test.ts. Drop the stale file-shaped root from both digest copies and their pins; the commands/review/ directory root covers the validator at its new home, and the two utils helpers keep their file-shaped roots.
yiliang114
requested review from
qwen-code-review-bot
and removed request for
qwen-code-review-bot
August 22, 2026 17:50
Collaborator
|
No blocking findings. The module moves and import rewrites preserve the existing behavior, and the new ESLint rule covers static imports, re-exports, and statically resolvable dynamic imports escaping Non-blocking: dedicated |
doudouOUC
reviewed
Aug 22, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: coverage — no plan was given, so this run cannot show that any of the diff was read.
— qwen-code-review via Qwen Code /review (v0.21.10)
doudouOUC
reviewed
Aug 22, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
双模型代码审查结果 · Round 1
PR: #9737 refactor(cli): enforce utils leaf-layer dependency direction (#9146)
模型： deepseek-v4-flash
结论： 0 Critical / 0 Suggestion inline，仅 2 条非阻塞建议
汇总
13 个审查代理全部完成，未发现阻塞性问题。本次变更是干净的机械式架构重构：
- 43 个
utils/向上的值导入已消除，仅剩 2 处 type-only 引用。 - 17 个模块
git mv到归属目录，所有导入路径重写后可解析。 - 新 ESLint 规则
architecture/no-utils-upward-import已覆盖 type-only 豁免与动态导入检测。 normalizePartList正确去重提取，MergeStrategy正确归位。
非阻塞建议
- ESLint 规则中的路径规范化使用
nodePath.replaceAll('\\', '/')，可考虑补充 Windows 路径的显式测试。 - PR 描述中 Windows 测试状态标注为 "N/A"，但规则已验证可在 Windows 路径下工作，描述可更新为已测试状态。
该评论由 qwen review bot 自动生成。
This was referenced Aug 22, 2026
qwen-code-ci-bot
requested changes
Aug 22, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — ended at the 5-round cap with round 5 still reporting (no two consecutive dry rounds); cap-round findings were verified in the bounded tail.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/utils/deepMerge.test.ts — no such file or directory; src/config/settingsSchema.test.ts — no such file or directory; src/i18n/languageUtils.test.ts — no such file or directory; src/utils/normalize-part-list.test.ts — no such file or directory; utils/modelConfigUtils.ts — no such file or directory; and 3 more.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — ended at the 5-round cap with round 5 still reporting (no two consecutive dry rounds); cap-round findings were verified in the bounded tail。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/utils/deepMerge.test.ts — no such file or directory; src/config/settingsSchema.test.ts — no such file or directory; src/i18n/languageUtils.test.ts — no such file or directory; src/utils/normalize-part-list.test.ts — no such file or directory; utils/modelConfigUtils.ts — no such file or directory; and 3 more。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…9146) main updated settings.test.ts after this branch moved settingsUtils.ts from utils/ into config/, and the merge kept main's old import specifier, which vite fails to resolve. Repoint it at ./settingsUtils.js; every other consumer already uses the new path.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- reverse-audit candidate: stale
utils/settingsUtils.tsreference at docs/design/hot-reload/settings-change-detection.md:422 — already reported as R2-3 item (1) (comment 3837358257), re-posted under R2-3 - reverse-audit candidate: stale
utils/sandbox.tsreference at .qwen/skills/autofix/scripts/run-agent.mjs:224 — already reported as R2-3 item (2) (comment 3837358257), re-posted under R2-3
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 1b": none — no check was cut short. (I verified statically; I did not execute lint/test suites, which is outside the removed-behavior dimension.).
Test Plan (not a blocker): src/utils/deepMerge.test.ts — no such file or directory; src/config/settingsSchema.test.ts — no such file or directory; src/i18n/languageUtils.test.ts — no such file or directory; src/utils/normalize-part-list.test.ts — no such file or directory; utils/modelConfigUtils.ts — no such file or directory; and 3 more.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
scripts/tests/no-utils-upward-import.test.js:7 — [test] new rule tests in scripts/tests sit outside every workspace npm test lane
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 1b"：none — no check was cut short. (I verified statically; I did not execute lint/test suites, which is outside the removed-behavior dimension.)。
Test Plan（非阻断）：src/utils/deepMerge.test.ts — no such file or directory; src/config/settingsSchema.test.ts — no such file or directory; src/i18n/languageUtils.test.ts — no such file or directory; src/utils/normalize-part-list.test.ts — no such file or directory; utils/modelConfigUtils.ts — no such file or directory; and 3 more。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Collaborator
Author
|
@qwen-code /review |
Contributor
|
Qwen Code review request accepted. Review is queued in workflow run. |
Collaborator
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
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: reverse audit — ended at the 5-round cap with round 5 still reporting (no two consecutive dry rounds).
Not reviewed: "agent verify (round 3)" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/commands/review/lib/stale-bundle.test.ts:480 — [probe] shared makeStaleBundleFixture still materializes the removed utils/findings.ts rootscripts/tests/no-utils-upward-import.test.js:56 — [probe] zero-specifier side-effect import has no test pinscripts/tests/no-utils-upward-import-config.test.js:35 — [probe] export-side inline type specifier has no test pinscripts/tests/no-utils-upward-import.test.js:61 — [probe] re-export allowed side (intra-utils export from sibling) has no test pin
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — ended at the 5-round cap with round 5 still reporting (no two consecutive dry rounds)。
未审查："agent verify (round 3)"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Collaborator
Author
|
Addressed the two current-head review suggestions in additive commit 1445a76:
Verification: both rule suites pass (14/14), plus targeted ESLint, Prettier, and git diff --check. |
chiga0
approved these changes
Aug 23, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings.
Scope: reviewed the ESLint rule, its test suite, the .sb profile co-location, and the docs/comment sweep. File moves and import rewrites verified by green CI.
Checked:
no-utils-upward-import.js: correctly exempts only statement-levelimport type/export type; inline{ type X }specifiers are reported as value imports underverbatimModuleSyntax(fix commit21cfacf8). Computed dynamic imports fail closed when the statically known prefix is relative (fix commit764d603f). CLI baseUrl (src/-rooted) specifiers correctly resolved frompackages/cli/.- Seatbelt profiles:
sandbox-macos-*.sbfiles co-located withsandbox.tsinserve/(commit0dca7b09);resolveSeatbeltProfileFileresolves viaimport.meta.url→ same directory → resolution intact. - Docs sweep: all path references in design docs updated to post-move locations.
CI: Test (ubuntu-latest, Node 22.x) pass · Desktop Shell (ubuntu + windows) pass · web-shell E2E, Real daemon E2E, Java matrix all pass. Integration Tests (CLI, No Sandbox) SKIPPED — merge_group-only gate, standard for PR check.
Cross-check: prior CHANGES_REQUESTED items (R1: .sb stranding, R1-3: type-specifier exemption; R2-1: verbatimModuleSyntax inline; R2-4: computed dynamic imports) all addressed by commits on this head.
Reviewed with AI assistance.
doudouOUC
approved these changes
Aug 23, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Independent review at 1445a767 — both Criticals verified fixed; no findings; approving
Same gate note as its core-side sibling #9778: this is a large refactor touching packages/cli/src/** and crossing packages, which is what AGENTS.md's two-tier rule targets — but the rule exempts maintainer-authored PRs, and yiliang114 holds admin here, so this gets a normal review rather than an automatic block.
The two Criticals — verified at this head
Stranded seatbelt profiles. Moving sandbox.ts into serve/ left the six sandbox-macos-*.sb profiles behind in utils/, while resolveSeatbeltProfileFile resolves them relative to the module — so every sandbox-enabled macOS start from source or an unbundled dist/ would have thrown FatalSandboxError. Fixed by moving all six profiles beside sandbox.ts; the diff shows them as pure renames (+0/-0) into packages/cli/src/serve/, which is the right shape for a git mv. This was the one finding on either PR with real user-visible blast radius, and it is the kind only a path-aware reader catches — a .sb file has no import edge to follow.
The inline type-specifier exemption (R2-1). This is the sharp one. The rule had exempted import { type X } from / export { type X } from on the theory that type-only constructs erase — but under this repo's verbatimModuleSyntax: true, import { type X } from './b.js' emits import {} from './b.js', a surviving runtime edge, so the exemption blessed precisely the upward dependency the rule exists to forbid, and a test pinned that wrong behaviour. At this head the rule reports them and says why in place:
Inline type specifiers (
import { type X } from/export { type X } from) are reported instead: under this repo'sverbatimModuleSyntax, tsc keeps the …
with the same reasoning repeated at the check itself (no-utils-upward-import.js:140-147), and only statement-level importKind/exportKind exempted. Correct, and correctly explained — this file is now the reference wording, which is why I asked #9778 to mirror it.
What I checked independently
A mechanical import refactor's characteristic failure is a moved module leaving a stale vi.mock first argument behind: the mock silently stops intercepting, the real module runs, and the test still passes. So I resolved every relative vi.mock/vi.doMock specifier in every test file on the branch against the branch's own file list, baselined on main:
origin/main 1275 specs 13 unresolved
pr-9737 (1445a767) 1275 specs 13 unresolved -> no new unresolved specs, none moved
I also checked for tests orphaned from their source by the moves. One candidate surfaced — packages/cli/src/services/housekeeping/non-interactive-scheduler.test.ts with no non-interactive-scheduler.ts beside it — but it is pre-existing rather than introduced: on main the same file sits at utils/housekeeping/ with no such sibling either, and at this head it imports './scheduler.js', which the PR moves into the same directory. Relocated, not orphaned. Nothing to report.
The rule is wired rather than declared: eslint.config.js carries it (+13/-16 there, consolidating the plugin block), and the added process-env-guard.test.ts (+54) and sandbox.test.ts changes (+22/-7) pin the two behaviours the moves put at risk.
Where I land
Approving, with no findings of my own. Both Criticals had real teeth — one would have broken sandboxed macOS startup, the other would have left the new rule licensing the exact edge it forbids — and both are closed at this head with the reasoning recorded beside the code rather than only in a thread.
CI is green apart from review-pr, still running. reviewDecision still reads CHANGES_REQUESTED, from a bot review several commits behind this head.
Not verified by me: I did not run the suites (no node_modules in this worktree — CI covers them green), so the mock-resolution result is static rather than a test run, and I did not exercise sandbox-exec on macOS to confirm the profile lookup end to end — that path is judged from the resolver and the file locations.
TianYuan1024
added a commit
to TianYuan1024/qwen-code
that referenced
this pull request
Aug 23, 2026
One conflict, in the workspace-scope strip this PR had added to. `main` (QwenLM#9098, QwenLM#9737) generalised that strip into a single data list: WORKSPACE_RESTRICTED_SETTINGS in settingsUtils.ts now drives the strip, the "your workspace value was ignored" warning, and the settings dialog's scope filter, so the three surfaces cannot drift apart. This PR had hand-rolled the same thing for permissions.planMode — a second branch inside stripWorkspaceSecurityBypasses and a third copy of the warning text. Both are deleted in favour of main's version, and the setting is registered as one entry in the list instead. The behaviour is unchanged and the four scope tests added here still pass against the generic mechanism; the dialog filter comes along for free, though planMode sets showInDialog: false.
5 tasks
This was referenced Aug 26, 2026
Collaborator
|
Released in v0.22.2. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Makes packages/cli/src/utils/ a true leaf layer and enforces the boundary mechanically. This is the CLI slice of #9146.
This moves 17 domain-coupled modules out of utils/ into their owner directories: config, i18n, nonInteractive, serve, services/housekeeping, commands/review, ui/commands, and ui. All moves are git mv with import specifiers rewritten across the importing modules.
It also extracts the generic normalizePartList helper into utils/normalize-part-list.ts, moves MergeStrategy to its owner utils/deepMerge.ts, and adds the error-level ESLint rule architecture/no-utils-upward-import for relative imports escaping utils/ in production TS/TSX while allowing type-only constructs that are erased at compile time.
Why it's needed
Before this change, utils/ imported back into config, ui, i18n, nonInteractive, serve, and commands. #9146 measured 43 upward value imports across 20 files, which kept the CLI directory graph cyclic. After this change there are zero upward value imports out of utils/; the only two remaining upward references are import type, which cannot form a runtime cycle.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: 43 upward value imports out of utils/ at #9146's baseline. After: zero value imports; the remaining upward references are exactly two type-only imports: utils/modelConfigUtils.ts imports the Settings type, and utils/sessionPaths.ts imports the CommandContext type.
Tested on
Environment (optional)
Local Node workspace. Verified on the rebased branch on top of main @ c10143a with TypeScript, ESLint, focused Vitest suites, and npm run build.
Risk & Scope
Linked Issues
References #9146.
中文说明
本 PR 做了什么
让 packages/cli/src/utils/ 成为真正的叶层，并用机制强制约束边界。这是 #9146 的 CLI 部分。
本 PR 将 17 个域耦合模块从 utils/ 移到其归属目录：config、i18n、nonInteractive、serve、services/housekeeping、commands/review、ui/commands 和 ui。所有移动均使用 git mv，并重写相关导入路径。
此外，本 PR 将通用的 normalizePartList 提取到 utils/normalize-part-list.ts，将 MergeStrategy 移到其归属 utils/deepMerge.ts，并新增 error 级 ESLint 规则 architecture/no-utils-upward-import，禁止生产 TS/TSX 中逃出 utils/ 的相对运行时导入，同时允许编译期擦除的 type-only 构造。
为什么需要
变更前，utils/ 会反向导入 config、ui、i18n、nonInteractive、serve 和 commands。#9146 测得 20 个文件里有 43 个向上的值导入，使 CLI 目录图保持循环。变更后，utils/ 不再有向上的值导入；仅剩两处 import type，不会形成运行时循环。
评审测试计划
如何验证
证据（Before & After）
Before：#9146 基线测得 utils/ 有 43 个向上的值导入。After：零值导入；仅剩两处向上引用且均为 type-only：utils/modelConfigUtils.ts 导入 Settings 类型，utils/sessionPaths.ts 导入 CommandContext 类型。
测试环境
环境（可选）
本地 Node workspace。在基于 main @ c10143a 的 rebase 分支上用 TypeScript、ESLint、聚焦 Vitest suites 和 npm run build 验证。
风险与范围
关联 Issue
References #9146。