feat(skills): add overridable default-disabled state - #7357
Conversation
E2E test reportValidated the latest-main-rebased final bundle at head Daemon HTTP flow:
Real PTY
The daemon, PTY process, and temporary fixtures were cleaned up after verification. |
|
Thanks for the PR! Template looks good ✓ Problem: real and well-evidenced. Issue #7347 describes a genuine gap — Direction: aligned. Skills are a first-class capability and the override surfaces ( Size: this PR touches core paths ( Approach: the three-list precedence model ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实且有据。Issue #7347 描述了一个实际的缺口—— 方向：对齐。Skills 是一等能力，覆盖入口（ 规模：本 PR 触及核心路径（ 方案：三列表优先级模型（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
🩺 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 |
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.
— qwen3.7-max via Qwen Code /review
Code Review —
|
8c4c1ff to
6a86cc9
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Code Review —
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 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. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
) - Fix disabledChanged comparison in SkillsManagerDialog to use previousDisabled (locked names filtered) instead of workspaceDisabled, preventing spurious settings writes when a skill is disabled at both workspace and higher scope - Import SettingScope as a value instead of string-casting literals in skill-settings.ts for compile-time safety - Add dual-key change test: enabling a workspace-hard-disabled default-disabled skill produces both skills.disabled and skills.enabled changes in one operation - Add legacy inactive-extension branch tests: reject when disabledReason is undefined and skill is not in settings disablements; allow when it is disabled by settings
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: Review feedback addressedInline comments[rc:3619128890] The Fix: Changed the comparison to use [rc:3617091523] Dual-key change path untested — IMPLEMENTED Added a test in [rc:3619128870] Legacy inactive-extension branch untested — IMPLEMENTED Added two tests in
Issue-level review findings@wenshao finding #2: Replaced the three string-cast constants ( @wenshao finding #1 (dialog-confirm test), #3 (stale enabled entries), #4 (performance note) — DEFERRED
Verification
中文说明已处理的审查反馈行内评论[rc:3619128890]
修复： 将比较改为使用 [rc:3617091523] 双键变更路径未测试 — 已实现 在 [rc:3619128870] 旧版未激活扩展分支未测试 — 已实现 在
Issue 级审查发现@wenshao 发现 #2： 将 @wenshao 发现 #1（对话框确认测试）、#3（过期 enabled 条目）、#4（性能说明） — 延后处理
验证结果
Base-conflict check: no conflict with main. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 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.
Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: No action takenReview feedback: The automated reviewer (qwen-code-ci-bot) reported 1 Suggestion-level finding that could not be anchored to a changed line and was dropped. No inline comments or issue-level comments require a response. CI failure ( All PR-specific tests pass:
Other checks pass locally: 中文说明未采取行动审查反馈： 自动审查机器人 (qwen-code-ci-bot) 报告了 1 个建议级别的发现，但无法锚定到已更改的行，因此被丢弃。没有需要回复的行内评论或 issue 级别评论。 CI 失败 ( 所有 PR 相关测试均通过：
其他检查在本地通过： Base-conflict check: no conflict with main. 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Could not address the latest feedback automatically (round 2/100). A human should take over this PR. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/29843397391 🧠 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.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
|
🤖 Could not address the latest feedback automatically (round 3/100). A human should take over this PR. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/29850967686 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: Review feedback addressed[rc:3628065665] Safe-mode status/execution disagreement — ImplementedFinding: Decision: Required — real status/execution disagreement affecting clients that rely on the status API (e.g. Web Shell skill picker). Change: Removed the Test: Added Conflict notesNo conflicts ( Verification
中文说明已处理的评审反馈[rc:3628065665] 安全模式下状态/执行不一致 — 已实现发现： 决定： 必须修复——存在真实的状态/执行不一致，影响依赖状态 API 的客户端（如 Web Shell 技能选择器）。 变更： 移除了 测试： 添加了 冲突说明无冲突（ 验证结果
Base-conflict check: no conflict with main. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: Autofix review triage — PR #7357Outcome: no action taken. Feedback triageNo review comments were found to triage — no inline comments, issue-level comments, or review bodies from maintainers or the automated reviewer. Failed checks investigationTest (ubuntu-latest Node 22.x): FAILURE All tests touched by this PR pass locally:
Full package test suites for The only local failures found are in Additional local verification:
Without access to CI logs, the exact failing test in the CI "Test" check cannot be identified, but no evidence links the failure to this PR's changes. review-pr: FAILURE The automated code review check failed but generated no review comments or findings to address. 中文说明Autofix 审查分类 — PR #7357结果：未采取任何操作。 反馈分类未发现需要分类的审查评论——没有行内评论、issue 级别评论或来自维护者或自动审查器的审查正文。 失败检查调查Test (ubuntu-latest Node 22.x): FAILURE 本 PR 涉及的所有测试在本地均通过：
本地发现的唯一失败在 其他本地验证：
由于无法访问 CI 日志，无法确定 CI "Test" 检查中具体失败的测试，但没有证据将该失败与本 PR 的变更相关联。 review-pr: FAILURE 自动代码审查检查失败，但未生成任何需要处理的审查评论或发现。 Base-conflict check: no conflict with main. 🧠 Handled by Qwen Code · model/模型 |
gwinthis
left a comment
gwinthis
left a comment
There was a problem hiding this comment.
Architecture Review — PR #7357
Verdict: ✅ Approve — clean three-tier skill availability model.
Design pattern: Three-tier precedence with disablement tracking
skills.disabled (hard denylist — always wins)
> skills.enabled (explicit opt-in — overrides defaultDisabled)
> skills.defaultDisabled (soft default — project can opt in)
The key insight: skills.disabled is a policy mechanism (operator lock), while skills.defaultDisabled is a default mechanism (starts off, available for opt-in). Conflating them (the old model) made it impossible for a project to enable a skill the user had soft-disabled.
Key design decisions
-
Centralized resolver:
resolveSkillSettings()is the single source of truth for effective skill availability. It replaces 4+ scattered inline normalization blocks (config.ts, run-qwen-serve.ts, workspace-service/index.ts, workspace-skills-status.ts) with one function. Every consumer reads through the same lens. -
Disablement reason + lock scope:
disabledReason: 'hard' | 'default' | 'inactive_extension'+lockedScope— clients can distinguish WHY a skill is disabled and WHETHER it can be overridden. This is what makes the UI toggle correctly gray out locked skills while allowing opt-in for default-disabled ones. -
Orphan preservation: Workspace disables for skills not currently loaded (different git branch, uninstalled extension, deleted skills dir) are preserved verbatim. The picker only mutates currently-loaded skills. This is load-bearing — without it, opening /skills on a different branch would silently drop the user's prior disable settings.
-
Canonical name persistence: Case-insensitive matching but canonical name persisted.
updateTarget()collapses case variants and duplicates in one pass. -
Batch settings mutation:
setValues()for atomic multi-key updates — when enabling a workspace-hard-disabled default-disabled skill, bothskills.disabled(remove) andskills.enabled(add opt-in) change in one write.
Test coverage
228-line skill-settings.test.ts covers the resolver, the workspace list updater, and the batch update computer. The orphan preservation case is explicitly pinned. The integration test in run-qwen-serve.test.ts verifies the dual-write path end-to-end.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: Autofix review pass — no code changesThe new feedback on this PR is a single architecture review that approves Still-red checks were investigated
No commits were made and the working tree is unchanged. 中文说明Autofix 复查结论 —— 无代码改动本轮针对该 PR 的新反馈是一条批准该改动的架构评审（结论：✅ Approve，"清晰的三层 skill 可用性模型"）。它没有指出任何缺陷，也没有行内评论，因此本轮没有需要实现或拒绝的内容。 已排查仍处于失败状态的检查项
未创建任何提交，工作树保持不变。 Base-conflict check: no conflict with main. 🧠 Handled by Qwen Code · model/模型 |
Local verification of #7357 — overridable default-disabled skillsI built this PR from source and verified it end-to-end on my machine. Feature behavior is correct and well-covered, but there is one required-CI merge blocker with a root cause and a verified fix below. Environment: PR HEAD TL;DR
1. Behavior — real
|
| Suite | Result |
|---|---|
config/skill-settings.test.ts |
✅ |
config/config.integration.test.ts |
✅ |
serve/workspace-skills-status.test.ts (real-file daemon status) |
✅ |
serve/workspace-skills-mapping.test.ts |
✅ |
serve/workspace-service/__tests__/facade.test.ts |
✅ |
serve/run-qwen-serve.test.ts |
✅ 191 |
| Total | 330 passed |
I also added two real-file integration checks, because the PR's precedence tests use a fake LoadedSettings and never exercise the real settings.json merge:
- Real multi-scope merge: wrote real
system-defaults/user/workspacesettings, ran the realloadSettings+resolveSkillSettings. ConfirmsdefaultDisabledisUNION-merged across scopes,disabled > enabled > defaultDisabledprecedence holds, case-insensitive canonicalisation works, andlockedScopeis reported (user/systemDefaults). ✅ - Real
/skillsrender + persist: rendered the actualSkillsManagerDialogagainst a realSkillManagerand asserted the exact bytes written back. ✅
3. ⚠️ Merge blocker — serve fast-path bundle closure
Root cause. run-qwen-serve.ts is a serve pre-listen root, and the PR makes it statically import ../config/skill-settings.js:
// run-qwen-serve.ts (top level)
import {
resolveSkillSettings,
skillSettingStrings,
updateWorkspaceSkillSettingLists,
} from '../config/skill-settings.js';skill-settings.js statically imports settings.js, which drags @iarna/toml and glob into the daemon cold-start closure. scripts/check-serve-fast-path-bundle.js forbids exactly those from the pre-listen path:
Serve fast-path bundle closure includes pre-listen runtime modules:
- glob vendor package static path: run-qwen-serve-*.js -> chunk-*.js
- @iarna/toml vendor package static path: run-qwen-serve-*.js -> chunk-*.js
Negative control (so this is the PR, not the environment): with the identical local build method, the merge-base 962f9e0 passes (Startup bundle closure checks passed.) and this PR fails. This matches CI.
Those three symbols are used only inside persistDisabledSkillsFn — a post-listen skill-toggle handler (inside withSettingsLock) — so a dynamic import is safe and mirrors how the sibling workspace-service / workspace-skills-status modules are already dynamically imported in this file.
Verified fix (rebuilt → Startup bundle closure checks passed., and all 191 run-qwen-serve tests still green):
-import {
- resolveSkillSettings,
- skillSettingStrings,
- updateWorkspaceSkillSettingLists,
-} from '../config/skill-settings.js';
@@ persistDisabledSkillsFn … withSettingsLock(workspace, async () => {
+ const {
+ resolveSkillSettings,
+ skillSettingStrings,
+ updateWorkspaceSkillSettingLists,
+ } = await import('../config/skill-settings.js');
const fresh = settingsRuntime.settings.loadSettings(workspace);Note: the ACP side of the same gate is fine — acpAgent.ts's new resolveSkillSettings / loadSettings imports do not pull ink/react into the ACP boundary; that half passes.
Recommendation
The feature itself is solid and matches the described precedence model — I'd be happy to merge once the serve fast-path gate is green. Please apply the dynamic-import fix above (or equivalent) so run-qwen-serve.ts's cold-start closure no longer statically pulls in skill-settings.js → settings.js.
中文说明
本地验证 #7357 —— 可覆盖的“默认禁用”技能
我从源码构建并在本地做了端到端验证。功能行为正确、测试覆盖充分，但有一个必过 CI 的合并阻塞项，下面给出根因和已验证的修复。
环境： PR HEAD 419611ef，从该分支构建的打包 CLI v0.20.1，Node 22.22，Linux。全程不涉及模型请求 —— /skills 是纯设置流程。
结论速览
- ✅ 行为正确 —— 用真实的多作用域
settings.json、真实SkillManager扫描真实SKILL.md、以及真实打包 CLI 在终端里实际操作验证（见下方截图）。 - ✅ 聚焦测试全过 —— 6 个改动文件共 330 个测试通过；我另外补了 4 个真实文件集成测试，覆盖 PR 自带测试（用假的 settings）没覆盖到的部分。
⚠️ 合并阻塞： 必过任务Test (ubuntu-latest, Node 22.x)→ 步骤Check serve fast-path bundle closure失败（CI 记录）。已定位根因并验证修复（merge-base 通过、本 PR 失败、修复后重新通过）。
1. 行为 —— 真实 /skills 选择器（打包 CLI）
按 reviewer 测试计划：用户级 skills.defaultDisabled = ["Opt-In-Skill"]（大小写混合）+ 真实项目技能 opt-in-skill。
（见上方第一张截图）
- ①
opt-in-skill显示为[ ]未选中（默认软禁用）；9 个内置技能仍是[x]—— 说明未选中确实由defaultDisabled驱动，且大小写不敏感匹配。 - ② 空格 →
[x]选中；③ Esc →● Skills configuration saved. - 落盘的工作区
.qwen/settings.json={"skills":{"enabled":["opt-in-skill"]}}—— 写入的是规范名称，且不会写skills.disabled。✔️
高作用域的硬禁用仍然优先，并显示为锁定（见上方第二张截图）：用户级 skills.disabled = ["Opt-In-Skill"] 时，opt-in-skill 进入 “Locked by higher-scope settings”，标注 [locked: User]，在工作区作用域无法切换。✔️
2. 测试
6 个改动文件共 330 个测试通过（其中 run-qwen-serve.test.ts 191 个）。
另外我补了两个真实文件集成测试，因为 PR 自带的优先级测试用的是假的 LoadedSettings，从未走真实的 settings.json 合并：
- 真实多作用域合并： 真实写入 system-defaults / user / workspace 设置，跑真实的
loadSettings+resolveSkillSettings，确认defaultDisabled跨作用域UNION合并、disabled > enabled > defaultDisabled优先级成立、大小写规范化正确、lockedScope（user/systemDefaults）上报正确。✅ - 真实
/skills渲染 + 落盘： 用真实SkillManager渲染真实的SkillsManagerDialog，断言回写的确切内容。✅
3. ⚠️ 合并阻塞 —— serve fast-path 打包闭包
根因。 run-qwen-serve.ts 是 serve 的 pre-listen 根，本 PR 让它静态 import ../config/skill-settings.js；而 skill-settings.js 静态 import settings.js，从而把 @iarna/toml 和 glob 拉进了守护进程冷启动闭包。scripts/check-serve-fast-path-bundle.js 恰好禁止这些模块出现在 pre-listen 路径里。
反向对照（证明是 PR 而非环境问题）： 用完全相同的本地构建方法，merge-base 962f9e0 通过，本 PR 失败，与 CI 一致。
这三个符号只在 persistDisabledSkillsFn（listen 之后的技能切换处理函数，位于 withSettingsLock 内）里用到，所以改成动态 import 是安全的，也与本文件里 workspace-service / workspace-skills-status 已经采用的动态 import 一致。
已验证的修复（重新打包后 → Startup bundle closure checks passed.，且 191 个 run-qwen-serve 测试仍全过）：把顶层静态 import 删掉，改为在 persistDisabledSkillsFn 里 await import('../config/skill-settings.js')（diff 见上方英文部分）。
补充：同一 gate 的 ACP 部分没问题 —— acpAgent.ts 新增的 resolveSkillSettings / loadSettings import 不会把 ink/react 拉进 ACP 边界，那一半是通过的。
建议
功能本身没问题、与描述的优先级模型一致 —— serve fast-path gate 变绿后我很乐意合入。请应用上面的动态 import 修复（或等价方案），让 run-qwen-serve.ts 的冷启动闭包不再静态拉入 skill-settings.js → settings.js。
🤖 Verified locally with Claude Code (Opus 4.8): source build of the bundled CLI, real /skills run in a terminal, real multi-scope settings merge, and a merge-base negative control reproducing the CI gate.
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: Review feedback addressedThanks to @wenshao for the thorough local verification. One required merge 1. Required — serve fast-path bundle closure failure (merge blocker)Finding. The required CI job Decision: fixed. The three symbols ( Change ( -import {
- resolveSkillSettings,
- skillSettingStrings,
- updateWorkspaceSkillSettingLists,
-} from '../config/skill-settings.js';
@@ persistDisabledSkillsFn … withSettingsLock(workspace, async () => {
+ const {
+ resolveSkillSettings,
+ skillSettingStrings,
+ updateWorkspaceSkillSettingLists,
+ } = await import('../config/skill-settings.js');
const fresh = settingsRuntime.settings.loadSettings(workspace);2. Informational — feature behavior and tests confirmed correctThe reviewer verified the real Conflict notes
VerificationCommands actually run on this branch after the change:
中文说明已处理的评审反馈感谢 @wenshao 详尽的本地验证。评审提出了一项必过的合并阻塞项；其余各点均为对功能行为与测试覆盖的正面确认，无需改动代码。 1. 必过项 —— serve fast-path 打包闭包失败（合并阻塞）问题。 必过 CI 任务 处理：已修复。 这三个符号（ 改动（ 2. 信息性 —— 功能行为与测试已确认正确评审验证了真实 冲突说明
验证改动后在本分支实际运行的命令：
Base-conflict check: no conflict with main. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 Handled by Qwen Code · model/模型 |
…ault-disabled # Conflicts: # packages/cli/src/config/settingsSchema.ts # packages/vscode-ide-companion/schemas/settings.schema.json
|
🤖 Addressed the latest review feedback (round 8/100). What changed, and what I pushed back on: Address Review Summary — PR #7357Feedback pointsNo review feedback (reviews, inline comments, issue-level comments, or failed checks) was present in this round. Conflict resolutionMerged
No semantic conflicts: the Verification
中文说明处理审查总结 — PR #7357反馈要点本轮没有审查反馈（无评审意见、行内评论、issue 级别评论或失败的检查）。 冲突解决将
无语义冲突： 验证
Base-conflict check: conflicted with main — resolved in this push. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 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.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
ZijianZhang989
left a comment
ZijianZhang989
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix Review Response — PR #7357No changes made. The latest review round contains no actionable feedback:
No code changes are required for this round. 中文说明Autofix 审查回应 — PR #7357未做任何更改。最新一轮审查没有需要处理的反馈：
本轮无需进行任何代码更改。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Addressed the latest review feedback (round 9/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 9/100 轮）。改动内容与我反驳保留之处如下： Autofix review summary (PR #7357)No review feedback needed addressing this round: the feedback batch contained no Conflict resolution
One auto-merged test needed a follow-up fix: Verification
中文说明Autofix 审查总结（PR #7357）本轮没有需要处理的审查反馈：反馈批次中不包含任何比上次评估更新的审查、行内评论、issue 级评论或失败检查。本轮的工作是解决与 冲突解决
有一个自动合并的测试需要后续修复： 验证
Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
github-actions
Bot
added
the
review/self-reported
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. 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-preview via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review triage — no action takenThe only new feedback on PR #7357 since the last evaluation is a single
This is an informational disclosure of the reviewer's own coverage limit, not Because there is nothing actionable, no code change was made and no review 中文说明Autofix 审查分诊 — 未采取任何操作自上次评估以来，PR #7357 上唯一的新反馈是来自自动审查器
这是审查器对自身审查覆盖范围限制的信息性说明，并非变更请求。该审查为 由于没有任何可操作的内容，因此未做任何代码变更，也没有需要解决的审查线程。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Local re-verification of #7357 (round 2) — head
|
| Area | Result |
|---|---|
| Serve fast-path bundle closure (my earlier blocker) | ✅ fixed — gate passes, and a negative control proves the fix is load-bearing |
Reviewer test plan steps 1–5 via the real /skills TUI |
✅ all pass (screenshots below) |
Reviewer test plan steps 2–5 via a real qwen serve daemon over HTTP |
✅ 14/14 wire checks |
| Does the setting actually withhold the skill (not just uncheck a box)? | ✅ verified at the slash-command layer, both directions |
| Acceptance criteria of #7347 | ✅ all four met |
| Focused suites / typecheck / lint / i18n / schema regen | ✅ 347 tests, all gates clean |
| Mutation matrix (9 mutations of the new logic) | ✅ 9/9 caught — no vacuous tests |
Safe mode (--safe-mode) status vs. execution |
ℹ️ divergence exists, pre-existing on main, not a blocker — details at the end |
1. The merge blocker is fixed — with a negative control
npm run check:serve-fast-path-bundle now prints Startup bundle closure checks passed. at this head. To make sure that is the fix and not my environment, I restored the old static import … from '../config/skill-settings.js' in run-qwen-serve.ts and rebuilt: the gate fails again, pulling glob, chokidar and fzf into the pre-listen closure. Reverting restores green.
2. The real /skills picker
Fixture: a real project with two real project skills, real settings.json files at user and workspace scope, driven through a pty against the bundled CLI.
User-scope skills.defaultDisabled: ["Opt-In-Skill"] (mixed case) → the skill is the only unchecked row; Space checks it:
After Esc, the workspace file is exactly {"skills":{"enabled":["opt-in-skill"]}} — canonical (lower-cased) name, and no skills.disabled key. Opening the dialog and pressing Esc without toggling anything writes no file at all (the workspace .qwen/settings.json never appears).
The picker's save handler — orphan preservation + a dual-key write in one Esc (this was the "no direct test" gap I flagged in my first review; it is now extracted into computeWorkspaceSkillListUpdates and covered, and here it is against real files):
ghost-skill (an entry for a skill that is not loaded) survives verbatim, the case-variant Always-On-Skill is removed from skills.disabled when re-checked, and the opt-in lands in skills.enabled — all from a single dialog close.
User-scope skills.disabled still hard-locks the row:
3. Availability oracle — the setting really withholds the skill
A checkbox proves nothing on its own, so I checked whether the skill is actually registered as a command in a live session:
- default-disabled →
/opt-inoffers nothing, while the control skill completes in the same session; - after a project-scope
skills.enabledopt-in (settings file only, user settings untouched) → the command is back — which is exactly issue feat(skills): add overridable defaultDisabled separate from skills.disabled #7347's ask; - user
skills.disabled+ workspaceskills.enabled→ still withheld, sodisabled > enabled > defaultDisabledholds at execution, not only in the UI.
4. Real daemon over HTTP — 14/14
Started a real qwen serve against the same fixture and walked the reviewer test plan over the wire, reading the workspace settings.json back off disk after every call:
Also confirmed on a live session (not just the deferred path): the dual-key change emits one settings_changed event per changed key (skills.disabled then skills.enabled) on the session SSE stream, and the toggle returns activation: "applied", sessionsRefreshed: 1, sessionsFailed: 0.
5. Tests, gates, and a mutation matrix
skill-settings/config.integration/workspace-skills-mapping/workspace-skills-status/run-qwen-serve/workspace-service facade→ 6 files, 347 tests, all green.npm run typecheck✅ ·npm run lint:ci✅ ·npm run check-i18n✅ ·npm run generate:settings-schema→ no diff (generated vscode schema is in sync).- Mutation matrix (screenshot in §1): I flipped nine load-bearing lines one at a time — the
enabledoverride,lockedScopereporting, case folding, the redundant-opt-in guard, locked-name filtering, orphan preservation, the daemon'sskills.enabledchange emission, the daemon lock rejection, and the status mapping's new fields. All nine turn the suites red, so the new tests are genuinely load-bearing.
6. One note (not a blocker): safe mode reports disablements it does not enforce
With QWEN_CODE_SAFE_MODE=1, GET /workspace/skills reports review as {"status":"disabled","disabledReason":"hard","lockedScope":"user"} and stuck as {"disabledReason":"default"} — but the real CLI in safe mode still registers /review and /stuck. The reason is packages/cli/src/config/config.ts:2048-2049, which passes disabledSkillNamesProvider: undefined in safe/bare mode, so Config.getDisabledSkillNames() is empty and nothing is enforced.
To be clear about ownership: reading the base file, readDisabledSkillNames() on main had no safe-mode gate either, so the skills.disabled half of this divergence pre-dates this PR; the PR extends the same reporting to the new default reason. I mention it only because round 6 (fix(cli): resolve skill disablements in safe mode for status API, test name resolves disablements in safe mode (status matches execution)) reads as if this were settled — in safe mode, execution enforces nothing at all. Worth a follow-up issue rather than more churn here.
Minor, also non-blocking: a skills.enabled entry whose name is no longer in defaultDisabled stays in the file (inert; it self-cleans on the next disable/enable cycle).
Verdict
The feature does what #7347 asked for, the precedence rule holds at every layer I could reach (picker, status API, toggle API, command registration), backward compatibility with plain skills.disabled is intact, and the CI blocker I raised earlier is genuinely fixed. LGTM — happy to merge.
中文说明
本地复验 #7357（第 2 轮）—— head 085398f
在最新一次合并 main 之后，我重新从源码构建并完整跑了一遍端到端验证。我 7 月 22 日提出的合并阻塞项已修复，本轮没有再发现功能性问题 —— 从我这边看可以合入。
环境： PR head 085398f（7 月 25 日合并 main），独立 worktree，npm ci + npm run build + npm run bundle 生成打包 CLI v0.21.0，Node 22.23.1，macOS（arm64）。全程不涉及模型请求 —— /skills、设置解析与 daemon 切换都是纯设置流程（配置的 base URL 是死端口，从未被访问）。
结论速览
| 项目 | 结果 |
|---|---|
| serve fast-path 打包闭包（我此前的阻塞项） | ✅ 已修复 —— gate 通过，并用反向对照证明该修复是必要的 |
用真实 /skills TUI 跑 reviewer 测试计划第 1–5 步 |
✅ 全部通过（见截图） |
用真实 qwen serve daemon 走 HTTP 跑第 2–5 步 |
✅ 14/14 项线上检查通过 |
| 设置是否真的让技能不可用（而不只是取消勾选） | ✅ 在 slash 命令层双向验证 |
| issue #7347 的验收标准 | ✅ 四条全部满足 |
| 聚焦测试 / typecheck / lint / i18n / schema 重新生成 | ✅ 347 个测试，所有 gate 干净 |
| 变异测试（对新逻辑做 9 处变异） | ✅ 9/9 全部被测试捕获，无空转测试 |
安全模式（--safe-mode）状态与执行 |
ℹ️ 存在不一致，但在 main 上即已存在，非阻塞 —— 见文末 |
1. 合并阻塞项已修复（含反向对照）
在该 head 上 npm run check:serve-fast-path-bundle 输出 Startup bundle closure checks passed.。为确认这是修复起作用而非环境差异，我把 run-qwen-serve.ts 中的动态 import 改回旧的静态 import 并重新打包：gate 再次失败，把 glob、chokidar、fzf 拉进了 pre-listen 闭包；改回后重新变绿。
2. 真实 /skills 选择器
用真实项目、两个真实项目技能、用户级与工作区级真实 settings.json，通过 pty 驱动打包 CLI：
- 用户级
skills.defaultDisabled: ["Opt-In-Skill"]（大小写混合）→ 该技能是唯一未勾选行；空格勾选后按Esc，工作区文件恰好为{"skills":{"enabled":["opt-in-skill"]}}（规范小写名，且不会写skills.disabled）。打开对话框但不做任何切换直接Esc，则完全不会写文件。 - 选择器保存逻辑（我第一轮 review 指出缺少直接测试的部分，现已抽成
computeWorkspaceSkillListUpdates并有测试覆盖）：孤儿条目ghost-skill原样保留、大小写变体Always-On-Skill重新勾选后从skills.disabled移除、opt-in 写入skills.enabled—— 一次Esc同时写两个 key。 - 用户级
skills.disabled仍然硬锁定该行，显示在「Locked by higher-scope settings」并标注[locked: User]，关闭对话框不会写任何工作区设置。
3. 可用性验证 —— 设置确实屏蔽了技能
仅看勾选框说明不了问题，因此我检查了技能是否真的注册为可用命令：默认禁用时输入 /opt-in 没有任何补全（同一会话中的对照技能正常补全）；仅通过项目级 skills.enabled（不改用户设置）opt-in 后命令恢复 —— 这正是 issue #7347 的诉求；用户级 skills.disabled + 工作区 skills.enabled 时依然不可用，说明 disabled > enabled > defaultDisabled 在执行层同样成立，而不只是 UI 表现。
4. 真实 daemon HTTP 流程 —— 14/14
启动真实 qwen serve，按 reviewer 测试计划走接口，并在每次调用后回读磁盘上的工作区 settings.json：默认禁用上报 disabledReason: "default"；大小写混合的启用返回 200 并只写入一次规范名；重复启用为 changed:false 且文件不变；禁用后移除 opt-in 并写入规范硬禁用；高作用域硬禁用时上报 lockedScope: "user"，工作区启用被 409 skill_not_toggleable 拒绝且不会创建项目设置文件。
另外在有活跃会话的情况下确认：双 key 变更会在会话 SSE 流上按每个变更 key 各发一次 settings_changed 事件（skills.disabled 与 skills.enabled），接口返回 activation: "applied", sessionsRefreshed: 1, sessionsFailed: 0。
5. 测试、gate 与变异测试
- 6 个相关测试文件共 347 个测试全部通过。
npm run typecheck✅ ·npm run lint:ci✅ ·npm run check-i18n✅ ·npm run generate:settings-schema无 diff（生成的 vscode schema 与源同步）。- 变异测试：逐个翻转 9 处关键逻辑（
enabled覆盖、lockedScope上报、大小写归一、冗余 opt-in 的防护、锁定名过滤、孤儿条目保留、daemon 的skills.enabled变更发布、daemon 锁定拒绝、状态映射新增字段），9 处全部让测试变红，说明新增测试确实有效。
6. 一个说明（非阻塞）：安全模式会上报它并不执行的禁用状态
设置 QWEN_CODE_SAFE_MODE=1 后，GET /workspace/skills 会把 review 报为 {"status":"disabled","disabledReason":"hard","lockedScope":"user"}、stuck 报为 {"disabledReason":"default"}，但安全模式下的真实 CLI 仍然注册 /review 与 /stuck。原因在 packages/cli/src/config/config.ts:2048-2049：safe/bare 模式下传入 disabledSkillNamesProvider: undefined，因此 Config.getDisabledSkillNames() 为空集，什么也不会被执行层拦截。
需要说明归属：查阅 base 上的 readDisabledSkillNames() 同样没有安全模式判断，因此 skills.disabled 这一半的不一致在本 PR 之前就存在；本 PR 只是把同样的上报扩展到新的 default 原因。我提这一点，是因为第 6 轮（提交 fix(cli): resolve skill disablements in safe mode for status API，测试名 resolves disablements in safe mode (status matches execution)）读起来像是已经解决 —— 而安全模式下执行层其实什么都不拦。建议另开 issue 跟进，不必在本 PR 继续折腾。
另一个非阻塞的小问题：若某个名字已不在 defaultDisabled 中，其 skills.enabled 条目会滞留在文件里（无副作用，且在下一次禁用/启用循环中会自动清理）。
结论
该特性实现了 #7347 的诉求，优先级规则在我能触达的每一层（选择器、状态 API、切换 API、命令注册）都成立，与原有 skills.disabled 的兼容性完好，我此前提出的 CI 阻塞项也确实修好了。LGTM —— 可以合入。
Verified locally with Claude Code (Opus 5): source build of the bundled CLI, real /skills pty session, real qwen serve HTTP flow, a fast-path gate negative control, and a 9-mutation matrix.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action neededThe only new feedback on PR #7357 is an issue-level comment from @wenshao containing a comprehensive local re-verification (round 2, head There are no inline comments, no review items, no failed checks, and no change requests. Non-blocking observations (no action taken)
中文说明无需操作PR #7357 上唯一的新反馈是 @wenshao 发布的一条 issue 级评论，包含完整的本地复验结果（第 2 轮，head 没有行内评论、没有 review 条目、没有失败的 CI 检查、也没有变更请求。 非阻塞观察（未采取行动）
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Released in v0.21.1. |
|
|
7 participants
What this PR does
This PR adds an overridable default-disabled skill state.
skills.defaultDisabledsupplies soft defaults,skills.enabledrecords explicit opt-ins, and the existingskills.disabledremains a hard disable that always wins. Skill names are matched case-insensitively and persisted with their canonical names.The same precedence is applied by CLI startup, ACP status, daemon status and persistence, and the existing
/skillspicker:disabled > enabled > defaultDisabled.The implementation is intentionally scoped to settings resolution and the existing status/toggle surfaces. It does not modify core skill loading, skill discovery, extension loading, or locale copy.
Why it's needed
Extensions and higher-level configuration need a way to ship skills turned off by default while still allowing a user or project to opt in. Reusing
skills.disabledcannot express that distinction because it intentionally remains a hard union across scopes.Reviewer Test Plan
How to verify
skills.defaultDisabled; confirm the skill is default-disabled, unlocked, and unchecked in/skills.skills.enabled.skills.disabled, and status becomes hard-disabled.skills.disabled, then try to enable it from a project workspace; confirm the picker shows the higher-scope lock and the daemon rejects the toggle without creating project settings.Focused configuration, status, mapping, facade, ACP, and daemon tests passed. Full build, typecheck, lint, and bundle generation passed. The final bundle was also exercised through the daemon HTTP flow and a real PTY
/skillssession.Evidence (Before & After)
Before, the default-disabled skill is unchecked:
After pressing Space, it is checked, and saving writes
{"skills":{"enabled":["opt-in-skill"]}}without writingskills.disabled:Tested on
Environment (optional)
Node.js 22+, locally rebuilt and bundled CLI, daemon HTTP flow on localhost, real PTY with sandboxing disabled; no model request was sent.
Risk & Scope
skills.disabledbehavior remains hard and unchanged; the new fields are optional.Linked Issues
Fixes #7347
中文说明
本 PR 做了什么
本 PR 新增可覆盖的技能默认禁用状态。
skills.defaultDisabled提供软默认值，skills.enabled记录显式启用，现有skills.disabled仍是始终优先的硬禁用。技能名称按大小写不敏感匹配，并以规范名称持久化。CLI 启动、ACP 状态、daemon 状态与持久化，以及现有
/skills选择器使用相同优先级：disabled > enabled > defaultDisabled。实现范围刻意收敛在设置解析和现有状态/切换界面，不修改 core 技能加载、技能发现、扩展加载或多语言文案。
为什么需要
扩展和更高层级配置需要让技能默认关闭，同时仍允许用户或项目显式启用。复用
skills.disabled无法表达这一差异，因为它仍会在各作用域间合并为硬禁用并集。Reviewer Test Plan
如何验证
skills.defaultDisabled中放入大小写混合的技能名称；确认该技能默认禁用、未锁定，并在/skills中未选中。skills.enabled只保存一次规范名称。skills.disabled，状态变为硬禁用。skills.disabled，再从项目工作区尝试启用；确认选择器显示更高作用域锁定，daemon 拒绝切换，且不会创建项目设置。配置、状态、映射、facade、ACP 和 daemon 的聚焦测试均已通过；完整 build、typecheck、lint 和 bundle 生成已通过。最终 bundle 还通过 daemon HTTP 流程和真实 PTY
/skills会话验证。证据（之前与之后）
之前，默认禁用技能未选中：
按 Space 后变为选中；保存会写入
{"skills":{"enabled":["opt-in-skill"]}}，且不会写入skills.disabled：测试平台
环境（可选）
Node.js 22+、本地重新构建并打包的 CLI、localhost daemon HTTP 流程、关闭 sandbox 的真实 PTY；未发送模型请求。
风险与范围
skills.disabled的硬禁用行为保持不变；新增字段均为可选。关联 Issue
Fixes #7347