refactor(core): make utils/ a leaf layer - #9778
Merged
Merged
Conversation
yiliang114
requested review from
LaZzyMan,
doudouOUC,
tanzhenxin and
wenshao
as code owners
August 23, 2026 06:04
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: real and tracked — this is the core half of #9146, which quantifies the debt (107 upward imports from 51 files make the utils/ graph cyclic). The CLI half already landed via #9147, so this continues an established, maintainer-driven architecture initiative rather than introducing a new direction. (Minor: the description says "the CLI half already merged in #9737", but #9737 — the CLI lint enforcement follow-up — is still open; the merged CLI half was #9147.) Direction: aligned. utils/ as a dependency-free leaf with mechanical enforcement matches how this repo already guards its architecture ( Size: maintainer-authored, so the two-tier core gate doesn't apply. For the record: ~1,041 production-logic lines (additions + deletions, per the standard count — which includes 280 lines for the new eslint rule and its test) plus ~178 lines of test import updates across 126 files. The shape is mostly mechanical: 32 renames + 88 import-site updates, with only four small new leaf modules and the lint rule as genuinely new code. Approach: the scope feels right — relocate domain-coupled helpers to their owners, sink generic constants/types into utils, extract shared values ( Risk: Stage 1e matched three revert-correlated paths — Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实且有跟踪——这是 #9146 的 core 部分，该 issue 已量化了债务（51 个文件中的 107 个向上 import 使 utils/ 的依赖图成环）。CLI 部分已通过 #9147 合入，本 PR 是既定架构专项的延续，而非新方向。（小提醒：描述中写"CLI half already merged in #9737"，但 #9737（CLI 侧 lint 强制的后续 PR）仍是 open 状态；已合入的 CLI 部分是 #9147。） 方向：对齐。utils/ 作为无依赖叶子层并用机械化规则强制，与仓库现有的架构守护方式一致（main 上已有 规模：维护者本人提交，两级核心门禁不适用。记录备查：按标准口径约 1,041 行生产逻辑改动（含新 eslint 规则及其测试共 280 行），另有约 178 行测试 import 更新，共 126 个文件。主体是机械性改动：32 个重命名 + 88 处 import 更新，真正的新代码只有四个小叶模块和 lint 规则。 方案：范围合理——领域耦合的辅助函数移回属主模块，通用常量/类型下沉到 utils，共享值（ 风险：Stage 1e 命中三个与 revert 相关的路径—— 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
@yiliang114 Code review below. The refactor itself is in good shape — the part that blocks is CI, which fails on this commit. Code reviewIndependent baseline first: for "make utils/ a leaf" I would have done exactly what this PR does — relocate domain-coupled helpers to their owners, sink generic constants/types into utils, extract shared values as small leaf modules re-exported from the original homes, and add an ESLint guard that permits type-only imports. The PR matches that proposal; I didn't find a simpler path it missed. What I verified by reading the diff against the tree:
Finding 1 — stale
I verified this against the source at the reviewed commit (e.g. Minor, non-blocking: Files changed126 files, but the shape is simple — mostly mechanical relocation: Files changed (14 groups of 126 shown)
CI evidence (PR's own checks, fetched via API — no PR code executed here)The Failing step excerpt from job 97151391358: Likely mechanism (hypothesis from reading, not from a build): the relocation changes esbuild's chunk grouping so that the static
Real-scenario testing: N/A — unattended CI run, and a pure refactor with nothing user-visible to drive. 中文说明重构本身形状很好，阻塞点在 CI：本提交上 代码审查：方案与独立推演一致（域耦合辅助函数移回属主、通用常量下沉 utils、共享值抽成叶模块并原地 re-export、新增 lint 守护）。已核实 barrel 公共表面完整保留（如 发现 1（需修复）：四个测试文件的 次要（不阻塞）： CI 证据： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 2/5 — the refactor's shape is right and most of the execution is clean, but the commit fails a required CI check that it itself broke, and the rename left four test files with dead mock paths. Stepping back: this PR does the right thing, the right way. The approach matches what I'd propose independently, the barrel surface is preserved, history is kept through renames, and the new lint rule turns the invariant from prose into a build failure — which is exactly how the last few architecture PRs in this repo landed. In six months this is a thank-them change, not a curse-them one. But I can't look past what actually happened on this commit:
Both are concrete and likely small fixes; once they land and CI is green this is in good shape to merge. Requesting changes on the two items rather than deferring, because neither needs a human judgment call — the evidence is in the CI log and the diff. @yiliang114 中文说明置信度：2/5 —— 重构方向正确、大部分执行干净，但本提交弄红了一个必需的 CI 检查，且重命名在四个测试文件里留下了失效的 mock 路径。 整体看：这个 PR 用正确的方式做了正确的事——方案与独立推演一致，barrel 表面保留，重命名保留历史，新 lint 规则把不变量从口头约定变成构建失败，这正是仓库近几次架构 PR 的落地方式。半年后这是让人感谢的改动。 但本提交上实际发生的事无法忽略：
两个问题都具体且大概率是小修复；修好并且 CI 转绿后，这个 PR 状态很好、可以合入。选择 request changes 而不是转交人工，因为这两项都不需要人为判断——证据就在 CI 日志和 diff 里。@yiliang114 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
previously requested changes
Aug 23, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Two concrete items before this can merge — full detail in my stage-2/stage-3 comments above:
-
Test (ubuntu-latest, Node 22.x)fails on this commit atnpm run check:serve-fast-path-bundle: the ACP static import closure now reaches iconv-lite's encoding tables (node_modules/iconv-lite/lib/bom-handling.jsvia a shared chunk statically imported from the acpAgent chunk). The same check is green on contemporaneous PRs #9776 and #9770 against the same main, so the relocation changed esbuild chunk grouping and pulled the staticutils/iconvHelper.tsiconv-lite import into the ACP closure. The failure also happens before the unit suite runs, so this commit has no unit-test signal yet. -
The
utils/tool-result-boundary-diagnostics.ts→tools/rename left thevi.mock(...)first argument on the deleted path in four test files (only thetypeof import()annotation was updated):agents/runtime/agent-core.test.ts,core/coreToolScheduler.test.ts,followup/speculation.test.ts,services/chatRecordingService.test.ts. Point them attools/tool-result-boundary-diagnostics.js.
The refactor itself is in good shape — surface preserved, rule well designed — so this should be a quick round. 🙏
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. |
doudouOUC
reviewed
Aug 23, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
PR head 已漂移（review 启动时 head 为 ，当前 head 为 ），原 inline 评论可能失效，仅保留汇总。
审查结论（deepseek-v4-flash）
发现 5 个 Critical 问题：
packages/core/src/agents/runtime/agent-core.test.ts:
Finding 1 (Critical) — stale vi.mock path. The vi.mock first argument still points at ../../utils/tool-result-boundary-diagnostics.js, but the file was renamed to ../../tools/tool-result-boundary-diagnostics.js. The typeof import() annotation inside the factory was correctly updated (line 63), but the outer mock path was not. This means the mock will never intercept the import — the real module runs instead. Change the first argument to '../../tools/tool-result-boundary-diagnostics.js'.
packages/core/src/core/coreToolScheduler.test.ts:
Finding 2 (Critical) — stale vi.mock path. Same pattern as agent-core.test.ts: the first argument is '../utils/tool-result-boundary-diagnostics.js' but the source file now imports from '../tools/tool-result-boundary-diagnostics.js'. The typeof import() annotation on line 152 is correct, but the outer mock path is stale. Fix to '../tools/tool-result-boundary-diagnostics.js'.
packages/core/src/followup/speculation.test.ts:
Finding 3 (Critical) — stale vi.mock path. Same pattern: vi.mock first argument is '../utils/tool-result-boundary-diagnostics.js' but the source now imports from '../tools/tool-result-boundary-diagnostics.js'. Fix to '../tools/tool-result-boundary-diagnostics.js'.
packages/core/src/services/chatRecordingService.test.ts:
Finding 4 (Critical) — stale vi.mock path. Same pattern: vi.mock first argument is '../utils/tool-result-boundary-diagnostics.js' but the source now imports from '../tools/tool-result-boundary-diagnostics.js'. Fix to '../tools/tool-result-boundary-diagnostics.js'.
integration-tests/globalSetup.ts:
Finding 5 (Critical) — stale deep import path. memory/const.ts was renamed to utils/memory-constants.ts, but this import still targets the old path @qwen-code/qwen-code-core/src/memory/const.js. This will break all integration tests at import time. Fix to @qwen-code/qwen-code-core/src/utils/memory-constants.js or import from the barrel export.
由 qwen-code PR 审查任务自动生成
doudouOUC
reviewed
Aug 23, 2026
yiliang114
enabled auto-merge
August 23, 2026 10:00
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): 6 passing — this review observed 20953, 1685, 23452, 1653, 495, 4159, 599 passed.
— qwen3.8-max via Qwen Code /review (v0.22.0)
Collaborator
Author
|
Addressed all seven round-1 suggestions in 9aecfff:
Verification: boundary rule 9/9; focused Core tests 190/190; Core typecheck; targeted ESLint; Prettier; and path-scoped test discovery all passed. |
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 (merge_group-only job) and its end-to-end suite did not run locally.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
eslint.config.js:242 — [probe] The rule's flat-config registration is untested — anchored on code unchanged since round 1 (code-age rule)eslint-rules/no-core-utils-upward-import.js:109 — [probe] The cross-package delegation branch has zero test coverage — anchored on code unchanged since round 1 (code-age rule)scripts/tests/no-core-utils-upward-import.test.js:30 — [probe] Side-effect, default, and namespace import shapes are unpinned — anchored on code unchanged since round 1 (code-age rule)
— qwen3.8-max via Qwen Code /review (v0.22.0)
Collaborator
Author
|
Addressed both round-2 current-head suggestions in additive commit d26fc42:
Verification: related architecture rule suites pass (24/24); the full Core utils directory passes ESLint; an exports-map sweep confirms transcriptRecords is allowed while all five named subpaths owned outside utils are rejected; targeted Prettier and diff checks pass. |
chiga0
previously 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 esbuild config fix, and CI results on the current head.
Checked:
no-core-utils-upward-import.js: readspackages/core/package.jsonat lint time to resolve named self-reference subpaths; handles@qwen-code/qwen-code-core/src/,/dist/, and./subpathexport map forms. Allowlist correctly carries the two deferred inversions (debugLogger.ts→config/storage+telemetry/trace-context). Statement-levelimport type/export typecorrectly exempted; inline{ type X }not checked (consistent with behavior intent documented in the rule header).esbuild.config.js: filter regex updated fromutils/sync-file-encoding→services/sync-file-encoding(commite385caeffixed the iconv-lite tree-shaking regression cited in the CHANGES_REQUESTED).- Mock path fixes:
tool-result-boundary-diagnosticsvi.mock(...)arguments corrected to post-move paths (commit90d102f6).
CI (current head d26fc42b): Test (ubuntu-latest, Node 22.x) SUCCESS (25 min) · Desktop Shell (ubuntu + windows) pass · web-shell E2E, Real daemon E2E, Java matrix, Post Coverage Comment all pass. Integration Tests (CLI, No Sandbox) SKIPPED — merge_group-only.
Cross-check: prior CHANGES_REQUESTED items (iconv-lite bundle failure, mock path stale refs) addressed in commits e385caef and 90d102f6.
Reviewed with AI assistance.
doudouOUC
previously 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 d26fc42b — my five earlier Criticals are fixed; two small findings; approving
First, the gate question, because this is a large refactor touching packages/core/src/**: the AGENTS.md two-tier rule hard-blocks large-scope core refactors, but exempts maintainer-authored PRs. yiliang114 holds admin on this repo, so the exemption applies and this gets a normal review rather than an automatic block. Worth stating explicitly since the diff shape (100+ files, refactor(core)) is exactly what that rule targets.
My five earlier Criticals — all fixed
I reported five at 90d102f6: four stale vi.mock first arguments left pointing at the pre-move path for tool-result-boundary-diagnostics, and a stale deep import in integration-tests/globalSetup.ts. At this head:
agent-core.test.ts:60mocks'../../tools/tool-result-boundary-diagnostics.js',coreToolScheduler.test.ts:149mocks'../tools/…', and the module is atpackages/core/src/tools/tool-result-boundary-diagnostics.ts. Both resolve. The CLI's own copy atpackages/cli/src/utils/…still backshistory-replay-page.test.ts, correctly.globalSetup.tsno longer deep-imports at all —DEFAULT_CONTEXT_FILENAMEcomes from the@qwen-code/qwen-code-corebarrel, which was the second remedy I offered.
Rather than spot-check the five, I checked the whole class, since a moved module strands mocks in tests the refactor never touched and a stale first argument fails silently — the real module runs and the test still passes. I resolved every relative vi.mock/vi.doMock specifier in every test file on the branch against the branch's own file list, with main as the baseline:
origin/main 1275 specs 13 unresolved
pr-9778 (d26fc42b) 1275 specs 13 unresolved -> no new unresolved specs
The one entry that differs is core/environmentContext.test.ts:50 mocking '../tools/read-many-files.js' — the same unresolved spec main already carries at utils/environmentContext.test.ts:50, moved with its file. Not introduced here. (For the record, and not this PR's: '../tools/read-many-files' is unresolved at three sites on main — the file is readManyFiles.ts — so those mocks never intercept today. Worth a separate issue.)
Two findings, both Suggestion-level
1. packages/core/src/memory/const.test.ts is orphaned by this PR. The source moved to packages/core/src/utils/memory-constants.ts, but the test stayed in memory/ under the old name. Its imports were updated correctly ('../utils/memory-constants.js', '../tools/memory-config.js' — both resolve), so nothing is broken; the problem is placement. On main both memory/const.ts and memory/const.test.ts exist, so this PR is what separated them, and AGENTS.md asks for tests collocated (file.test.ts next to file.ts). Rename to packages/core/src/utils/memory-constants.test.ts. I checked this systematically too — of five orphan candidates my scan raised, the other four (forkedAgent.agent, forkedAgent.cache, environmentContext.mcp-subagent, tool-response-finalizer.integration) are the repo's ordinary X.variant.test.ts convention with X.ts present, so this is the only real one.
2. The new rule's message and rationale contradict its own (correct) behaviour. no-core-utils-upward-import.js reports inline type specifiers — there is no specifier-level filtering, only the statement-level importKind === 'type' / exportKind === 'type' checks — and that is right: under this repo's verbatimModuleSyntax: true, import { type X } from './b.js' emits import {} from './b.js', a real runtime edge. But the @fileoverview says type-only imports are permitted "because they are erased at compile time", and the diagnostic ends with "Type-only imports are allowed." A developer flagged on import { type X } from '../config/foo.js' will read that message and go looking for a bug in the rule instead of at their own runtime edge.
The sibling PR #9737 hit exactly this as a Critical (its rule had actually exempted inline specifiers) and its fixed rule now documents the distinction in place — "Inline type specifiers … are NOT exempt: under this repo's verbatimModuleSyntax, tsc keeps the declaration". Mirroring that wording here costs two comment edits and keeps the two rules explaining the same law the same way.
Other checks
The rule is wired, not a dead switch: imported in eslint.config.js, registered under the architecture plugin, and enabled as 'error' scoped to packages/core/src/**/*.{ts,tsx} — and the config comment correctly notes the rule self-scopes to utils production files so it can share the existing block. CI is green apart from review-pr, still running. reviewDecision still reads CHANGES_REQUESTED from the bot review at 661c59a6, several commits back.
Where I land
Approving. Both findings are placement and prose — the rule behaves correctly and no import is broken — so neither is worth another round on a 100-file mechanical refactor. Both are cheap enough to fold in if you are pushing anyway.
Not verified by me: I did not run the suites (no node_modules in this worktree — CI covers them), so "the mocks resolve" is a static resolution result, not a test run. Integration Tests (CLI, No Sandbox) was skipped in CI and I did not cover it either.
Collaborator
Author
|
Updated the PR at
Focused verification:
GitHub CI and current-head review are now rerunning. |
yiliang114
dismissed
qwen-code-ci-bot’s stale review
August 23, 2026 15:00
Superseded by fixes in later additive commits. The iconv-lite bundle closure and all four stale mock paths were corrected, the previous head d26fc42 passed CI, and two collaborator reviews explicitly verified those blockers as resolved. Current head c089a41 only merges latest main and addresses the remaining non-blocking suggestions; its CI/review gates are still running.
Collaborator
Author
|
Synced the branch with current
Also folded in the two non-blocking current-head suggestions: Post-merge verification:
The current Ubuntu CI failure is runner infrastructure, before checkout/test execution: cleanup hit |
Collaborator
Author
|
CI follow-up at |
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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI (merge_group-only job) and its end-to-end suite did not run locally.
Not explored to full depth (tool budget reached): chunk 3: none — though I did not execute the affected vitest suites, relying instead on commit-level existence/export/reference verification of every re-routed import.; "agent 1b": none — no check was cut short..
Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/ci.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
Test Plan (not a blocker): 6 passing — this review observed 21009, 1685, 23491, 1653, 495, 4159, 599 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
scripts/tests/no-core-utils-upward-import.test.js:93 — [probe] Inline type specifier rejection is unpinned in the new lint rule's test suite
— qwen3.8-max via Qwen Code /review (v0.22.0)
yiliang114
force-pushed
the
codex/9146-utils-leaf-core
branch
from
August 24, 2026 03:08
4773897 to
38e07d6
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
chiga0
previously approved these changes
Aug 24, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Round 2 review — head 38e07d6088d0
Prior round (R1) at d26fc42b: no blockers. Scope: ESLint rule, test suite, esbuild path fix, CI state.
New commits since R1: one — 38e07d6088d0 ci: re-trigger after self-hosted runner checkout EACCES — zero file changes.
CI (38e07d6088d0)
| Check | Result |
|---|---|
Test (ubuntu-latest, Node 22.x) |
PASS |
Test (macos-latest, Node 22.x) |
SKIPPED |
Test (windows-latest, Node 22.x) |
SKIPPED |
Integration Tests (CLI, No Sandbox) |
SKIPPED (merge_group-only) |
| Desktop Shell (ubuntu + windows) | PASS |
| web-shell E2E, Real daemon E2E, Java matrix | PASS |
Findings
No blockers. Two suggestion-level items confirmed by cross-check:
S1 — packages/core/src/memory/const.test.ts is orphaned from its source.
The source module moved to packages/core/src/utils/memory-constants.ts, but the test stayed in memory/const.test.ts. Its imports resolve correctly (../utils/memory-constants.js, ../tools/memory-config.js), so nothing is broken functionally. The placement is inconsistent with the repo convention of file.test.ts beside file.ts. If convenient, rename to packages/core/src/utils/memory-constants.test.ts.
S2 — ESLint rule error message contradicts the rule's (correct) behavior for inline type specifiers.
no-core-utils-upward-import.js correctly flags import { type X } from '../config/foo.js' — under verbatimModuleSyntax: true that form emits a runtime-present edge. But the diagnostic message ends with "Type-only imports are allowed." A developer flagged on this form will read the message and go looking for a rule bug rather than at their own code. The rule behavior is right; only the message is misleading. Suggest replacing the trailing sentence with wording that distinguishes statement-level import type { X } (exempted via importKind) from inline import { type X } (not exempted, since importKind is value).
Unreviewed dimensions
- macOS and Windows test jobs SKIPPED in CI; not run locally (no working tree available).
Integration Tests (CLI, No Sandbox)SKIPPED; merge_group-only job, expected.- Execution rungs 1-3 not run.
No blockers. Both S1 and S2 are placement and prose — the rule is correct, no import is broken, TypeScript and ubuntu test suite pass.
Reviewed with AI assistance.
yiliang114
force-pushed
the
codex/9146-utils-leaf-core
branch
from
August 24, 2026 05:09
38e07d6 to
fbae4f2
Compare
Eliminate every runtime (value) upward import from packages/core/src/utils production modules so utils/ can become a leaf layer with no runtime dependency on the rest of core. Two mechanisms, no behavior change: - Relocate domain-coupled modules out of utils/ into their owning module (agents, config, core, memory, services, tools), and move generic constants/types that live elsewhere into utils/. All `git mv` moves keep history; every import that pointed at a moved file is rewritten. - Extract the remaining value imports as small leaf modules inside utils/ (AuthType, isTool, ToolErrorType, DEFAULT_QWEN_MODEL) and re-export them from their original owners so cross-package consumers are unaffected. doesToolInvocationMatch moves into shell-utils, its only production consumer. Only type-only imports now cross the utils/ boundary. The two deferred inversions in debugLogger (Storage, getTraceContext) are stateful and left for a follow-up.
Add architecture/no-core-utils-upward-import, which flags runtime (value) imports that leave packages/core/src/utils. Type-only imports, sibling utils imports, and external package specifiers stay allowed; the two deferred debugLogger inversions (config/storage, telemetry/trace-context) are carried on an explicit allowlist. Enable the rule as an error on core sources and cover it with Linter-based tests.
The utils leaf-layer refactor moved sync-file-encoding from utils/ to services/, but the esbuild tree-shake plugin still matched the old ./utils/ specifier, so its sideEffects:false marker no longer applied and the ACP startup closure regained a static iconv-lite import. Point the onResolve filter at the new ./services/ path.
yiliang114
force-pushed
the
codex/9146-utils-leaf-core
branch
from
August 24, 2026 05:19
fbae4f2 to
3821ef1
Compare
chiga0
approved these changes
Aug 24, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Round 3 re-review — head 3821ef1b5752
Prior-round ledger
| id | class | site | status |
|---|---|---|---|
| R2-1 | suggestion | eslint-rules/no-core-utils-upward-import.js — allowlist keyed only by target |
Refuted at current head: ALLOWED_UPWARD_IMPORTS is keyed by importer filename, not target |
| R2-2 | suggestion | ESLint rule error message omits re-export path | Fixed — message now reads "Move the value into utils/ (or re-export it from its owner module)" |
| S1 | suggestion | const.test.ts import offset |
Fixed — now imports from '../utils/memory-constants.js' |
New commits reviewed (16 since 38e07d60)
Leaf-layer refactor (68ac4d66 + follow-ups), ESLint enforcement rule, Moonshot/Kimi provider, workflow pre-approval dialog, WSL terminal-fix, workflow reapproval fix, seenSources cleanup in approval registry.
What I checked
Class 1 (contract): AuthType extracted to utils/auth-type.ts, re-exported unchanged through contentGenerator.ts and publicly via index.ts. ToolErrorType extracted to utils/tool-error-type.ts, re-exported via tools/tool-error.ts. index.ts updated for all 12 relocated exports — no public symbol dropped.
Class 2 (API compat): doesToolInvocationMatch moved from utils/tool-utils.ts → utils/shell-utils.ts; index.ts line 637 does export * from './utils/shell-utils.js', so the symbol remains in the public surface. Tests moved with the function; all existing assertions preserved.
Class 3 (error handling): readMetaForConfirmation wraps extractAndStripMeta in try/catch and returns null on any parse error, so a malformed export const meta cannot take down the approval dialog. Correct.
Class 4 (credentials): Moonshot provider uses envKey: 'MOONSHOT_API_KEY'; no key material in source. Two-region base-URL list. No exposure path found.
Class 10 (stated intent): PR claims "pure refactor, no runtime changes" for the leaf-layer work — consistent with the diff (only import paths and file locations change, no logic edits). getConfirmationDetails override for WorkflowToolInvocation: hideAlwaysAllow: true for inline scripts matches the stated goal of preventing blanket pre-approval of model-authored source.
Bot suggestions cross-check (all from qwen-code-ci-bot):
- Items on
environmentContext.mcp-subagent.test.tsandtool-response-finalizer.integration.test.tsinutils/: refuted — both files are renamed tocore/andtools/respectively at this head. - R2-2 (
dist/branch mis-maps path whendist/src/prefix is absent): confirmed as a real precision gap. The affected code path is@qwen-code/qwen-code-core/dist/<path-without-src>, which no file in the repo currently uses. Named-export resolution (the real path) is handled correctly viapackage.jsonexports. Minor precision gap, not a functional blocker. - Remaining bot suggestions: minor test-coverage observations; no new defect mechanism identified.
CI: Test (ubuntu-latest, Node 22.x) — SUCCESS. Test (macos-latest), Test (windows-latest), Integration Tests — SKIPPED, same as prior rounds.
Unreviewed dimensions: macOS/Windows execution rungs; Integration Tests; working tree unavailable (rungs 1-3).
No blocking findings.
Reviewed with AI assistance.
doudouOUC
approved these changes
Aug 24, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed current head 3821ef1b after the rebase. No blocking findings.
Verified the full 138-file diff and the current call sites/consumers: the relocations preserve the public barrel surface and runtime identities for the extracted values; no stale pre-move imports or mocks remain; the previously reported integration deep import and ACP/iconv bundle regression are fixed. The new utils boundary rule covers relative imports plus source, dist, and named package self-references, scopes the two deferred runtime exceptions to debugLogger.ts, and the exact head contains no other runtime upward imports from core utils.
Current-head evidence: git diff --check is clean; Ubuntu Node 22 tests, Desktop Shell on Ubuntu/Windows, web-shell E2E, real-daemon E2E, Java matrix, secret scan, and dependency audit pass. The no-sandbox integration and macOS/Windows unit jobs are skipped by workflow policy. The remaining pending checks are review routing/automation, not code-test failures.
The author has repository admin permission, so the maintainer exemption applies to the large core-refactor triage gate. Prior suggestion-level placement/message wording notes are already recorded and are non-blocking under the review-convergence rule.
Reviewed with AI assistance.
5 tasks
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
4 participants
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
Completes the core half of the utils leaf-layer work. Every runtime (value) import that leaves the core utils directory is eliminated so utils has no runtime dependency on the rest of the core package. Domain-coupled helpers are relocated into the modules that own them, generic constants and types that lived elsewhere are consolidated into utils, and the few remaining cross-boundary values are extracted as small leaf modules and re-exported from their original owners. Type-only imports are still permitted because they are erased at compile time. This is a pure refactor with no behavior change.
Why it's needed
It enforces a strict layering where utils is a dependency-free leaf, mirroring the CLI half already merged in #9737. A new architecture lint rule guards the invariant going forward: any value import that leaves utils fails the build. This closes out the core slice of #9152.
Reviewer Test Plan
How to verify
From the repository root, with a worktree whose dependencies are installed:
Expected: typecheck reports no errors, the rule test passes, and lint on utils reports no violations. The new lint rule is also exercised end to end by temporarily adding a value import that leaves utils and confirming it is flagged, then reverting.
Evidence (Before & After)
N/A — non-user-visible refactor, types, and tooling. Verification output:
tsc --noEmitexits 0;no-core-utils-upward-import.test.jsreports 6 passing tests;eslint 'packages/core/src/utils/**/*.ts'exits 0. Targeted unit tests for the moved and extracted modules (tool-utils, fileUtils, sideQuery, contentGenerator, truncation, environmentContext, shellContextEnv) pass 382/382.Tested on
Environment (optional)
Local worktree;
npx tsc --noEmit, vitest, and eslint only.Risk & Scope
git mvhistory and typecheck plus targeted tests confirm no dangling references.StorageandgetTraceContext) are stateful and remain as the only value imports out of utils, carried on an explicit allowlist in the lint rule. Resolving them needs a larger refactor and is tracked separately.Linked Issues
Part of #9146 (core half; CLI half is #9737). Closes the core slice of #9152.