fix(serve): allow preinstall skill batch toggles - #9139
Merged
Conversation
Treat names absent from the installed Skill snapshot as valid batch targets so their workspace state can be declared before installation. Preserve the existing persistence semantics: an undeclared enable is a no-op, an existing workspace disable is removed, and disable writes the declaration.
Collaborator
Author
E2E Test ReportStatus: PASSED The bundled CLI was rebuilt and started as a daemon on an ephemeral loopback port with isolated workspace, home, runtime, and trust state. All requests targeted a valid Skill name absent from the runtime catalog.
Additional verification:
The sandboxed daemon could not bind loopback ( |
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, at the API-contract level — since the batch toggle landed (#8664), any valid-but-uninstalled Skill name deterministically returns Direction: aligned. The batch endpoint is exposed through the SDK ( Size: not core infrastructure. 10 production logic lines ( Approach: the scope feels right — the existence gate is the whole problem, and dropping it in the batch validation loop while keeping hidden / inactive-extension / higher-scope-locked per-target errors is the minimal change. Updating the design doc in the same PR is the right call. One question to keep in mind during review: the single-name toggle ( Risk: no elevated risk signals — no high-risk paths matched, no core modules touched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到（API 契约层面）——批量开关上线（#8664）以来，任何合法但未安装的 Skill 名称都会确定性地返回 方向：对齐。批量端点已通过 SDK（ 规模：未触及核心基础设施。生产逻辑 10 行（ 方案：范围合理——存在性门槛就是问题本身，在批量校验循环中移除它、同时保留 hidden / inactive-extension / 更高层级锁定的逐目标错误，是最小改动。在同一 PR 中同步更新设计文档是正确做法。review 中值得留意的一个问题：单名称开关（ 风险：无升级风险信号——未命中高风险路径，未触及核心模块。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent proposal before reading the diff: the existence gate only lives in the facade's validation loop — the persistence layer is purely name-based ( What I verified around the diff:
No correctness, security, or convention issues found. The change is exactly the minimal 10-line production diff the description promises, and it reuses the existing persistence machinery instead of adding parallel logic. CI test evidence (the PR's own CI, read via API — no PR code executed here)The main Linux suites are still running on the reviewed commit. The macOS/Windows test jobs and the no-sandbox integration job are merge-queue-only by design ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the remainder: Real-scenario testing: N/A — this is an unattended CI run, so no PR code is executed; live daemon behavior is exercised only by the isolated 中文说明代码审查读 diff 前的独立方案：存在性门槛只在 facade 的校验循环里——持久化层是纯名称语义（ 围绕 diff 核实的内容：
未发现正确性、安全性或规范问题。改动恰为描述所承诺的最小 10 行生产 diff，复用既有持久化机制而非新增并行逻辑。 CI 测试证据（读取本 PR 自身 CI——此处未执行任何 PR 代码）主 Linux 套件在受审 commit 上仍在运行。macOS/Windows 测试与无沙箱集成任务按设计仅在 merge queue 运行（ （CI 表格见英文部分，finalize 工作流会在 CI 结束后原地更新该区域。） 沙箱验证可了结剩余部分： 真实场景测试：不适用——本次为无人值守 CI 运行，不执行 PR 代码；实际 daemon 行为仅由隔离的 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — clean, minimal review of a semantics refinement by the author of the API being refined; only non-blocking notes are confirming the single/batch endpoint divergence is intentional and that the author's smoke run was macOS-only. Stepping back: this is the author of the batch toggle API (#8664, merged last week) closing a real gap in its contract. The existence check made workspace skill state order-dependent — you could not declare "this Skill is disabled here" until after installation, while the settings model itself already treats such orphan declarations as first-class, preserved state. Removing the gate in the batch path only (single toggle keeps its direct feedback, hidden/inactive/locked errors intact) is exactly the minimal edit, and the design doc moved with the code. My independent proposal before reading the diff was the same shape; I found no simpler path it missed, no correctness or security issues, and the new tests pin the change where it lives (the facade), with the real-settings harness covering the persistence semantics. Two notes, neither blocking: the single-name endpoint now diverges from the batch endpoint for uninstalled names — the PR says that's deliberate scope, worth a one-line confirmation from the author; and the manual daemon verification was macOS-only, which the Verdict: approve. CI is still running on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 对被完善 API 的语义 refinement 做了一次干净、最小的审查，作者正是该 API 的作者；仅有的两条非阻塞备注是确认单名称/批量端点的分叉为有意为之，以及作者的冒烟验证仅在 macOS 上进行。 退一步看：这是批量开关 API（#8664，上周合入）的作者本人在修补其契约中的真实缺口。存在性检查使 workspace skill 状态依赖执行顺序——安装完成前无法声明"此 Skill 在这里被禁用"，而设置模型本身早已把这类孤立声明当作一等公民予以保留。只在批量路径移除该门槛（单名称开关保留直接反馈，hidden/inactive/locked 错误保持原样）恰为最小改动，设计文档也随代码同步。我在读 diff 前的独立方案与之同形；没有找到更简的路径、正确性或安全问题，新测试在改动所在处（facade）固定了行为，真实设置文件的集成测试覆盖了持久化语义。 两条备注均不阻塞：单名称端点现在对未安装名称与批量端点行为分叉——PR 声明这是有意的范围取舍，值得作者一句话确认；手工 daemon 验证仅在 macOS 上进行，若维护者希望合并前获得 Linux 端到端证明，我审查评论中的 结论：批准。受审 commit 的 CI 仍在运行，因此批准推迟到该 commit 的 CI 全绿后执行——finalize 工作流将在所有 check 绿后以 commit 锚定方式提交批准；若有 check 变红或 head 移动则不会批准。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these 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.
LGTM, looks ready to ship — CI landed green after the review. ✅
ytahdn
reviewed
Aug 14, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: review dimensions 0, 3a/3b/3c, 4, 5, 6a/6b/6c, 7 — skipped per user request (no test/build agents; total cap of 5 agents).
Not reviewed: build-and-test — the CI test jobs (Test macos/windows, Integration Tests, build-cli) did not verify this change at the reviewed commit and no local suite ran (Agent 7 skipped per user request).
Not explored to full depth (tool budget reached): "本 PR（QwenLM/qwen-code#9139）让 workspace 批量 Skill 开关接受未安装的…": 未运行测试套件（改动仅验证逻辑），未检查 TUI/Web-Shell 对 settings_changed 事件中名称的渲染路径（React/Ink 默认转义，未发现 dangerouslySetInnerHTML 证据即不深挖）。.
Not reviewed: the linked-issue fidelity pass, the reuse and duplication pass, the altitude and abstraction pass, the consistency and clarity pass, the performance pass, the test-coverage pass, the open-ended audit (attacker mindset), the open-ended audit (oncall mindset), the open-ended audit (maintainer mindset), the build-and-test check — no record shows its brief reaching an agent, so this dimension was reviewed, if at all, from a prompt the run wrote for itself.
Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：review dimensions 0, 3a/3b/3c, 4, 5, 6a/6b/6c, 7 — skipped per user request (no test/build agents; total cap of 5 agents)。
未审查：build-and-test — the CI test jobs (Test macos/windows, Integration Tests, build-cli) did not verify this change at the reviewed commit and no local suite ran (Agent 7 skipped per user request)。
未探索到全部深度（达到工具调用预算）："本 PR（QwenLM/qwen-code#9139）让 workspace 批量 Skill 开关接受未安装的…"：未运行测试套件（改动仅验证逻辑），未检查 TUI/Web-Shell 对 settings_changed 事件中名称的渲染路径（React/Ink 默认转义，未发现 dangerouslySetInnerHTML 证据即不深挖）。。
未审查：关联 issue 一致性检查、复用与重复代码检查、修复层次与抽象合理性检查、一致性与可读性检查、性能检查、测试覆盖检查、开放式审计（攻击者视角）、开放式审计（值班排障视角）、开放式审计（后续维护者视角）、构建与测试验证——没有记录表明它的 brief 到达过任何 agent，这个维度即便被审查过，也是基于这次 run 自行编写的 prompt。
未审查：反向审计——没有审计 agent 是用本 skill 构建的 prompt 启动的——负责搜寻评审其余部分遗漏问题的这道工序，即便运行过，也缺失了 brief 承载的方法。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)
Comment on lines
+30
to
+31
| that are not installed remain valid so callers can declare their state before | ||
| installation. Enabling one removes a matching workspace `skills.disabled` |
Collaborator
There was a problem hiding this comment.
[Suggestion] The batch contract changed but the canonical protocol reference and the SDK batch error union were not updated — Failure scenario: a client/SDK implementer working from docs/developers/qwen-serve-protocol.md § POST /workspace/skills/enable writes a handler for errors[].code === 'skill_not_found' in batch responses to detect typos; it never fires after this PR, while a misspelled name sent with enabled: false is silently persisted into workspace skills.disabled with changed: true and HTTP 200. The protocol doc's example response (missing → skill_not_found in errors) describes a shape the endpoint no longer produces. Suggested fix: update the protocol doc's batch section to the new semantics (unknown names are valid targets; disable writes skills.disabled, enable is a no-op that removes a matching entry), including the example response, drop skill_not_found from the batch target-error sentence, and trim 'skill_not_found' from DaemonSkillBatchToggleErrorCode in packages/sdk-typescript/src/daemon/types.ts — it is unreachable for the batch endpoint (the single-skill 404 is a separate HTTP surface).
中文说明
批量端点契约已变更，但规范文档与 SDK 类型未同步：docs/developers/qwen-serve-protocol.md 的 POST /workspace/skills/enable 章节仍记录未知名称返回 skill_not_found（示例响应仍展示 missing → errors），DaemonSkillBatchToggleErrorCode 也仍包含该错误码。按文档实现的客户端将永远收不到该错误，而拼错的名称会在 enabled: false 时被静默写入 skills.disabled（changed: true, HTTP 200）。建议同步更新协议文档（未知名称是合法目标、禁用写 skills.disabled、启用是移除匹配项的 no-op），并从 SDK 批量错误联合类型中移除 'skill_not_found'。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)
Comment on lines
962
to
+965
| if (!skill) { | ||
| domainError = new WorkspaceSkillNotFoundError(requestedName); | ||
| } else if (skill.userInvocable === false) { | ||
| targets.push({ requestedName, skillName: requestedName }); | ||
| continue; | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] Batch and single-Skill toggle endpoints now diverge for unknown names — Failure scenario: a client toggling an uninstalled skill future-skill gets POST /workspace/skills/future-skill/enable → 404 (not persisted), but POST /workspace/skills/enable with {"skillNames":["future-skill"],"enabled":false} → 200, changed: true, and workspace skills.disabled now contains future-skill. The divergence is deliberate and documented in the design doc, but it is invisible in the client-facing protocol reference and is not pinned by a consistency test. Suggested fix: state the divergence explicitly in the protocol doc (which merges with the batch-section rewrite) and add a consistency test pinning the two endpoints' different handling of the same unknown name — do not align the single endpoint's 404.
中文说明
本 diff 让批量端点接受未安装名称，而单数端点仍对未知名称返回 404 skill_not_found（index.ts:827）。该分裂是设计文档明确支持的刻意设计（单数端点保持不变），但面向客户端的协议文档对此不可见，且没有一致性测试固定两端点对同一未知名称的不同处理。建议：在协议文档中明示该分裂（可与批量章节重写合并），并补充一致性测试；不要改变单数端点的 404 行为。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)
ytahdn
approved these changes
Aug 14, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
Approve. The change is minimal (10 source lines), the packages/cli unit suite is green on ubuntu (Test (ubuntu-latest, Node 22.x) pass), and no blockers were found across correctness, removed-behavior, cross-file, and security review. Two non-blocking follow-ups from the review are worth tracking: sync docs/developers/qwen-serve-protocol.md's batch section and the SDK DaemonSkillBatchToggleErrorCode union with the new unknown-name semantics, and add a consistency test pinning the single-vs-batch divergence.
批准。改动很小（源码 10 行），packages/cli 单元测试（ubuntu）已跑绿，正确性/删除行为/跨文件/安全四个维度的审查均未发现阻塞问题。两条非阻塞跟进项建议跟踪：同步 qwen-serve-protocol.md 批量章节与 SDK DaemonSkillBatchToggleErrorCode 联合类型；补充单数/批量端点行为分裂的一致性测试。
Collaborator
|
Released in v0.21.12. |
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
3 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 workspace batch Skill toggle now accepts valid Skill names that are not currently installed, allowing callers to declare disabled state before installation. Enabling an uninstalled Skill follows the existing disabled-list semantics: it removes a matching workspace disable declaration, or returns an unchanged no-op when no declaration exists; it does not create a
skills.enabledentry solely because the Skill is absent.Installed Skills keep their existing validation and activation behavior. Hidden, inactive-extension, and higher-scope-locked Skills still return per-target errors without blocking valid targets in the same batch.
Why it's needed
The batch endpoint previously treated the installed Skill snapshot as an existence gate, so a caller could not predeclare state for a Skill that would be installed later. This made configuration order-dependent and prevented provisioning workflows from writing the desired workspace state ahead of installation.
Reviewer Test Plan
How to verify
Start the daemon with an isolated workspace and submit a valid uninstalled Skill name to the batch enable endpoint. With
enabled: trueand no prior declaration, expect HTTP 200,errors: [], andchanged: false, with no settings file write and noskills.enabledentry. Submitenabled: falseand expect the name in workspaceskills.disabled; submitenabled: trueagain and expect only that disabled declaration to be removed. In a mixed batch, confirm installed valid Skills still refresh once while hidden, inactive-extension, and higher-scope-locked Skills retain their existing per-target errors.Evidence (Before & After)
Before: a valid uninstalled target returned
skill_not_foundand was omitted from the persistence batch. After: the built daemon accepts the target, preserves the no-op/remove-disabled/write-disabled semantics above, and leaves authentication and input validation unchanged.Tested on
Environment (optional)
Node.js v22.17.0; rebuilt
dist/cli.jsdaemon on an ephemeral loopback port with isolated workspace and home directories.Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
workspace 批量 Skill 开关现在接受当前尚未安装的合法 Skill 名称，因此调用方可以在安装前声明禁用状态。对未安装 Skill 执行启用仍遵循现有的 disabled-list 语义：如果 workspace 已有对应的禁用声明则移除；如果没有任何声明则返回未变更的 no-op；不会仅仅因为 Skill 尚未安装就创建
skills.enabled条目。已安装 Skill 的既有校验与生效行为保持不变。隐藏 Skill、扩展未激活 Skill、以及被更高层级锁定的 Skill 仍返回逐目标错误，并且不会阻塞同一批次中的合法目标。
为什么需要
此前批量接口把已安装 Skill 快照当作存在性门槛，因此调用方无法为稍后才会安装的 Skill 预先声明状态。这使配置流程依赖执行顺序，也阻止了部署流程在安装前写入期望的 workspace 状态。
Reviewer 测试计划
如何验证
使用隔离 workspace 启动 daemon，并向批量启用接口提交一个合法但未安装的 Skill 名称。当
enabled: true且此前没有声明时，应返回 HTTP 200、errors: []和changed: false，不写 settings 文件，也不创建skills.enabled。提交enabled: false后，应在 workspaceskills.disabled中看到该名称；随后再次提交enabled: true，应只移除这条禁用声明。在混合批次中，还应确认已安装的合法 Skill 仍只刷新一次，而隐藏 Skill、扩展未激活 Skill、以及被更高层级锁定的 Skill 继续返回既有逐目标错误。证据（修复前后）
修复前：合法但未安装的目标返回
skill_not_found，并被排除在持久化批次之外。修复后：重新构建的 daemon 接受该目标，保持上述 no-op 启用、写入禁用、移除禁用语义，同时鉴权与输入校验保持不变。已测试平台
环境（可选）
Node.js v22.17.0；使用隔离 workspace 与 home 目录，在临时 loopback 端口运行重新构建的
dist/cli.jsdaemon。风险与范围
关联 Issue
无