refactor(cli): keep acp-integration off serve internals (#8084) - #9144
Merged
Conversation
The dependency direction set in #8084 regressed: native Live Voice (a5c637b) added four acp-integration imports of serve/live modules, because nothing in the repo enforces the boundary the issue defines. Ownership, measured by consumer rather than by directory: - capture-screen-context, live-task-tools, live-speak-to-user and live-backend-instructions each have exactly one production consumer, acp-integration/session/Session.ts, and import nothing from serve/. They move to acp-integration/live/ with their tests. - conversations/session-source is shared by acpAgent and four serve modules, has no imports, and takes its reader as a parameter, so it moves to runtime/live-session-source.ts alongside the other neutral contracts. Renamed because every symbol in it is Live-specific. Adds a no-restricted-imports rule for acp-integration/** so the next feature spanning both surfaces gets a lint error pointing at runtime/, rather than silently reopening the criterion. No behavior change: moves, import rewrites, and the lint block.
5 tasks
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
qwen-code-ci-bot
requested changes
Aug 14, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
@yiliang114 thanks for the PR — the description is unusually thorough and the problem is real (I independently confirmed the five acp-integration → serve/ imports on base), but the body doesn't follow the repository's pull request template, so it pauses here before code review.
What's missing:
## What this PR does— the## Changessection already covers this; it just needs the template's heading.## Why it's needed—## Motivationcovers it; same ask.### How to verifybelongs nested under## Reviewer Test Plan, alongside### Evidence (Before & After)(N/A is fine for a refactor) and the### Tested ontable — your honest note aboutmulti-workspace-sessions.test.tsbelongs there.## Risk & Scope— the "no behavior change" statement and the follow-up notes fit here.## Linked Issues— #8084 is only referenced in the title and prose today.- The Chinese
<details>translation the template asks for.
Once the body is reshaped, re-run with @qwen-code /triage and it moves on to code review.
中文说明
@yiliang114 感谢提交 —— PR 描述写得非常详尽，问题也是真实存在的（我已在 base 上独立确认了 5 处 acp-integration → serve/ 的导入），但正文没有遵循仓库的 PR 模板，所以先停在代码审查之前。
缺失的部分：
## What this PR does——## Changes的内容已经覆盖，只需要换成模板的标题。## Why it's needed——## Motivation已经覆盖，同上。### How to verify应嵌套在## Reviewer Test Plan下，并补充### Evidence (Before & After)（重构类可以写 N/A）和### Tested on表格 —— 关于multi-workspace-sessions.test.ts的诚实说明适合放在那里。## Risk & Scope—— "无行为变化" 的声明和后续跟进注记适合放在这里。## Linked Issues—— 目前 #8084 只出现在标题和正文叙述里。- 模板要求的中文
<details>翻译。
正文调整后，用 @qwen-code /triage 重新运行即可进入代码审查。
— Qwen Code · qwen3.8-max
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores and enforces the intended dependency direction between packages/cli/src/acp-integration/ and packages/cli/src/serve/ by removing ACP’s imports of serve/ internals, relocating the Live-only ACP modules under ACP ownership, moving the genuinely shared Live session-source contract into runtime/, and adding an ESLint guard to prevent future regressions (per #8084).
Changes:
- Moved ACP-only Live Voice modules out of
serve/and intoacp-integration/live/, updating ACP consumers accordingly. - Introduced
runtime/live-session-source.tsas the neutral/shared Live session-source contract and rewiredserve/+ ACP callers. - Added a scoped
no-restricted-importsrule to mechanically preventacp-integration/**from importingserve/internals.
Reviewed changes
Copilot reviewed 11 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/serve/routes/session.ts | Repoints Live session-source import to runtime/live-session-source. |
| packages/cli/src/serve/multi-workspace-sessions.test.ts | Updates test import to the new runtime/live-session-source. |
| packages/cli/src/serve/live/live-task-service.ts | Uses runtime/live-session-source for Live provenance restore metadata. |
| packages/cli/src/serve/live/live-task-service.test.ts | Updates prefix import to runtime/live-session-source. |
| packages/cli/src/serve/live/live-session-coordinator.ts | Repoints import/export of LIVE_SESSION_SOURCE_PREFIX to runtime/live-session-source. |
| packages/cli/src/serve/acp-http/dispatch.ts | Uses runtime/live-session-source for ACP restore metadata. |
| packages/cli/src/runtime/live-session-source.ts | Adds lifecycle-free Live session-source helpers/contract in the neutral runtime/ surface. |
| packages/cli/src/runtime/live-session-source.test.ts | Updates tests to target live-session-source and validates accepted/rejected lineage cases. |
| packages/cli/src/acp-integration/session/Session.ts | Removes ACP → serve/live/* imports by switching to acp-integration/live/*. |
| packages/cli/src/acp-integration/session/Session.test.ts | Updates tests to import tool names from acp-integration/live/*. |
| packages/cli/src/acp-integration/live/live-task-tools.ts | Adds ACP-owned Live task tool definitions (moved out of serve/). |
| packages/cli/src/acp-integration/live/live-task-tools.test.ts | Adds unit coverage for task tool registration and execution serialization. |
| packages/cli/src/acp-integration/live/live-speak-to-user.ts | Adds ACP-owned “speak_to_user” tool (moved out of serve/). |
| packages/cli/src/acp-integration/live/live-speak-to-user.test.ts | Adds unit coverage for permission/default behavior and executor forwarding. |
| packages/cli/src/acp-integration/live/live-backend-instructions.ts | Adds ACP-owned Live backend instruction strings (moved out of serve/). |
| packages/cli/src/acp-integration/live/live-backend-instructions.test.ts | Adds unit checks for key invariant phrases in the backend instructions. |
| packages/cli/src/acp-integration/live/capture-screen-context.ts | Adds ACP-owned screen capture tool implementation (moved out of serve/). |
| packages/cli/src/acp-integration/live/capture-screen-context.test.ts | Adds unit tests for untrusted-data escaping, symlink rejection, and path scoping. |
| packages/cli/src/acp-integration/acpAgent.ts | Updates Live session-source compatibility import to runtime/live-session-source. |
| eslint.config.js | Adds scoped no-restricted-imports guard preventing acp-integration/** from importing serve/**. |
💡 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 |
5 tasks
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. |
This was referenced Aug 14, 2026
Closed
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 explored to full depth (tool budget reached): "This PR removes five acp-integration → serve/ imports by…": did not run full npm run build / npm run typecheck — dependent packages ( core , acp-bridge ) have no built dist/ in this worktree, so a standalone tsc …; "This PR removes five acp-integration → serve/ imports by…": none — but node_modules is not installed in this review worktree, so I did not execute the moved test files; the static evidence (byte-identical renames, ./ -r…; "You are review agent reverse-audit — Reverse audit agent…": full repo npm run build && npm run typecheck and whole-suite npm run lint not run — the stale-reference surface was instead covered by exhaustive repo-wide …; "This PR removes five acp-integration → serve/ imports by…": a byte-level before/after bundle diff via the esbuild metafile (I did not run npm run build && npm run bundle ). The inspection above shows the module graph is….
Test Plan (not a blocker): src/runtime/live-session-source.test.ts — no such file or directory; src/acp-integration/session/Session.test.ts — no such file or directory; src/serve/live/live-task-service.test.ts — no such file or directory; src/serve/multi-workspace-sessions.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.21.11)
# Conflicts: # eslint.config.js
Collaborator
Author
|
Status update from resolve-pr-comments: Changed: updated the PR body to the current template and resolved the latest-base conflict in Verified: Prettier, Pending: CI and automatic review on head 99a1172. |
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 explored to full depth (tool budget reached): "This PR refactors packages/cli so acp-integration/ no…": full npm run build && npm run typecheck with dependencies built first was not run; the PR-touched import graph was instead verified via the guard probe, clean…; "This PR refactors packages/cli so acp-integration/ no…": full repo-wide npm run lint / root npm run build not run (scoped eslint + tsc -b packages/cli used instead)..
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："This PR refactors packages/cli so acp-integration/ no…"：full npm run build && npm run typecheck with dependencies built first was not run; the PR-touched import graph was instead verified via the guard probe, clean…；"This PR refactors packages/cli so acp-integration/ no…"：full repo-wide npm run lint / root npm run build not run (scoped eslint + tsc -b packages/cli used instead).。
— qwen3.8-max via Qwen Code /review (v0.21.11)
- Flag the bare '../serve' directory specifier, which resolves to the serve/ barrel and skipped the trailing-segment group patterns (also added to the utils/ guard for symmetry). - Extend the same boundary to runtime/, the layer the rule directs authors to, so the #8084 coupling cannot reform one hop away. - Cover dynamic imports: no-restricted-imports never visits ImportExpression, so a no-restricted-syntax selector now enforces the boundary for await import('../serve/...') too. The acp-integration block moves after the general TS block (flat config lets the last matching block win per rule) and restates its no-restricted-syntax selectors so the override drops nothing. - Document that CI lint is the enforcement point; no fixture test pins the block. Verified: synthetic fixtures for all three violation shapes are rejected; full npm run lint passes with no live violations.
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 explored to full depth (tool budget reached): "This PR refactors packages/cli so acp-integration/ no…": none.**.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："This PR refactors packages/cli so acp-integration/ no…"：none.**。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Collaborator
Author
|
Status update from resolve-pr-comments: Changed: pinned the CLI serve-boundary lint rules with an ESLint regression test, shared the repeated serve import patterns, and covered runtime dynamic imports plus template-literal/path-traversal dynamic imports. Verified: Pending: CI/automatic review on the new head. |
qwen-code-ci-bot
requested changes
Aug 15, 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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Collaborator
Author
|
Closeout from resolve-pr-comments: Changed: tightened the dynamic serve-import selector for normalized traversal spellings, pinned the missing acp static and utils boundary fixtures, and reused the shared restricted-syntax constants in the general TS block. Verified: Pending: post-push CI/review on |
qwen-code-ci-bot
requested changes
Aug 15, 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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.12)
- reject computed dynamic-import sources (concatenation, new URL) and type-level imports fail-closed; rounds 2-5 each demonstrated a new per-spelling regex entrance, so non-literal forms are blocked outright (R4-1) - rewrite the boundary patterns without nested quantifiers; the previous shape backtracked exponentially (~4x per two ../ segments, lint-time ReDoS) (R5-2) - build the three guarded override blocks no-restricted-syntax arrays from one shared helper so flat config last-wins cannot silently drop selectors (R5-3) - pin the bare-directory barrel specifier in fixtures (R5-4) and add a string-throw probe pinning the restated selectors in the overrides (R5-5) - replace the **/serve* static globs with enumerated relative depths so third-party serve-named packages are never flagged (R5-7)
qwen-code-ci-bot
requested changes
Aug 15, 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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.21.12)
…andling - read the type-import specifier at argument.literal.value: @typescript-eslint wraps it in a TSLiteralType, so argument.value was dead code and the old fail-closed TSImportType selector over-matched every type-level import (37 errors in files this PR never touches) (round-6 Critical) - reject computed template literals (templates containing expressions) fail-closed; pure-literal templates stay covered by the quasis pattern selectors — the old blanket TemplateLiteral exemption contradicted the fail-closed comment above it (round-6 Critical) - give the fail-closed selectors a distinct message: computed sources cannot be checked against the boundary, which is not the same policy as importing serve/ (round-6 suggestion) - pin the depth-enumeration loop beyond depth 1 with a depth-2 fixture, pin the fixed type-import selector with a negative typeof-import control, and pin the computed-template fail-closed path (round-6 suggestion)
Complements the previous commit (which fixed the TSImportType selector
path and computed-template fail-closed) with the R4-1 entrances it left
open, each pinned by a fixture:
- percent-encoded segments (`../%73erve/index.js`): Node percent-decodes
segments when mapping the resolved URL to the filesystem, so raw-text
patterns cannot see through them — any `%` in a guarded-tree specifier
is now rejected with a dedicated message.
- static traversal twins: the pattern regexes now run over static
ImportDeclaration/ExportNamedDeclaration/ExportAllDeclaration sources
too, closing `import './../serve/x'`, `import '../runtime/../serve/x'`,
and `import '..//serve/x'`, whose dynamic twins were already blocked.
- leading literal segment: a traversal-anywhere pattern catches
`import('foo/../../../serve/x')` past the dot-slash anchor.
- vitest module-loading calls (vi.mock/doMock/importActual/importMock)
resolve and load the real module, so they get the same patterns plus
fail-closed coverage for computed arguments.
Collaborator
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
approved these changes
Aug 20, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
added a commit
that referenced
this pull request
Aug 20, 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: 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) were skipped in CI; their suites ran on Linux locally but not on those platforms.
Deferred under the convergence posture (round 22, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/session/Session.ts:226 — [probe] the move drops the relocated modules out of process-env-guard.test.ts's scannedRoots — the process.env hygiene audit silently shrinks
中文说明
仅完成部分审查，审查缺口已披露。
未审查：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) were skipped in CI; their suites ran on Linux locally but not on those platforms。
收敛姿态下延后（第 22 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
Collaborator
Author
|
@qwen-code /review --timeout=360 |
qwen-code-ci-bot
previously approved these changes
Aug 20, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 6a": could not run the guard test or inspect node_modules/eslint/lib/rules/no-restricted-imports.js (no node_modules in the review worktree) — the ImportExpressio….
Deferred under the convergence posture (round 23, not a blocker) — recorded, not requested in this round:
eslint.config.js:107 — [probe] utils/ 守卫缺少 '**/serve' 裸桶模式，与新增 acp-integration 守卫不对称（裸 import … from '../serve' 可绕过，probe 已证实；一行可修）
中文说明
无阻断问题。LGTM！✅
未探索到全部深度（达到工具调用预算）："agent 6a"：could not run the guard test or inspect node_modules/eslint/lib/rules/no-restricted-imports.js (no node_modules in the review worktree) — the ImportExpressio…。
收敛姿态下延后（第 23 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
doudouOUC
reviewed
Aug 21, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Not explored to full depth (tool budget reached): "This PR moves session-source.ts from serve/conversations/…": None. All planned checks completed.; "This PR moves session-source.ts from serve/conversations/…": 无。所有预定的检查均已完成，未达到 40 次调用的软上限。.
中文说明
已审查。 建议见行内评论。 1 条建议级发现无法锚定到改动行，已丢弃；此处无需进一步处理。
未探索到全部深度（达到工具调用预算）："This PR moves session-source.ts from serve/conversations/…"：None. All planned checks completed.；"This PR moves session-source.ts from serve/conversations/…"：无。所有预定的检查均已完成，未达到 40 次调用的软上限。。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
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 |
Collaborator
Author
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Deferred under the convergence posture (round 24, not a blocker) — recorded, not requested in this round:
eslint.config.js:237 — [review] R24-1 expression-free backtick dynamic import escapes the new serve/ guard (open R3-2)scripts/tests/acp-serve-boundary-guard.test.js:24 — [review] R24-2 the commit's .js coverage is pinned by no test probeeslint.config.js:226 — [review] R24-3 repeated general selectors in the acp override are pinned by no testeslint.config.js:237 — [probe] R24-4 type-position import() (TSImportType) escapes both boundary ruleseslint.config.js:92 — [probe] R24-5 .mjs/.cjs files fall outside the guard's widened globs
中文说明
收敛姿态下延后（第 24 轮，非阻断）——已记录，本轮不要求修改：共 5 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 3a": none — no check was cut short..
Deferred under the convergence posture (round 24, not a blocker) — recorded, not requested in this round:
scripts/tests/acp-serve-boundary-guard.test.js:71 — [probe] dynamic bare-barrel import('../serve') unpinned — the regex $-alternative mutation survives the suitescripts/tests/acp-serve-boundary-guard.test.js:26 — [probe] probe pins one point of the files-glob coverage — extension and **/ recursion axes both mutation-surviveeslint.config.js:234 — [probe] the repeated ...generalRestrictedSyntaxSelectors spread is unpinned — dropping it legalizes require()/string throws in acp fileseslint.config.js:237 — [probe] TSImportType inline type imports (type T = import('../serve/…').X) escape both guard rules
中文说明
未探索到全部深度（达到工具调用预算）："agent 3a"：none — no check was cut short.。
收敛姿态下延后（第 24 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
yiliang114
requested review from
qwen-code-ci-bot
and
a lite review from Copilot
August 22, 2026 12:25
The no-restricted-syntax selector for dynamic import() of serve/ was case-sensitive, so a macOS case-variant specifier (`../Serve/...`) would resolve to the daemon barrel without tripping the guard. Add the /i flag and cover case-variant plus computed-specifier behavior.
chiga0
approved these changes
Aug 22, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
No blocking findings.
Scope: source only. Reviewed all 23 changed/renamed files at head 5e7511ac.
Checked:
- ESLint config (
eslint.config.js):no-restricted-importspatterns (**/serve,**/serve/**) block both bare specifiers (../serve) and deep internals. Dynamicimport()guard usessource.valueregex/(^|\x2f)serve(\x2f|$)/i— the/iflag added in the tip commit correctly extends coverage to case-variant spellings (../Serve/index.js).generalRestrictedSyntaxSelectorsextracted to a shared constant — both acp-integration override blocks repeat them to avoid drift under flat-config last-wins semantics. - File moves (
serve/live/ → acp-integration/live/): all 4 files (capture-screen-context, live-task-tools, live-speak-to-user, live-backend-instructions) renamed; all import sites updated inSession.ts,Session.test.ts, andacpAgent.ts. live-session-source.tsextraction (serve/conversations/ → runtime/): relative import path inside the file corrected. All 7 consumers (dispatch.ts, live-session-coordinator.ts, live-task-service.ts + test, multi-workspace-sessions.test.ts, routes/session.ts, acpAgent.ts) updated. Re-export in live-session-coordinator.ts updated to match.cross-package-contracts.test.js:LIVE_TASK_TOOL_NAMES/LiveTaskToolNamesource paths updated toacp-integration/live/live-task-tools.ts. Cross-package contract still tracked.- Boundary guard tests (
acp-serve-boundary-guard.test.js): exercises static imports, bare-barrel specifiers, re-exports, type-only imports, dynamicimport(), case-variant dynamic import (../Serve/index.js), and confirms computed specifiers (import(target)) are correctly not flagged (nosource.valueon non-literals) andruntime/stays allowed. - CI: Test (ubuntu), Desktop Shell (ubuntu/windows), web-shell E2E — all green at current head.
- Cross-check: all prior CHANGES_REQUESTED reviews were on older commits. Deferred items from prior rounds (case-sensitivity of dynamic guard) addressed in the tip commit. Computed import coverage explicitly pinned as fail-open by a new negative test.
Reviewed with AI assistance.
yiliang114
commented
Aug 22, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
Author
There was a problem hiding this comment.
Scanned the full diff at 01475ab — no blockers from my side.
Verified:
- No
serve/imports remain inacp-integration/production files, and a repo-wide grep finds no stale references to the old paths (serve/conversations/session-source,serve/live/{capture-screen-context,live-task-tools,live-speak-to-user,live-backend-instructions}). runtime/live-session-source.tsonly depends on the neutralconfig/session-id.js, matching its intended shared-but-lifecycle-free placement.- Flat-config ordering is correct: the acp
no-restricted-syntaxblock sits after the general TS block and re-declares the hoisted general selectors, so the require/throw-literal restrictions survive the last-wins override. - The dynamic-import regex
(^|\x2f)serve(\x2f|$)does not false-positive on../services/ormcp-server-*.jsspecifiers. cross-package-contracts.test.jspath assertions are updated alongside the move.
Waiting on the pending ubuntu test run; everything else is green.
qwen-code-review-bot
approved these changes
Aug 22, 2026
qwen-code-review-bot
left a comment
qwen-code-review-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. 干净的边界收敛重构，无 blocker：
- 移到 （lifecycle-free 共享契约）， 工具移到 （ACP 专属）
- 全部 rename 为 100% similarity 纯移动，import 路径同步更新
- eslint 边界 guard 覆盖静态/动态/type-only/barrel 四种 import 形态，配套 用例完备
- CI 绿，无业务逻辑改动
Collaborator
|
|
yiliang114
added a commit
to yiliang114/qwen-code
that referenced
this pull request
Aug 22, 2026
Resolve eslint.config.js conflict: keep both the root barrel boundary block (QwenLM#9152) and the acp/serve dynamic-import guard (QwenLM#8084/QwenLM#9144).
Collaborator
|
Released in v0.22.0. |
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
This PR completes the remaining #8084 boundary cleanup by removing the current production imports from
acp-integrationintoserve/and adding an ESLint guard so the dependency direction stays enforced. Four Live Voice helpers that only serve ACP move underacp-integration/live/; the lifecycle-free live session source moves toruntime/because it is shared by ACP and daemon code. There is no intended behavior change.Why it's needed
#8084 established that ACP runtime code may consume neutral contracts, but should not import daemon implementation modules. That boundary regressed because it lived only in review/design discussion, not in a mechanical check. This PR puts the misplaced modules with their real owner, keeps the genuinely shared logic in a neutral location, and makes the forbidden edge fail ESLint.
Reviewer Test Plan
How to verify
Confirm the dependency direction is clean by checking that production files under
packages/cli/src/acp-integration/no longer import fromserve/. Confirm the guard by temporarily adding anyserve/import underacp-integration/and running ESLint; it should fail with the #8084 message. Live Voice behavior should remain unchanged because the moved helpers keep the same implementation and only import paths changed.Evidence (Before & After)
N/A — this is an internal dependency-direction refactor with no user-visible UI.
Tested on
Environment (optional)
Local repository worktree. Earlier focused validation covered the moved ACP live suites, live-session-source tests, the affected Session/live-task consumers, and ESLint on touched files. In this closeout pass I resolved the latest-base merge conflict in
eslint.config.jsand verified Prettier,git diff --check, and ESLint configuration loading on representative ACP/utils files.Risk & Scope
serve -> acp-integrationplusserve -> uiedges noted below.Linked Issues
Refs #8084.
中文说明
这个 PR 做了什么
这个 PR 完成 #8084 剩余的边界清理：移除当前生产代码中
acp-integration对serve/的导入，并添加 ESLint guard，防止依赖方向再次回退。四个只服务 ACP 的 Live Voice helper 移到acp-integration/live/；真正由 ACP 和 daemon 共享、且不含生命周期依赖的 live session source 移到runtime/。预期没有行为变化。为什么需要
#8084 已经确定 ACP runtime 可以消费中立 contract，但不应该导入 daemon 的实现模块。这个边界之前只存在于 review/design 讨论里，没有机械检查，所以后续跨 surface 功能第一次出现时就回退了。本 PR 把错放的模块放回实际 owner，把真正共享的逻辑放到中立目录，并让 forbidden edge 通过 ESLint 失败。
Reviewer 测试计划
如何验证
确认
packages/cli/src/acp-integration/下的生产文件不再从serve/导入。确认 guard 的方式是在acp-integration/下临时添加任意serve/导入并运行 ESLint；应看到包含 #8084 的错误信息。Live Voice 行为应保持不变，因为被移动的 helper 实现不变，只改 import path。证据（Before & After）
N/A — 这是内部依赖方向重构，没有用户可见 UI。
测试平台
环境（可选）
本地仓库 worktree。此前 focused validation 覆盖了移动后的 ACP live suites、live-session-source tests、受影响的 Session/live-task consumers，以及 touched files 的 ESLint。本轮 closeout 解决了 latest-base merge conflict，并验证了 Prettier、
git diff --check、以及代表性 ACP/utils 文件上的 ESLint 配置加载。风险与范围
serve -> acp-integration和serve -> ui边界。关联 Issue
Refs #8084.