fix(serve): treat Skill toggles as settings writes - #10077
Open
callmeYe wants to merge 9 commits into
Open
Conversation
Collaborator
Author
E2E / behavioral verification reportEnvironment: macOS, Node.js 22.17.0, npm, isolated worktree based on current Baseline confirmed before the fix:
Post-fix verification:
Focused results: Repository checks: The complete 1,079-test HTTP server file passed all 12 changed Skill cases. Its full-file run had two unrelated existing resource-sensitive failures ( |
Collaborator
|
⏳ Approval still deferred — 1 PR CI workflow run(s) still in progress for ⏳ 审批仍在延迟中 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: real, and confirmed in the base code. Direction: aligned. Skill enablement is persisted configuration; decoupling the write contract from runtime catalog state is the natural completion of #9139, and the existing design docs are updated in place rather than forked. Upstream changelogs show skill/plugin enable-disable toggles are an actively developed area, though nothing maps directly to this settings-write contract — the merged predecessor is the strong direction signal here. Size: 149 production-logic lines (facade 123, route 21, SDK 5) vs 447 test lines vs 98 docs lines. The change spans two packages (cli + sdk-typescript), but the SDK delta is 3 compatibility lines that keep the legacy per-target error shapes for older daemons. Approach: scope feels right — single and batch endpoints, SDK compat types, and the docs describing the contract all move together; cutting any of them would leave the contract inconsistent. One thing I'll check in code review: whether anything in-repo still relies on 404/409 toggle responses to infer catalog state. Risk: no elevated risk signals (no high-risk paths matched). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 真实存在，且已在 base 代码中核实。 方向： 对齐。Skill 启用状态是持久化配置；把写入契约与运行时目录状态解耦是 #9139 的自然收尾，既有设计文档也是在原文上更新而非另起炉灶。上游产品的更新日志显示 skill/plugin 启停开关是活跃迭代的方向，虽然没有与本次 settings 写入契约直接对应的条目——已合并的前序 PR 就是最强的方向信号。 规模： 生产逻辑 149 行（facade 123、route 21、SDK 5），测试 447 行，文档 98 行。改动跨两个包（cli + sdk-typescript），但 SDK 侧只有 3 行兼容性改动（为旧 daemon 保留遗留的逐目标错误结构）。 方案： 范围合理——单个/批量接口、SDK 兼容类型和描述契约的文档一起变更；砍掉任何一部分都会留下不一致的契约。代码审查阶段会重点确认：仓库内是否还有客户端依赖 404/409 的开关响应来推断目录状态。 风险： 无升级风险信号（未命中高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewI formed my baseline from the problem statement before reading the diff: drop the catalog lookup from both facade toggles, persist the trimmed requested name, keep the post-write activation/event flow, preserve batch request order, keep the legacy error shapes in the SDK, and move the docs with the contract. The PR does exactly that, and nothing extra. What I verified against the surrounding, unchanged code:
No correctness, security, or convention findings. sequenceDiagram
participant P1 as Daemon client
participant P2 as Toggle route
participant P3 as Workspace facade
participant P4 as Settings persistence
participant P5 as ACP child
P1->>P2: POST skills enable (single name or batch)
P2->>P2: shape, auth, trust, generation gates
P2->>P3: setWorkspaceSkill(s)Enabled
P3->>P4: persist trimmed names in one locked write
P4-->>P3: changed, settingsChanges
P3->>P3: invalidate cached skill snapshot
P3->>P5: workspaceSkillsRefresh (when a child is live)
P3-->>P1: ordered results with requested spellings
The point of the diagram is what is absent from it: no runtime Skill catalog query anywhere on the write path. Files changed (14 of 14)
TestingThis is an unattended CI run — nothing in this PR was built or executed by the reviewer; the evidence below is the PR's own CI on the reviewed commit, fetched through the API. CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Green so far with no failures: the real daemon E2E lane and both Desktop Shell lanes completed successfully, and the SDK Java lanes passed on all three runtimes. The main unit suite and Serve A/B are still in flight, so the verdict waits for them; the finalize job will update the table above when CI settles. The author posted a before/after verification report in this thread — that is their claim from a macOS-only run, not independently re-run evidence. Sandboxed verification would settle the remaining behavioural gap: 中文说明代码审查读 diff 之前我先根据问题描述形成了自己的方案：两个 facade 开关都去掉目录查询，直接持久化裁剪后的请求名称，保留写入后的激活/事件流程，批量保持请求顺序，SDK 保留遗留错误结构，文档随契约一起更新。PR 的实现与此完全一致，没有多余改动。 对照未改动的周边代码核实：
正确性、安全性、项目规范均无发现。 时序图的要点在于其中没有运行时 Skill 目录查询——写入路径上不存在这一步。 测试本次为无人值守 CI 运行——审查方没有构建或执行 PR 的任何代码；以上证据是 PR 自身 CI 在被审提交上的结果，通过 API 获取。目前全绿无失败：真实 daemon E2E 与两个 Desktop Shell 通道已成功，SDK Java 三个运行时全部通过；主单测套件与 Serve A/B 仍在运行，结论需等待，finalize 任务会在 CI 落定后更新表格。作者在线程中贴出的 before/after 验证报告是其本人在 macOS 上的声明，不是独立复核证据。 沙箱验证可以补上剩余的行为缺口： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Author
|
@qwen-code /verify |
Collaborator
|
Confidence: 4/5 — a clean, deletion-driven implementation of an already-accepted direction; the one reservation is about evidence, not code — end-to-end wire behaviour rests on mocked suites and a macOS-only author run until CI (or a Stepping back: this is the natural completion of #9139. That PR opened the batch endpoint to pre-install names but left the single endpoint and the installed-target validation coupled to the runtime catalog, so a persisted configuration write could still be rejected — or transiently undone — by discovery state. The fix makes the honest observation that settings ownership lives in the settings scopes, not in a runtime snapshot, and then simply deletes the two validation regimes instead of patching them. My independent baseline before reading the diff was the same shape, and the PR goes one step further in the right direction by also lifting the write-side lock gate while leaving the effective-availability resolver untouched — workspace declarations are recorded freely, higher scopes still decide what is actually available. The diff reads as removal, which is what I want to see: roughly 150 production lines that are almost entirely subtracted, with the additions concentrated in tests that pin the new contract (catalog query asserted never-called, requested spellings echoed, user-scope files byte-identical after workspace writes) and in docs that move with the contract across all four surfaces. The compatibility decisions are deliberate and named: legacy error shapes survive for older daemons, capability tags are unchanged, no wire-shape changes. Every file earns its place; nothing is drive-by. What keeps this at 4 rather than 5: the behavioural core — real HTTP requests persisting unknown, hidden, inactive, and higher-scope-locked names — is verified by suites that mock the daemon boundary, and the author's live run was macOS-only. The real daemon E2E lane passing is a good signal but its toggle coverage is not something I can attest to from here. That gap is named in the review comment with the lane that closes it; none of it blocks on the code itself. Verdict: approve. The main unit suite and Serve A/B are still in flight on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 实现干净、以删除为主，方向也是早已确立的；唯一的保留意见关于证据而非代码：在 CI（或 整体看：这是 #9139 的自然收尾。那个 PR 让批量接口接受安装前声明的名称，但单个接口和已安装目标的校验仍耦合在运行时目录上，一次持久化配置写入仍可能被发现状态拒绝、甚至短暂撤销。本次修复给出了诚实的判断：配置的所有权属于各配置层，而不是运行时快照——于是直接删掉两套校验，而不是打补丁。我在读 diff 之前形成的独立基线就是同样的形状；PR 还更进一步，把写入侧的锁校验也一并移除，同时保持有效可用性解析器不变——workspace 层可以自由记录自己的声明，更高配置层依旧决定最终可用性。 diff 读起来是"减法"，这正是我想看到的：约 150 行生产代码几乎全部是删除，新增集中在钉住新契约的测试（断言目录查询从未发生、响应回显请求拼写、workspace 写入后 user 层文件逐字节不变）和跟随契约同步更新的四处文档。兼容性决策是有意且明示的：遗留错误结构为旧 daemon 保留，能力标签不变，线上形状无变化。每个文件都有存在理由，没有顺手改动。 之所以是 4 而不是 5：行为核心——真实 HTTP 请求持久化未知、隐藏、未激活、被更高配置层禁用的名称——由 mock 了 daemon 边界的套件验证，作者的实机运行也只覆盖 macOS。真实 daemon E2E 通道通过是好信号，但其对开关接口的覆盖我无法在此确认。该缺口已在审查评论中点名，并给出了可补齐它的通道；这不影响代码本身。 结论：批准。主单测套件与 Serve A/B 仍在被审提交上运行，批准将推迟到 CI 在该提交上全绿后由 finalize 任务发出。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "workspace_skill_settings_toggle" |
features[] |
— | "workspace_skill_settings_batch_toggle" |
features[] |
"workspace_skill_toggle" |
— |
features[] |
"workspace_skill_batch_toggle" |
— |
— Qwen Code · serve A/B
Collaborator
|
Sandboxed verification: ✅ passed — merge-ready (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: 1625 passed · 0 failed · 1625 total Flakiness gate: ✅ 4 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：1625 通过 · 0 失败 · 1625 总计 抖动门：✅ 4 changed test file(s) x 5 identical rounds, no divergence Verification reportVerification report — PR #10077 "fix(serve): treat Skill toggles as settings writes"Verdict: 中文摘要
Central claim and A/B proofCentral claim: the single and batch Skill toggle endpoints are pure settings writes — any structurally valid name persists to workspace Two harnesses, both mock-free with respect to the unit under test, run against the real built
Daemon HTTP A/B (
|
| Cell | Scenario | Base (0756be0) |
Head (5a3d000) |
|---|---|---|---|
| C1 | single disable unknown verify-ghost |
404 skill_not_found, workspace lists untouched |
200, skillName: verify-ghost, changed: true, workspace disabled: ['orphan','verify-ghost'] |
| C2a | single disable locked-skill (user-scope disabled, on-disk skill) |
409 skill_not_toggleable, reason: locked, lockedScope: user |
200 changed: true, workspace records its own declaration; user-scope skills byte-identical |
| C2b | single enable locked-skill again |
409 locked | 200 changed: true, workspace declaration removed; user scope untouched |
| C6 | GET /workspace/skills after C2a/C2b |
seeded skill status: disabled, disabledReason: hard, lockedScope: user |
same — higher scope still authoritative |
| C3 | batch disable [ghost-two, orphan, locked-skill] |
results [ghost-two, orphan] in order; errors: [{locked…}]; workspace excludes locked |
results all three, request order; errors: []; workspace ['orphan','verify-ghost','ghost-two','locked-skill']; exactly one settings_changed event whose mutation lists only the two changed names |
| C3c | repeat identical batch | 200, all changed: false, 0 events |
200, all changed: false, 0 events |
| C4 | gates: 101-name batch / non-boolean flag / empty name / 300-char name / no token / empty array | 400/400/400/400/401/400 | identical |
| C5 | single enable opt-in-skill (user defaultDisabled) |
404 | 200 with workspace skills.enabled: ['opt-in-skill'] |
Facade A/B (facade-catalog.mjs) — base 14/14, head 15/15
| Cell | Scenario | Base | Head |
|---|---|---|---|
| F1 | unknown name | throws WorkspaceSkillNotFoundError; catalog queried ≥1; persistence 0 |
resolves; catalog queries 0; persistence gets trimmed Ghost-Skill; response echoes it |
| F2 | non-user-invocable hidden |
NotToggleable(not_user_invocable) |
persists |
| F3 | inactive extension ext-skill |
NotToggleable(inactive_extension) |
persists |
| F4 | batch [hidden, unknown-batch, ext-skill], live channel |
errors = both legacy codes; results [unknown-batch]; persistence gets only accepted name; 1 refresh |
results all three in order, errors: []; persistence gets all three; exactly 1 refresh; 1 settings_changed listing all changed names |
| F5 | batch with no changes | 0 refreshes, 0 events | 0 refreshes, 0 events |
The 404→200 / 409→200 flips are the load-bearing proof: base can only reject verify-ghost by consulting the catalog, and head persists it with zero catalog queries (F1 counter), so the catalog dependency is removed, not relocated.
Test vacuity / mutation matrix
Witness: evidence/03-mutation-matrix.png (aggregated by mutation-summary.mjs, which re-reads the saved vitest logs; 12/12 checks pass).
| Mutation | Suite | Result |
|---|---|---|
M_ws: revert workspace-service/index.ts to base (single+batch catalog validation) |
facade.test.ts |
10 red / 106 green — exactly the PR's new toggle tests (e.g. "persists by requested name without reading the runtime Skill catalog": rejected WorkspaceSkillNotFoundError instead of resolving) |
| M_ws (same revert) | server.test.ts |
4 red / 1075 green — the four renamed/new route tests |
M_rqs: restore hard-lock throws in run-qwen-serve.ts (single+batch) |
run-qwen-serve.test.ts |
2 red / 325 green — the two updated persistence tests (rejected WorkspaceSkillNotToggleableError instead of resolving) |
C_ws control: changed: !persisted.changed in the same facade file |
facade.test.ts |
4 red — runner can fail this file |
C_rqs control: changed: false in batch outcomes, same file |
run-qwen-serve.test.ts |
1 red — runner can fail this file |
All failures quote expected-vs-actual (behavioral), not import/compile errors. No mutant regressed a killed test into a survivor (matrix rows are single-hunk reverts of disjoint hazards; the two facade guards defend different endpoints, and the persistence revert row covers both lock hunks together).
Targeted gates
| Gate | Result |
|---|---|
packages/cli facade.test.ts + routes/workspace-skills.test.ts |
124 passed (2 files) |
packages/cli server.test.ts |
1079 passed (exit 1 came from a coverage-dir ENOENT race between my two concurrent vitest runs, after all tests passed — see gate-server.log) |
packages/cli run-qwen-serve.test.ts |
327 passed |
tsc --noEmit packages/cli |
clean (exit 0) |
tsc --noEmit (+ test-fence) packages/sdk-typescript |
clean (exit 0) |
SDK diff is comment-only (DaemonClient.ts doc comments, one comment line in types.ts); covered by the SDK typecheck. Legacy error codes (skill_not_found/skill_not_toggleable/skill_inactive_extension) survive only as wire-compat types in the SDK (for clients on older daemons) and in the shared skill-management error mapping still used by the delete/install routes — a repo-wide grep found no behavioral consumer inferring the Skill catalog from toggle responses.
Findings
No blocking findings. Non-blocking observations:
- Nit (pre-existing, both arms): the settings machinery stamps
$version: 4into a settings file on first load even without a toggle write. My harness initially compared whole-file bytes and flagged this; comparing theskillssections (the claim under test) shows user-scope declarations untouched. Not caused by this PR. - Informational:
DELETE /workspace/skills/:namestill 404s unknown names (it deletes a skill file, a catalog-dependent operation), while toggles no longer validate. This asymmetry is intentional and the protocol doc keeps the delete 404; noted so a reader does not read the two endpoints as one contract.
Not covered
- Live ACP child refresh path (
isChannelLive: truewith a real child): exercised via the facade harness's recordinginvokeWorkspaceCommand(count 1 per changed batch, 0 for no-change) and by the PR's own suite; the daemon harness ran withisChannelLive: false(activation: deferred). - Inactive-extension cell at HTTP level: the daemon-local skills provider intentionally omits extension skills outside a child session, so that catalog shape cannot be produced over HTTP; covered at the facade level through the production
queryWorkspaceStatusseam (F3/F4). - Windows/macOS, full-repo typecheck (the PR itself notes a pre-existing unrelated Web Shell error there), and the full repo test suite.
- Web Shell UI: no consumer of the removed 404/409 codes found in
packages/web-shell(grep), so no UI regression probe was built. - Base arm rebuild: the base worktree lacks
packages/cli/node_modules(worktrees don't inherit per-package installs), sotsc --buildthere reports pre-existing test-file type errors (@testing-library/reactmissing); emit completed and the base dist was verified to carry the base-only code paths (not_user_invocable, hard-lock check) before use.
Methodology
Ran in the CI verify container (node:22-bookworm) at the merge-ref checkout (depth 2; HEAD^1=base tip, HEAD^2=verified head; rev-list matches the 1-commit metadata). Base control: scratch worktree tmp/base-tree at 0756be0 with the root node_modules symlinked (internal @qwen-code/* links resolve to packages/core/acp-bridge, which the PR does not touch — asserted via readlink -f and quoted in mutation-summary.mjs), packages/cli recompiled there. Harnesses (ab-daemon.mjs, facade-catalog.mjs, mutation-summary.mjs) live in this artifact dir; raw per-run logs (base-run.log, head-run.log, facade-*.log, gate-*.log, mut-*.log, ctrl-*.log, typecheck-*.log) are beside them. Images produced with scripts/verify-capture.mjs from fresh runs of the same harnesses.
Flakiness gate log
rounds=5 files=4 skipped=0
file packages/cli/src/serve/routes/workspace-skills.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/routes/workspace-skills.test.ts
file packages/cli/src/serve/run-qwen-serve.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/run-qwen-serve.test.ts
file packages/cli/src/serve/server.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/server.test.ts
file packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/workspace-service/__tests__/facade.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/serve/routes/workspace-skills.test.ts: PPPPP
packages/cli/src/serve/run-qwen-serve.test.ts: PPPPP
packages/cli/src/serve/server.test.ts: PPPPP
packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: PPPPP
verdict: pass
summary: 4 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/routes/workspace-skills.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/run-qwen-serve.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/routes/workspace-skills.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/run-qwen-serve.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/routes/workspace-skills.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/run-qwen-serve.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/routes/workspace-skills.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/run-qwen-serve.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/routes/workspace-skills.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/run-qwen-serve.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/server.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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. |
ytahdn
reviewed
Aug 26, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
独立复审（head 5a3d0008，单提交，14 个文件）。我在 scratch clone 中检出该 head 做了静态交叉验证，并对 bot stage-2/3 与 /verify 的关键论断逐条独立复核。总体结论：daemon 层契约改动干净、以删除为主、测试钉得充分，方向认可；独立发现一条客户端侧的后续跟进项（🟡，不阻塞本 PR）。
已独立验证
- 生产代码为纯减法，且无悬挂引用。 单个/批量 toggle 都移除了目录查询与运行时校验（
workspace-service/index.ts中getWorkspaceSkillsStatus调用点全部删除）；run-qwen-serve.ts两处 higher-scope 硬锁拒绝整体删除。删后resolved = resolveSkillSettings(fresh)仍被defaultDisabledNames/enabledNames判定（opt-in 逻辑）使用，不是死代码。 - 持久化语义健全。
config/skill-settings.ts的updateTarget大小写不敏感匹配、收敛大小写变体、按请求拼写更新存目，不会累积重复项；有效可用性解析器未动，更高配置层在 effective 层面仍然权威（与 /verify A/B 的 C6 一致：user 层锁定名称的状态仍是disabled/hard/lockedScope: user）。 - 各道门未动。 路由层 trim、空名/256+ 字符 →
400 invalid_skill_name、批量大小写不敏感去重（保留首次出现的顺序与拼写）、认证/客户端身份/信任/generation 门均保持；路由文件本身不在 diff 内。 - 兼容性处理是有意的且无残留产生方。 serve 内不再有任何代码产生
WorkspaceSkillNotToggleableError（仅剩类型定义、再导出、错误映射与 lazy-import 注册表）；deleteWorkspaceSkill保留目录查询是正确的（需要installedPath），与文档声明的有意不对称一致。SDK 仅注释更新，遗留错误码联合类型保留给旧 daemon。 - 测试钉住新契约。 facade 测试用 mock-reject 的
queryWorkspaceStatus断言目录查询从未发生；ReViEw大小写端到端贯穿持久化参数、响应与事件；user 层settings.json在 workspace 写入后逐字节不变；批量结果保序、errors为空、一次写入一次刷新。/verify 的变异矩阵（回退校验后 10+4+2 条变红、正向对照可红）与 5 轮抖动门进一步证明套件非空转。四处文档（design×2、protocol、SDK、用户文档）同步一致。 - CI。 实质性通道在
5a3d0008全绿（ubuntu 单测、Serve A/B、Real daemon E2E、web-shell smoke、Desktop Shell、安全扫描；macOS/Windows 单测与集成测试按条件 skipping）。当前 pending 的 review-pr 是 bot 自身的评审工作流。
🟡 客户端跟进项（不阻塞本 PR）：web-shell 乐观覆盖会掩盖"写入成功但 effective 未变"
SkillsManagerPage.tsx 的开关菜单项只由能力位与 userInvocable === false 守护，没有对 lockedScope/inactive-extension 做任何提示。此前对 user 层锁定（或 inactive Extension）的名称点"启用"会收到 409 与错误提示；本 PR 之后该请求返回 200，于是 toggleSkill 走到成功分支：弹"已启用"提示、写入乐观覆盖 statusOverrides[name] = 'ok'。但 higher scope 仍然权威，reload() 返回的状态仍是 disabled，而覆盖清理副作用只在服务端状态等于覆盖值时才删除覆盖——两者永不相等，覆盖会一直把徽章显示为"已启用"，直到页面卸载重建。即：用户看到成功反馈与"已启用"状态，而该 Skill 实际依旧不可用。同一机制也覆盖 inactive Extension 场景（文档已明示"运行时仍不可用，与 workspace 是否记录该名称无关"）。
这是本 PR 新引入的客户端表现分歧（此前 409 路径不会设置乐观覆盖），修复属于 web-shell 侧：可在 reload() 后按新状态对账乐观覆盖（不一致即撤销），或利用状态单元里现成的 lockedScope/disabledReason 对这类目标禁用开关或加锁提示。daemon 契约本身按设计正确，故不阻塞；建议记录为后续跟进。另注：/verify 报告称"web-shell 中未发现被移除 404/409 码的消费者"，实际 toggleErrorMessage 消费了其中两个码（仅作错误文案兜底，码不再出现后无害）——不影响其结论，但 UI 对账缺口不在该报告的探测范围内。
其余
无其他正确性/安全性/规范发现。批量响应装配按请求顺序、persistedByName 键与路由去重后的名称一一对应；"缺失持久化结果"抛错在生产实现下不可达。纯大小写差异的请求会按 updateTarget 语义改写存目拼写并计 changed: true，与文档"折叠大小写变体、回显请求拼写"的契约一致，属有意行为。
Independent re-review at head 5a3d0008 (single commit, 14 files). I checked out this head in a scratch clone for static cross-verification and independently re-validated the key claims in the bot's stage-2/3 reviews and the /verify report. Overall: the daemon-side contract change is clean, deletion-driven, and well-pinned by tests; the direction is sound — with one client-side follow-up item found independently (🟡, non-blocking for this PR).
Independently verified
- Production code is pure subtraction with no dangling references. Both single and batch toggles drop the catalog query and runtime validation (every
getWorkspaceSkillsStatuscall removed from the toggle paths inworkspace-service/index.ts); both higher-scope hard-lock rejections are deleted wholesale fromrun-qwen-serve.ts. After the removal,resolved = resolveSkillSettings(fresh)is still consumed by thedefaultDisabledNames/enabledNamesopt-in logic — not dead code. - Persistence semantics are sound.
updateTargetinconfig/skill-settings.tsmatches case-insensitively, collapses case variants, and refreshes stored spellings to the requested one, so no duplicates accumulate; the effective-availability resolver is untouched, so higher scopes stay authoritative at the effective level (consistent with /verify A/B cell C6: a user-locked name still reportsdisabled/hard/lockedScope: user). - All gates intact. Route-level trim, empty/256+ char →
400 invalid_skill_name, case-insensitive batch dedup preserving first-seen order and casing, and the auth/client-identity/trust/generation gates are unchanged; the route file itself is not in the diff. - Compatibility is deliberate with no remaining producer. Nothing in serve produces
WorkspaceSkillNotToggleableErroranymore (only the type definition, re-exports, error mapping, and the lazy-import registry remain);deleteWorkspaceSkillcorrectly keeps its catalog lookup (it needsinstalledPath), matching the documented intentional asymmetry. SDK changes are comment-only; the legacy error-code union survives for older daemons. - Tests pin the new contract. Facade tests use a mock-rejecting
queryWorkspaceStatusand assert the catalog is never consulted; theReViEwcasing flows end to end through the persistence argument, the response, and the event; the user-scopesettings.jsonstays byte-identical after workspace writes; batch results keep request order with emptyerrors, one settings write, and one refresh. The /verify mutation matrix (10+4+2 red on reverted validation, positive controls able to fail) and the 5-round flakiness gate further show the suites are non-vacuous. All four doc surfaces (two design docs, protocol, SDK, user doc) move together consistently. - CI. Substantive lanes are green at
5a3d0008(ubuntu unit suite, Serve A/B, Real daemon E2E, web-shell smoke, Desktop Shell, security scans; macOS/Windows unit and integration lanes skip by condition). The pending review-pr lane is the bot's own review workflow.
🟡 Client-side follow-up (non-blocking): web-shell optimistic override can mask "write succeeded, effective state unchanged"
The toggle menu item in SkillsManagerPage.tsx is guarded only by the capability flag and userInvocable === false; nothing surfaces lockedScope or the inactive-Extension state. Previously, clicking "Enable" on a user-locked (or inactive-Extension) name answered 409 with an error notice; after this PR the request returns 200, so toggleSkill takes the success branch: it shows an "Enabled" notice and records the optimistic override statusOverrides[name] = 'ok'. But higher scopes remain authoritative, so reload() still reports disabled, and the override-pruning effect only deletes an override when the server status equals it — which never happens here. The badge keeps showing "enabled" until the page unmounts, i.e. the user sees success feedback and an enabled state for a Skill that is effectively still unavailable. The same mechanism covers the inactive-Extension scenario (the docs now explicitly state runtime availability is unaffected by workspace declarations).
This client-side divergence is newly introduced by this PR (the old 409 path never set an optimistic override), and the fix belongs to web-shell: reconcile the optimistic override against the reloaded status (drop it on mismatch), or use the existing lockedScope/disabledReason status fields to disable or annotate the toggle for such targets. The daemon contract itself is correct by design, so this does not block; suggesting it be tracked as a follow-up. Note: the /verify report states "no consumer of the removed 404/409 codes found in packages/web-shell", while toggleErrorMessage does consume two of them (as message fallbacks only — harmless once the codes stop arriving); this does not affect its conclusion, but the UI reconciliation gap was outside that probe's scope.
Otherwise
No other correctness, security, or convention findings. The batch result assembly preserves request order and the persistedByName keys correspond one-to-one to the route-deduplicated names; the "missing persisted outcome" throw is unreachable under the production persistence implementation. A request differing from a stored entry only in casing rewrites the stored spelling and reports changed: true per updateTarget semantics — consistent with the documented "collapse case variants / echo the requested spelling" contract, and intentional.
samuelhsin
previously requested changes
Aug 26, 2026
samuelhsin
left a comment
samuelhsin
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes for two independent contract regressions at this head: the settings-only behavior is introduced under existing stable capability tags, and the in-repo WebShell consumer treats the new 200 response as effective availability. Both need to be addressed before merge.
qwen-code-ci-bot
previously requested changes
Aug 26, 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.22.0)
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
samuelhsin
reviewed
Aug 26, 2026
samuelhsin
left a comment
samuelhsin
left a comment
Collaborator
There was a problem hiding this comment.
独立复核补充发现（independent re-review at head 5a3d000）
独立拉取 head SHA 完整源码交叉复核后补充：方向认可，daemon 层改动是干净的纯减法——无悬挂引用（resolved 仍被 opt-in 判定使用，WorkspaceSkillNotFoundError 仍被 deleteWorkspaceSkill 合法使用），各道门（trim / 空名 / 256 上限 / 批量去重 / 信任·认证·身份·generation）原封未动，持久化语义健全。问题仍在契约外溢，以下两条补充发现与此前 review 中两个阻塞项同源，不阻塞但值得记录。
🟡 1. skill_toggle mutation 事件的语义漂移（semantic drift）
现在对 higher-scope 锁定名称的写入成功也会发出 settings_changed mutation，且 skills 数组中列出该名称。事件本身按"工作区配置确实变了"是自洽的，但任何把 mutation.skills 理解为"有效状态变了"的消费方会误渲染。与 web-shell 问题同源，建议在协议文档中写明，或让消费方对账。
🟡 2. 纯大小写差异的请求触发一次真实写入与 session 刷新
对已存名称仅改大小写再 toggle，updateTarget 会改写存目拼写并计 changed: true，从而触发一次本可避免的 live-session 刷新。这与文档"折叠大小写变体、回显请求拼写"的契约一致，属有意行为，但代价是一次多余刷新，可考虑在仅拼写变化时不触发 refresh。
✅ 已验证无问题的点
- 各道门未动；路由文件不在 diff 内（
400校验、去重、信任门都保持） - 无安全/注入风险：名称仍受空名与 256 字符上限约束，写入仅限可信工作区
- 批量结果按请求顺序装配，
persistedByName键与路由去重后的名称一一对应；"缺失持久化结果"抛错在生产实现下不可达 deleteWorkspaceSkill保留目录查询是正确的（需要installedPath），与文档声明的不对称一致- CI 实质通道在该 head 全绿（Integration Tests 被 skip 的缺口 ci-bot 已披露）
English summary
Independent re-review at head 5a3d0008 with full source cross-checked, not just the diff. The two blocking issues from my earlier review stand: (1) the settings-only behavior change ships under the existing stable capability tags (workspace_skill_toggle / workspace_skill_batch_toggle still since: 'v1'), while this repo explicitly supports mixed-version clients where capability tags serve as the version-negotiation mechanism (precedents: extension_batch_activation_v2, extension_management_v2); (2) the in-repo web-shell consumer (SkillsManagerPage.tsx) unconditionally sets the optimistic statusOverrides[name] = 'ok' on the new 200 response, but for names locked by a higher scope the status after reload() remains disabled — the override is never reclaimed (cleanup only fires when override === server status) and the badge keeps showing "enabled" while the skill remains unavailable. Before this PR the same path returned 409 and never set the override.
Two additional, non-blocking findings:
- The
skill_togglemutation event now also fires for successful writes to higher-scope-locked names; consumers readingmutation.skillsas "effective state changed" will misrender. Same root cause as the web-shell issue — worth documenting in the protocol or reconciling on the consumer side. - Case-only toggle requests rewrite the persisted spelling and count
changed: true, triggering an avoidable live-session refresh. Consistent with the documented contract, but could skip the refresh when only spelling changes.
Verified clean: all guards untouched, no dangling references, batch results assembled in request order with persistedByName keys matching deduplicated names, no security/injection risk, deleteWorkspaceSkill's retained directory lookup is correct (needs installedPath), CI substantively green at this head.
Collaborator
Author
|
@qwen-code /review |
Contributor
|
Qwen Code review request accepted. Review is queued in workflow run. |
callmeYe
dismissed stale reviews from samuelhsin and qwen-code-ci-bot
August 26, 2026 11:15
Stale after 808d41a. Both requested fixes are present on the current head: the existing routes and request bodies are unchanged while corrected settings semantics use replacement capability tags, and WebShell both preflights the replacement tag and reconciles optimistic state with the authoritative reload. Focused regression tests plus full build/typecheck pass, and no review thread remains unresolved.
qwen-code-ci-bot
previously requested changes
Aug 26, 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; the changed file (integration-tests/cli/qwen-serve-routes.test.ts, 36/36 green against the bundle) ran locally, but the full integration suite did not.
Not reviewed: reverse audit — stopped at the 5-round plan cap while round 5 was still reporting findings (loop did not converge).
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI; the changed file (integration-tests/cli/qwen-serve-routes.test.ts, 36/36 green against the bundle) ran locally, but the full integration suite did not。
未审查：reverse audit — stopped at the 5-round plan cap while round 5 was still reporting findings (loop did not converge)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
5 tasks
callmeYe
dismissed
qwen-code-ci-bot’s stale review
August 26, 2026 12:26
Stale after 0fb288b. All three Critical findings are fixed on the current remote head: v1 capability replacement lifecycle is documented, higher-scope semantics are narrowed at both protocol and design anchors, and WebShell reports changed:false accurately with focused coverage. All five non-blocking Suggestions are explicitly deferred to #10152 after the five-round cap. Tests, lint, typecheck, and WebShell build pass; no review thread remains unresolved.
qwen-code-ci-bot
previously requested changes
Aug 26, 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 explored to full depth (tool budget reached): chunk 8: running SkillsManagerPage.test.tsx under vitest to confirm it is green — the review worktree has no node_modules installed, and install-plus-build exceeds t….
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/workspace-service/index.ts:838 — [review] case-only re-toggle reports changed: true, wasting a settings write + live-session refresh (probe-verified)packages/cli/src/serve/workspace-qualified-rest.test.ts:1242 — [review] unchanged qualified-route test still pins the retired 404 contract via a mock-injected WorkspaceSkillNotFoundErrorpackages/cli/src/serve/workspace-service/index.ts:838 — [review] toggle path persists . / .. names the sibling install/delete routes reject (probe-verified)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 8：running SkillsManagerPage.test.tsx under vitest to confirm it is green — the review worktree has no node_modules installed, and install-plus-build exceeds t…。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.0)
qwen-code-ci-bot
previously requested changes
Aug 26, 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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R3-2 renamed-capability envelope test unreachable from workspace-scoped suites — already reported (comment 3863658330)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/web-shell/client/components/skills/SkillsManagerPage.tsx:243 — [probe] R2-4 carried: !refreshedSkill notice branch and disable-side notice untested (deferred to (10152) by the author)packages/web-shell/client/App.tsx:4866 — [probe] R2-5 carried: all-declaration-only filter branch ( pendingForSession.length === 0 ) untested (deferred to (10152) by the author)
Convergence: round 3 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 8 (8 new). Findings keep coming back to the same files: docs/developers/qwen-serve-protocol.md (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 8 条（其中 8 条首次提出）。发现反复回到同一批文件：docs/developers/qwen-serve-protocol.md（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
5 tasks
callmeYe
dismissed stale reviews from qwen-code-ci-bot and qwen-code-ci-bot
August 26, 2026 15:46
Stale after 4175296: the Critical batch-persistence documentation mismatch is fixed across the protocol, user guide, and SDK guide; both non-blocking Suggestions are explicitly deferred to #10177 after multiple review rounds. All associated threads are resolved, formatting and diff checks pass, and no live human objection remains.
Collaborator
Author
|
Review-body follow-up for the non-inline findings:
The two current Critical documentation findings were fixed in 4175296; all four inline threads are resolved. |
doudouOUC
reviewed
Aug 26, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Static review (second opinion)
I reviewed the full diff (28 files, +821/-593) at the current head aa9a4d960a. Git fetch is unavailable on this Windows machine, so I could not build or test the PR — this is a static cross-reference against the existing reviews and the code as shown in the diff.
Context
This PR has already been through 3 rounds of CI bot review, plus independent reviews by @ytahdn (COMMENTED) and @samuelhsin (both DISMISSED and COMMENTED). All 9 blocker threads from the CI bot reviews were addressed by the author across 7 fix commits. CI is green at the current head.
Verified against the diff
I independently verified the following fix claims against the code in the current diff:
-
R1-1 / R2-3 (WebShell optimistic override):
SkillsManagerPage.tsxnow usessetEnabledresult'schangedflag, reloads authoritative status, and provides appropriate notices. ThestatusOverridesmechanism was removed entirely. ✅ -
R1-2 (userInvocable reference):
docs/developers/qwen-serve-protocol.mdwas updated to state thatuserInvocabledoes not gate the settings-only toggle routes. ✅ -
R1-3 (session reconciler):
App.tsxfilters declaration-only enables from the pending session set after reload. Names with nostatus === 'ok'row are removed. ✅ -
R1-4 (mutation.skills semantics): Protocol docs now describe
mutation.skillsas requested names and requested enabled values whose workspace declarations changed. ✅ -
R1-5 (higher-scope sentence): The protocol doc now narrows the scope to hard
skills.disabledonly, matchingqwen-serve.md. ✅ -
R2-1 (batch contract scope): Protocol docs scope the batch contract to resulting declaration changes in at most one locked settings write. ✅
-
R2-2 ("Older daemons return 404"): The capability section distinguishes between missing routes and the retired contract served at the same paths. ✅
-
R2-4 (lifecycle rule):
docs/developers/daemon/11-capabilities-versioning.mddocuments corrected-behavior replacement within v1. ✅
No new findings
- Production code is pure deletion: both toggle paths drop the catalog query, the higher-scope lock rejections are removed, and there are no dangling references.
WorkspaceSkillNotToggleableErrorhas no remaining producer in serve. - All gates (trim, 256-char limit, batch dedup, auth, trust, client identity, generation) remain intact.
- The SDK change is comment-only; legacy error types survive for older daemons.
- Documentation is updated consistently across all 7 changed doc files.
- The
mapWorkspaceSkillToggleErrorfunction correctly handles onlyWorkspaceSkillNotFoundError(404) now. sendBridgeErroralways returns 404 for the remaining skill error type.
Unreviewed dimensions
- Build and test: git fetch is unavailable on this Windows machine, so I could not build, typecheck, or run the test suite. The author's verification report and green CI at this head are relied upon.
Conclusion
No new Criticals or Suggestions. The PR is a clean, deletion-driven implementation of an already-accepted direction. All prior blocker threads have been addressed.
— qwen3.8-max via Qwen Code /review
qwen-code-ci-bot
previously requested changes
Aug 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- docs/users/qwen-serve.md:20 feature-overview bullet still describing the route as toggling a "loaded skill" — already reported (comment 3863658324) and deferred by the author to the follow-up issue (10177)
Convergence: round 4 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 1 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛情况：第 4 轮发布了 4 条行内评论，其中 4 条是首次提出；上一轮发布了 1 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
callmeYe
dismissed
qwen-code-ci-bot’s stale review
August 26, 2026 17:32
Stale after 86b4069: all four R4 Critical documentation findings are fixed through a root-cause sweep across the protocol, user guide, SDK guide, and both design docs. Activation is independent from changed, no-op refresh behavior is explicit, and case-normalization writes are documented. The repeated Suggestion remains explicitly deferred to #10177 after multiple review rounds, every R4 thread is resolved, formatting and diff checks pass, and no live human objection remains.
doudouOUC
reviewed
Aug 26, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed.
Not reviewed: build-and-test — git fetch blocked by a dead hosts entry (no worktree; build/test impossible on this machine); Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 8: running SkillsManagerPage.test.tsx under vitest to confirm it is green — this machine has no node_modules and npm install/build/test is blocked (same constrai…; One-line change summary: PR #10077 converts the daemon's single and batch Skill toggles from runtime-catalog-validated toggles into settings-only writes keyed by the requested name, retires capability tags workspace_skill_toggle / workspace_skill_batch_toggle in favor of workspace_skill_settings_toggle / workspace_skill_settings_batch_toggle, and updates the WebShell consumer to reconcile authoritative status after writes plus documentation. Reviewed head aa9a4d9 against base main. Environment: git network access is blocked on this machine (no git worktree, no npm install/build/test — do not attempt them). Every changed file (28) is mirrored verbatim at head in C:\Users\jinye.djy.qoderwork\workspace\mspqz3u5etjh72hs\qwen-code.qwen\tmp\review-pr-10077-r3-head\ — read changed files there (treat it as the worktree). To read any OTHER file at head: gh api "repos/QwenLM/qwen-code/contents/?ref=aa9a4d960a670cd07fca8b19f7b9b7f4895edaad" --jq .content | base64 -d (for files over 1MB use the git/trees + git/blobs API); retry transient network errors once or twice. The main checkout C:\Users\jinye.djy.qoderwork\workspace\mspqz3u5etjh72hs\qwen-code (branch pr-8966) is close to base only — never cite it as head evidence without verifying against the mirror/API. Discussion history, including already-fixed blocker threads and Suggestions deferred to follow-up issues #10152/#10177 (do NOT re-report those), is in C:\Users\jinye.djy.qoderwork\workspace\mspqz3u5etjh72hs\qwen-code.qwen\tmp\qwen-review-pr-10077-r3-context.md. Note: two other kinds of agents are reviewing this same change from different angles — the issue fidelity agent judges scope against the linked issue, and the territory agents each own one line-range of the diff. If one of them reports on the claim you are investigating, do not duplicate their work; focus on your dimension, the behavioral-claims-vs-tests matrix.: none — all checks completed within budget..
中文说明
已审查。
未审查：build-and-test — git fetch blocked by a dead hosts entry (no worktree; build/test impossible on this machine); Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 8：running SkillsManagerPage.test.tsx under vitest to confirm it is green — this machine has no node_modules and npm install/build/test is blocked (same constrai…；One-line change summary: PR #10077 converts the daemon's single and batch Skill toggles from runtime-catalog-validated toggles into settings-only writes keyed by the requested name, retires capability tags workspace_skill_toggle / workspace_skill_batch_toggle in favor of workspace_skill_settings_toggle / workspace_skill_settings_batch_toggle, and updates the WebShell consumer to reconcile authoritative status after writes plus documentation. Reviewed head aa9a4d9 against base main. Environment: git network access is blocked on this machine (no git worktree, no npm install/build/test — do not attempt them). Every changed file (28) is mirrored verbatim at head in C:\Users\jinye.djy.qoderwork\workspace\mspqz3u5etjh72hs\qwen-code.qwen\tmp\review-pr-10077-r3-head\ — read changed files there (treat it as the worktree). To read any OTHER file at head: gh api "repos/QwenLM/qwen-code/contents/?ref=aa9a4d960a670cd07fca8b19f7b9b7f4895edaad" --jq .content | base64 -d (for files over 1MB use the git/trees + git/blobs API); retry transient network errors once or twice. The main checkout C:\Users\jinye.djy.qoderwork\workspace\mspqz3u5etjh72hs\qwen-code (branch pr-8966) is close to base only — never cite it as head evidence without verifying against the mirror/API. Discussion history, including already-fixed blocker threads and Suggestions deferred to follow-up issues #10152/#10177 (do NOT re-report those), is in C:\Users\jinye.djy.qoderwork\workspace\mspqz3u5etjh72hs\qwen-code.qwen\tmp\qwen-review-pr-10077-r3-context.md. Note: two other kinds of agents are reviewing this same change from different angles — the issue fidelity agent judges scope against the linked issue, and the territory agents each own one line-range of the diff. If one of them reports on the claim you are investigating, do not duplicate their work; focus on your dimension, the behavioral-claims-vs-tests matrix.：none — all checks completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.10)
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.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- docs/users/qwen-serve.md:20 feature-overview bullet still describing the route as toggling a 'loaded skill' — already reported (comment 3863658324) and deferred by the author to the follow-up issue (10177)
Convergence: round 5 posted 1 inline comment(s), 1 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: docs/design/daemon-skill-toggle.md (findings in round 4; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查——无阻断问题。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
收敛情况：第 5 轮发布了 1 条行内评论，其中 1 条是首次提出；上一轮发布了 4 条（其中 4 条首次提出）。发现反复回到同一批文件：docs/design/daemon-skill-toggle.md（第 4 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (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
5 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
The daemon's single and batch Skill toggle endpoints now persist the trimmed requested names directly to workspace Skill settings without consulting the loaded runtime Skill catalog. Unknown names, non-user-invocable Skills, Skills from inactive Extensions, and names disabled by a higher settings scope all use the same settings write path. Higher scopes remain authoritative when effective availability is calculated, but no longer prevent the workspace scope from recording or removing its own declaration.
The existing endpoint paths and request bodies are unchanged. The daemon replaces the retired catalog-validated capability tags with
workspace_skill_settings_toggleandworkspace_skill_settings_batch_toggle, so clients can explicitly opt into the corrected settings-write contract. WebShell gates the toggle on the replacement capability and reconciles optimistic state with the authoritative reloaded status. The response keeps the requested spelling, batch results keep request order, and legacy per-target error shapes remain in the SDK for compatibility with older daemons.Why it's needed
#9139 made unknown names valid only for the batch endpoint and intentionally retained runtime validation for installed targets, while the single endpoint remained unchanged. That still made a settings mutation depend on whether a same-named Skill was currently loaded, user-invocable, or owned by an active Extension. As a result, runtime discovery state could reject or transiently undo a valid workspace settings declaration.
Skill enablement is persisted configuration and may be declared before installation or outlive the current catalog. The write contract therefore needs to be based on the settings name, not runtime ownership metadata.
Reviewer Test Plan
How to verify
workspace_skill_toggletag does not expose the corrected toggle behavior, while a client that seesworkspace_skill_settings_toggledoes.Evidence (Before & After)
N/A — the UI change is behavioral capability gating and state reconciliation, covered by component tests; there is no visual asset change.
Tested on
Environment (optional)
Node.js 22.17.0 with npm in an isolated worktree. Focused facade, route, SDK, WebShell, and bundled CLI integration tests passed; the full repository build and typecheck passed; every changed file passed lint, formatting, and
git diff --check.Risk & Scope
workspace_skill_settings_toggleorworkspace_skill_settings_batch_toggle; the retired catalog-validated tags are no longer advertised. Legacy error item types remain available for clients connected to older daemon versions.Linked Issues
Follow-up to #9139.
中文说明
本 PR 做了什么
Daemon 的单个与批量 Skill 开关接口现在会把裁剪后的请求名称直接持久化到 workspace Skill 配置，不再查询已加载的运行时 Skill 目录。未知名称、不可由用户调用的 Skill、来自未激活 Extension 的 Skill，以及被更高配置层禁用的名称，都走同一条 settings 写入路径。更高配置层在计算最终有效状态时仍然拥有优先级，但不再阻止 workspace 记录或移除自己的声明。
现有 endpoint 路径和请求体保持不变。Daemon 用
workspace_skill_settings_toggle和workspace_skill_settings_batch_toggle取代旧的 catalog 校验 capability，让客户端明确选择修正后的 settings 写入契约。WebShell 使用新 capability 开启开关，并在重新加载后用服务端权威状态对账乐观状态。响应保留请求中的名称拼写，批量结果保持请求顺序，SDK 中继续保留旧的逐目标错误结构以兼容旧 daemon。为什么需要
#9139 只让批量接口接受未知名称，并且有意保留了对已安装目标的运行时校验，单个接口则完全没有变化。因此，一次 settings 变更仍然会依赖同名 Skill 当前是否已加载、是否可由用户调用、是否属于已激活的 Extension。运行时发现状态仍可能拒绝或短暂撤销一个有效的 workspace 配置声明。
Skill 启用状态是持久化配置，可以在安装前声明，也可以比当前运行时目录存在得更久。因此写入契约应当基于 settings 名称，而不是运行时所有权元数据。
Reviewer 测试计划
如何验证
workspace_skill_toggle标签的客户端不会暴露修正后的开关行为，而看到workspace_skill_settings_toggle的客户端会暴露。Before / After 证据
N/A — UI 变更仅是 capability 行为门控和状态对账，已由组件测试覆盖；没有视觉资产变化。
已测试平台
环境
在隔离 worktree 中使用 Node.js 22.17.0 与 npm。Facade、route、SDK、WebShell 和真实 bundle CLI 集成专项测试通过；全仓 build 和 typecheck 通过；所有变更文件均通过 lint、format 和
git diff --check。风险与范围
workspace_skill_settings_toggle或workspace_skill_settings_batch_toggle；旧的 catalog 校验标签不再发布。旧错误 item 类型继续保留，用于兼容连接到旧 daemon 版本的客户端。关联
#9139 的后续修复。