fix(serve): resolve skill identity from the catalog for toggle guards - #11831
Conversation
Co-authored-by: Codex <noreply@openai.com> Agent-Id: agent-a304b4b8-80dd-4b84-ab54-17d0f7ab5cf7
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
Thanks for the PR — this one is easy to get behind. It implements exactly "PR A" from #11699 (items 1 and 2), which the issue says belong together with item 1 fixed first, and it follows the direction the issue prescribed rather than inventing a new one. Template looks good ✓ — every required section is filled in, and both design-doc languages are linked and updated in the same change (the new Problem: observed, not theoretical. #11699 item 1 names the exact code ( Direction: aligned. Worth naming one contrary signal honestly: the reference agent went the other way on this exact ambiguity and rejected Size: core paths are touched ( Approach: the scope feels right and I don't see anything to cut. I wrote down my own plan before reading the diff, and it landed in the same place: resolve the name against the authoritative list, no alias for non-extension levels, strip the known Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. On the core-path gate I can name every consumer of the changed signature: both production call sites (the single and batch persistence closures, exactly the two the issue identified) and the two unit tests. All four are updated, and typecheck is green, so nothing is left calling the old string form. Two things I checked because they were the plausible failure modes, both fine: the catalog read sits inside the per-workspace settings lock, but it is bounded — Moving on to code review. 🔍 中文说明感谢贡献！这个 PR 很容易支持——它正好实现了 #11699 里的 "PR A"（第 1、2 项），而该 issue 明确说这两项应放在同一个 PR 里，且必须先修第 1 项；实现也遵循了 issue 给出的方向，而不是另起一套。 模板完整 ✓ —— 所有必填章节都写清楚了，设计文档的中英文版本在同一次改动中一起更新并互相链接（新增的 问题： 是已观测到的，不是理论性加固。#11699 第 1 项点名了具体代码（ 方向： 对齐。这里也如实说明一个相反信号：参考 agent 在同一处歧义上走了另一条路，直接禁止 agent 名称包含 规模： 触及核心路径（ 方案： 范围合理，我看不出有什么该砍的。我在读 diff 之前先写下了自己的方案，结论落在同一处：用权威列表解析名称，非 extension 层级不设别名，extension 层级剥掉已知的 风险： 无升级风险信号——改动文件都不匹配与回滚相关的高风险路径。关于核心路径门禁，我能点名这次签名变更的每一个消费者：两个生产调用点（单项与批量持久化闭包，正是 issue 指出的那两处）以及两个单测。四处全部更新，类型检查通过，因此没有遗留仍按旧的字符串形式调用的地方。 有两点因为是可能的失效模式所以我专门查了，结论都没问题：目录读取位于逐工作区设置锁内部，但它是有界的—— 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Evidence carried here: this is an unattended CI run, so per the review rules I did not build, run, or test any of this PR's code. The testing section below quotes the PR's own CI check names and conclusions fetched from the API for Code reviewNo correctness blockers. I went looking for the ways this could plausibly be wrong, and the load-bearing assumptions all hold:
The new tests are the strong part: four behavioural cases, and crucially a negative one asserting the legacy bare-name block still bites both for extension skills and for names absent from the catalog ( Three non-blocking notes, none of which I'd hold the PR for:
Sequence: where the catalog read sitssequenceDiagram
participant P1 as Client (Web Shell or SDK)
participant P2 as Enable route
participant P3 as Settings lock
participant P4 as Identity resolver
participant P5 as ACP child catalog
participant P6 as Block guard
P1->>P2: enable request (names trimmed, deduped)
P2->>P3: acquire per-workspace lock
P3->>P4: resolve identities once for whole request
P4->>P5: getWorkspaceSkillsStatus (timeout bounded)
P5-->>P4: catalog or initialized false
P4-->>P3: name plus authoredName, or legacy fallback
loop bounded passes over refused items only
P3->>P6: block guard with resolved identity
P6-->>P3: block, or clear to apply
end
P3->>P3: single settings write
P3-->>P1: changed plus outcomes
Testing — the PR's own CI at
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | success |
| Test (macos-latest, Node 22.x) | skipped |
| Test (windows-latest, Node 22.x) | skipped |
| Lint & Static (ubuntu-latest, Node 22.x) | success |
| Integration Tests (no-AK, No Sandbox) | success |
| Integration Tests (CLI, No Sandbox) | skipped |
| Serve A/B (ubuntu-latest, Node 22.x) | success |
| Real daemon E2E / Java 11 | success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | success |
| Desktop Shell (ubuntu-22.04) | success |
| Desktop Shell (windows-2022) | success |
| TUI parity snapshots (ink vs opentui) | success |
| OpenTUI no-flicker gate | success |
| Classify PR | success |
| ubuntu-latest / Java 11 | success |
| ubuntu-latest / Java 17 | success |
| ubuntu-latest / Java 21 | success |
| macos-latest / Java 21 | success |
| windows-latest / Java 21 | success |
| precheck-pr / precheck | success |
| review-pr | in_progress (bot job, not PR CI) |
| triage | in_progress (this run) |
Sandboxed verification would settle the remaining gap: @qwen-code /verify — specifically, that with a live ACP child supplying a real catalog, enabling a cataloged extension skill still honours a higher-scope bare-name deny, and that a cataloged non-extension skill whose name contains a colon actually persists changed: true. That is the branch the mocked bridge stands in for, and it is the one claim in this PR that neither static review nor the green suite substantiates. The author does not have write access, so this would be a sponsored run: a maintainer's @qwen-code /verify comment approves the head it was written against, and that run carries a pre-execution risk screen plus a full workspace wipe before any PR code runs. Worth reading the resulting report with the same skepticism as the fork's own CI logs — the code under verification is adversarial input and can shape what a report says, even though the sandbox bounds what it can do.
中文说明
本节所承载的证据： 这是一次无人值守的 CI 运行，因此按照审查规则，我没有构建、运行或测试本 PR 的任何代码。下方测试部分引用的是通过 API 取到的、cbfb999 上本 PR 自己的 CI 检查名称与结论，以及独立运行的 Serve A/B 结果。所有关于代码的判断都来自将 diff 与基线代码树进行的静态阅读。
代码审查： 没有发现正确性阻塞项。我专门去找这个改动可能出错的地方，几个关键假设都成立：前缀剥离是可靠的（SkillManager 用 qualifySkillName 纯拼接生成注册名，且 mapSkillConfigToStatus 只在 level === 'extension' 时输出 extensionName，因此那个 || !skill.extensionName 分支是恰当的防御而非放行）；不存在未裁剪字符串的隐患（两个 REST 解析器和两个 service 方法都会 trim，blockIn 还会再 trim 一次，所以按长度切片不会错位）；空的 authoredName 不会造成问题（skillRestrictionNames 会把它丢掉）；不动点循环能够终止且不会削弱阻止语义（受 skillNames.length 轮上限约束，并在没有项目被拒绝或列表未变化时提前 break；工作区作用域的豁免条件原样保留，因此只有本批次自己移除的 Workspace 级 disabled 条目才可能被越过，User/Project 级阻止每一轮都会拒绝）；单次写入语义保持不变（setValues 仍在循环后调用一次，测试断言 toHaveBeenCalledOnce()）；被删掉的那段文档注释删得对——它描述的标点推断已不复存在，留着反而是在描述假话。
新增测试是这次改动最扎实的部分：四个行为用例，尤其关键的是一个反向用例，断言旧式裸名阻止对 extension 技能以及目录中不存在的名称（rust:legacy）依然生效。这正是宽松修法会悄悄引入的回归。
三点非阻塞意见，我都不会因此卡住这个 PR：
buildHigherDisabled(fresh)是循环不变量，却按每个项目、每一轮重复计算。 循环内被修改的只有next，fresh从未变动，而每次skillToggleBlockForName调用都会重建整个更高作用域索引。旧代码在这里是 O(n)，不动点把最坏情况推到批次上限 100 时的 O(n²)。它只是在已加载的设置上做纯内存 Map/Set 构造，且常见情况一两轮就收敛，所以不是现实问题——但把索引提到循环外就能彻底消除这个疑问。- 解析器里的空
catch {}会静默吞掉所有目录读取失败。 周边代码对同类降级是有日志的（readWorkspaceSkillsStatus在查询失败和本地 provider 失败时都会写 stderr）。如果目录读取出现系统性失败，每次启用都会悄悄走回退路径而毫无提示。加一行 debug 日志就能让它可见。 - 注册名冲突时按 Map 插入顺序决定。 如果 extension 技能
rust:pdf与另一个同名的非 extension 技能同时存在于目录中，status.skills里靠后的那个会赢得小写键，extension 那个可能丢掉pdf别名。我核实了这是否会绕过运维的硬禁用——不会：更高作用域的disabled条目按不区分大小写合并，且限制会同时匹配两种拼写，因此该技能在运行时仍然是被禁用的。影响范围更窄——路由会对一个合并配置其实仍拒绝的启用返回changed: true，而这恰恰是门禁自身注释所说要防止的情况。触发它需要相当特殊的组合（名为rust的扩展提供pdf，另有手写的rust:pdf，再加一条裸名pdf禁用）。优先选择 extension 层级的条目，或在同一名称映射到多个条目时保留别名，可以使其行为确定。
测试： 本 PR 触发的检查全部通过。Qwen Code CI、Serve A/B、tui-parity 作为 pull_request 工作流运行全部 success 完成，无待定项，因此这是已确定的结果而非运行中的快照。没有检查为红，所以没有失败日志可引用。这里最有价值的单一信号是 Serve A/B（ubuntu-latest, Node 22.x）：success —— 该任务分别构建基线与 head，对两者驱动同一组固定端点并比对 JSON，其发布结论是"在 12 个场景上相对 PR 基线无响应变化"，独立印证了本 PR 关于响应契约未变的说法。鉴于本 PR 重构了批量持久化循环，这正是你最希望用来确认没有东西泄漏到线上结构里的检查。
关于覆盖率，有两点需要如实说明：单元测试矩阵只在 Linux 上运行——Test (macos-latest, Node 22.x) 与 Test (windows-latest, Node 22.x) 均为 skipped，Integration Tests (CLI, No Sandbox) 同样如此。这是工作流自身的矩阵条件，不是失败，且本 diff 没有平台相关内容；但作者的"已测试平台"表只声明了 macOS，因此两边加起来没有任何 Windows 或 macOS 的执行证据。目录路径始终只对着被 mock 的 bridge 验证——captureSkillPersistence 用桩替换 queryWorkspaceStatus 返回手工构造的状态列表。作为解析器的单元测试这是对的做法，但意味着本 PR 没有任何测试通过存活的 ACP 子进程驱动身份解析；而 PR 自己的"已知限制"说明没有存活子进程时守护进程本地目录会遗漏 extension 技能。因此 extension 技能分支（level === 'extension' → 剥离前缀）是对着 mock 证明的，而非对着真实守护进程。未验证：extension 分支在真实守护进程下的行为，并且单元测试按其构造无法弥补这一缺口。
沙箱化验证可以解决剩余缺口：@qwen-code /verify —— 具体而言，验证在有存活 ACP 子进程提供真实目录时，启用一个已收录的 extension 技能仍然遵守更高作用域的裸名禁用，以及启用一个名称含冒号的已收录非 extension 技能确实会持久化为 changed: true。这正是 mock bridge 所替代的分支，也是本 PR 中静态审查与绿色测试套件都无法证实的唯一主张。作者没有写权限，因此这将是一次受助运行：由维护者发出 @qwen-code /verify 评论来批准其所针对的 head，且该运行会在执行任何 PR 代码前带有执行前风险筛查与完整工作区清除。阅读由此产生的报告时，建议保持与看待 fork 自身 CI 日志同样的审慎——被验证的代码是对抗性输入，即使沙箱限定了它能做什么，它仍可能影响报告说什么。
— Qwen Code · qwen3.8-max-2026-09-02
Reviewed at cbfb9990b227e7cbfc5aaed542b243b4a8897c2e · re-run with @qwen-code /triage
|
Confidence: 4/5 — clean review across every stage; the only reservations are two small robustness nits and one branch that CI cannot reach by construction, none of them blocking. My independent proposal, written before I opened the diff, was: resolve the requested name against the authoritative catalog, drop the alias entirely for non-extension levels, strip the known What I'd thank the author for in six months is the test set, not the fix. The fix is eleven net lines deleted from the core file. The tests include the negative case — that a higher-scope bare-name deny still bites for an extension skill and for a name the catalog has never heard of. That's the assertion a lazier version of this change would have dropped, and dropping it is how a correctness fix quietly becomes a security regression. The batch test also asserts one settings write and that a fully-blocked batch writes nothing, which pins the two invariants most likely to be broken by a future edit to the loop. The judgement call I want to be explicit about, since it's the one place this PR makes behaviour looser: for a cataloged non-extension skill whose name contains a colon, a higher-scope bare entry that used to block the enable no longer does. That is not an oversight — it is the bug being fixed, and #11699 states the reasoning ( Reservations, all non-blocking and all named in the Stage 2 comment: the higher-scope block index is rebuilt per item per pass inside the fixpoint when it is loop-invariant (O(n²) at the batch cap of 100, harmless in practice); the resolver's empty Scope is exactly the "PR A" split #11699 prescribed, with items 3–8 explicitly left out and named as out of scope. No drive-by refactors, no formatting churn, and both design-doc languages updated together. CI is green with nothing pending on this commit, so I'm approving against 中文说明信心：4/5 —— 各阶段审查均无问题；保留意见只有两处小的健壮性细节，以及一个 CI 按其构造无法覆盖的分支，均不构成阻塞。 我在打开 diff 之前写下的独立方案是：用权威目录解析请求名称，非 extension 层级彻底不设别名，extension 层级剥掉已知的 半年后我会感谢作者的是那组测试，而不是修复本身。修复只是从核心文件里净删了 11 行。测试里包含了那个反向用例——更高作用域的裸名禁用对 extension 技能、以及对目录从未听说过的名称依然生效。这正是一个偷懒版本的改动会省掉的断言，而省掉它就是一次正确性修复悄悄变成安全回归的方式。批量测试还断言了只写入一次设置、以及被完全阻止的批次不产生任何写入，这钉住了后续修改循环时最容易被破坏的两个不变量。 我想明确说明这里的判断取舍，因为这是本 PR 唯一让行为变宽松的地方：对于一个已收录、名称含冒号的非 extension 技能，此前会阻止启用的更高作用域裸名条目现在不再阻止。这不是疏忽，而正是被修复的 bug，#11699 给出了理由（ 保留意见全部非阻塞，且都已在 Stage 2 评论中点名：更高作用域阻止索引在不动点循环内按每个项目、每一轮重建，而它是循环不变的（批次上限 100 时为 O(n²)，实际上无害）；解析器的空 范围正好是 #11699 规定的 "PR A" 拆分，第 3–8 项被明确排除并列为不在范围内。没有顺手重构，没有格式化噪音，设计文档的中英文版本一起更新。该提交上 CI 全绿且无待定项，因此我直接针对 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 3b": none — no check was cut short..
Test Plan (not a blocker): Tests 608 passed — this review observed 31266 passed; 459 passed — this review observed 31266 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 3b"：none — no check was cut short.。
Test Plan（非阻断）：Tests 608 passed — this review observed 31266 passed; 459 passed — this review observed 31266 passed。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const blocked = await persistDisabledSkillsBatch!( | ||
| workspace, | ||
| ['rust:locked'], |
There was a problem hiding this comment.
[Suggestion] R1-1: The rewritten batch loop has no test for a mixed enable batch — one name permanently refused by a higher-scope block while another is applied in the same request — which is exactly the shape the rewrite makes newly possible to get wrong.
The loop now pre-fills outcomes with {changed:false} for every requested index, overwrites an entry only when that index is processed out of pending, and accumulates the single write in next across passes. Every multi-name batch in the suite resolves all names (the order-independence case forward and reversed, and the three-name alias chain), and the only refusal case is this one-name batch, which asserts the opposite shape — expect(setValues).not.toHaveBeenCalled(). So a regression in which a permanent higher-scope refusal poisons its siblings, returning all-changed:false with no write, would make the common Web Shell multi-select ("enable these four skills", one of which a user-scope skills.disabled entry locks) report changed:false for the three selectable skills and persist nothing, with the whole suite green. Nothing currently distinguishes "refused" from "the batch silently no-oped".
Worth knowing, because it changes which mutant to write: the obvious one — an early return { outcomes, settingsChanges: [] } in the refusal branch — is caught, since both shipped multi-name batches contain transient Workspace-scope refusals that the fixpoint later clears. It is the permanent-refusal mutant that survives all seven shipped tests.
Witness:
MUTANT (permanent higher-scope refusal poisons the batch:
`if (block.scope !== 'Workspace') higherRefused = true;` then after the loop
`if (higherRefused) return { outcomes: all changed:false, settingsChanges: [] };`)
PASS uses catalog identity for a non-extension skill whose name contains a colon
PASS preserves the legacy bare-name block for extensions and uncatalogued skills
PASS makes a batch enable order-independent without bypassing a user block
PASS rechecks a batch until a multi-step alias chain converges
PASS canonicalizes, deduplicates, preserves orphans, and serializes updates across settings scopes
PASS produces both skills.disabled and skills.enabled changes when enabling a workspace-hard-disabled default-disabled skill
PASS persists a Skill batch with one settings write across settings scopes
FAIL mixed enable batch, one refused one applied <- the ONLY failure (1 failed | 9 passed)
PROBE INTACT PR: outcomes [{rust:locked,changed:false},{pdf,changed:true}],
settingsChanges [{skills.enabled:["pdf"]}], setValuesCalls 1
PROBE MUTANT: outcomes [{rust:locked,changed:false},{pdf,changed:false}],
settingsChanges [], setValuesCalls 0 — nothing written
Extend this case to a mixed batch against the same fixtures (workspace.disabled: [], user.disabled: ['locked']):
const mixed = await persistDisabledSkillsBatch!(
workspace,
['rust:locked', 'pdf'],
true,
);
expect(mixed.outcomes).toEqual([
{ skillName: 'rust:locked', changed: false },
{ skillName: 'pdf', changed: true },
]);
expect(setValues).toHaveBeenCalledOnce();
// and the workspace settings.json now holds skills.enabled: ['pdf']The added case must keep the batch at exactly one settings write, per expect(setValues).toHaveBeenCalledOnce() at :1180 and the design doc's "while retaining one settings write" / "同时仍只写入一次设置" (daemon-skill-toggle.md:26).
The new mixed-batch assertion is itself the acceptance criterion: it must go red against the permanent-refusal mutant above and green against the real code. Since all seven shipped tests stay green under that mutant, this assertion is the only thing in the suite that pins the branch.
中文说明
重写后的批量循环没有针对混合启用批次的测试——即同一请求中一个名称被更高作用域永久拒绝、另一个名称被应用。而这正是重写之后新出现、也最容易写错的形态。
循环现在会为每个请求下标预填 {changed:false}，只在某个下标从 pending 中被处理完时才覆盖该条目，并在多轮之间把唯一的一次写入累积在 next 中。测试套件里每一个多名称批次都会解析全部名称（顺序无关用例的正序与反序，以及三名称别名链），而唯一的拒绝用例就是这个单名称批次，它断言的是相反形态——expect(setValues).not.toHaveBeenCalled()。因此一旦出现「永久性更高作用域拒绝连带毒化同批其他名称、返回全部 changed:false 且不写入」这样的回退，Web Shell 中常见的多选场景（「启用这四个技能」，其中一个被用户作用域 skills.disabled 锁定）就会对三个本可选中的技能报告 changed:false 并且什么都不持久化，而整个测试套件仍然是绿的。目前没有任何测试能区分「被拒绝」与「批次静默空转」。
有一点值得说明，因为它决定了该写哪个变异体：最显然的那个——在拒绝分支中提前 return { outcomes, settingsChanges: [] }——是能被捕获的，因为已有的两个多名称批次都包含随后被不动点清除的临时性 Workspace 作用域拒绝。真正能存活于全部七个已有测试的，是永久性拒绝的变异体。
把该用例扩展为针对相同夹具（workspace.disabled: []、user.disabled: ['locked']）的混合批次即可（代码见上）。
新增用例必须保持批次只写入一次设置，依据是 :1180 的 expect(setValues).toHaveBeenCalledOnce() 以及设计文档中的「while retaining one settings write」/「同时仍只写入一次设置」（daemon-skill-toggle.md:26）。
这条新的混合批次断言本身就是验收标准：它必须在上述永久性拒绝变异体下变红、在真实代码下变绿。由于全部七个已有测试在该变异体下仍然是绿的，这条断言是套件中唯一固定该分支的东西。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const status = | ||
| await workspaceRuntime?.workspaceService.getWorkspaceSkillsStatus({ | ||
| route: 'skill settings persistence', |
There was a problem hiding this comment.
[Suggestion] R1-2: The catalog fetch is unconditional on every enable, but for a requested name with no : past index 0 it provably cannot change the outcome — every branch collapses to { name }, the same value the fallback already produces — and the read is not cached across successive toggles either.
Walking the branches: a colon-free name yields { name } from the fallback because prefixEnd > 0 is false (:5387-5392); the catalog branch yields { name: skillName } for any non-extension entry (:5395-5397); an extension entry can never match a colon-free request, because registry names are built as `${extensionName}:${authoredName}` (packages/core/src/skills/types.ts:339); and even a malformed match is neutralized, because skillRestrictionNames drops an authoredName that is empty or equal to the canonical name (types.ts:366-370). So enabling pdf — the common case, and the spelling the new user/project-level tests use — pays a full catalog read for nothing.
The cost compounds because each successful toggle calls invalidateWorkspaceSkillsSnapshot() (workspace-service/index.ts:865, :895, :989), which clears the 5s snapshot (WORKSPACE_SKILLS_SNAPSHOT_TTL_MS = 5_000, index.ts:128) and calls workspaceSkillsStatusProvider?.invalidate?.(boundWorkspace) = managers.delete(workspaceCwd) (workspace-skills-status.ts:96). A client enabling N skills one at a time therefore performs N full reads: N child RPCs with a live child, or with none, N loadSettings reads plus N freshly constructed SkillManagers re-scanning and re-parsing every project/user skill directory — precisely the re-scan the provider's own comment says its cache exists to avoid. And on that no-child path getActiveExtensions: () => [] (:141) omits the very extension skills the read exists to resolve, so there the work buys nothing at all.
Witness:
3 colon-free enables driven over the REAL HTTP route
(POST /workspace/skills/{alpha,beta,gamma}/enable), bridge catalog empty,
each returning 200 {changed:true, activation:"applied"}:
INTACT PR: "skillStatusQueries": 3
WITH SHORT-CIRCUIT: "skillStatusQueries": 0
single toggle: colon-free 'pdf' INTACT q=1 -> FIX q=0
colon-bearing 'rust:chat' INTACT q=1 -> FIX q=1 (read correctly retained)
outcomes byte-identical on both arms:
colonFreeEnable {changed:true, settingsChanges:[{key:"skills.enabled",value:["pdf"]}]}
whole describe with the fix applied: Tests 10 passed | 425 skipped — including all four new tests
Short-circuit before touching the registry or the status service:
const resolveSkillToggleIdentities = async (
workspace: string,
skillNames: readonly string[],
) => {
if (!skillNames.some((name) => name.indexOf(':') > 0)) {
return skillNames.map((name) => ({ name }));
}
// ... unchanged ...
};The test must use the same > 0 comparison as the fallback's const prefixEnd = skillName.indexOf(':') at :5387-5392 — not >= 0 and not includes(':'), or a name beginning with : changes behaviour. With > 0 the short-circuit is behaviour-identical for every input including ':foo': the only catalog entry that could match a name beginning with a colon would need extensionName === '', and !skill.extensionName at :5395 already returns { name } for that case.
Please pin the short-circuit with a call-count assertion using captureSkillPersistence (whose mock bridge already exposes queryWorkspaceStatus: vi.fn()): queryWorkspaceStatus is not called for persistDisabledSkills(workspace, 'pdf', true) and is called once for persistDisabledSkills(workspace, 'rust:pdf', true). Deleting the short-circuit turns the first assertion red, and the four shipped identity tests must stay green.
Note this mitigates the in-lock await reported at :5457 only for the all-colon-free case — one colon-bearing name anywhere in a batch still awaits the read under the lock.
中文说明
每次启用都会无条件读取目录，但对于不含「下标 0 之后的冒号」的请求名称，这次读取可以证明不会改变结果——所有分支都收敛为 { name }，与回退分支产出的值完全相同——而且这次读取在连续切换之间也没有缓存。
逐分支来看：不含冒号的名称走回退分支得到 { name }，因为 prefixEnd > 0 为假（:5387-5392）；目录分支对任何非 extension 条目返回 { name: skillName }（:5395-5397）；extension 条目永远不可能匹配不含冒号的请求，因为注册名按 `${extensionName}:${authoredName}` 构造（packages/core/src/skills/types.ts:339）；即使出现畸形匹配也会被中和，因为 skillRestrictionNames 会丢弃为空或等于规范名的 authoredName（types.ts:366-370）。所以启用 pdf——最常见的情况，也是新增的 user/project 级别测试所用的拼写——白白付出一次完整目录读取。
成本还会叠加，因为每次成功的切换都会调用 invalidateWorkspaceSkillsSnapshot()（workspace-service/index.ts:865、:895、:989），它既清空 5 秒快照（WORKSPACE_SKILLS_SNAPSHOT_TTL_MS = 5_000，index.ts:128），又调用 workspaceSkillsStatusProvider?.invalidate?.(boundWorkspace)，即 managers.delete(workspaceCwd)（workspace-skills-status.ts:96）。因此客户端逐个启用 N 个技能会执行 N 次完整读取：有存活子进程时是 N 次子进程 RPC；没有子进程时是 N 次 loadSettings 读取，外加 N 个新构造的 SkillManager 重新扫描并重新解析每个项目/用户技能目录——而这正是 provider 自己的注释所说、其缓存存在就是为了避免的重扫。并且在无子进程路径上，getActiveExtensions: () => []（:141）恰好省略了这次读取本来要解析的 extension 技能，所以那种状态下这次工作完全没有收益。
修复方式是在触碰注册表或状态服务之前短路（代码见上）。
短路判断必须与回退分支 :5387-5392 处 const prefixEnd = skillName.indexOf(':') 使用相同的 > 0 比较——不能用 >= 0，也不能用 includes(':')，否则以冒号开头的名称行为会改变。使用 > 0 时，该短路对包括 ':foo' 在内的所有输入都行为等价：唯一可能匹配以冒号开头名称的目录条目需要 extensionName === ''，而 :5395 处的 !skill.extensionName 已经对这种情况返回 { name }。
请用 captureSkillPersistence（其 mock bridge 已暴露 queryWorkspaceStatus: vi.fn()）以调用次数断言固定该短路：persistDisabledSkills(workspace, 'pdf', true) 不调用 queryWorkspaceStatus，而 persistDisabledSkills(workspace, 'rust:pdf', true) 调用一次。删除短路会让第一条断言变红，且四个已有身份测试必须保持绿色。
请注意：这只能缓解 :5457 处报告的锁内 await 在「全部名称都不含冒号」时的情况——批次中只要有一个含冒号的名称，仍会在持锁期间等待该读取。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const [skillIdentity] = await resolveSkillToggleIdentities( | ||
| workspace, | ||
| [skillName], | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-3: The new catalog read is awaited inside withSettingsLock, so a skill enable now holds the per-workspace settings write lock across an ACP-child RPC — or, with no live child, a full daemon-local filesystem enumeration — where before this change the critical section held only local file reads and one write.
withSettingsLock (run-qwen-serve.ts:1852-1860) is a per-workspace FIFO promise chain shared with persistDisabledToolsFn, persistSettingFn, persistSettingsFn and the approval-mode and voice-settings writes. On a snapshot-cache miss the read awaits the child under withTimeout(..., initTimeoutMs) with DEFAULT_INIT_TIMEOUT_MS = 10_000 (packages/acp-bridge/src/bridge.ts:2677), or runs a readdir plus frontmatter parse of every project/user skill directory. A wedged-but-live child therefore stalls every other settings write for that workspace for up to ~10s per enable, and each queued enable extends the stall. The 5s snapshot cache does not amortize it, because setWorkspaceSkillEnabled calls invalidateWorkspaceSkillsSnapshot() after every committed change (workspace-service/index.ts:867, :981), which clears the snapshot and deletes the provider's cached SkillManager (workspace-skills-status.ts:96) — so a user working down the Web Shell picker pays a cold read on each successive toggle.
There is a second, separate consequence: this await is the first yield point between loadSettingsForPersistence (:5442) and fresh.setValues (:5494), a span that was fully synchronous before this diff. LoadedSettings.setValues writes from its in-memory model, so an edit a user makes to .qwen/settings.json in an editor while a toggle is in flight is now reverted for any key the stale model already holds. The lock's own doc comment says it exists to collapse "a textbook lost-update window"; this widens that window again. (It is narrower than a whole-file clobber — updateSettingsFilePreservingFormat re-reads and deep-merges, so an externally added key survives.)
Witness:
INTACT (PR):
PROBE-A other settings write finished while catalog read pending: false
PROBE-A other settings write finished after catalog read settled: true
PROBE-A settings.json after both writes: {"skills":{"disabled":[],"enabled":["rust:pdf"]},...}
HOISTED (resolveSkillToggleIdentities moved above withSettingsLock):
PROBE-A other settings write finished while catalog read pending: true
PROBE-A settings.json after both writes: {"skills":{"disabled":["external-edit"],"enabled":["rust:pdf"]},...}
PROBE-A catalog reads issued while toggle in flight: 1 (identical in both arms)
All 7 of this PR's own tests pass with the read hoisted.
Resolve identities before acquiring the lock in both persistDisabledSkillsFn and persistDisabledSkillsBatchFn — they depend only on the catalog and the workspace registry, never on fresh — keeping loadSettingsForPersistence, the block decision and both assertGenerationOpen?.() calls inside:
const skillIdentity = enabled
? (await resolveSkillToggleIdentities(workspace, [skillName]))[0]
: undefined;
return withSettingsLock(workspace, async () => {
assertGenerationOpen?.();
// ... unchanged ...
if (enabled) {
const block = skillToggleBlockForName(fresh, skillIdentity!);
// ... unchanged ...
}
});If you would rather keep snapshot coherence than shorten the lock, bounding the read with a short explicit timeout inside the lock is an equally defensible answer — but note that a hoist means the catalog snapshot is taken before the lock is acquired, so it can go stale while the toggle waits; the generation guard covers runtime replacement, not in-place catalog change. Either way, step 2 of docs/design/daemon-skill-toggle.md and its zh-CN twin need updating in the same change, since both currently place the read under the lock.
The constraint that actually binds is one catalog read per single or batch request, not "inside the lock" — a hoist preserves it (measured: 1 read in both arms), and the doc sentence placing it under the lock is a line this PR itself added, so it is yours to amend. assertGenerationOpen?.() must stay immediately before fresh.setValues (:5491, :5599); it is the only re-check that the runtime generation survived the await.
Please pin the lock boundary with a test that goes red if the read moves back inside: give the captureSkillPersistence bridge a queryWorkspaceStatus returning a deferred the test controls, start persistDisabledSkills(workspace, 'x', true) without awaiting, then await persistDisabledTools(workspace, 't', false) and assert it resolved before the deferred settles. No existing test pins the lock-hold duration — all four new tests resolve queryWorkspaceStatus immediately.
中文说明
新增的目录读取在 withSettingsLock 内部被 await，因此一次技能启用现在会跨 ACP 子进程 RPC 持有该工作区的设置写锁；在没有存活子进程时，则跨一次完整的守护进程本地文件系统枚举。而在本次改动之前，临界区只包含本地文件读取和一次写入。
withSettingsLock（run-qwen-serve.ts:1852-1860）是按工作区串行的 FIFO promise 链，与 persistDisabledToolsFn、persistSettingFn、persistSettingsFn 以及审批模式和语音设置写入共用。快照缓存未命中时，该读取会在 withTimeout(..., initTimeoutMs) 下等待子进程，其中 DEFAULT_INIT_TIMEOUT_MS = 10_000（packages/acp-bridge/src/bridge.ts:2677）；或者执行一次对每个项目/用户技能目录的 readdir 加 frontmatter 解析。因此一个卡住但仍存活的子进程会让该工作区的所有其他设置写入排队最长约 10 秒，而每个排队的启用请求都会延长这个阻塞。5 秒快照缓存也无法摊薄它，因为 setWorkspaceSkillEnabled 在每次提交变更后都会调用 invalidateWorkspaceSkillsSnapshot()（workspace-service/index.ts:867、:981），它既清空快照，又删除 provider 缓存的 SkillManager（workspace-skills-status.ts:96）——所以在 Web Shell 选择器中逐个勾选技能的用户，每次切换都要付一次冷读。
还有第二个独立的后果：这个 await 是 loadSettingsForPersistence（:5442）与 fresh.setValues（:5494）之间第一个让出点，而这段区间在本 diff 之前是完全同步的。LoadedSettings.setValues 依据内存模型写入，因此用户在切换进行中于编辑器里修改 .qwen/settings.json，凡是陈旧模型已持有的键都会被回退。锁自身的文档注释说明它存在的目的正是消除「典型的丢失更新窗口」；本改动重新扩大了这个窗口。（它比整文件覆盖要窄——updateSettingsFilePreservingFormat 会重新读取并深度合并，因此外部新增的键能保留。）
修复方式：在 persistDisabledSkillsFn 与 persistDisabledSkillsBatchFn 两处都在获取锁之前解析身份（它们只依赖目录和工作区注册表，从不依赖 fresh），同时把 loadSettingsForPersistence、阻止判定以及两处 assertGenerationOpen?.() 保留在锁内（代码见上）。
如果你更愿意保留快照一致性而不是缩短持锁时间，那么在锁内用一个较短的显式超时限制该读取同样是合理选择——但请注意，提出锁外意味着目录快照是在获取锁之前采集的，因此在切换等待期间它可能变陈旧；generation 守卫覆盖的是运行时被替换，而不是目录内容原地变化。无论采用哪种方式，docs/design/daemon-skill-toggle.md 的第 2 步及其中文版都需要同在同一次改动中更新，因为两者目前都写明该读取在锁内进行。
真正有约束力的前提是「每次单项或批量请求只读取一次目录」，而不是「在锁内」——提出锁外同样满足它（实测两种写法都是 1 次读取），而文档中把它放在锁内的那句话正是本 PR 自己新增的行，因此可以由本 PR 修改。assertGenerationOpen?.() 必须紧贴在 fresh.setValues 之前（:5491、:5599）；它是唯一一次「运行时 generation 在这次 await 之后仍然有效」的复查。
请补一个在读取被移回锁内时会变红的测试来固定锁边界：让 captureSkillPersistence 的 bridge 中 queryWorkspaceStatus 返回一个由测试控制的 deferred，先不 await 地发起 persistDisabledSkills(workspace, 'x', true)，随后 await persistDisabledTools(workspace, 't', false)，并断言它在 deferred 释放之前就已 resolve。目前没有任何测试固定持锁时长——四个新测试都让 queryWorkspaceStatus 立即 resolve。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const skillIdentities = enabled | ||
| ? await resolveSkillToggleIdentities(workspace, skillNames) | ||
| : skillNames.map((name) => ({ name })); |
There was a problem hiding this comment.
[Suggestion] R1-4: The only thing keeping the catalog read off the disable path is this ternary (and the single-toggle if (enabled) guard at :5456), and no test pins either — so a contract the design doc states in both languages is unwitnessed and can be removed without anything going red.
Deleting the ternary and resolving unconditionally leaves the entire shipped file green at 432 passed (432), and hoisting the single-toggle resolve out of its if (enabled) guard does the same. Nothing else in the repo can catch it either: server.test.ts:28740-28887 stubs both persist deps with vi.fn(), so run-qwen-serve.test.ts is the only file that exercises the real closure, and the three real disable batches already there (:1560, :1676, :1685) assert nothing about the read. A regressed disable would then pay an ACP-child RPC — or with no live child a full daemon-local filesystem enumeration — while holding the per-workspace settings write lock, which is the hazard reported separately at :5457 extended to a path docs/design/daemon-skill-toggle.md:23 explicitly exempts ("Disables do not read the catalog."; zh-CN "禁用操作不读取目录。").
The gap cannot be closed incidentally by a more thorough outcome test, which is what makes it worth pinning deliberately: identities are consumed only inside if (enabled), so on a disable they are discarded whatever the catalog says, and the response is byte-identical across both arms. Only a call-count assertion on the status read can observe the regression.
Witness:
mutation A (batch ternary -> unconditional resolve), whole file: Tests 432 passed (432)
mutation B (single-toggle resolve hoisted out of `if (enabled)`): Tests 432 passed (432)
PRISTINE + suggested spy assertion, whole file: Tests 433 passed (433)
MUTATION-A + suggested spy assertion: Tests 1 failed | 432 passed (433)
FAIL > workspace skill settings persistence > never reads the catalog on the disable path
AssertionError: expected "spy" to not be called at all, but actually been called 1 times
1st spy call: Array [ "qwen/status/workspace/skills", [Function anonymous] ]
cross-arm PROBE-SINGLE / PROBE-BATCH: byte-identical (outcome cannot distinguish the arms)
PROBE-COST enabled=false n=3 resolveMs=5 (first disable batch, daemon-local 15-entry enumeration)
captureSkillPersistence already builds a queryWorkspaceStatus: vi.fn() spy at run-qwen-serve.test.ts:1095 and simply does not return it, so exposing it is a five-line change with zero blast radius on makeRuntimeBridge's 78 call sites — the spread keeps all four existing destructure sites working:
return { ...deps!, queryWorkspaceStatus: bridge.queryWorkspaceStatus };Then add a disable case asserting the catalog is never read, covering both closures, alongside the existing changed / disabled / setValues assertions so the write itself stays pinned:
const { persistDisabledSkills, persistDisabledSkillsBatch, queryWorkspaceStatus } =
await captureSkillPersistence(...);
queryWorkspaceStatus.mockClear();
await persistDisabledSkills!(workspace, 'rust:pdf', false);
expect(queryWorkspaceStatus).not.toHaveBeenCalled();
await persistDisabledSkillsBatch!(workspace, ['rust:pdf', 'pdf'], false);
expect(queryWorkspaceStatus).not.toHaveBeenCalled();The contract is written for both closures, so the assertion has to cover the single and the batch disable path — not just one. Call queryWorkspaceStatus.mockClear() immediately after the runtime is ready and make the disable the first status-touching call: the assertion otherwise holds only because the 5s workspace-service snapshot cache (WORKSPACE_SKILLS_SNAPSHOT_TTL_MS = 5_000, workspace-service/index.ts:128) happens to be cold, and a future test that warms it within the TTL would let the mutated read be served from cache and pass on a mutant.
The new case must fail with expected "spy" to not be called at all against either mutation above and pass against the real code; that flip is the acceptance criterion, since no shipped assertion changes.
中文说明
唯一让目录读取不落在禁用路径上的，就是这个三元表达式（以及 :5456 处单项切换的 if (enabled) 守卫），而两者都没有测试固定。于是设计文档用两种语言写明的契约处于无见证状态，可以被删掉而不会有任何测试变红。
删掉三元表达式改为无条件解析，整个测试文件仍然是绿的 432 passed (432)；把单项切换的解析提出 if (enabled) 守卫，结果相同。仓库中也没有别处能捕获它：server.test.ts:28740-28887 用 vi.fn() 直接桩掉了两个持久化依赖，因此 run-qwen-serve.test.ts 是唯一真正执行该闭包的文件，而其中已有的三个真实禁用批量（:1560、:1676、:1685）对这次读取没有任何断言。一旦回退，禁用请求就会在持有该工作区设置写锁的同时付出一次 ACP 子进程 RPC——或者在没有存活子进程时付出一次完整的守护进程本地文件系统枚举——也就是 :5457 处单独报告的那个风险，被扩展到 docs/design/daemon-skill-toggle.md:23 明确豁免的路径上（「禁用操作不读取目录。」；中文版同）。
这个缺口无法靠更周全的结果断言顺带补上，这正是需要刻意固定它的原因：身份只在 if (enabled) 内部被消费，因此在禁用时无论目录返回什么都会被丢弃，两种写法的响应逐字节相同。只有对状态读取的调用次数断言才能观察到这个回退。
修复成本很低：captureSkillPersistence 在 run-qwen-serve.test.ts:1095 已经构造了 queryWorkspaceStatus: vi.fn() 间谍，只是没有返回它，因此把它暴露出来是约五行改动，对 makeRuntimeBridge 的 78 个调用点零影响——展开写法能让已有的四个解构点原样工作（代码见上）。随后补一个断言目录从未被读取的禁用用例，覆盖两个闭包，并保留已有的 changed / disabled / setValues 断言，使写入本身仍被固定。
该契约是对两个闭包一起写的，所以断言必须同时覆盖单项与批量禁用路径，而不是只覆盖其一。请在运行时就绪后立即调用 queryWorkspaceStatus.mockClear()，并让禁用成为第一个触碰状态的调用：否则该断言之所以成立，只是因为 5 秒的工作区服务快照缓存（WORKSPACE_SKILLS_SNAPSHOT_TTL_MS = 5_000，workspace-service/index.ts:128）恰好是冷的；将来若有测试在 TTL 内预热了它，被变异的读取就会由缓存提供服务，从而在变异体上通过。
新用例必须在上述任一变异下以 expected "spy" to not be called at all 失败，并在真实代码上通过；由于没有任何已有断言会变化，这个翻转就是验收标准。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| const block = skillToggleBlockForName( | ||
| fresh, | ||
| skillIdentities[index], | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-5: The fixpoint rebuilds a loop-invariant index on every iteration: skillToggleBlockForName is now a verbatim pass-through — return buildHigherDisabled(settings).blockIn(skill); (packages/cli/src/config/skill-settings.ts:287-292) — and buildHigherDisabled reconstructs scopeOfEntry, hardEntries, defaultEntries, enabledEntries and workspaceDisabled from all four scopes times three settings lists, each via normalizeSkillNames (an intermediate array plus a Set of trim().toLowerCase() strings) at skill-settings.ts:180-235. fresh is loaded once at :5521 and first mutated at fresh.setValues(...) on :5597, strictly after the loop, so every rebuild inside it recomputes an identical answer. The removed code called this once per name; the fixpoint calls it once per still-pending name per pass. All of it is synchronous and inside withSettingsLock, so it also occupies the event loop while holding the chain that serializes every other settings write for that workspace.
To be fair about the magnitude, since it decides how much this matters: pass count equals the longest alias-dependency chain among the requested names, so the ~5,050-rebuild worst case at MAX_WORKSPACE_SKILL_BATCH_SIZE = 100 (routes/workspace-skills.ts:35) needs a 100-link chain no picker produces, and at a single pass the loop does exactly N rebuilds — the same as the removed code. So this is wasted work and a maintainability trap rather than a live latency bug, and it is worth fixing because the hoist is free and the codebase already has the pattern.
Witness:
PROBE-E, real buildHigherDisabled from packages/cli/src/config/skill-settings.ts,
unmodified in both arms:
entries/list=3: 100 rebuilds=1.52ms 5050 rebuilds=75.90ms per-rebuild=0.0150ms
entries/list=100: 100 rebuilds=22.46ms 5050 rebuilds=1141.04ms per-rebuild=0.2259ms
`fresh` unmutated across the loop, confirmed by reading: loaded :5521,
first mutated :5597 after the loop; updateWorkspaceSkillSettingLists
mutates only the local `next`.
Hoist the derivation above the pass loop, following the pattern already at packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:176 (const higher = useMemo(() => buildHigherDisabled(settings), [settings]);) — add buildHigherDisabled to the existing dynamic import, then:
const { blockIn } = buildHigherDisabled(fresh);
// ... inside the pass loop, in place of the skillToggleBlockForName call:
const block = blockIn(skillIdentities[index]);Only the index build may move. The Workspace-scope exemption must stay inside the pass loop, because it reads the evolving next.disabled rather than fresh:
!(next.disabled.some((name) => name.trim().toLowerCase() === block.entry))and the hoisted index must keep being built from fresh, not from next, or that exemption would double-count in-flight grants. Caching each index's blockIn result across passes is also safe if you prefer it — blockIn closes only over the constant fresh, and the exemption is evaluated in the caller — but hoisting the build is the smaller change.
Please do not "simplify" the pass loop away while you are here. A single-pass projection of next.disabled looks equivalent and passes every test this PR adds, but it is fail-open. With workspace skills.disabled: ['a:b'], user skills.disabled: ['b'], and two well-formed extension rows a:b (extension a, authored b) and c:a:b (extension c, authored a:b), a batch ['a:b','c:a:b'] is correctly refused in full by the fixpoint, while the projection grants c:a:b and writes skills.enabled: ['c:a:b'] even though skills.disabled: ['a:b'] still stands — a changed: true for a grant the merged config denies, which is the exact outcome this PR exists to prevent. The projection treats a name as cleared merely because it was requested, even when the name that would have removed it was itself refused.
Witness for that rejection, so the loop's depth is not re-litigated later:
INTACT (fixpoint): outcomes [{"skillName":"a:b","changed":false},{"skillName":"c:a:b","changed":false}]
settingsChanges []
PROJECTION: outcomes [{"skillName":"a:b","changed":false},{"skillName":"c:a:b","changed":true}]
settingsChanges [{"key":"skills.enabled","value":["c:a:b"]}]
all four of this PR's new tests PASS under the projection — green tests do not establish equivalence
The hoist is behaviour-preserving, so makes a batch enable order-independent without bypassing a user block and rechecks a batch until a multi-step alias chain converges must simply stay green, including their expect(setValues).toHaveBeenCalledOnce(). If you want the reuse itself pinned rather than implied, spy on the dynamically imported ../config/skill-settings.js module and assert buildHigherDisabled is called once per batch request rather than once per name per pass — that assertion is what goes red if the hoist is later reverted.
中文说明
不动点循环在每次迭代都重建一个循环不变的索引：skillToggleBlockForName 现在只是逐字转发——return buildHigherDisabled(settings).blockIn(skill);（packages/cli/src/config/skill-settings.ts:287-292）——而 buildHigherDisabled 会从四个作用域乘三份设置列表重建 scopeOfEntry、hardEntries、defaultEntries、enabledEntries 和 workspaceDisabled，每一项都经过 normalizeSkillNames（一个中间数组加一个由 trim().toLowerCase() 字符串构成的 Set），见 skill-settings.ts:180-235。fresh 在 :5521 只加载一次，首次被修改是在 :5597 的 fresh.setValues(...)，严格位于循环之后，因此循环内每次重建都算出完全相同的答案。被删除的代码每个名称只调用一次；不动点则是每轮对每个仍待处理的名称各调用一次。这些都是同步执行且位于 withSettingsLock 内部，因此在持有「串行化该工作区所有其他设置写入」的 promise 链的同时占用事件循环。
关于量级需要公允说明，因为它决定了这件事的重要程度：轮数等于请求名称中最长别名依赖链的长度，所以在 MAX_WORKSPACE_SKILL_BATCH_SIZE = 100（routes/workspace-skills.ts:35）下约 5,050 次重建的最坏情况，需要一条没有任何选择器会产生的一百环链；而在单轮情况下循环恰好执行 N 次重建——与被删除的代码相同。因此这是无谓开销和可维护性陷阱，而不是现实中的延迟缺陷；值得修是因为提出循环外是零成本的，而且代码库中已有这种写法。
请把该推导提到轮次循环之上，沿用 packages/cli/src/ui/components/skills/SkillsManagerDialog.tsx:176 已有的模式（const higher = useMemo(() => buildHigherDisabled(settings), [settings]);）——在现有动态导入中加入 buildHigherDisabled（代码见上）。
只有索引的构建可以外提。Workspace 作用域的豁免判断必须留在轮次循环内部，因为它读取的是不断演进的 next.disabled 而不是 fresh；并且外提的索引必须继续基于 fresh 构建，而不是基于 next，否则该豁免会重复计入本批次进行中的授权。如果你更愿意缓存每个下标的 blockIn 结果，跨轮缓存也是安全的——blockIn 只闭包常量 fresh，而豁免判断是在调用方求值的——但外提构建是更小的改动。
请顺便不要把轮次循环「简化」掉。 对 next.disabled 做单轮投影看起来等价，并且能通过本 PR 新增的全部测试，但它是失败即放开的。设工作区 skills.disabled: ['a:b']、用户 skills.disabled: ['b']，以及两个格式良好的 extension 条目 a:b（扩展 a，作者名 b）与 c:a:b（扩展 c，作者名 a:b），批次 ['a:b','c:a:b'] 在不动点下会被完整地正确拒绝，而投影会授予 c:a:b 并写入 skills.enabled: ['c:a:b']，尽管 skills.disabled: ['a:b'] 仍然存在——对一个合并配置所拒绝的授权返回 changed: true，而这正是本 PR 要防止的结果。投影之所以出错，是因为它把某个名称视为已清除，仅仅由于该名称被请求过，即使那个本应移除它的名称自己已被拒绝。
该外提是行为等价的，因此 makes a batch enable order-independent without bypassing a user block 与 rechecks a batch until a multi-step alias chain converges 只需保持绿色，包括它们的 expect(setValues).toHaveBeenCalledOnce()。如果你希望把这次复用本身固定下来而不是隐含保证，可以对动态导入的 ../config/skill-settings.js 模块打间谍，断言 buildHigherDisabled 每个批量请求只调用一次，而不是每个名称每轮各一次——将来若外提被回退，正是这条断言会变红。
— qwen3.8-max via Qwen Code /review (v0.23.3)
| block.scope === 'Workspace' && | ||
| block.list === 'disabled' && | ||
| !next.disabled.some( |
There was a problem hiding this comment.
[Suggestion] R1-6: The batch fixpoint re-considers refused names only against the projected next.disabled, never against next.enabled, so a reason: 'default' block that an earlier grant in the same batch cancels is refused on every pass and the loop stops short of the fixed point of its own guard.
Take user-scope skills.defaultDisabled: ['pdf'] with no workspace hard entry, a catalog holding pdf at level user and rust:pdf at level extension/rust, and one batch enabling ['pdf', 'rust:pdf']. The pdf grant lands, so next.enabled gains pdf and resolveSkillSettings now cancels the defaultDisabled entry — rust:pdf is effectively enabled at runtime. But blockIn rebuilds enabledEntries from the pre-write fresh on every pass, so pdf is absent from it, the block stays {reason:'default', list:'defaultDisabled', entry:'pdf', scope:'User'}, the Workspace exemption cannot match it (block.list !== 'disabled', block.scope !== 'Workspace'), and nothing in the loop consults next.enabled. The first request therefore reports {skillName:'rust:pdf', changed:false} while committing enabled:['pdf'], and a second identical request returns {rust:pdf, changed:true} and writes enabled:['pdf','rust:pdf']. In the Web Shell that is a bulk Enable reporting a refusal for a name the same request's own committed write unblocked: the user clicks twice, and the second click performs a settings write, a cache invalidation, an ACP refresh and a settings_changed event for a state that was already effective.
This contradicts the design doc the same PR adds, in both languages — docs/design/daemon-skill-toggle.md:27, "reconsider refused names against the evolving final workspace lists" (plural), and :23, "A second identical request is also a no-op."; zh-CN :25 and :21. The shipped order-independence test does not reach this branch, because its fixture uses user disabled:['locked'] with no defaultDisabled anywhere.
Witness:
INTACT PROBE-C11-B {"firstOutcomes":[{"skillName":"pdf","changed":true},{"skillName":"rust:pdf","changed":false}],
"afterFirst":{"skills":{"enabled":["pdf"]}},
"secondOutcomes":[{"skillName":"pdf","changed":false},{"skillName":"rust:pdf","changed":true}],
"afterSecond":{"skills":{"enabled":["pdf","rust:pdf"]}}}
FIXED PROBE-C11-B {"firstOutcomes":[{"skillName":"pdf","changed":true},{"skillName":"rust:pdf","changed":true}],
"afterFirst":{"skills":{"enabled":["pdf","rust:pdf"]}},
"secondOutcomes":[{"skillName":"pdf","changed":false},{"skillName":"rust:pdf","changed":false}]}
A second, trust-gated exemption arm clears a default block that this batch's own grants cancelled:
if (
block &&
!(
(block.scope === 'Workspace' &&
block.list === 'disabled' &&
!next.disabled.some(
(name) => name.trim().toLowerCase() === block.entry,
)) ||
(fresh.isTrusted &&
block.reason === 'default' &&
next.enabled.some(
(name) => name.trim().toLowerCase() === block.entry,
))
)
) {The fresh.isTrusted gate is required rather than optional: enabledEntries folds the Workspace scope in only under trust (packages/cli/src/config/skill-settings.ts:210-217) and loadSettingsForPersistence skips workspace settings when untrusted (run-qwen-serve.ts:5356-5363), so an ungated projection of next.enabled would cancel a defaultDisabled entry for an untrusted workspace and turn this fails-closed defect into a fail-open one.
Please pin the new arm with a test that goes red without it: seed user {skills:{defaultDisabled:['pdf']}} and workspace {skills:{disabled:[]}} with catalog [skillStatus('pdf','user'), skillStatus('rust:pdf','extension','rust')], then assert one batch returns [{pdf,changed:true},{rust:pdf,changed:true}] with setValues called once, and that repeating the identical batch is a no-op.
中文说明
批量不动点循环在重新考虑被拒绝的名称时，只依据投影后的 next.disabled，从不依据 next.enabled。因此当同一批次中先前的授权已经抵消了某个 reason: 'default' 阻止项时，该名称在每一轮都会被拒绝，循环无法收敛到自身门禁的不动点。
设用户作用域有 skills.defaultDisabled: ['pdf']（工作区没有硬禁用项），目录中 pdf 为 user 级别、rust:pdf 为 extension/rust 级别，一次批量启用 ['pdf', 'rust:pdf']。pdf 的授权生效后，next.enabled 加入 pdf，resolveSkillSettings 随即抵消该 defaultDisabled 项——rust:pdf 在运行时实际上已被启用。但 blockIn 每一轮都从写入前的 fresh 重建 enabledEntries，因此其中没有 pdf，阻止项始终是 {reason:'default', list:'defaultDisabled', entry:'pdf', scope:'User'}；工作区豁免条件无法匹配（block.list !== 'disabled'、block.scope !== 'Workspace'），而循环中没有任何地方读取 next.enabled。于是第一次请求在提交 enabled:['pdf'] 的同时报告 {skillName:'rust:pdf', changed:false}，第二次发送完全相同的请求才返回 {rust:pdf, changed:true} 并写入 enabled:['pdf','rust:pdf']。在 Web Shell 中，这意味着批量启用会对一个被同一次请求自身写入所解锁的名称报告失败：用户需要点击两次，而第二次点击会为已经生效的状态再执行一次设置写入、缓存失效、ACP 刷新和 settings_changed 事件。
这与本 PR 自己新增的设计文档相矛盾，且中英文皆然——docs/design/daemon-skill-toggle.md:27 写的是「根据不断演进的最终工作区列表（复数）反复重新考虑被拒绝的名称」，:23 写的是「第二次相同请求同样是无操作。」；中文版见 :25 与 :21。已有的顺序无关性测试触不到这一分支，因为它的夹具使用的是用户作用域 disabled:['locked']，完全没有 defaultDisabled。
建议增加第二个受信任门禁约束的豁免分支，用于清除被本批次自身授权抵消的 default 阻止项（代码见上）。
其中 fresh.isTrusted 门禁是必需的，而非可选：enabledEntries 只在受信任时才纳入 Workspace 作用域（packages/cli/src/config/skill-settings.ts:210-217），而 loadSettingsForPersistence 在不受信任时会跳过工作区设置（run-qwen-serve.ts:5356-5363）。因此若不加门禁地投影 next.enabled，就会为不受信任的工作区抵消 defaultDisabled 项，把这个「失败即关闭」的缺陷变成「失败即放开」。
请为该分支补一个去掉修复即变红的测试：用户作用域写入 {skills:{defaultDisabled:['pdf']}}、工作区写入 {skills:{disabled:[]}}，目录为 [skillStatus('pdf','user'), skillStatus('rust:pdf','extension','rust')]，断言一次批量返回 [{pdf,changed:true},{rust:pdf,changed:true}] 且 setValues 只调用一次，并断言重复发送相同批次是无操作。
— qwen3.8-max via Qwen Code /review (v0.23.3)
Independent E2E verification on Linux — head
|
| BASE arm | HEAD arm | |
|---|---|---|
| worktree HEAD | ea1ad7f995a841a8cd3f235ac6e3c774f7e76b1c (= this PR's base.sha) |
cbfb9990b227e7cbfc5aaed542b243b4a8897c2e (= this PR's head) |
GIT_COMMIT_INFO stamped by scripts/generate-git-commit-info.js |
ea1ad7f995 |
cbfb9990b2 |
resolveSkillToggleIdentities occurrences in packages/cli/src/serve/run-qwen-serve.ts |
0 | 3 |
| daemon | qwen serve listening on http://127.0.0.1:18832 (mode=http-bridge) |
…:18831 (mode=http-bridge) |
dev.js was chosen deliberately: its module loader remaps @qwen-code/qwen-code-core and every subpath export into the worktree's own packages/core/src, so a shared node_modules cannot silently serve one arm the other's code. Both arms spawned a real ACP child (session spawned, clientId=client_…), and each drive is witnessed by the daemon's own per-route log line (route=POST /workspace/skills/rust:chat/enable … status=200).
Precondition asserted before every drive, in both arms — GET /workspace/skills returned initialized: true, 16 skills, and the fixture skill present at a non-extension level, identically in both arms:
{"kind":"skill","status":"disabled","name":"rust:chat","level":"project",
"extensionName":null,"disabledReason":"hard",
"installedPath":"<arm>/ws/.qwen/skills/rust:chat/SKILL.md"}This matters because the whole fix is gated on a catalog hit. Had the catalog been unavailable, resolveSkillToggleIdentities would have taken its documented punctuation fallback in both arms and the A/B would have painted an identical, meaningless green. A colon in the name is legal — SKILL_NAME_PATTERN = /^[\p{L}\p{N}_:.-]+$/u, and types.ts names rust:chat as the intended case — so the fixture is a supported shape, not a synthetic one.
Fixture: workspace .qwen/settings.json = {"skills":{"disabled":["rust:chat","chat"]}}, i.e. a hard workspace disable on the project skill rust:chat plus an unrelated entry on chat.
1. Single enable — the false refusal this PR fixes
POST /workspace/skills/rust:chat/enable {"enabled":true}, HTTP 200 in both arms:
BASE ea1ad7f995 |
HEAD cbfb9990b227 |
|
|---|---|---|
changed |
false |
true |
block |
{"reason":"hard","list":"disabled","entry":"chat","scope":"Workspace"} |
absent |
activation / sessionsRefreshed |
applied / 0 |
applied / 1 |
| settings file after | unchanged: disabled:["rust:chat","chat"] |
disabled:["chat"], enabled:["rust:chat"] |
Base blames chat — an entry naming a different skill — and refuses an enable it has no grounds to refuse. Head grants it, and the unrelated legacy chat entry survives untouched, which is the orphan-preservation behaviour the design doc promises. Reproduced across two independent daemon starts with identical payloads.
2. Batch enable — order-independence
POST /workspace/skills/enable, same fixture reset before each call:
| requested order | BASE ea1ad7f995 |
HEAD cbfb9990b227 |
|---|---|---|
["rust:chat","chat"] |
rust:chat changed:false, chat changed:true; file left at disabled:["rust:chat"], enabled:["chat"] |
both changed:true |
["chat","rust:chat"] |
both changed:true |
both changed:true |
Base gives different outcomes for the same requested set depending on argument order — a client cannot predict whether a batch enable will take. Head returns the same outcome for both orders, and still writes settings once. This is the fixpoint doing real work through the route, not only in the unit test.
3. The guard is not weakened on the fallback path
With a user-scope hard disable on pdf (~/.qwen/settings.json → skills.disabled:["pdf"]), POST /workspace/skills/rust:pdf/enable where rust:pdf is not in the catalog:
both arms → {"changed":false,"block":{"reason":"hard","list":"disabled","entry":"pdf","scope":"User"}}
Identical in both arms, so the legacy bare-name block still holds when identity cannot be resolved — the fail-safe direction the design doc specifies.
Not covered by this arm
The level === 'extension' branch was not reached end-to-end. A workspace extension fixture (<ws>/.qwen/extensions/rust/ with a skill authored pdf) did not appear in the catalog — still 16 skills, no entry carrying extensionName — so scenario 3 exercised the uncatalogued fallback rather than the extension path. That branch is therefore unverified here, not verified-good. Its evidence remains the PR's own run-qwen-serve.test.ts cases under the green Test (ubuntu-latest, Node 22.x) lane, plus a static invariant: qualifySkillName(extensionName, authoredName) returns `${extensionName}:${authoredName}` and skill-manager.ts sets name: qualifySkillName(extension.name, skill.name) alongside extensionName: extension.name, so name always begins with extensionName + ':' and name.slice(extensionName.length + 1) recovers the authored spelling. Reading the changed files in full also closed four further hypotheses: a leading-whitespace mis-slice is unreachable because both paths trim (routes/workspace-skills.ts:188, workspace-service/index.ts:942); an empty or garbage authoredName degrades safely because skillRestrictionNames lowercases both spellings and drops a falsy one; the changed skillToggleBlockForName signature has exactly two production call sites and both are updated, so there is no stale string caller; and the catalog read awaited inside withSettingsLock cannot deadlock, since getWorkspaceSkillsStatus is TTL-cached, in-flight-deduped, and answered by the child — its lock-hold latency is already on the record as the bot's Suggestion R1-3, which is the right severity for it.
One CI-coverage note, not a defect in this PR. The Serve A/B lane reported "No response changes against the PR base across 12 scenario(s)", but its driver (.github/scripts/serve-ab-drive.mjs) only exercises /health, /health?deep=1, /capabilities, POST /session, session-restore and session-admission — no /workspace/skills/* route at all. So that clean diff is silent about this PR's changed surface, and scenario 1 above is the first A/B witness that the changed route actually differs. Worth knowing before anyone reads the lane as confirmation.
CI state at this head (complete census, not a rollup-only read)
REST commits/cbfb9990…/check-runs: total_count=153, 153 fetched across 3 pages (page 3 empty) ⇒ MATCH=True; every run status=completed; conclusions {skipped: 122, success: 31} — zero failure, zero cancelled, zero timed_out, so no superseded bot run is propping the rollup up or dragging it down. GraphQL statusCheckRollup.state = SUCCESS on the same head corroborates it.
Product lanes SUCCESS by name: Test (ubuntu-latest, Node 22.x), Lint & Static (ubuntu-latest, Node 22.x), Integration Tests (no-AK, No Sandbox), Serve A/B, web-shell E2E Smoke, TUI parity snapshots (ink vs opentui), OpenTUI no-flicker gate, Desktop Shell (ubuntu-22.04) and (windows-2022), precheck-pr / precheck, Real daemon E2E / Java 11, and the Java 11/17/21 lanes. Test (macos|windows) and Integration Tests (CLI, No Sandbox) are skipped. The review-pr job concluded here (success, 10 runs) — the hang signature reported on other PRs this week is absent on this one. No qwen-pr-precheck:manual-required marker in any issue comment. mergeable_state=blocked with reviewDecision=REVIEW_REQUIRED, expected for a fork PR awaiting a maintainer; that field is a snapshot and flaps, so it is not being treated as an invariant.
The bot's own /review round 1 at this exact head posted 6 findings, all sev:"S", no Critical. I concur on severity: nothing I measured or read is merge-blocking.
Conclusion
No blocking issue found in what this arm measured. The two behaviours the PR exists to change — refusing an enable on an unrelated punctuation-derived alias, and a batch enable whose outcome depends on argument order — are both reproduced on the base arm and gone on the head arm, through the real routes, with the catalog precondition asserted rather than assumed; and the guard still blocks a genuine user-scope disable on the fallback path. The extension-level branch is the one gap and is named above as unverified here. Nothing in this arm contradicts the approval already on the PR.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Approving on the strength of the independent E2E report above (comment 5665849503), which was run at this same head cbfb9990b227 and concluded no blocking issue.
Gate state as read immediately before submitting this approval:
- Newest verdict row at head is an approval.
qwen-code-ci-botAPPROVED atcbfb9990b227(10:00:28Z, review5196335552) is the only verdict row at head; the laterCOMMENTEDrow (13:07:33Z) does not displace it. It is causally valid — submitted ~1.5 h after the head commit's own committer date (08:29:56Z). Worth stating explicitly because this head is aMerge remote-tracking branch 'origin/main'commit, which is the shape that otherwise produces false gate-lives. - Zero Criticals, screened across all three surfaces.
[Critical]was counted over review bodies, inline comments and issue comments: 2 review rows, 6 inline comments, 6 issue comments. No page cap was hit on any surface, so these are totals, not lower bounds. The only occurrence anywhere is inside our own E2E report's sentence stating that the token appears zero times — a self-match, quarantined by author and surface rather than counted as a finding. Excluding it leaves zero Criticals by anyone. The bot's own/reviewround at this head posted 6 findings and its embedded machine-readable ledger marks all 6"sev":"S"atshacbfb9990b227…. - CI green on a complete census, not a rollup-only read. REST
commits/cbfb9990…/check-runs:total_count=153, 153 fetched across 3 pages ⇒ MATCH=True; every runstatus=completed; conclusions{skipped: 122, success: 31}— zerofailure, zerocancelled, zerotimed_out, zeroaction_required, so no superseded bot run is propping the rollup up or dragging it down. 48 distinct lane names, latest-per-lane allsuccess/skipped. 22 product lanes SUCCESS by name, includingTest (ubuntu-latest, Node 22.x),Lint & Static (ubuntu-latest, Node 22.x),Integration Tests (no-AK, No Sandbox),Serve A/B,web-shell E2E Smoke,TUI parity snapshots (ink vs opentui),OpenTUI no-flicker gate,Desktop Shell (ubuntu-22.04)and(windows-2022),precheck-pr / precheck, and the Java 11/17/21 lanes. The product-lane count is nonzero, so this green is not a precheck-blocked rollup. GraphQLstatusCheckRollup.state = SUCCESSon the same head corroborates it. Noqwen-pr-precheck:manual-requiredmarker in any issue comment.
The one gap the report names still stands: the level === 'extension' branch of skill identity resolution has no E2E witness in that arm. That is a coverage gap in our verification, not a defect found in the diff, and it is not merge-blocking on anything measured here.
Everything above is a statement about the state as read immediately before this approval was submitted. A later push, or a newer CHANGES_REQUESTED/DISMISSED verdict row at head, supersedes it.
4 participants
What this PR does
Skill enable requests now resolve extension identity from the authoritative workspace skill catalog before evaluating higher-scope disablement guards. Single toggles resolve the catalog once, while batch enables revisit only refused items until the result reaches a stable fixpoint, so input order no longer changes outcomes. Response shapes and the single persisted settings write are unchanged.
The existing punctuation-based identity heuristic remains as a compatibility fallback when the catalog is unavailable, uninitialized, or does not contain the requested name. The skill-toggle design is updated in English and Chinese.
Why it's needed
An authored non-extension skill name may legally contain a colon. Inferring extension identity by splitting at the first colon can therefore invent an alias and incorrectly refuse an enable request. Batch enables also evaluated blockers against order-sensitive intermediate state, so two permutations of the same request could produce different results and multi-step alias chains could stop before converging.
Catalog-backed identity removes the false extension inference when authoritative metadata is available, and bounded fixpoint evaluation makes batch results deterministic while preserving higher-scope blocks.
Reviewer Test Plan
How to verify
rust:chatandchat, enable a cataloged non-extension skill namedrust:chat; expectchanged: trueand onlychatto remain disabled.pdfdisabled, enablepdfandrust:pdfin both input orders; expect both outcomes to reportchanged: trueand one settings write in either order.Evidence (Before & After)
Before, the regression capture on the recorded base failed exactly the three new behavioral assertions:
After, the focused exact-head capture passed all relevant configuration and daemon persistence tests:
Tested on
Environment (optional)
Node.js 22.22.3 in an isolated macOS worktree. Focused tests, the isolated daemon server suite (1,281/1,281), build, typecheck, lint, and format checks passed at head
b7899c77014496a085644302a56d33fbcbca595f. Full local preflight reached the test phase but had environment-sensitive failures in unchanged server and Git tests (a fixed 15-second timeout, a timing-dependent flood status, and installed-Git error wording) plus a Vitest worker RPC timeout; repository CI is the authoritative full-suite run.Risk & Scope
setWorkspaceSkillEnabledthroughPOST /workspace(s)/:ws/skills/:name/enable,setWorkspaceSkillsEnabledthrough the batch route, bothcreateServeAppinjection sites forpersistDisabledSkillsandpersistDisabledSkillsBatch, the SDK single and batch clients (response shape unchanged), and the Web Shell skills page (behavior changes only where correct enables now persist).Linked Issues
Refs #11699
This change was prepared with AI assistance and independently reviewed against the current implementation.
中文说明
本 PR 的改动
技能启用请求现在会先从权威的工作区技能目录解析扩展身份，再评估更高作用域的禁用门禁。单项切换只解析一次目录；批量启用则只重新检查被拒绝的项目，直到结果达到稳定不动点，因此输入顺序不再影响结果。响应结构和单次持久化设置写入均保持不变。
当目录不可用、未初始化或不包含请求名称时，现有基于标点的身份推断仍作为兼容性回退。技能切换设计已同步更新为英文版和中文版。
为什么需要此改动
合法的非扩展技能名称可以包含冒号。按第一个冒号拆分来推断扩展身份，可能会凭空产生别名并错误拒绝启用请求。批量启用此前还会根据与顺序相关的中间状态评估阻止条件，因此同一请求的两种排列可能产生不同结果，多步别名链也可能在收敛前停止。
当权威元数据可用时，基于目录的身份解析消除了错误的扩展推断；有界不动点求值则让批量结果保持确定性，同时保留更高作用域的阻止语义。
审阅者测试计划
验证方式
rust:chat和chat，然后启用目录中标记为非扩展的rust:chat技能；预期返回changed: true，且只有chat仍被禁用。pdf，分别以两种输入顺序启用pdf和rust:pdf；预期两种顺序下的两个结果都返回changed: true，并且都只写入一次设置。证据（改动前后）
改动前，在记录的基线提交上运行回归测试，恰好有三个新增行为断言失败：
改动后，在精确 head 上运行聚焦测试，相关配置和 daemon 持久化测试全部通过：
已测试平台
环境（可选）
在隔离的 macOS worktree 中使用 Node.js 22.22.3。聚焦测试、隔离运行的 daemon server 测试套件（1,281/1,281）、构建、类型检查、lint 和格式检查均在 head
b7899c77014496a085644302a56d33fbcbca595f上通过。本地完整 preflight 已进入测试阶段，但未改动的 server 和 Git 测试出现了与环境相关的失败（固定 15 秒超时、依赖时序的 flood 状态以及取决于已安装 Git 版本的错误文案），同时出现 Vitest worker RPC 超时；仓库 CI 是完整测试套件的权威结果。风险与范围
POST /workspace(s)/:ws/skills/:name/enable使用的setWorkspaceSkillEnabled、通过批量路由使用的setWorkspaceSkillsEnabled、createServeApp中persistDisabledSkills与persistDisabledSkillsBatch的两个注入点、SDK 的单项与批量客户端（响应结构不变），以及 Web Shell 技能页面（只在正确启用现在能够持久化时改变行为）。关联 Issue
Refs #11699
此改动借助 AI 完成，并依据当前实现接受了独立审阅。
AI assistance disclosure
This change was developed with AI assistance: implementation and tests were written with OpenAI Codex and independently reviewed with Claude, under human direction, with the contributor reviewing and approving the final diff. The commit carries the corresponding
Co-authored-bytrailer.