refactor(cli): extract ACP skill management - #8865
Conversation
|
Re-ran the gate against the current head now that the review rounds and the autofix pass have settled. Nothing in the iteration changed the gate verdict: Template: still complete ✓ — all required sections present, bilingual body included. Problem: unchanged and still real — the ACP agent file sits at ~13k lines on current main, with roughly a thousand of those being cohesive Skill source-acquisition and mutation logic. This is a measured maintainability problem, not a theoretical one. Direction: still aligned. The Skill lifecycle gets its own modules while the extension-method surface ( Size: the changed paths ( Approach: shape unchanged since the first pass — two modules along the two responsibilities, an eight-line change at the agent boundary (five import lines, three delegations), no drive-by edits. The design doc under Risk: every changed file still lives under Gate holds. Moving on to code review. 🔍 中文说明在 review 轮次与 autofix 处理收敛后，对当前 head 重新执行门禁。迭代没有改变门禁结论： 模板：依然完整 ✓ —— 所有必填小节齐全，含中文翻译。 问题：不变且真实存在 —— ACP agent 文件在当前 main 上约 1.3 万行，其中约一千行是内聚的 Skill 来源获取与变更逻辑。这是可度量的可维护性问题，不是理论问题。 方向：依然对齐。Skill 生命周期拆出独立模块，扩展方法面（ 规模：改动路径（ 方案：形态与首轮一致——按两个职责拆两个模块，agent 边界仅八行改动（五行 import、三行委托），无顺手夹带。 风险：所有改动文件仍在 门禁通过，进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-review of the current head (the branch has moved through several review rounds and the autofix pass since my last look). My independent proposal for this problem is unchanged — two modules split along source acquisition vs mutations, with the agent delegating through the existing extension-method cases — and the PR still matches it. What I verified this time, concretely: I ran a line-set comparison between everything deleted from the agent file and everything added to the two new modules, then read both new modules end to end. Every delta is structural, not semantic: class methods became standalone functions taking The boundary change in the agent file is exactly eight lines — five imports and three delegations. I also checked beyond the diff: nothing outside the agent and its tests imports the moved symbols (the desktop No blockers, no behavior change found. The only standing nit remains the triple-copied Files changed (7 of 7 shown)
Testing evidenceUnattended CI run — per policy I don't build or execute PR code; the evidence is the PR's own CI on the reviewed commit, fetched once through the API. At fetch time every completed check is green, including the deciding
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Real-scenario (tmux) testing: N/A — unattended CI never drives the product live, and this PR asserts no user-visible behavior change (internal ACP module extraction); the live-behavior signal, if wanted, comes from the lane named above. 中文说明代码审查对当前 head 的重新审查（自上次查看后，分支经历了多轮 review 和 autofix 处理）。我对这个问题的独立方案不变——按来源获取与变更操作拆两个模块、agent 通过现有扩展方法分支委托——PR 仍然与之吻合。这次具体验证了： 我对"从 agent 文件删除的所有行"与"两个新模块新增的所有行"做了行集合比对，再通读两个新模块。所有差异都是结构性的而非语义性的：类方法变成接收 agent 文件的边界改动恰好八行——五行 import、三行委托。还检查了 diff 之外：除 agent 及其测试外没有任何文件导入被搬移的符号（desktop 的 无阻断项，未发现行为变化。唯一遗留的小瑕疵仍是三份拷贝的 测试证据无人值守 CI 运行——按策略不构建、不执行 PR 代码；以上证据来自 PR 自身 CI 在被审提交上的结果，通过 API 一次性获取。获取时刻所有已完成的检查全部为绿，包括决定性的
（CI 表格见上方标记区域，由 finalize 任务在 CI 结束后自动更新。） 真实场景（tmux）测试：N/A——无人值守 CI 不实际操作产品，且本 PR 声明无用户可见行为变化（内部 ACP 模块提取）；如需真实行为信号，见上文点名的通道。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — a faithful, well-guarded extraction re-verified after the review rounds settled; the only nit is the triple-copied parsing helpers, and nothing in the iteration changed the picture. Stepping back: this is still what a good refactor looks like. The problem is measured, not vibes — a ~13k-line agent file carrying security-sensitive skill-installation logic. The shape matches what I'd propose blind: two modules along the two responsibilities, an eight-line delegation change at the extension-method boundary, everything else a byte-faithful move. This pass I didn't just re-read the diff, I diffed the deleted line set against the added line set and accounted for every delta — all structural (methods to functions, license headers, duplicated parsing helpers); the SSRF allowlists, redirect validation, archive guards, traversal rejections, atomic install, and frontmatter surgery all moved with their rationale comments intact. The untouched If I'm maintaining this in six months I'll thank whoever split it. Approving, pinned to the reviewed commit. 中文说明信心：4/5 —— 在 review 轮次收敛后重新验证的一次忠实、守卫完备的提取；唯一的小瑕疵是解析辅助函数的三份拷贝，迭代没有改变整体判断。 退一步看：这仍是一次好的重构该有的样子。问题是量出来的，不是感觉——约 1.3 万行的 agent 文件承载着安全敏感的 skill 安装逻辑。形态与我盲写的方案一致：按两个职责拆两个模块，扩展方法边界八行委托改动，其余全部是字节级忠实的搬移。这一轮我不只重读了 diff，还把删除行集合与新增行集合做了比对，逐一解释每处差异——全部是结构性的（方法变函数、许可证头、复制的解析辅助函数）；SSRF 白名单、重定向校验、归档守卫、穿越拒绝、原子安装、frontmatter 外科手术都连同说明注释原样搬移。保持不动的 六个月后维护这段代码时，我会感谢做这次拆分的人。批准，锚定在被审提交。 — Qwen Code · qwen3.8-max Reviewed at |
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 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.8)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): PR #8865 "refactor(cli): extract ACP skill management" ex...: literal line-by-line paging of the ~10,500 unchanged lines of acpAgent.ts outside the diff regions and the state/timer/collection hotspots — deprioritized becau…; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget..
Test Plan (not a blocker): 405 tests pass — this review observed 18790 passed.
中文说明
未探索到全部深度（达到工具调用预算）：PR #8865 "refactor(cli): extract ACP skill management" ex...：literal line-by-line paging of the ~10,500 unchanged lines of acpAgent.ts outside the diff regions and the state/timer/collection hotspots — deprioritized becau…；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.。
Test Plan（非阻断）：405 tests pass — this review observed 18790 passed。
— qwen3.8-max via Qwen Code /review (v0.21.8)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; chunk 7: could not run the test file or npm run typecheck — the worktree has no node_modules ; runtime/type conclusions are from static tracing against the implementa…; You are review agent reverse-audit — Reverse audit agen...: none — all checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent Agent 8b — diff-specialized finder...: none — all five invariants were walked to completion within budget., and 2 more.
Test Plan (not a blocker): 405 tests pass — this review observed 18791 passed.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；chunk 7：could not run the test file or npm run typecheck — the worktree has no node_modules ; runtime/type conclusions are from static tracing against the implementa…；You are review agent reverse-audit — Reverse audit agen...：none — all checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent Agent 8b — diff-specialized finder...：none — all five invariants were walked to completion within budget.，另有 2 条。
Test Plan（非阻断）：405 tests pass — this review observed 18791 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Superseded by fixes in cac7889 and 021a801: protected the shared skills root, canonicalized duplicate and alternate frontmatter forms, and added real-parser regression coverage. All associated threads are resolved; focused tests, repository build and typecheck, plus scoped lint and format checks pass.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; PR #8865 extracts ACP Skill install/delete/enable-disable...: none — all checks above completed within budget..
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；PR #8865 extracts ACP Skill install/delete/enable-disable...：none — all checks above completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.9)
R4-12 was fixed and verified in 3a99b00; all review threads were answered and resolved. The remaining findings are non-blocking Suggestions deferred under the repository review-round policy.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): chunk 8: none — all checks I started completed within budget.; chunk 4: could not execute skill-management.test.ts ** — the review worktree has no node_modules (verified), and a fresh install exceeded the tool budget; verificatio….
中文说明
未探索到全部深度（达到工具调用预算）：chunk 8：none — all checks I started completed within budget.；chunk 4：could not execute skill-management.test.ts ** — the review worktree has no node_modules (verified), and a fresh install exceeded the tool budget; verificatio…。
— qwen3.8-max via Qwen Code /review (v0.21.9)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — 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 and their suites did not run locally.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds.
Not explored to full depth (tool budget reached): chunk 3: none — all planned checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; PR #8865 (review round 6) is a behavior-preserving refact...: none — all planned checks completed within budget..
中文说明
未审查：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 and their suites did not run locally。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds。
未探索到全部深度（达到工具调用预算）：chunk 3：none — all planned checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above were completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above were completed within budget.；PR #8865 (review round 6) is a behavior-preserving refact...：none — all planned checks completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.9)
All six R6 threads were evaluated against the PR base 77bd04b. The five production findings describe behavior already present in the original acpAgent.ts and are out of scope for this behavior-preserving extraction; the remaining item is a non-blocking test-quality Suggestion. Each thread now records the exact disposition and is resolved. No code change is warranted for this review.
Restore the three post-review files to the initial extraction commit. The removed changes addressed pre-existing Skill behavior and test coverage rather than regressions caused by the module split. Latest origin/main changes only unrelated ACP agent sections, so no extracted Skill logic needs to be carried forward.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — 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 and their suites did not run locally.
Not explored to full depth (tool budget reached): PR #8865 (QwenLM/qwen-code, review round 7) is a behavior...: none — I did not read every one of the 11,268 lines sequentially; I read the full diff slice, the field-declaration block, all three timer sites, the call-site …; PR #8865 (QwenLM/qwen-code, review round 7) is a behavior...: none — all planned checks completed within budget (~12 of ~52 calls).; PR #8865 (QwenLM/qwen-code, review round 7) is a behavior...: none — all checks I started were completed within budget (~10 tool calls used)..
Test Plan (not a blocker): 405 tests pass — this review observed 18784 passed.
中文说明
未审查：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 and their suites did not run locally。
未探索到全部深度（达到工具调用预算）：PR #8865 (QwenLM/qwen-code, review round 7) is a behavior...：none — I did not read every one of the 11,268 lines sequentially; I read the full diff slice, the field-declaration block, all three timer sites, the call-site …；PR #8865 (QwenLM/qwen-code, review round 7) is a behavior...：none — all planned checks completed within budget (~12 of ~52 calls).；PR #8865 (QwenLM/qwen-code, review round 7) is a behavior...：none — all checks I started were completed within budget (~10 tool calls used).。
Test Plan（非阻断）：405 tests pass — this review observed 18784 passed。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Dismissed after source audit: all four findings concern behavior or coverage already present in the pre-PR parent 77bd04b and intentionally restored by c33a5d0 to keep this PR a behavior-preserving extraction. Each thread has been answered with provenance; the bug fixes and coverage hardening belong in separate focused changes.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds.
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 and their suites did not run locally.
Not explored to full depth (tool budget reached): "PR #8865 (QwenLM/qwen-code) is a behavior-preserving…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget (~11 tool calls).; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above were completed within budget..
中文说明
仅完成部分审查，审查缺口已披露。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds。
未审查：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 and their suites did not run locally。
未探索到全部深度（达到工具调用预算）："PR #8865 (QwenLM/qwen-code) is a behavior-preserving…"：none — all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget (~11 tool calls).；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above were completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Dismissed as repeated out-of-scope findings: all three Critical items explicitly state they are pre-existing and byte-identical to the merge-base implementation, while this PR is a behavior-preserving extraction. The remaining item is a non-blocking test suggestion after ten review rounds. Each thread has an evidence-based reply and is resolved; CI is green.
|
Branch update: normally merged current |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": I did not run npm run typecheck to independently confirm no unused imports remain in acpAgent.ts — I relied on the grep residue sweep (zero matches for every …; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget., and 3 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：I did not run npm run typecheck to independently confirm no unused imports remain in acpAgent.ts — I relied on the grep residue sweep (zero matches for every …；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.，另有 3 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Dismissed as repeated out-of-scope findings: three Critical items explicitly state they are pre-existing and byte-identical to the merge base, and the fourth is a Suggestion about pre-existing URL acceptance after eleven review rounds. All four threads have evidence-based replies and are resolved. This PR is a behavior-preserving extraction.
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
|
@qwen-code /takeover from 5 |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This window's round counter starts at 5 (the rounds this PR spent in review before takeover), so the Critical-only brake engages after 0 more change-producing round(s) instead of a full fresh 5. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本窗口轮次计数从 5 起算（即本 PR 托管前已进行的评审轮数），因此再经过 0 个产生改动的轮次即进入 Critical-only，而非重新计满 5 轮。移除 |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no code changesThis round makes no code changes and no commit. Every actionable finding was classified, and all classified items are deferrals of verified pre-existing defects that lie outside this PR's behavior-preserving scope; nothing in the actionable feedback is a regression introduced by this PR, a failing check, or a base-conflict resolution request. Context
Evidence gathered this roundEach claimed defect region was diffed against the pre-extraction implementation at the merge base (
The defect mechanisms themselves are statically evident in the carried-over code (regex with no quoted/escaped/indented key forms, first-match-only DispositionsDeferred to the follow-up queue (recorded in
The review-body summary (rv:4908500739, findings R1-1…R1-7) maps onto the same five items above (R1-3/R1-4/R1-5 → item 1, R1-6 → item 2, R1-7 → item 3, R1-1 → item 4, R1-2 → item 5); it has no thread of its own, so its disposition is recorded here. No action (dispositional/informational input): the remaining inline entries are the PR author's own recorded dispositions for earlier rounds (confirmations, "Fixed in …" replies superseded by the intentional Why not fix the Criticals hereAll three blocker families are genuine defects, but they exist identically on 中文说明Autofix 评审轮次 — 无代码变更本轮不做任何代码变更、不产生提交。所有可操作发现均已分类，且全部为"已核实的预存在缺陷、超出本 PR 保持行为不变的范围"的延期处理项；可操作反馈中不存在本 PR 引入的回归、失败检查或基线冲突解决请求。 背景
本轮收集的证据每个被声称的缺陷区域都与提取前的实现（合并基
缺陷机制本身在搬运来的代码中静态可见（正则不含带引号/转义/缩进键形、 处置结果转入后续跟进队列（记录于
评审正文摘要（rv:4908500739，发现 R1-1…R1-7）与上述五项一一对应（R1-3/R1-4/R1-5 → 第 1 项，R1-6 → 第 2 项，R1-7 → 第 3 项，R1-1 → 第 4 项，R1-2 → 第 5 项）；它没有自己的线程，故处置记录于此。 无需处理（处置记录/信息性输入）：其余行内条目均为 PR 作者在此前轮次记录的处置（确认、被 为何不在此处修复这些 Critical三个阻断家族都是真实缺陷，但它们今天同样存在于 Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by
中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped at the 5-round cap without two consecutive dry rounds.
Test Plan (not a blocker): 405 tests pass — this review observed 19668 passed.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/cli/src/acp-integration/skill-management.ts:202 — [test] efficacy-probe mutant survived on the enable no-op guard (disabledLineIndex !== -1)packages/cli/src/acp-integration/skill-source-download.ts:71 — [review] R11-1 still stands: www.github.com not in ALLOWED_SKILL_SOURCE_HOSTS
Mechanism health: this round did not close cleanly, so it withholds the incremental anchor — and the round it recovered had none either, so the next review re-reads the whole diff and will keep doing so until a round's marker carries an anchor again. (Stated, not acted on — this changes nothing about what the round posts.)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the 5-round cap without two consecutive dry rounds。
Test Plan（非阻断）：405 tests pass — this review observed 19668 passed。
收敛姿态下延后（第 12 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
机制健康：本轮未能干净收尾，因而扣留了增量锚点，而它恢复到的那一轮也没有锚点，因此下一次评审将重读整个 diff——并会一直如此，直到某一轮的标记重新带上锚点。（仅陈述，不据此行动——这不改变本轮发布的任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
This automated blocking review only reasserts three previously dispositioned defects and explicitly confirms that each is pre-existing and byte-identical to the merge-base implementation. The current head remains byte-identical to the agreed 84f680d baseline for skill-management.ts (blob 55038c30854a2a34afc1248d205c96083688c02f). Fixing these findings would change setEnabled, install, or delete behavior and violate this PRs behavior-preserving extraction scope. Evidence-backed out-of-scope replies were posted on all three threads and the threads were resolved. Human approval from wenshao remains required.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no code change (all findings deferred to the follow-up queue)This round triaged the three re-asserted Critical findings from the round-12 review ( What was verified (probes at acc07e2, source-blind)
Why deferred instead of fixed in this PR
Accordingly the three findings are recorded in Not touched this round
中文说明Autofix 轮次：无代码改动（所有发现均延后至跟进队列）本轮分诊了第 12 轮评审（ 已验证内容（在 acc07e2 上探针证实，来源无关）
为何延后而非在本 PR 内修复
因此三条发现已记入 本轮未触碰
Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by 中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no code changesCritical-only mode is active for this window. The round contains no failed checks, no conflict to resolve, and no request to change code: the three actionable inline items are the author's out-of-scope declinations of the re-asserted findings R7-1, R8-2, and R7-2. Every factual claim in them was verified against the exact current head, and the underlying hardening findings were recorded for the follow-up queue instead of being implemented here. Feedback triage
Verification evidence (probes run on this checkout)
DispositionsAll three underlying findings are real hardening opportunities in pre-existing code carried over verbatim by this behavior-preserving extraction. Fixing them would change public 中文说明Autofix 本轮：无代码改动本窗口已处于仅处理 Critical 的模式。本轮没有失败的检查、没有需要解决的冲突，也没有任何要求改动代码的反馈：三条可执行的行内条目均为作者对重新提出的发现 R7-1、R8-2、R7-2 所作的"超出范围"拒绝回复。其中每一项事实性论断都已在当前确切 head 上核实；底层加固发现已记录到跟进队列，而不是在本 PR 中实现。 反馈分类
验证证据（在本检出上实际执行的探针）
处置结论三条底层发现都是预存在代码中真实存在的加固点，且被本次保持行为的抽取原样搬入。修复它们会改变公开的 Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by 中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 180 passed · 1 failed · 181 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：180 通过 · 1 失败 · 181 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #8865 — refactor(cli): extract ACP skill managementVerdict: 中文摘要
Scope
Structural identity (static half of the proof)
A/B cell table (behavioral half)Methodology: base worktree at
Result: 37/37 cells byte-identical (result + fsTree + fetchLog + refreshCalls per cell), and 65/65 scripted expectations pass on each arm ( Suite gates
Delta reconciles exactly: +2 files (+20 focused tests) −10 moved-out tests (4 tar + 6 redirect) = +10 tests. Mutation matrix (vacuity of the new tests)
M3a/M3b are the layered-guards pair: reverting either check alone is held by the other (M3a), and reverting the set survives only because the new FindingsF1 — Suggestion: the HTTPS source guard is not pinned by any message-matched assertion (new test carries a pre-existing looseness)Disabling both Measured minimal fixIn F2 — Observation (pre-existing, preserved byte-identically): enabling a skill whose
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action takenRound summary: Critical-only mode is active for PR #8865 — the counting window was seeded at round 5 by
Two non-Critical items remain open in the deferred section as audit records only (a sandboxed-verification status comment from the CI bot, and a note that @callmeYe has reached this window's regular-feedback budget); per the Critical-only rules they were not modified, resolved, or replied to. No code changes were made and nothing was committed. To continue with non-Critical feedback, tag it [Critical], submit a Request-changes review, or comment 中文说明Autofix 评审轮次：未采取任何行动轮次摘要： PR #8865 当前处于 Critical-only（仅处理 Critical）模式——本计数窗口由
延后区中有两条非 Critical 条目，仅作为审计记录保留（一条是 CI 机器人发布的沙箱验证状态评论，另一条提示 @callmeYe 已用完本窗口的常规反馈预算）；按照 Critical-only 规则，未对它们做任何修改、未解决、也未回复。 本轮未做任何代码改动，也没有提交任何内容。如需继续处理非 Critical 反馈，请将其标记为 [Critical]、提交 Request changes 评审，或评论 Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by
中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
✅ Local sandbox verification: merge-ready — 48/48 scripted assertions passedVerified PR head: 中文摘要结论：可以合并。 我在无宿主凭据的 定向测试也通过：base 路由 7/7，head 路由 7/7，head 新拆分模块 20/20。未发现本 PR 引入的回归。 重复 未覆盖：真实 GitHub 外网、tar/重定向链、故障注入下的 Central claimThis refactor claims to extract ACP Skill source acquisition and mutation logic without changing the observable contract of I drove the built CLI over its real ACP NDJSON transport in two independent, credential-free Linux containers. The only substituted boundary was GitHub HTTP: an offline preload returned real WHATWG
The A/B matrix covered install responses and installed bytes, staging cleanup, identical GitHub Contents/raw requests with Supporting gates
Inherited non-blocking riskThe duplicate-frontmatter-key probe reproduced the same pre-existing behavior on both arms: enabling a skill whose frontmatter contains FindingsNo PR-introduced correctness, security, or compatibility finding was produced by the executed A/B matrix or targeted gates. Not covered
MethodologyI resolved the exact OIDs from the upstream PR, archived each commit into a separate container-internal Linux filesystem, and ran Local maintainer verification; advisory evidence for merge review. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — PR #8865: no action takenRound summary: Critical-only mode is active (the counting window was seeded at round 5 by Inputs reviewed
Dispositions
ResultNo changes and no commit. No inline findings were resolved and none require a reply. Read-only checks performed
No build/typecheck/lint/test commands were run this round because no code changed. 中文说明Autofix 评审轮次 — PR #8865：无需改动本轮摘要： 当前处于仅处理 Critical 的模式（本计数窗口由 已审阅的输入
处置结论
结果无改动、无提交。没有已解决的 inline 发现，也没有需要回复的发现。 本轮执行的只读检查
由于本轮未改动任何代码，未运行 build / typecheck / lint / 测试命令。 Deferred non-Critical feedbackCritical-only mode is active: the round counter reached 5 (this window was seeded at round 5 by
中文说明已进入仅处理 Critical 的模式：轮次计数已达 5（本窗口由 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Released in v0.22.2. |
5 participants
What this PR does
This refactor moves ACP Skill source acquisition and managed Skill mutations into cohesive internal modules. The ACP agent now delegates install, delete, and enable/disable requests through the same extension-method routes, while the existing validation, filesystem safety guards, cache refresh ordering, responses, errors, and requested-working-directory behavior remain unchanged.
Focused tests now live alongside the extracted responsibilities, and the existing ACP integration coverage remains in place to verify the public request boundary end to end.
Why it's needed
Skill installation and management previously occupied more than one thousand lines inside the already-large ACP agent implementation. Giving this cohesive behavior a dedicated boundary reduces the agent's responsibilities and makes the Skill lifecycle easier to review and maintain without changing any user-visible capability.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — internal refactor with no UI or user-visible behavior change.
Tested on
Environment (optional)
Node.js 22 workspace; local Vitest, repository build, typecheck, ESLint, and Prettier checks.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
本次重构将 ACP Skill 来源获取与受管 Skill 变更拆分到职责内聚的内部模块中。ACP agent 现在通过原有扩展方法路由委托安装、删除和启用/停用请求；现有的参数校验、文件系统安全保护、缓存刷新顺序、返回结构、错误信息以及请求工作目录处理均保持不变。
拆分职责对应的定向测试现在与实现就近放置，同时保留现有 ACP 集成测试，以端到端验证公开请求边界。
为什么需要
Skill 安装与管理此前在已经很大的 ACP agent 实现中占据一千多行。为这组内聚行为建立独立边界，可以减少 agent 的职责，使 Skill 生命周期更易审查和维护，同时不改变任何用户可见能力。
Reviewer 测试计划
如何验证
证据（Before & After）
N/A — 内部重构，不涉及 UI 或用户可见行为变化。
测试平台
环境（可选）
Node.js 22 workspace；本地运行 Vitest、仓库构建、类型检查、ESLint 和 Prettier 检查。
风险与范围
关联 Issue
N/A