feat(skills): extension skills are named by their extension - #10841
Conversation
…9408) Extension skills register as `<extension>:<name>` everywhere the registry name is used; `authoredName` preserves the authored spelling. Restrictions (`skills.disabled`, `skills.defaultDisabled`, `slashCommands.disabled`) match under either spelling; `skills.enabled` matches the registry identity only, so a rename never grants capability. The picker and the Skills dialog name the owner, the picker guard names the settings entry that blocks a skill, and a stale bare `skills.enabled` entry is announced at startup with its qualified replacement. Mirrors gemini-cli#23566. Fixes QwenLM#9408
github-actions
Bot
added
the
review/self-reported
|
Qwen precheck requires maintainer approval before automated triage/review. Head SHA: Reason:
A maintainer with write access can inspect the PR and manually request a run with |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
activeWorkStaleMs |
6 |
11 |
— Qwen Code · serve A/B
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical: the completion-popup label container change kills Ctrl+R expand/collapse and turns the Test lane red
Reviewed at head fc04e97d46. The naming rule itself checks out, and I could not break it:
qualifySkillNamecan never produce an invalid registry identity. Both manifest formats constrain the extension name to a subset ofSKILL_NAME_PATTERN—validateName(extensionManager.ts:3369,/^[a-zA-Z0-9-_.]+$/) on theqwenpath andisValidPluginName(agent-plugins-v1/manifest.ts:115) on the plugin path, which returns beforevalidateNamebut is stricter.renderAvailableSkillsBlockXML-escapes every name anyway, so the qualified form adds no injection surface to<available_skills>.- The ACP dedup fix is correct and I verified the two keys really meet: registry rows build
extension:rust:rust:pdf(acpAgent.ts:7123-7131,${level}:${extensionName}:${name}withnamealready qualified) and the manifest rows now build the identical string (:7141). Without the hunk they never meet, exactly as the comment says. commandRestrictionNamescannot lose the post-rename name. The collision rename inCommandService.createfires only for a command with a top-levelextensionName(CommandService.ts:92), andSkillCommandLoadersetsextensionNameonly insideskillDetail(SkillCommandLoader.ts:151) — socmd.name === cmd.skillDetail.namefor every skill command and keying the matcher offskillDetailis safe.isSkillEnabledasks the manifest and the workspace store for the authored spelling and the grant for the registry identity, which matchesresolveSkillSettings' verbatim opt-in cancellation (skill-settings.ts:60-62), so the documented asymmetry is what the code actually does. The dialog writes the registry spelling into workspaceskills.disabled/skills.enabled(SkillsManagerDialog.tsx:355-359), consistent with both rules.- Deleting
formatCommandSourceLabelleaves nothing dangling — 0 references at head.packages/vscode-ide-companion/schemas/settings.schema.jsonis in sync with thesettingsSchema.tsdescriptions, andskills.disabledLevels, newly named in theskills.enableddescription, does exist.
The blocking problem is unrelated to naming: it is the completion-popup layout hunk.
packages/cli/src/ui/components/SuggestionsDisplay.tsx:263
Where — the label container was changed from
<Box>
<Box flexShrink={0}>{labelElement}</Box>
…argumentHint / sourceBadge…
</Box>to a single <Text wrap="truncate-end"> holding the same three children.
Trigger — Ctrl+R reverse command search over a match at least MAX_WIDTH (150) characters long, then Right to expand / Left to collapse. Pinned by InputPrompt.test.tsx:4653-4691 (expands and collapses long suggestion via Right/Left arrows), which is not touched by this PR.
Why — PrepareLabel returns <Text wrap="wrap"> in both states: the collapsed path windows the label to MAX_WIDTH with ... markers (PrepareLabel.tsx:36-44) and the expanded path returns the whole label (:98-114). In Ink the outermost Text owns wrapping, so the new wrap="truncate-end" overrides the inner wrap="wrap" and clips the label to the column width in both states. isExpanded (SuggestionsDisplay.tsx:224) still flips, so the arrow indicator still changes — the text just stops responding to it.
Impact — the Test lane is red at this head (run 33646320868, job 100301967843: Test Files 1 failed | 1004 passed (1005)), and the expand/collapse affordance is functionally dead: both states now render one truncated line that differs only in the arrow. I reproduced it in a checkout of this head and got a byte-identical diff to CI:
- Expected
+ Received
- > lllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllll ←
- lllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllll
- llllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllll"
+ > llllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllll… ←"
and the collapsed snapshot loses its second and third lines the same way.
Causation is proven by mutation, not inferred: changing only wrap="truncate-end" → wrap="wrap" on line 263 turns the test green (1 passed), and nothing else in the tree was modified. The file was restored sha256-identical afterwards (9c77d47bfae6f7ce…).
Fix direction — the badge-overflow problem the hunk sets out to solve is real (a flexShrink: 0 label plus a wrappable badge in a half-width column does squeeze the badge one character per line), but it has to be fixed without taking wrapping away from PrepareLabel. Note that the obvious minimal conditional is not sufficient — I tried wrap={isExpanded ? 'wrap' : 'truncate-end'} and the expanded snapshot passes while the collapsed one still fails, because the collapsed state also depended on wrap="wrap" to render PrepareLabel's 150-char window across three lines. So either keep the pre-PR Box structure and bound the badge on its own (its own truncating Text, or a flexShrink/max-width on the badge rather than on the whole container), or truncate argumentHint/sourceBadge only and leave the label container wrappable.
Whichever shape it takes, the two command-search-*-match snapshots are the oracle and need to pass unmodified — regenerating them would delete the only coverage this feature has, and the popup would ship with expand/collapse reduced to an arrow that does nothing.
Everything else I ran at this head
The suites this PR touches are green, so the red lane is entirely the one file above:
packages/core src/skills/types.test.ts + skill-manager.test.ts + config.test.ts + tools/skill.test.ts
823 passed | 1 failed
packages/cli CommandService, commandMetadata, commandUtils, skill-settings, skill-level-label,
SuggestionsDisplay.qualifiedLabels, extension-skills, workspace-skills-mapping,
skillsCommand, nonInteractiveCliCommands 180 passed (10 files)
packages/cli SkillsManagerDialog 13, workspace-qualified-extensions 54, acpAgent 593 660 passed | 1 failed
Neither local failure is this PR's, and neither appears in the CI failure list: config.test.ts > initialize > releases a pending lease while a real baseline read is gated 25ms is a timing-sensitive lease test the diff does not touch, and acpAgent.test.ts > createWorkspaceMcpBudget — env parsing > accepts a plain positive decimal integer is an env-dependent test the diff does not touch that passes in CI. Both are host artifacts of my checkout, reported only so the counts above are not mistaken for PR damage.
Non-blocking, and pre-existing rather than introduced here: inactiveExtensionSkillNames (extension-skills.ts:28) still collects authored spellings and has no non-test consumer at this head. Worth a look while the two-spelling distinction is fresh, because a future caller that compares it against a registry identity would read as "not inactive" — the same fail-open the neighbouring authoredSkillName doc comment warns about.
…extension-skill-qualified-names
fc04e97 to
22402b6
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）为单个提交。 |
|
@qqqys both items are addressed at
236/236 across the InputPrompt, SuggestionsDisplay, and extension-skills suites; cli typecheck clean after a full workspace build. |
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 3: install workspace dependencies and run packages/cli acp-integration tests (worktree has no node_modules/dist); chunk 4: could not execute skill-settings.test.ts , nonInteractiveCliCommands.test.ts , extension-skills.test.ts (no node_modules in the review worktree; vitest un….
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 3：install workspace dependencies and run packages/cli acp-integration tests (worktree has no node_modules/dist)；chunk 4：could not execute skill-settings.test.ts , nonInteractiveCliCommands.test.ts , extension-skills.test.ts (no node_modules in the review worktree; vitest un…。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| // never meet, and a stale registry row plus its manifest entry emit | ||
| // the same skill twice under two names. What the manifest-only rows | ||
| // display is unchanged. | ||
| const key = `extension:${extensionName}:${qualifySkillName( |
There was a problem hiding this comment.
[Critical] R1-3: [certifies-falsely] [regression] The manifest-only rows this loop emits for inactive extensions still carry the bare authored name ({...skill} spread), while grants now match the qualified registry identity only — so the one name the daemon surfaces for such a skill is a spelling that can never grant. The web-shell Skills page offers "Enable" on these rows regardless of disabledReason, and the daemon persists whatever name it is given verbatim (the facade test pins that unknown/inactive names are not validated), so enabling writes workspace skills.enabled: ['audit'] and returns changed: true. When the extension later activates, isSkillEnabled grants only on enabledSkillNamesProvider().has('ext:audit'), the audit entry never matches, and the skill stays disabled with a dead settings entry. Before this PR the identical toggle worked because the registry name was the authored name, so the rename is what breaks it; the declared skill_inactive_extension error code is rendered by web-shell i18n but emitted nowhere.
Witness:
manifest-only row: {"kind":"skill","status":"disabled","name":"audit",
"disabledReason":"inactive_extension","extensionName":"ext"} <- no qualified spelling on the row
grant key required at activation: ext:audit
config.test.ts pins: bare grants do not open qualified skills
Emit the qualified identity on manifest-only rows too (name: qualifySkillName(extensionName, skill.name), carrying authoredName for lookups), or qualify/validate the name in the toggle persistence path, or raise the already-declared skill_inactive_extension error for these rows. Whatever name the manifest rows emit must keep the two dedup keys meeting (registry rows are keyed level:extensionName:name with the qualified name at acpAgent.ts:7125, manifest rows at :7141), and web-shell re-finds the toggled row by exact name after reload, so the row name must stay stable across the toggle→reload round trip. Fix witness: extend the acpAgent.test.ts skills-status block so that for a skill present only in a deactivated extension's manifest, the emitted row's name is <ext>:<authored> — removing the qualification must turn it red.
中文说明
[Critical] R1-3：该循环为未激活扩展发出的“仅清单行”仍然携带裸的原始名（{...skill} 展开），而授权现在只匹配限定后的注册身份——因此守护进程为这类技能呈现的唯一名字是一个永远无法生效的拼写。Web-shell 的 Skills 页面不论 disabledReason 都会在这些行上提供“启用”，守护进程对提交的名字原样持久化（facade 测试固定了不校验未知/未激活名称），于是启用会写入工作区 skills.enabled: ['audit'] 并返回 changed: true。当扩展之后被激活时，isSkillEnabled 只在 enabledSkillNamesProvider().has('ext:audit') 时授权，audit 条目永远匹配不上，技能保持禁用，设置条目成为死条目。本 PR 之前同样的开关是有效的（注册名就是原始名），是改名破坏了它；已声明的 skill_inactive_extension 错误码在 web-shell i18n 中有渲染但没有任何地方发出。
建议：在仅清单行上也发出限定身份（name: qualifySkillName(extensionName, skill.name)，同时携带 authoredName 供查找），或在开关持久化路径中限定/校验名称，或对这些行抛出已声明的 skill_inactive_extension 错误。无论仅清单行发出什么名字，两个去重键必须继续相遇（注册行在 acpAgent.ts:7125 以“级别:扩展名:限定名”为键，仅清单行键在 :7141），且 web-shell 重载后按精确名称重新定位被切换的行，因此行名必须在“切换→重载”往返中保持稳定。修复见证：扩展 acpAgent.test.ts 的 skills-status 块，使仅存在于已停用扩展清单中的技能，其发出行的 name 为 <ext>:<authored>——移除限定必须使该测试变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * both spellings normalized and registry-first, so there is nothing to | ||
| * re-normalize here and the registry-name entry wins a tie. | ||
| */ | ||
| export function lookupSkillSetting<T>( |
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] [regression] Restriction matching now blocks under either spelling, but every workspace write path (updateTarget/computeWorkspaceSkillListUpdates, used by the /skills dialog and the REST toggle persist) still removes only entries spelled exactly like the toggled registry name. A workspace-scope skills.disabled: ['pdf'] written before this rename therefore keeps gating rust:pdf, the row renders unlocked (buildHigherDisabled locks only System/User/SystemDefaults), and toggling it on persists "successfully" while changing nothing: enabled: ['rust:pdf'] is written next to the surviving 'pdf', deny precedes grant, the skill stays disabled, the dialog prints "Skills configuration saved.", and the REST surface returns changed: true / activation: 'applied'. The checkbox reverts on reopen. Pre-PR the identical toggle cleared the entry, because the registry name was the bare name — the read side's new dual-spelling semantics are what make the unchanged write path newly wrong. The bareDisablementBlocksQualifiedGrantWarnings startup warning fires only at the next startup, after the failed toggle created the qualified grant. The defaultDisabled cohort of this same lineage is the finding on SkillsManagerDialog.tsx:249, and it needs the opposite fix direction.
Witness:
PR arm: updates {"disabled":["pdf"],"enabled":["rust:pdf"],"disabledChanged":false,"enabledChanged":true}
surviving blocking entries: [ 'pdf' ] (rust:pdf still blocked)
BASE arm: same write code, pre-PR naming -> updates {"disabled":[],"enabled":["pdf"]}
fix flip: clearing the authored spelling empties disabled
Surface the conflict instead of confirming a no-effect write: classify a row whose sole blocker is a workspace-scope entry matching via the authored spelling as locked-with-reason (mirroring the higher-scope lock/blame UI), or have persistChanges/the REST route report the surviving blocking entry at save time. Two constraints: skill-settings.test.ts 'writes the registry name and clears only that entry' pins expect(updates.disabled).toEqual(['pdf', 'other']) — "enabling one skill can never clear a broader disablement" — and config.ts isSkillEnabled checks restrictions before grants, so no enabled entry can outweigh the stale bare disablement. Fix witness: add a skill-settings.test.ts case running computeWorkspaceSkillListUpdates(['pdf'], [], [{ name: 'rust:pdf', authoredName: 'pdf', wasEnabled: false, isEnabled: true }]) — whatever shape the fix takes, removing it must turn the new assertion red.
中文说明
[Critical] R1-1：限制匹配现在在任一拼写下阻止，但所有工作区写入路径（updateTarget/computeWorkspaceSkillListUpdates，被 /skills 对话框与 REST 开关持久化使用）仍然只移除与被切换注册名完全同拼写的条目。因此改名前写入的工作区作用域 skills.disabled: ['pdf'] 会继续阻止 rust:pdf，而该行显示为未锁定（buildHigherDisabled 只锁定 System/User/SystemDefaults），打开开关会“成功”持久化却什么都没改变：enabled: ['rust:pdf'] 被写入、'pdf' 留存，拒绝优先于授权，技能保持禁用，对话框打印“Skills configuration saved.”，REST 返回 changed: true / activation: 'applied'。重新打开时复选框复位。本 PR 之前同样的开关会清除该条目（注册名就是裸名）——是读侧新的双拼写语义让未改动的写路径新近出错。bareDisablementBlocksQualifiedGrantWarnings 启动警告只在失败开关创建了限定授权之后的下一次启动才出现。同一谱系的 defaultDisabled 群组是 SkillsManagerDialog.tsx:249 的发现，修复方向相反。
建议：呈现冲突而不是确认一次无效写入：将唯一阻止者是工作区作用域条目（经原始拼写匹配）的行归类为“带原因锁定”（镜像更高作用域的锁定/归责 UI），或让 persistChanges/REST 路由在保存时报告存活的阻止条目。约束：skill-settings.test.ts 的 'writes the registry name and clears only that entry' 固定了 expect(updates.disabled).toEqual(['pdf', 'other'])（“启用一个技能永远不能清除更宽泛的禁用”）；config.ts 的 isSkillEnabled 先检查拒绝再检查授权。修复见证：新增 skill-settings.test.ts 用例，执行上述调用——无论修复采取何种形态，移除它必须使新断言变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| const unlockedSkills = useMemo( | ||
| () => allSkills.filter((s) => !higher.set.has(lower(s.name))), | ||
| [allSkills, higher.set], | ||
| () => allSkills.filter((s) => higher.lockedIn(s) === null), |
There was a problem hiding this comment.
[Critical] R1-2: [certifies-falsely] [regression] A skill gated only by a bare pre-prefix skills.defaultDisabled entry renders as a toggleable row here (buildHigherDisabled reads only skills.disabled), but toggling it on persists a qualified grant that cannot cancel the bare entry — resolveSkillSettings cancels defaultDisabled only on identical spelling (skill-settings.ts:61). The toggle silently no-ops while the dialog reports "Skills configuration saved.": enabled: ['rust:pdf'] is written, 'pdf' stays in disabledNames, the skill remains disabled, and the checkbox reverts on reopen. Before this PR the same toggle wrote enabled: ['pdf'], which cancelled the entry and enabled the skill — the rename newly breaks this cohort. Unlike the hard-disabled cohort (the finding on skill-settings.ts:110), defaultDisabled is designed to be overridable, so locking these rows is the wrong fix.
Witness:
PR arm: writes skills.enabled [ 'rust:pdf' ]; disabledNames [ 'pdf' ]; rust:pdf still blocked = true
BASE arm: writes [ 'pdf' ]; disabledNames []
fix flip: companion bare grant -> disabledNames [], skill enabled
Make the toggle effective: when enabling, also write the authored spelling into skills.enabled — the bare companion cancels the identical-spelling defaultDisabled entry while the qualified entry carries the grant — or make defaultDisabled cancellation registry-aware. Note bareEnabledGrantWarnings must then exempt deliberately-written cancelling companions (see the finding on config.ts:1574). Do NOT fix via the lock-the-row direction. Constraint: cancellation is by identical spelling (if (!enabled.has(name)) disablements.set(name, { reason: 'default' }), skill-settings.ts:61), and skill-settings.test.ts pins that enabling must never clear a broader disablement — add a cancelling grant, never delete entries. Fix witness: with defaultDisabled: ['pdf'] and a toggle { name: 'rust:pdf', wasEnabled: false, isEnabled: true }, applying the update output through resolveSkillSettings must leave 'pdf' out of disabledNames; removing the fix flips it red.
中文说明
[Critical] R1-2：仅被改名前的裸 skills.defaultDisabled 条目阻止的技能在此处显示为可切换行（buildHigherDisabled 只读 skills.disabled），但打开开关持久化的是一个无法取消该裸条目的限定授权——resolveSkillSettings 只在拼写完全相同时取消 defaultDisabled（skill-settings.ts:61）。开关静默无效，而对话框报告“Skills configuration saved.”：写入 enabled: ['rust:pdf']，'pdf' 留在 disabledNames，技能保持禁用，复选框在重新打开时复位。本 PR 之前同样的开关写入 enabled: ['pdf']，能取消条目并启用技能——改名新近破坏了这一群组。与硬禁用群组（skill-settings.ts:110 的发现）不同，defaultDisabled 本就设计为可覆盖，因此锁定这些行是错误的修复方向。
建议：让开关真正生效：启用时同时把原始拼写写入 skills.enabled——裸伴随条目取消同拼写的 defaultDisabled，限定条目承载授权——或让 defaultDisabled 的取消感知注册名。注意 bareEnabledGrantWarnings 届时必须豁免有意写入的取消性伴随条目（见 config.ts:1574 的发现）。不要采用“锁定该行”的方向。约束：取消按相同拼写进行（skill-settings.ts:61），且 skill-settings.test.ts 固定了“启用不能清除更宽泛的禁用”——应添加取消性授权，绝不删除条目。修复见证：在 defaultDisabled: ['pdf'] 且开关为 { name: 'rust:pdf', wasEnabled: false, isEnabled: true } 时，将更新输出经 resolveSkillSettings 应用后 'pdf' 必须不在 disabledNames 中；移除修复必须使其变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| */ | ||
| export const MAX_EXTENSION_OWNER_LABEL_WIDTH = 35; | ||
|
|
||
| function truncate(text: string, max: number): string { |
There was a problem hiding this comment.
[Suggestion] R1-4: The new badge cap truncate() measures UTF-16 .length while the layouts consuming the badge count terminal columns, and it slices UTF-16 code units — two newly introduced failure modes the fixed pre-PR [Extension] badge never had. First, a wide-character owner label passes the cap untouched and still overflows: an extension displayName of 20 Chinese characters yields a 25-unit label ≤ 35, so truncate is a no-op, yet the bracketed badge renders ≈41 columns against the 37-column budget, and the /help meta row wraps where the pre-PR badge fit on one line (a pure-ASCII 35-cap label plus [all] [model] wraps on 80-column terminals too). Second, the UTF-16 slice can split a surrogate pair: displayName: "xxxxxxxxxxxxxxxxxxxxxx🧩y" cuts mid-emoji and emits a lone high surrogate into the badge, rendering tofu in the completion popup, /help, and the duplicate truncate at SkillsManagerDialog.tsx:186.
Witness:
INTACT: cjkBadgeColumns=41 (cap budget 37); /help row wraps (allLine=7, modelLine=8)
baseline [Extension] badge: both on line 7
emoji fixture badge: "[Extension: xxxxxxxxxxxxxxxxxxxxxx\\ud83e…]" (lone surrogate)
FIXED: width-aware truncate -> cjkBadgeColumns=36; overflow assertion flips red
Cap by display width and cut on code-point/grapheme boundaries — e.g. iterate Array.from(text) accumulating until stringWidth (already a direct dependency of packages/cli) exceeds the cap, then append …. Keep both copies (commandMetadata.ts and SkillsManagerDialog.tsx:186) and the MAX_EXTENSION_OWNER_LABEL_WIDTH/LOCKED_ORIGIN_COLUMN cap semantics consistent. Constraint: the cap's own comment requires keeping the bracketed badge inside the popup's half-width cap (Math.min(maxLabelLength, Math.floor(contentWidth * 0.5)) at SuggestionsDisplay.tsx:162, ≤ 39 columns at 80 columns). Fix witness: extend the badge-bounds test in commandMetadata.test.ts to assert stringWidth(badge) <= MAX_EXTENSION_OWNER_LABEL_WIDTH + 2 for an all-wide-character displayName, and no unpaired surrogate for an emoji straddling the cut — both red today.
中文说明
[Suggestion] R1-4：新的徽章上限 truncate() 以 UTF-16 .length 计量，而使用徽章的布局按终端列数计数，且它按 UTF-16 码元切片——这是改动前固定的 [Extension] 徽章从没有过的两个新失效模式。其一，宽字符所有者标签会原样通过上限并仍然溢出：20 个汉字的 displayName 产生 25 个单元（≤35）的标签，truncate 不起作用，但带括号徽章渲染约 41 列、超出 37 列预算，/help 元信息行在改动前能单行放下的位置发生换行（纯 ASCII 的 35 上限标签加 [all] [model] 在 80 列终端也会换行）。其二，UTF-16 切片可能拆开代理对：displayName: "xxxxxxxxxxxxxxxxxxxxxx🧩y" 在 emoji 中间切开，向徽章发出孤立的高位代理，在补全面板、/help 以及 SkillsManagerDialog.tsx:186 的重复 truncate 处渲染为豆腐块。
建议：按显示宽度设上限并在码点/字位边界处切割——例如遍历 Array.from(text) 累加直到 stringWidth（已是 packages/cli 的直接依赖）超过上限，再追加 …。保持两处副本（commandMetadata.ts 与 SkillsManagerDialog.tsx:186）以及 MAX_EXTENSION_OWNER_LABEL_WIDTH/LOCKED_ORIGIN_COLUMN 的上限语义一致。约束：上限自身注释要求带括号徽章留在弹出框半宽上限内（SuggestionsDisplay.tsx:162，80 列时 ≤39 列）。修复见证：扩展 commandMetadata.test.ts 的徽章边界测试——全宽字符 displayName 断言 stringWidth(badge) <= MAX_EXTENSION_OWNER_LABEL_WIDTH + 2，跨切割位的 emoji 断言无孤立代理——两者当前都应变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * value: `skillItemValue` carries only what the pick guard matches, and the | ||
| * owner is display-only. | ||
| */ | ||
| export function skillRowLabel(skill: SkillConfig): string { |
There was a problem hiding this comment.
[Suggestion] R1-5: skillRowLabel appends the origin label uncapped, so an extension's unbounded displayName can make MultiSelect rows arbitrarily wide — the one skills-dialog surface the PR's own width-capping misses. The same file already solves this exact case for locked rows via truncate(skillOriginLabel(s), LOCKED_ORIGIN_COLUMN) (line ~707), so listed and locked rows now disagree on the same invariant. Concretely, with an 84-char extensionDisplayName, extensionOwnerLabel returns the untruncated label, MultiSelect renders item.label in a plain unconstrained <Text>, and the row wraps onto multiple lines, breaking the name/description/origin column alignment and consuming vertical space against maxItemsToShow.
Witness:
INTACT: label length 131, wrappedAcrossLines=true (origin split across lines 7–8), locked-row cap 37
FIXED: origin truncated to LOCKED_ORIGIN_COLUMN -> one line, label length 73
Cap the origin in the listed-row label the same way the locked row does — truncate skillOriginLabel(skill) to LOCKED_ORIGIN_COLUMN before appending it, so the two row kinds agree. Constraint: the locked section already budgets the origin at LOCKED_ORIGIN_COLUMN (SkillsManagerDialog.tsx:707); reuse that same column. Fix witness: add a describe('skillRowLabel') case with an extensionDisplayName exceeding the width budget and assert the returned label stays within NAME_COLUMN + 1 + 80 + 2 + LOCKED_ORIGIN_COLUMN; removing the truncation must red it.
中文说明
[Suggestion] R1-5：skillRowLabel 追加来源标签时不设上限，扩展不受限的 displayName 可以使 MultiSelect 行任意宽——这是本 PR 自身的宽度限制唯一遗漏的技能对话框界面。同一文件已经用 truncate(skillOriginLabel(s), LOCKED_ORIGIN_COLUMN)（约第 707 行）为锁定行解决了同样的问题，因此列表行与锁定行现在在同一不变量上不一致。具体地，84 字符的 extensionDisplayName 下，extensionOwnerLabel 返回未截断标签，MultiSelect 以普通无约束 <Text> 渲染 item.label，行被折成多行，破坏名称/描述/来源的列对齐，并占用 maxItemsToShow 的纵向空间。
建议：按锁定行的方式给列表行的来源标签设上限，使两种行保持一致。约束：锁定部分已按 LOCKED_ORIGIN_COLUMN（SkillsManagerDialog.tsx:707）为来源分配预算，复用同一列宽。修复见证：新增 describe('skillRowLabel') 用例，extensionDisplayName 超出宽度预算时断言返回标签不超过 NAME_COLUMN + 1 + 80 + 2 + LOCKED_ORIGIN_COLUMN；移除截断必须使其变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| if (registry.has(authored)) continue; | ||
| if (!enabled.has(authored)) continue; | ||
| warnings.push( | ||
| `Warning: skills.enabled lists '${authored}' by bare name, which no ` + |
There was a problem hiding this comment.
[Suggestion] R1-10: bareEnabledGrantWarnings calls every bare skills.enabled entry matching an authored name a stale grant ("no longer enables") without ever seeing that a bare entry can be load-bearing — cancelling an identically-spelled skills.defaultDisabled entry — so it emits a false warning whose advice disables a currently working skill (the runtime twin of the false "grants nothing" doc claim reported on docs/users/configuration/settings.md:553; doc fixes alone do not silence this warning). With defaultDisabled: ['pdf'] (e.g. system scope) + enabled: ['rust:pdf', 'pdf'], resolveSkillSettings cancels the entry and the skill is enabled, yet the warning fires (signature (enabled, skills) cannot see defaultDisabled). Since 'rust:pdf' is already listed, the only actionable change is removing 'pdf'; doing so un-cancels defaultDisabled: 'pdf' and the working skill becomes blocked. When the defaultDisabled entry lives in a system/admin file the user cannot edit it back out.
Witness:
intact: warning fires; disabledNames (bare pdf kept) [] ;
disabledNames (bare pdf removed per advice) [ 'pdf' ] <- the advised edit revives the block
fix flip: load-bearing case silent; hard-survives and pinned stale cases still warn
Make the warning defaultDisabled-aware: pass the resolved disablements (or the defaultDisabled names) at the config.ts:3496-3506 wiring, where the providers already yield resolveSkillSettings output, and skip bare entries that cancel a reason: 'default' disablement — a bare entry never cancels a hard disable, so the warning stays valid for reason: 'hard' — or at minimum reword to name the exception. This is also the companion change the SkillsManagerDialog.tsx:249 finding's fix needs. Constraint: cancellation is entry-vs-entry identical-spelling only (resolveSkillSettings, skill-settings.ts), and the pinned no-defaultDisabled case (bareEnabledGrantWarnings(new Set(['pdf']), [rustPdf]) expects the warning) must keep firing. Fix witness: a case where a defaultDisabled entry 'pdf' exists asserts the warning stays silent for the bare 'pdf' grant; removing the exception branch reds it.
中文说明
[Suggestion] R1-10：bareEnabledGrantWarnings 把每个与原始名匹配的裸 skills.enabled 条目都称为过时授权（“不再启用”），却看不到裸条目可能是承重的——它可以取消同拼写的 skills.defaultDisabled 条目——于是发出错误警告，其建议会禁用当前可用的技能（这是 settings.md:553 错误文档说法的运行时孪生；仅修文档无法让该警告闭嘴）。在 defaultDisabled: ['pdf']（如 system 作用域）+ enabled: ['rust:pdf', 'pdf'] 时，resolveSkillSettings 取消该条目、技能被启用，但警告仍然出现（签名 (enabled, skills) 看不到 defaultDisabled）。由于 'rust:pdf' 已在列表中，唯一可执行的更改是移除 'pdf'；这样做会恢复 defaultDisabled: 'pdf' 的取消失效，可用技能被阻止。当 defaultDisabled 条目位于 system/admin 文件时，用户无法再改回来。
建议：让警告感知 defaultDisabled：在 config.ts:3496-3506 的接线处传入解析后的 disablements（或 defaultDisabled 名单），对取消了 reason: 'default' 禁用的裸条目跳过——裸条目永远不能取消硬禁用，因此对 reason: 'hard' 警告仍然有效——或至少改写消息指名这一例外。这也是 SkillsManagerDialog.tsx:249 发现的修复所需的配套更改。约束：取消仅按条目对条目的相同拼写进行；固定的无 defaultDisabled 用例必须继续触发。修复见证：存在 defaultDisabled 条目 'pdf' 时，断言裸 'pdf' 授权保持静默；移除例外分支必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| entries: ReadonlyMap<string, T>, | ||
| skill: { name: string; authoredName?: string }, | ||
| ): T | undefined { | ||
| for (const name of skillRestrictionNames(skill)) { |
There was a problem hiding this comment.
[Suggestion] R1-11: lookupSkillSetting's registry-first first-hit ordering returns the first spelling match even when the other spelling holds a stronger entry. With skills.disabled: ['pdf'] (hard, bare) plus skills.defaultDisabled: ['rust:pdf'] — exactly the mixed-spelling state this PR's migration creates — resolveSkillSettings keys the map under both spellings, and this loop walks them registry-first and returns the default entry, so mapSkillConfigToStatus and buildExtensionSkillStates report disabledReason: 'default' with no lockedScope for a hard block. The web shell / extension page then present an overridable, scope-unlocked row; enabling it persists enabled: ['rust:pdf'] without any reason-based refusal, the skill stays disabled, and the UI reports success. The dialog scope label can likewise blame a lower scope while a higher-scope bare entry gates. Enforcement is intact — deny-by-default holds — the attribution the surfaces depend on is wrong.
Witness:
intact: lookupSkillDisablement {"reason":"default"}; mapSkillConfigToStatus disabledReason "default"
while isSkillEnabled blocks: true
fix flip: prefer 'hard' across spellings -> {"reason":"hard","lockedScope":"user"};
all 28 pinned tests stay green
Collect hits across all skillRestrictionNames(skill) spellings in lookupSkillDisablement and prefer the strongest entry (reason: 'hard' over 'default'; among equal reasons keep registry-first and keep the one carrying lockedScope). Constraint: lookupSkillSetting's doc pins "the registry-name entry wins a tie" — keep registry-first when severities are equal; skill-settings.test.ts pins registry-first where the registry entry is the hard one, and pins that the bare spelling still matches. Fix witness: a lookupSkillDisablement case with entries {'pdf': {reason:'hard', lockedScope:'user'}, 'rust:pdf': {reason:'default'}} must return the hard entry; restoring flat first-hit order reds it.
中文说明
[Suggestion] R1-11：lookupSkillSetting 的“注册名优先、首个命中即返回”顺序，会在另一个拼写持有更强条目时仍返回首个拼写命中。在 skills.disabled: ['pdf']（硬、裸）加 skills.defaultDisabled: ['rust:pdf']——正是本 PR 迁移产生的混合拼写状态——下，resolveSkillSettings 以两个拼写为键，而此循环按注册名优先遍历并返回 default 条目，于是 mapSkillConfigToStatus 与 buildExtensionSkillStates 对硬阻止报告 disabledReason: 'default' 且无 lockedScope。Web shell / 扩展页随后呈现“可覆盖、无作用域锁定”的行；启用它会持久化 enabled: ['rust:pdf'] 而无任何基于原因的拒绝，技能保持禁用而 UI 报告成功。对话框的作用域标签同样可能把责任归到低作用域，而实际把关的是更高作用域的裸条目。执行本身完好——拒绝优先依然成立——错的是各界面赖以呈现的归因。
建议：在 lookupSkillDisablement 中收集 skillRestrictionNames(skill) 所有拼写的命中并优先最强条目（reason: 'hard' 优于 'default'；同级原因保持注册名优先并保留携带 lockedScope 的条目）。约束：lookupSkillSetting 文档固定“注册名条目在同级时获胜”；skill-settings.test.ts 固定了注册条目为硬条目时的注册优先，也固定裸拼写仍能匹配。修复见证：条目为 {'pdf': {reason:'hard', lockedScope:'user'}, 'rust:pdf': {reason:'default'}} 时必须返回硬条目；恢复“首个命中”顺序必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| isInactiveExtensionSkill( | ||
| { | ||
| name: cmd.skillDetail.name, | ||
| authoredName: cmd.skillDetail.authoredName, |
There was a problem hiding this comment.
[Suggestion] R1-12: This authoredName propagation into buildAvailableCommandsSnapshot's isInactiveExtensionSkill call has no test that would red if the line were removed — every inactive-extension fixture in Session.test.ts uses one spelling for manifest and registry and never sets authoredName, and extension-skills.test.ts exercises the helper directly, bypassing this call site. Delete the line and isInactiveExtensionSkill falls back to the qualified name while inactiveExtensionSkillRefs holds the manifest's authored spelling, so the lookup misses and an inactive extension's skill command passes the filter into the ACP available_commands_update snapshot — a fail-open the mutation run proves invisible to the suite.
Witness:
mutation (line deleted): Session.test.ts + extension-skills.test.ts 815/815 pass <- mutation survives
probe test on the mutant: FAIL "expected [ 'ext:audit' ] to not include 'ext:audit'"
(inactive extension skill leaked into the snapshot)
restored: probe green
Add a Session.test.ts case to the inactive-extension block where the slash command's skillDetail carries name: 'ext:audit' and authoredName: 'audit' while the manifest holds the bare name, and assert the command is omitted from the snapshot. Fix witness: that test must red when this line is deleted.
中文说明
[Suggestion] R1-12：这行向 buildAvailableCommandsSnapshot 的 isInactiveExtensionSkill 调用传递 authoredName 的改动没有测试能在其被删除时变红——Session.test.ts 的所有未激活扩展夹具对清单与注册使用同一拼写且从不设置 authoredName，extension-skills.test.ts 又直接测试助手本身、绕过了此调用点。删除该行后，isInactiveExtensionSkill 回退到限定 name，而 inactiveExtensionSkillRefs 持有清单的原始拼写，查找落空，未激活扩展的技能命令穿过过滤器进入 ACP available_commands_update 快照——变异运行证明这种“开门”失效对测试套件不可见。
建议：在 Session.test.ts 的未激活扩展块中新增用例：斜杠命令的 skillDetail 携带 name: 'ext:audit' 与 authoredName: 'audit'，而清单持有裸名，断言该命令被排除在快照之外。修复见证：删除本行时该测试必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| ? { extensionName: skill.extensionName } | ||
| ? { | ||
| extensionName: skill.extensionName, | ||
| authoredName: skill.authoredName, |
There was a problem hiding this comment.
[Suggestion] R1-13: This propagation of authoredName onto skillDetail is untested: SkillCommandLoader.test.ts never asserts it (its skillDetail check is toMatchObject({ extensionName })), and every downstream test hand-builds its skillDetail fixtures, so the producer→consumer pairing is covered only on the consumer halves. Delete the line and every skill command loses its authored spelling, commandRestrictionNames degrades to the registry name alone, and a legacy slashCommands.disabled: ['pdf'] entry no longer hides /rust:pdf — silently lifting the restriction this PR exists to preserve (and starving Session.ts's inactive-skill filter of the same spelling).
Witness:
mutation (line deleted): SkillCommandLoader + CommandService + commandUtils
+ nonInteractiveCliCommands suites 125/125 pass <- mutation survives
probe assertion on the mutant: FAIL (authoredName absent from skillDetail)
restored: probe green
In SkillCommandLoader.test.ts's extension-skills block, give the fixture skill name: 'superpowers-lab:tmux' / authoredName: 'tmux' and assert commands[0].skillDetail carries authoredName: 'tmux'. Fix witness: that assertion must red when this line is removed.
中文说明
[Suggestion] R1-13：这行把 authoredName 传到 skillDetail 的改动没有测试：SkillCommandLoader.test.ts 从不对其进行断言（其 skillDetail 检查是 toMatchObject({ extensionName })），而所有下游测试都手工构造 skillDetail 夹具，因此“生产者→消费者”配对只在消费者一侧被覆盖。删除该行后，所有技能命令失去原始拼写，commandRestrictionNames 退化为仅注册名，遗留的 slashCommands.disabled: ['pdf'] 条目不再隐藏 /rust:pdf——静默解除了本 PR 存在所要保留的限制（同时也让 Session.ts 的未激活技能过滤器失去同一拼写）。
建议：在 SkillCommandLoader.test.ts 的扩展技能块中，让夹具技能为 name: 'superpowers-lab:tmux' / authoredName: 'tmux'，并断言 commands[0].skillDetail 携带 authoredName: 'tmux'。修复见证：删除本行时该断言必须变红。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| Two limits worth knowing: | ||
|
|
||
| - Cross-level precedence is unchanged and still compares registered names exactly (`project` > `user` > `extension` > `bundled`), so a personal or project Skill you author as `rust:pdf` outranks the extension's `pdf`. Bare-name collisions between a personal or project Skill and a bundled Skill are likewise still settled by that precedence, not by the prefix. A Skill that collides with a custom command is not — on the slash surface the last loader wins, and custom commands load after Skills, so `/pdf` runs the custom command while the Skill stays available to the model. | ||
| - Skill names are also used as filenames: the file a Skill reads its invocation arguments from replaces every character outside `[A-Za-z0-9._-]` with `_`, so an extension Skill registered as `rust:pdf` and a personal or project Skill authored `rust_pdf` both resolve to `qwen-skill-args-rust_pdf.txt` and share one arguments file. (The prefix does not collide with itself: `rust:rust_pdf` becomes `rust_rust_pdf`.) Non-ASCII letters fold the same way, so an authored `café` and an authored `caf_` land on `caf_` too — a limitation that predates the prefix, which only makes it easier to hit. Avoid a Skill name that is another name with `:` turned into `_`. |
There was a problem hiding this comment.
[Suggestion] R1-14: The parenthetical "The prefix does not collide with itself" is false as a general statement: extension names legally contain _ (validateName/assertIdentity accept /^[a-zA-Z0-9-_.]+$/, and my_extension is explicitly accepted by the extension-manager tests), so the : → _ fold in safe() can erase the owner/name boundary between two prefixed extension skills. Extension rust_pdf shipping skill x and extension rust shipping skill pdf_x are both valid; both get prefixed (rust_pdf:x, rust:pdf_x); and both sanitize to qwen-skill-args-rust_pdf_x.txt — invoking both with arguments in one session, the second write truncates the first's args file (O_TRUNC), and whichever skill reads its args file afterwards parses the other skill's invocation arguments. The closing avoidance advice covers authored-vs-prefixed pairs only, not the extension-name side; the rust:rust_pdf example itself is true, which is what makes the false generalization it supports easy to trust.
Witness:
skillArgsPath probe: rust_pdf:x -> qwen-skill-args-rust_pdf_x.txt
rust:pdf_x -> qwen-skill-args-rust_pdf_x.txt (identical path)
Replace the parenthetical with the true statement — prefixed names can also collide with each other when an extension name contains _, citing the rust_pdf:x / rust:pdf_x pair — and extend the avoidance advice to cover owner names, not just authored names.
中文说明
[Suggestion] R1-14：括号里的“前缀不会与自身碰撞”作为一般性说法是错误的：扩展名可以合法包含 _（validateName/assertIdentity 接受 /^[a-zA-Z0-9-_.]+$/，扩展管理器测试明确接受 my_extension），因此 safe() 中 : → _ 的折叠可能抹掉两个带前缀扩展技能之间的“所有者/名称”边界。扩展 rust_pdf 提供技能 x、扩展 rust 提供技能 pdf_x 都是合法的；两者都被加上前缀（rust_pdf:x、rust:pdf_x）；并且两者都被净化为 qwen-skill-args-rust_pdf_x.txt——在一个会话中先后带参数调用两者，第二次写入会截断第一个的参数文件（O_TRUNC），之后读取参数文件的技能解析到的是另一个技能的调用参数。结尾的规避建议只覆盖“原始名对前缀名”，没有覆盖扩展名一侧；rust:rust_pdf 示例本身为真，这正让它支撑的错误泛化容易被信任。
建议：把括号内容改为真实陈述——当扩展名包含 _ 时，带前缀的名字彼此也可能碰撞，并引用 rust_pdf:x / rust:pdf_x 这对例子——同时把规避建议扩展到所有者名，而不仅是原始名。
— qwen3.8-max via Qwen Code /review (v0.22.3)
…s, locked rows naming the blocker, qualified ACP rows, column-aware badges Review wave on QwenLM#10841: the startup warnings take the raw lists (a bare opt-in pair that cancels a defaultDisabled entry is load-bearing, not stale) and advise by block reason (hard entries are removed, not cancelled); the Skills dialog locks rows a toggle cannot change and names the blocking entry instead of saving into a no-op; ACP manifest-only rows carry the qualified name so the web shell cannot persist a bare entry that never grants; badge and row truncation count terminal columns; lookupSkillDisablement prefers the hard entry in mixed-spelling states; docs, schema description, and the regenerated settings schema state the bare-pair corner and the `_`-fold collision; delete-red tests pin the authoredName propagations.
…ed-row dialog test under jsdom
|
@qwen-code /review |
Round 5 at
|
| arm | what it is | bundle identity, checked on the emitted bytes |
|---|---|---|
| PR | 315017a726 |
new wording ×1, old pair warning ×1 |
| round-4 hunk | same tree, packages/core/src/config/config.ts reverted to aead7ca511, re-bundled |
new wording ×0, old pair warning ×1 |
| base | f1ed3bc31a, this PR's merge-base with origin/main, separate worktree |
×0 / ×0 |
Fixture: extension rust (skillStates:{pdf:false}, plus functions), extension python (pdf), extension go (report, default off), and a user skill pdf. Every skill body carries a unique token, and the verdict is read off the recorded request bytes — which tokens appear inside <available_skills> (advertised) and which appear outside it (body actually injected) — not off the pane. 22 settings shapes × the arms, plus 6 mutants.
N-4 is closed — the new test really is the witness
Mutants of packages/core/src/config/config.test.ts (645 tests):
| mutant | change | result |
|---|---|---|
| M1 | initialize() collects authoredSkillName(skill) again (the round-4 state) |
1 failed — exactly the new surfaces the default-off pair warning named by registry identity |
| M2 | drop !lists.hardDisabled.has(name) |
645 passed — SURVIVES |
| M3 | drop !lists.hardDisabled.has(authored) |
1 failed |
| M4 | onNames = offNames |
5 failed |
| M5 | stillOff = offNames |
2 failed |
| M6 | drop lists.enabled.has(name) |
4 failed |
N-5 is closed — same-tree, single-file A/B
skills.enabled:["pdf","rust:pdf"] + skills.defaultDisabled:["pdf"], rust declaring pdf default off:
| arm | startup warning | the panel from that same boot | rust:pdf on the wire |
|---|---|---|---|
| round-4 hunk | "…no longer enables the extension skill 'rust:pdf', which defaults off" |
rust:pdf [x] |
advertised |
| PR | "…but the qualified grant 'rust:pdf' in skills.enabled already enables it, so the bare pair changes nothing." |
rust:pdf [x] |
advertised |
All three reachable triggers reproduce: user-level pair + user-level qualified grant, workspace qualified grant + user-level bare pair, and the single-owner extension (go:report).
The mixed case splits correctly. With rust and python both declaring pdf default off and only rust:pdf granted, the round-4 hunk lumps them into one sentence ('python:pdf', 'rust:pdf', which default off) while rust:pdf is on; the new head emits two warnings, and the panel agrees — python:pdf [ ], rust:pdf [x].
N-7 (new, non-blocking) — the default-off pair warning never consults skills.disabled
enabled:["pdf"] + defaultDisabled:["pdf"] + disabled:["pdf"]. The only warning printed is "Replace the bare 'pdf' with 'rust:pdf' in both … to enable it." — and the dialog on that same boot already has both pdf skills in the locked section as [locked: User].
Following it verbatim:
| boot | settings | rust:pdf |
warning |
|---|---|---|---|
| 1 | enabled:[pdf] defaultDisabled:[pdf] disabled:[pdf] |
off | the pair warning, promising "to enable it" |
| 2 | replaced as instructed | still off | only now is the skills.disabled entry named |
| 3 | 'pdf' removed from skills.disabled |
enabled | none |
The non-pair branch of the same function already appends exactly that note ("A bare 'pdf' in skills.disabled also blocks it … remove that entry too."). The pair branch does not.
Sharper form: when the hard entry is written qualified (disabled:["python:pdf"]), bareDisablementBlocksQualifiedGrantWarnings never fires — it only matches a bare disable entry — so after following the advice there is no warning at all, a silent dead end. One line fixes both: append the existing hardNote to the stillOff warning.
N-8 (new, non-blocking) — surviving mutant M2: the qualified-hard-entry guard has no test
!lists.hardDisabled.has(name) inside the onNames filter is load-bearing, but deleting it leaves all 645 tests green. I rebundled the CLI with that one line gone and ran enabled:["pdf","rust:pdf"] + defaultDisabled:["pdf"] + disabled:["rust:pdf"] on the real path: the warning flips to "the qualified grant 'rust:pdf' … already enables it" while the panel from the same boot shows rust:pdf [locked: User] and the skill is absent from the wire — the N-5 defect you just fixed, back and unnoticed.
Your keeps the off-state claim when a hard entry defeats the qualified grant test only covers a bare hard entry, which is caught by the other clause (!lists.hardDisabled.has(authored)). One assertion with a qualified skills.disabled entry closes it.
Wording nit: with several granted members it reads "the qualified grant 'python:pdf', 'rust:pdf' … already enable them" — grant should be plural.
Carried, unchanged
- N-3 —
if (registry.has(authored)) continue;still swallows the whole group: with a user-levelpdfpresent,defaultDisabled:["pdf"] + enabled:["pdf"]leaves a genuinely default-offrust:pdfoff with no diagnostic. You deferred it; it stayed true on the new head. - N-6 — the advice is unreachable when the bare entry is in system settings. Two boots, following it verbatim in the only file the user can edit: identical warning, identical state. The dialog on the same boot already prints
[locked: skills.defaultDisabled 'pdf' (System)].
Regression sweep on the new head — clean, and the before/after is sharper than the PR description says
On origin/main, with a user pdf and an extension python shipping pdf, <available_skills> advertises the user's pdf while /pdf injects python's body. The model is told about one skill and handed another's instructions, and rust:pdf does not exist at all.
base f1ed3bc31a |
PR 315017a726 |
|
|---|---|---|
/skills panel |
16 skills, one pdf row |
18 skills, three rows, each with its owner |
/pdf on the wire |
advertises the user's, injects python's | advertises both, injects the one it advertised |
| ACP available commands | 51, skill rows functions, pdf |
52, skill rows python:pdf, rust:functions, rust:pdf |
@ext:rust |
Skills: functions, pdf |
Skills: rust:functions, rust:pdf |
--disabled-slash-commands pdf |
— | blocks both rust:pdf and python:pdf |
--disabled-slash-commands rust:pdf |
— | blocks only rust:pdf; python:pdf still runs |
--help either-spelling rule |
absent | present |
| locked-section i18n key | — | 9/9 locales carry the reworded key, no stale key |
/rust:pdf · /python:pdf · /rust:functions |
— | each injects its own body and only its own |
Gates
npm -w packages/core run typecheck and npm -w packages/cli run typecheck both exit 0. core src/skills src/config src/tools/skill.test.ts: 1408/1408 across 32 files (the skill-curator flake from round 4 did not recur). The 18 cli suites this PR touches: 2202/2202.
The red web-shell E2E Smoke is not yours. The failing spec, packages/web-shell/client/e2e/web-shell.history-viewport.spec.ts, arrived from main in #11208 (a4a62258b1); on this head packages/web-shell is byte-identical to origin/main, and the complete diff between this head and origin/main is exactly this PR's own 63 files plus one unrelated commit the branch is behind (#11315, CDP tunnel).
Recommendation
Merge-ready. N-4 and N-5 are closed for real: the fix changes observable behaviour on the real path, the new test kills the mutant that models the pre-fix state, and warning, panel and wire now agree on the same boot. N-7, N-8, N-3 and N-6 change no skill's reachable state, so all four are fine as follow-ups. If you take one, take N-8 — it is a single assertion, and it guards the exact defect class you just repaired.
The only thing blocking is the bot CHANGES_REQUESTED at e7223d6f5f (2026-09-05 09:19), nine commits stale — it predates every fix verified in rounds 3, 4 and 5.
Full Chinese report: https://github.com/wenshao/qwen-code/blob/assets-pr10841/REPORT-r5.zh-CN.md
中文说明
第 5 轮验证（315017a726）—— N-4、N-5 真正闭合；新增一条非阻断缺口及其测试孪生
增量轮，覆盖 c05ec5d824（合并 main）与 315017a726（N-4/N-5 修复）。三个臂全部真实构建（npm ci → npm run build → npm run bundle），并通过真实 TUI、真实 headless 路径与真实 ACP 传输驱动：
| 臂 | 内容 | bundle 身份（按产物字节核对） |
|---|---|---|
| PR | 315017a726 |
新文案 ×1、旧配对警告 ×1 |
| round-4 hunk | 同一棵树，只把 packages/core/src/config/config.ts 换回 aead7ca511 后重新 bundle |
新文案 ×0、旧配对警告 ×1 |
| base | f1ed3bc31a（本 PR 与 origin/main 的 merge-base），独立 worktree |
×0 / ×0 |
夹具：扩展 rust（skillStates:{pdf:false}，另带 functions）、扩展 python（pdf）、扩展 go（report，默认关闭）、用户级技能 pdf。每个技能正文带唯一 token，判据取自录制式 fake OpenAI 落盘的请求字节——哪些 token 出现在 <available_skills> 内（被广告）、哪些出现在块外（正文被真正注入）——而不是看面板文字。22 个配置形态 × 各臂，另加 6 个变异体。
N-4 已闭合 —— 新增测试确是见证
对 packages/core/src/config/config.test.ts（645 条）做变异：
| 变异体 | 改动 | 结果 |
|---|---|---|
| M1 | initialize() 改回收集 authoredSkillName(skill)（第 4 轮状态） |
1 条红 —— 正是新增的 surfaces the default-off pair warning named by registry identity |
| M2 | 去掉 !lists.hardDisabled.has(name) |
645 全绿 —— 存活 |
| M3 | 去掉 !lists.hardDisabled.has(authored) |
1 条红 |
| M4 | onNames = offNames |
5 条红 |
| M5 | stillOff = offNames |
2 条红 |
| M6 | 去掉 lists.enabled.has(name) |
4 条红 |
N-5 已闭合 —— 同树单文件 A/B
skills.enabled:["pdf","rust:pdf"] + skills.defaultDisabled:["pdf"]，rust 声明 pdf 默认关闭：
| 臂 | 启动警告 | 同一次启动的面板 | 请求字节里的 rust:pdf |
|---|---|---|---|
| round-4 hunk | "…no longer enables the extension skill 'rust:pdf'，which defaults off" |
rust:pdf [x] |
已广告 |
| PR | "…but the qualified grant 'rust:pdf' in skills.enabled already enables it，so the bare pair changes nothing." |
rust:pdf [x] |
已广告 |
三个可达触发器全部复现：用户级配对 + 用户级限定授权、workspace 限定授权 + 用户级裸配对、单一属主扩展（go:report）。
混合形态的拆分是正确的：rust 与 python 都声明 pdf 默认关闭而只授权 rust:pdf 时，round-4 臂用一句话把两者都说成默认关闭（而 rust:pdf 当时是开的）；新 head 拆成两条，面板一致：python:pdf [ ]、rust:pdf [x]。
N-7（新增，非阻断）—— 默认关闭配对警告从不查 skills.disabled
enabled:["pdf"] + defaultDisabled:["pdf"] + disabled:["pdf"]：唯一那条警告说 "Replace the bare 'pdf' with 'rust:pdf' in both … to enable it."，而同一次启动的对话框已经把两个 pdf 技能放进锁定区 [locked: User]。
逐字照做：
| 启动 | 设置 | rust:pdf |
警告 |
|---|---|---|---|
| 1 | enabled:[pdf] defaultDisabled:[pdf] disabled:[pdf] |
关闭 | 配对警告，承诺 "to enable it" |
| 2 | 按建议替换 | 仍关闭 | 这时才点名 skills.disabled 里的条目 |
| 3 | 删掉 disabled:[pdf] |
启用 | 无 |
同一函数的非配对分支已经有这段 hardNote（"…remove that entry too."），配对分支没有。
更尖锐的形态：硬条目写成限定名（disabled:["python:pdf"]）时，bareDisablementBlocksQualifiedGrantWarnings 只匹配裸条目，于是照做之后一条警告都不打印，静默死路。把已有的 hardNote 追加到 stillOff 那条警告即可同时修好两者。
N-8（新增，非阻断）—— 存活变异体 M2：限定硬条目那道守卫没有测试
onNames 过滤里的 !lists.hardDisabled.has(name) 是承重件，删掉它 645 条仍全绿。我用删掉该行重新打包的真实 CLI 跑 enabled:["pdf","rust:pdf"] + defaultDisabled:["pdf"] + disabled:["rust:pdf"]：警告翻成 "the qualified grant 'rust:pdf' … already enables it"，而同一次启动的面板显示 rust:pdf [locked: User]、请求字节里也没有它 —— 正是刚修好的 N-5 缺陷复现且无人察觉。
现有的 keeps the off-state claim when a hard entry defeats the qualified grant 只覆盖裸硬条目（由另一道守卫 !lists.hardDisabled.has(authored) 拦下）。补一条限定硬条目的断言即可。
文案小瑕疵： 多个成员同时被授权时读作 "the qualified grant 'python:pdf', 'rust:pdf' … already enable them"，grant 应为复数。
沿用的两条（未变）
- N-3 ——
if (registry.has(authored)) continue;依旧吞掉整组：存在用户级pdf时，defaultDisabled:["pdf"] + enabled:["pdf"]下确实默认关闭的rust:pdf静默保持关闭且零提示。你已说明延后，这里只记录它在新 head 上依然成立。 - N-6 —— 裸条目落在 system settings 时建议无法执行。两次启动在用户唯一可编辑的文件里逐字照做，警告与状态完全不变；而同一次启动的对话框已经打印
[locked: skills.defaultDisabled 'pdf' (System)]。
新 head 回归清单 —— 全部通过，且前后对比比 PR 描述更尖锐
在 origin/main 上，存在用户 pdf 与扩展 python 的 pdf 时，<available_skills> 广告的是用户的 pdf，而 /pdf 注入的是 python 的正文：模型被告知一个技能、拿到的却是另一个的指令，rust:pdf 则完全不存在。
base f1ed3bc31a |
PR 315017a726 |
|
|---|---|---|
/skills 面板 |
16 个技能，1 行 pdf |
18 个技能，3 行，各带属主 |
/pdf 的链路 |
广告用户的、注入 python 的 | 两者都广告，注入它广告的那个 |
| ACP 可用命令 | 51 条，技能行 functions, pdf |
52 条，技能行 python:pdf, rust:functions, rust:pdf |
@ext:rust |
Skills: functions, pdf |
Skills: rust:functions, rust:pdf |
--disabled-slash-commands pdf |
— | 同时挡住 rust:pdf 与 python:pdf |
--disabled-slash-commands rust:pdf |
— | 只挡 rust:pdf，python:pdf 仍可运行 |
--help 双拼写说明 |
无 | 有 |
| 锁定区 i18n 键 | — | 9/9 locale 含新键，无残留旧键 |
/rust:pdf · /python:pdf · /rust:functions |
— | 各注入且只注入自己的正文 |
门禁
npm -w packages/core run typecheck 与 npm -w packages/cli run typecheck 均退出码 0。core src/skills src/config src/tools/skill.test.ts：32 个文件、1408/1408 通过（第 4 轮那条 skill-curator 抖动未复现）。本 PR 触及的 18 个 cli 套件：2202/2202。
红掉的 web-shell E2E Smoke 与本 PR 无关。 失败用例文件 packages/web-shell/client/e2e/web-shell.history-viewport.spec.ts 由 main 的 #11208（a4a62258b1）引入；本 head 上 packages/web-shell 与 origin/main 逐字节相同，且本 head 与 origin/main 的全部差异恰好是本 PR 自己的 63 个文件，外加落后 main 的一个无关提交（#11315，CDP tunnel）。
结论
可合并。 N-4、N-5 是真闭合：修复在真实链路上改变了可观察行为，新测试杀死了建模修复前状态的变异体，警告、面板与链路字节在同一次启动上三方一致。N-7、N-8、N-3、N-6 都不改变任何技能最终的可达状态，四条都可作为后续。若只做一条，建议 N-8：只需一条断言，且它守的正是刚修好的那个缺陷类。
唯一阻断仍是 e7223d6f5f（2026-09-05 09:19）那条机器人 CHANGES_REQUESTED，它已落后 9 个提交，早于第 3、4、5 轮验证过的全部修复。
完整中文报告：https://github.com/wenshao/qwen-code/blob/assets-pr10841/REPORT-r5.zh-CN.md
🤖 Generated with Claude Code — Claude Opus 5 (1M context)
|
@wenshao I misread your last comment and rushed changes into this branch that were meant for a follow up PR, and that push caused a dismissal of your approval. The review bot also never finished its pass based on recent changes, so its request-for-changes still stands from an earlier commit of this PR. Sorry and Thanks |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review at head ab8e10e3b03db11aad81f3e01084e559e8a2dc3d — verdict: no Criticals, 2 Suggestions.
Naming / identity audit
Registry identity is extension:skill (skills/types.ts qualifySkillName), and the authored name is retained in SkillConfig.authoredName. The invariant is: restrictions match both spellings, grants match the registry spelling only. I checked every lookup and persist site:
- Reads:
Config.isSkillEnabled(blocks viaskillRestrictionNames, grants via registry name, extension-store read viaauthoredSkillName),lookupSkillSetting/lookupSkillDisablement(dual-spelling, hard-beats-default,lockedScopewins), the CommandService denylist (commandRestrictionNames),SkillCommandLoader, ACPavailableCommands,skillsCommand, and the suggestion/label surfaces. - Persist: the extension skill-state store stays keyed by the authored name on both sides —
buildExtensionSkillStatesreturnsname: skill.nameandgetExtensionSkillState(extension.id, skill.name, …)(serve/routes/workspace-extensions.ts:469-489) — so the Web Shell PUT round-trips authored names. No orphan-key mismatch. - R3-1 (round 3: the
@mentioncontext builder kept the bare name) is fixed at head:utils/extension-mention.ts:88now usesqualifySkillName(extension.name, s.name). - No semantic drift versus current
main: the only recent main change to this surface keys onskill.level === 'bundled' && skill.name === 'review'(tools/skill-utils.ts), and bundled skills are not prefixed.
Suggestions
- The non-TUI enable toggle is unguarded against a legacy bare disablement.
skill-settings.tsupdateTarget(unchanged by this PR) removes only the exact spelling passed in, soserve/run-qwen-serve.ts:5236-5296→workspace-service/index.ts:839-931returns{ skillName, enabled, changed: true }for a request enablingrust:pdfwhile a legacyskills.disabled: ["pdf"]still blocks it throughskillRestrictionNames. The TUI is mitigated (SkillsManagerDialog'slockedInlocks the row and names the entry and scope); the daemon/Web-Shell path reports success on a no-op. This matches the maintainers' own round-3 deferred probe (acpAgent.ts:7245), so I am filing it as a Suggestion rather than a new Critical. The fix is either to drop everyskillRestrictionNamesspelling on grant, or to surfacelockedScopein the toggle response. - Size and placement. ~220 of the added core lines are English warning-text composition inside
packages/core/src/config/config.ts(bareEnabledGrantWarnings,bareDisablementBlocksQualifiedGrantWarnings). Message text living in core is what keeps re-triggering the doc/schema wording findings — R1-6/7/8, R2-x and R3-2/3/4 are all the same sentence in four places. Total is ~1053 production lines, past the 1000-line advisory threshold.
Pre-existing threads re-checked at head
R1-1, R1-2, R1-3 (round 1), R1-1/R1-2 (round 2) and R3-1 (round 3) are all fixed by this diff, independently verified at head (dual-spelling lookupSkill*, lockedIn locking, qualified mention names). Remaining open items are Suggestion-severity only — notably R2-2/R3-5/R3-6, where the warning wiring is pinned by presence rather than behaviour.
Agent-assisted review. Findings were re-read in the file content at the exact head SHA above before filing. Posting as a comment only — no approval implied.
Round 6 at
|
| arm | what it is | identity, checked on the emitted bundle bytes |
|---|---|---|
| PR | ab8e10e3b0 |
so replacing the grant alone will not enable ×2, plural-grant template ×1 |
| head − hunk | same tree, only 6fe8cff55e's config.ts hunk reverse-applied, core rebuilt, re-bundled |
×1 / ×0 |
| base | cf987cdc59, this PR's merge-base with origin/main, separate worktree |
— |
The fixture is the same as round 5. Extension rust ships pdf (default off) and functions, extension python ships pdf, and one shape adds a user pdf. Every skill carries a unique token. Each verdict is read from the recorded request bytes, i.e. which skills are advertised to the model, and cross-checked against the /skills panel on the same boot. Coverage: 18 settings shapes × 3 arms, 8 mutants, 2 daemon arms and an 8-check smoke run.
N-7 is closed
Shape: enabled:["pdf"] + defaultDisabled:["pdf"] + disabled:["pdf"]. The pair warning now ends with "A bare 'pdf' in skills.disabled also blocks them under either spelling, so replacing the grant alone will not enable anything: remove that entry too." On the same boot, the /skills panel shows both pdf skills as [locked: User]. In the single-owner shape (only rust installed) the note reads "blocks it".
N-8 is closed, and its surviving mutant is now killed
Here the hard entry is written qualified, disabled:["rust:pdf"], with or without a qualified grant. The head − hunk arm printed "…to enable it." and stopped there, which was round 5's silent dead end. The PR now names the entry for each blocked member: "'rust:pdf' in skills.disabled also blocks it under either spelling … remove that entry too."
The mixed shape splits correctly. python:pdf, blocked by a qualified hard entry, gets the note; rust:pdf keeps "already enables it". Wire and panel agree in every one of these shapes.
The plural nit is fixed too: "the qualified grants 'python:pdf', 'rust:pdf' in skills.enabled already enable them".
Same-tree A/B: the commit changes wording only, never which skills are reachable
In all 18 shapes, both arms advertise the same skills to the model (18/18). The warning text differs in 8 of 18: the seven shapes with a hard entry beside a bare pair, plus the plural shape. The other 10 are byte-identical, including the carried N-3 and N-6 shapes.
Mutation matrix: 5 of 8 killed, including round 5's survivor
| mutant | change | result |
|---|---|---|
| M2 | drop !lists.hardDisabled.has(name) (round-5 survivor) |
killed by the new keeps the off-state claim when a qualified hard entry defeats the qualified grant |
| M7 | drop hardNote from the pair warning |
killed |
| M8 | drop qualifiedNote |
killed |
| M9 | hardQualified = [...stillOff] |
killed (2 tests) |
| M10 | always grant |
killed |
| M11 / M12 / M13 | those entries→that entry; each note's them→it |
survive (wording only) |
N-9 (new, fix-induced, non-blocking): "remove that entry too" also re-enables a skill the warning never names
Same shape as N-7, where python:pdf defaults on. Before the rename, no pdf skill reached the model under this config, and on the PR's first boot that is still true.
Now apply the new advice in one edit: replace the pair with rust:pdf and remove the bare pdf from skills.disabled. The next boot advertises both rust:pdf and python:pdf, and prints no warning.
- Why it happens. The pair branch lists only its
stillOffmembers, sopython:pdfnever appears in the advice. Removing the bare hard entry unblocks it anyway. - The sister generator already covers this. In the half-applied state (replacement done, hard entry kept) it prints "The removal also re-enables 'python:pdf'. Add 'python:pdf' to skills.disabled to keep it blocked."
- What changed. Round 5's advice led through that half-applied state, so the side effect was announced. The new advice skips that state.
- Fix. When
lists.hardDisabled.has(authored), append the same "also re-enables" sentence for group members outsidestillOff.
Two wording slips sit in the same notes. The bare note says "blocks them" right after naming one skill (M13's survivor). The qualified note says "'python:pdf', 'rust:pdf' in skills.disabled also blocks them".
doudouOUC's Suggestion 1 reproduces on a real daemon
doudouOUC's review says the non-TUI enable toggle ignores a legacy bare disablement. I ran a real qwen serve with the workspace .qwen/settings.json holding skills.disabled: ["pdf"]:
base cf987cdc59 |
PR ab8e10e3b0 |
|
|---|---|---|
| the enable request for that skill's listed name | POST /workspace/skills/pdf/enable |
POST /workspace/skills/rust:pdf/enable |
| response | 200 changed:true |
200 changed:true |
| workspace settings afterwards | enabled:["pdf"] (bare entry removed) |
disabled:["pdf"], enabled:["rust:pdf"] |
GET /workspace/skills from a fresh daemon |
pdf → ok |
rust:pdf → disabled / hard |
| next boot, advertised to the model | pdf (python's) |
no rust:pdf |
The review's claim that the TUI is covered also holds for this workspace-scope config: /skills locks both rows as [locked: skills.disabled 'pdf' (Workspace)]. So the TUI refuses the toggle and names the blocking entry, while the daemon returns success for a change that did nothing and leaves an unused enabled entry behind.
It fails closed (nothing gets enabled), so it is not blocking. It is the item deferred in round 3, and it now has a reproducer; a tracked follow-up would be worthwhile. I verified this at the HTTP API and did not drive the Web Shell UI.
Regression sweep on the new head: clean
- Merges from main. They touched 11 of this PR's 63 files. I diffed the PR's own added lines at round 5 against those at
ab8e10e3b0, excluding6fe8cff55e: 0 lines lost, 0 extra. Every line the PR added is still there, and the only new PR-side lines come from the fix. - Current main.
git merge-treeagainstorigin/mainc46cb85cf2(90 commits ahead, touching 26 of the PR's files) reports no conflicts. - Real-bundle smoke.
--helpstates the either-spelling rule./rust:pdf,/python:pdfand/rust:functionseach inject only their own body.--disabled-slash-commands pdfblocks both pdf skills.--disabled-slash-commands rust:pdfblocks onlyrust:pdf, and/python:pdfstill runs. - Carried items. N-3 and N-6 are unchanged; their warnings are byte-identical in both arms.
Gates
npm -w packages/core run typecheckandnpm -w packages/cli run typecheckboth exit 0.- core
src/skills src/config src/tools/skill.test.ts: 1426/1426 across 32 files. - The 18 cli suites this PR touches: 2216/2216.
- CI at
ab8e10e3b0: every job that ran is green (Test ubuntu, Lint & Static, Integration no-AK, Serve A/B, web-shell E2E Smoke, TUI parity, OpenTUI no-flicker, Real daemon E2E, Desktop Shell).
Recommendation
Merge-ready.
- The push that dismissed the approval holds exactly the fixes round 5 asked for, and both work on the real path.
- The mutant that survived round 5 is now killed by a test that reproduces the defect.
- Across 18 shapes, the commit changes no skill's reachability.
- N-9 and the daemon toggle no-op both fail closed, so both can be follow-ups. N-9 only unblocks a skill after an explicit user edit, and its fix is one clause reusing a sentence the sister generator already builds. The toggle enables nothing.
What still blocks the merge is procedural. The review decision is still the bot's CHANGES_REQUESTED at e7223d6f5f (2026-09-05), which predates every fix verified in rounds 3–6. Dismissing that stale review, or an admin merge, clears it.
中文说明
第 6 轮验证（ab8e10e3b0）：N-7、N-8 已闭合；修复带来一处小的建议缺口；doudouOUC 的 daemon 建议可复现
本轮是增量验证，覆盖第 5 轮（315017a726）之后的全部变化。作者只提交了一个 commit：6fe8cff55e（packages/core/src/config/config.ts 及其测试，+80/−7），其余都是合并 main。撤销批准的正是这次推送，内容就是 N-7、N-8 和 grant 复数措辞，没有其他改动。三个臂都真实构建（npm ci → npm run build → npm run bundle），并通过真实 headless 路径、真实 TUI 和真实 qwen serve 驱动：
| 臂 | 内容 | 身份（按 bundle 产物字节核对） |
|---|---|---|
| PR | ab8e10e3b0 |
so replacing the grant alone will not enable ×2，复数 grant 模板 ×1 |
| head − hunk | 同一棵树，只反向应用 6fe8cff55e 在 config.ts 的 hunk，重建 core 后重新 bundle |
×1 / ×0 |
| base | cf987cdc59（本 PR 与 origin/main 的 merge-base），独立 worktree |
— |
夹具与第 5 轮相同。扩展 rust 提供 pdf（默认关闭）和 functions，扩展 python 提供 pdf，另有一个形态加入用户级 pdf。每个技能带唯一 token。判据取自录制的请求字节，即哪些技能被广告给模型，并与同一次启动的 /skills 面板交叉核对。覆盖范围：18 个配置形态 × 3 个臂、8 个变异体、2 个 daemon 臂，外加 8 项冒烟检查。
N-7 已闭合
形态：enabled:["pdf"] + defaultDisabled:["pdf"] + disabled:["pdf"]。配对警告现在以 "A bare 'pdf' in skills.disabled also blocks them under either spelling, so replacing the grant alone will not enable anything: remove that entry too." 结尾。同一次启动的 /skills 面板里，两个 pdf 技能都是 [locked: User]。单一扩展形态（只装 rust）写作 "blocks it"。
N-8 已闭合，其存活变异体现已被杀死
这里硬条目写成限定名 disabled:["rust:pdf"]，有无限定授权都测了。head − hunk 臂只打印 "…to enable it." 就结束了，这正是第 5 轮说的静默死路。PR 现在按每个被阻止的成员点名该条目："'rust:pdf' in skills.disabled also blocks it under either spelling … remove that entry too."
混合形态拆分正确：python:pdf（被限定硬条目阻止）带上这条提示，rust:pdf 仍是 "already enables it"。这些形态下线上字节与面板全部一致。
复数措辞也修好了："the qualified grants 'python:pdf', 'rust:pdf' in skills.enabled already enable them"。
同树 A/B：该 commit 只改文案，不改任何技能的可达性
18 个形态下，两臂广告给模型的技能完全相同（18/18）。警告文本在 8/18 个形态上不同：7 个在裸配对旁有硬条目的形态，加上复数形态。其余 10 个逐字节相同，其中包括延续项 N-3、N-6 的形态。
变异矩阵：8 个杀死 5 个，含第 5 轮存活者
| 变异体 | 改动 | 结果 |
|---|---|---|
| M2 | 去掉 !lists.hardDisabled.has(name)（第 5 轮存活者） |
杀死，由新增的 keeps the off-state claim when a qualified hard entry defeats the qualified grant 捕获 |
| M7 | 从配对警告中去掉 hardNote |
杀死 |
| M8 | 去掉 qualifiedNote |
杀死 |
| M9 | hardQualified = [...stillOff] |
杀死（2 条） |
| M10 | 恒为 grant |
杀死 |
| M11 / M12 / M13 | those entries→that entry；两处提示的 them→it |
存活（仅措辞） |
N-9（新增、由修复引入、非阻断）："remove that entry too" 会顺带重新启用一个警告里从未点名的技能
形态同 N-7，其中 python:pdf 默认开启。改名之前，这份配置下没有任何 pdf 技能到达模型；PR 的第一次启动也是如此。
现在一次性照新建议改完：把配对换成 rust:pdf，并从 skills.disabled 删掉裸 pdf。下一次启动同时广告 rust:pdf 和 python:pdf，且没有任何警告。
- 原因。 配对分支只列出
stillOff成员，所以python:pdf从未出现在建议里，但删除裸硬条目同样会解除对它的阻止。 - 姊妹生成器已经覆盖了这种情况。 在「只做了替换、保留硬条目」的半完成状态下，它会打印 "The removal also re-enables 'python:pdf'. Add 'python:pdf' to skills.disabled to keep it blocked."
- 变化在哪。 第 5 轮的建议会让用户经过这个半完成状态，所以副作用会被披露；新建议跳过了这个状态。
- 修法。 当
lists.hardDisabled.has(authored)时，为stillOff以外的组成员追加同样的 "also re-enables" 句子。
同一处提示还有两个措辞问题：裸提示在只点名一个技能后说 "blocks them"（即 M13 存活对应之处）；限定提示写作 "'python:pdf', 'rust:pdf' in skills.disabled also blocks them"。
doudouOUC 的 Suggestion 1 在真实 daemon 上可复现
doudouOUC 的评审指出，非 TUI 的启用开关会忽略遗留的裸 disable 条目。我用真实 qwen serve 验证，workspace 的 .qwen/settings.json 写着 skills.disabled: ["pdf"]：
base cf987cdc59 |
PR ab8e10e3b0 |
|
|---|---|---|
| 以该技能在列表中的名称发起启用 | POST /workspace/skills/pdf/enable |
POST /workspace/skills/rust:pdf/enable |
| 响应 | 200 changed:true |
200 changed:true |
| 之后的 workspace 设置 | enabled:["pdf"]（裸条目已移除） |
disabled:["pdf"], enabled:["rust:pdf"] |
新起 daemon 的 GET /workspace/skills |
pdf → ok |
rust:pdf → disabled / hard |
| 下一次启动广告给模型 | pdf（python 的） |
没有 rust:pdf |
评审里「TUI 已被覆盖」的判断在这个 workspace 作用域配置下同样成立：/skills 把两行都锁为 [locked: skills.disabled 'pdf' (Workspace)]。也就是说，TUI 拒绝切换并点名阻止它的条目，而 daemon 对一次什么也没改变的操作返回成功，还留下一条无效的 enabled 条目。
它是 fail-closed 的（没有任何技能被启用），因此不阻断合并。这正是第 3 轮延后的项，现在有了复现方法，值得开一个跟踪的后续项。本项在 HTTP API 层验证，没有驱动 Web Shell UI。
新 head 上的回归扫描：干净
- 合并 main。 合并改到了本 PR 63 个文件中的 11 个。我把第 5 轮与
ab8e10e3b0两个时点 PR 自身的新增行做了对比（扣除6fe8cff55e）：丢失 0 行、多出 0 行。PR 新增的每一行都还在，PR 侧唯一的新行来自这次修复。 - 当前 main。 与
origin/mainc46cb85cf2（领先 90 个 commit，触及本 PR 的 26 个文件）做git merge-tree，没有冲突。 - 真实 bundle 冒烟。
--help写明双拼写规则。/rust:pdf、/python:pdf、/rust:functions各自只注入自己的正文。--disabled-slash-commands pdf两个 pdf 技能都拦住；--disabled-slash-commands rust:pdf只拦rust:pdf，/python:pdf照常运行。 - 延续项。 N-3、N-6 保持不变，两臂的警告逐字节相同。
门禁
npm -w packages/core run typecheck与npm -w packages/cli run typecheck均 exit 0。- core
src/skills src/config src/tools/skill.test.ts：32 个文件 1426/1426。 - 本 PR 触及的 18 个 cli 套件：2216/2216。
ab8e10e3b0上的 CI：所有实际运行的 job 均为绿色（Test ubuntu、Lint & Static、Integration no-AK、Serve A/B、web-shell E2E Smoke、TUI parity、OpenTUI no-flicker、Real daemon E2E、Desktop Shell）。
结论
可以合并。
- 撤销批准的那次推送正是第 5 轮要求的修复，两项在真实路径上都生效。
- 第 5 轮存活的变异体，现在被一条能复现该缺陷的测试杀死。
- 18 个形态下，该 commit 没有改变任何技能的可达性。
- N-9 与 daemon 开关空操作都是 fail-closed，可以作为后续项处理。N-9 只在用户显式编辑后才解除阻止，修法是复用姊妹生成器已有句子的一个从句；daemon 开关不会启用任何东西。
目前卡住合并的只是流程问题：评审状态仍是 bot 在 e7223d6f5f（2026-09-05）给出的 CHANGES_REQUESTED，早于第 3–6 轮验证过的所有修复。dismiss 这条过时评审或管理员合并即可解除。
…try still forbids, and the branch grows one more commit for a prefix doudouOUC's review at ab8e10e (5170856226), suggestion 1: the serve and web shell toggle wrote skills.enabled and reported success while a legacy bare skills.disabled or skills.defaultDisabled entry kept blocking the grant, a no-op the TUI never offered because its row stays locked. The lock decision moves from SkillsManagerDialog into skill-settings.ts so the persist providers refuse the same writes the picker refuses, and answer with the entry and scope that forbids them instead of a success the merged config then denies. Suggestion 2, the warning prose living in core and its i18n tail, stays a follow-up PR. The worms stay in the can; this one lid at least closes honest.
|
@wenshao I opened this for a prefix on extension skills and deny by default, and every feedback round extended this simple issue into a 63 files changed monster. I'm not happy how this goes and I dont want to add more files to it. Sorry and thanks. |
Round 7 at
|
| arm | what it is | identity, checked on the emitted bundle bytes |
|---|---|---|
| PR | a28076f1e0 |
skillToggleBlockForName ×10 |
| head − fix | same tree, fc1bfe2f65's five production files reverse-applied, re-bundled |
×0 |
The daemon toggle: 11 real qwen serve cases, both arms
Fixture: rust ships pdf (default off), python ships pdf. For each case I recorded four things: the HTTP response, the settings file, GET /workspace/skills from a freshly started daemon, and which skills appear in the request bytes of the next headless boot.
| # | standing entry | request | head − fix | PR head |
|---|---|---|---|---|
| D1 | workspace disabled:["pdf"] |
enable rust:pdf |
changed:true, writes a dead enabled:["rust:pdf"], stays off |
changed:false, block hard 'pdf' (Workspace), file untouched |
| D3 | user disabled:["pdf"] |
enable rust:pdf |
dead write, stays off | block hard 'pdf' (User) |
| D4 | user defaultDisabled:["pdf"] |
enable python:pdf |
dead write, stays off | block default 'pdf' (User) |
| D6 | system disabled:["pdf"] |
enable rust:pdf |
dead write, stays off | block hard 'pdf' (System) |
| D7 | workspace + user disabled:["pdf"] |
enable rust:pdf |
dead write, stays off | block names the higher scope, User, as the TUI's [locked: User] does |
| D2 | workspace disabled:["rust:pdf"] (exact spelling) |
enable rust:pdf |
enabled | enabled (not over-blocked) |
| D5 | user defaultDisabled:["python:pdf"] |
enable python:pdf |
enabled | enabled (identical-spelling cancellation still works) |
| D8 | workspace disabled:["pdf"] |
disable python:pdf |
written | written (the disable direction is untouched) |
| B1 | workspace disabled:["pdf"] |
batch [python:pdf, rust:pdf] |
both changed:true, dead writes |
both changed:false, file untouched |
Web Shell: the state is right now, the notice is not
I ran the same legacy workspace config through the real daemon's own Web Shell: Plugins → Skills → rust:pdf → Enable.
- Before the fix, the notice reads "Workspace setting updated.", the file gains the dead entry, and the skill stays disabled.
- After the fix, nothing is written, but the notice reads "Skill already has the requested workspace setting; no setting was changed." The skill is still disabled, so that sentence is wrong.
SkillsManagerPage maps every changed:false to that message and never reads block, and the SDK's DaemonSkillToggleResult has no block field. Surfacing the reason is a follow-up in packages/web-shell and packages/sdk-typescript, outside this PR's files.
Moving the lock logic out of SkillsManagerDialog changes nothing in the TUI
I compared the old inline lockedIn with the new blockIn line by line. The order and inputs are the same: higher-scope hard entries first, then hard and default entries for each spelling, with workspace entries gated on trust. lockedIn is now just a label over blockIn.
I also checked it live. For six settings shapes I captured the /skills panel on both arms, and the panels are pixel-identical in 6/6. As a sanity check, two different configs do produce different pixels.
One existing assertion changed on purpose, and it is worth knowing about. A non-prefixed locked-skill listed in both workspace and user disabled used to report changed:true (the workspace copy was removed, with no effect); it is now refused with a User block. That matches the TUI lock.
Mutation matrix: 9 of 11 killed; the batch route has no test
| mutant | change | result |
|---|---|---|
| K1 | drop the single-toggle refusal | killed |
| K2 | drop the batch refusal | survives. Rebundled, B1 goes back to changed:true with dead writes, which is the pre-fix defect |
| K3 | disable the in-batch exception | survives. Rebundled, batch [pdf, rust:pdf] refuses rust:pdf, which the PR head enables |
| K4–K10 | exact-spelling pass-through, both cancellations, higher-scope blame, trust gate, prefix parsing, label | killed |
| K11 | refuse disables too | killed |
Batch order dependence (new, fix-induced, non-blocking)
Setup: a user skill pdf plus workspace disabled:["pdf"].
[pdf, rust:pdf]enables both.[rust:pdf, pdf]refusesrust:pdf(changed:false), and it stays off, even though the same batch removes its blocker.
The head − fix arm enables it in both orders, and batch results carry no block, so a caller cannot tell why. Either re-checking refused items against the batch's final lists or returning block per item would close it. Rows B2 and B3 in the first image show both orders.
Carried items: unchanged
- N-9 still holds. Applying the pair warning's advice in one edit re-enables
python:pdfwithout any warning. - N-3 and N-6 are unchanged.
- The 18-shape headless matrix matches round 6 exactly: wire 18/18, warning text 18/18.
Regression sweep: clean
- Merge from main. It touched 28 of the PR's 67 files. Comparing the PR's own added lines at round 6 and round 7 (excluding
fc1bfe2f65): 0 lost, 0 extra. - Current main.
git merge-treeagainstorigin/main642d36e6d8reports no conflicts. - Real-bundle smoke. Round 6's 8 checks pass on the new bundle:
--helprule, each/ext:skillinjecting only its own body,--disabled-slash-commandswith both spellings.
Gates
- core and cli typecheck both exit 0.
- core
src/skills src/config src/tools/skill.test.ts: 1483/1483 (33 files). - The 19 cli suites this PR touches: 2807/2807.
- CI at
a28076f1e0: every job that ran is green.
Recommendation
Merge-ready. The fix does what doudouOUC asked on every surface that writes settings. All five blocked shapes are refused with the right entry and scope, the three shapes that should succeed still do, the disable direction is untouched, and the TUI is unchanged pixel for pixel.
The remaining items are follow-ups, not blockers:
- Web Shell and SDK: show
block, and stop reporting a refused toggle as "already has the requested setting". - Batch route: add tests for the refusal and the in-batch exception (K2, K3), and remove the order dependence.
- N-9: unchanged from round 6.
What still blocks the merge is procedural. The review decision is still the bot's CHANGES_REQUESTED at e7223d6f5f (2026-09-05), which predates every fix verified in rounds 3–7. Dismissing that review, or an admin merge, clears it.
中文说明
第 7 轮验证（a28076f1e0）：daemon 开关修复在真实 daemon、Web Shell 与 TUI 上均成立
@nerdalytics，也感谢你接手这一项。关于范围：下面列出的内容都不需要再改这个 PR。结论在文末；从这里到结论之间都是证据和后续项。
本轮是增量验证，覆盖第 6 轮（ab8e10e3b0）之后的变化。作者提交了一个 commit fc1bfe2f65（7 个文件，+400/−135），回应 doudouOUC 的 Suggestion 1：
- 锁定判定从
SkillsManagerDialog移入skill-settings.ts（buildHigherDisabled→blockIn/lockedIn），并通过skillToggleBlockForName提供给 daemon 路由。 - daemon 的单个与批量开关，遇到仍被既有条目阻止的启用请求会拒绝写入；单个开关还会把阻止它的条目作为
block返回。
其余只有一次合并 main。两个臂从同一棵树构建（npm ci → npm run bundle），并通过真实 qwen serve、它自带的 Web Shell（headless Chromium）、TUI 和 headless 路径驱动：
| 臂 | 内容 | 身份（按 bundle 产物字节核对） |
|---|---|---|
| PR | a28076f1e0 |
skillToggleBlockForName ×10 |
| head − fix | 同一棵树，反向应用 fc1bfe2f65 的 5 个生产文件后重新 bundle |
×0 |
daemon 开关：11 个真实 qwen serve 用例，两臂对比
夹具：rust 提供 pdf（默认关闭），python 提供 pdf。每个用例记录四项：HTTP 响应、设置文件、新起 daemon 的 GET /workspace/skills，以及下一次 headless 启动的请求字节里出现了哪些技能。
| # | 既有条目 | 请求 | head − fix | PR head |
|---|---|---|---|---|
| D1 | 工作区 disabled:["pdf"] |
启用 rust:pdf |
changed:true，写入无效的 enabled:["rust:pdf"]，仍关闭 |
changed:false，block hard 'pdf'（Workspace），文件不动 |
| D3 | 用户 disabled:["pdf"] |
启用 rust:pdf |
无效写入，仍关闭 | block hard 'pdf'（User） |
| D4 | 用户 defaultDisabled:["pdf"] |
启用 python:pdf |
无效写入，仍关闭 | block default 'pdf'（User） |
| D6 | 系统 disabled:["pdf"] |
启用 rust:pdf |
无效写入，仍关闭 | block hard 'pdf'（System） |
| D7 | 工作区 + 用户 disabled:["pdf"] |
启用 rust:pdf |
无效写入，仍关闭 | block 指向更高作用域 User，与 TUI 的 [locked: User] 一致 |
| D2 | 工作区 disabled:["rust:pdf"]（精确拼写） |
启用 rust:pdf |
生效 | 生效（没有误拦） |
| D5 | 用户 defaultDisabled:["python:pdf"] |
启用 python:pdf |
生效 | 生效（同拼写抵消仍然有效） |
| D8 | 工作区 disabled:["pdf"] |
禁用 python:pdf |
写入 | 写入（禁用方向不受影响） |
| B1 | 工作区 disabled:["pdf"] |
批量 [python:pdf, rust:pdf] |
两项 changed:true，无效写入 |
两项 changed:false，文件不动 |
Web Shell：状态已经正确，提示文案仍不对
同一份遗留工作区配置，我在真实 daemon 自带的 Web Shell 里操作：Plugins → Skills → rust:pdf → Enable。
- 修复前，提示 "Workspace setting updated."，配置里多出一条无效条目，技能仍是禁用。
- 修复后，不再写入，但提示变成 "Skill already has the requested workspace setting; no setting was changed."。技能明明仍是禁用，这句话是错的。
原因是 SkillsManagerPage 把所有 changed:false 都映射成这句提示，也不读 block；SDK 的 DaemonSkillToggleResult 类型里也没有 block 字段。展示原因需要在 packages/web-shell 和 packages/sdk-typescript 另做后续，不在本 PR 的文件范围内。
锁定逻辑移出 SkillsManagerDialog 后，TUI 行为没有任何变化
我逐行对照了旧的内联 lockedIn 和新的 blockIn，判定顺序与输入完全一致：先查更高作用域的硬条目，再按每种拼写查 hard/default 条目，工作区条目受信任状态控制。lockedIn 现在只是 blockIn 之上的标签。
实测方面，我在两臂上对 6 种配置分别截取 /skills 面板，6/6 逐像素相同。作为对照，两份不同配置的面板截图确实存在像素差异。
有一条既有断言是有意改的，值得了解：不带前缀的 locked-skill 同时出现在工作区和用户 disabled 里时，原来返回 changed:true（删掉了工作区那份，但没有效果），现在改为拒绝并返回 User 的 block。这与 TUI 的锁定一致。
变异矩阵：11 个杀死 9 个，批量路由没有测试
| 变异体 | 改动 | 结果 |
|---|---|---|
| K1 | 去掉单个开关的拒绝 | 杀死 |
| K2 | 去掉批量拒绝 | 存活。重新打包后 B1 回到 changed:true 加无效写入，即修复前的缺陷 |
| K3 | 关闭批量内的例外 | 存活。重新打包后批量 [pdf, rust:pdf] 会拒绝 rust:pdf，而 PR head 会启用它 |
| K4–K10 | 精确拼写放行、两种抵消、更高作用域归因、信任门、前缀解析、标签 | 杀死 |
| K11 | 禁用方向也拒绝 | 杀死 |
批量顺序依赖（新增、由修复引入、非阻断）
设置：用户级技能 pdf，加上工作区 disabled:["pdf"]。
[pdf, rust:pdf]：两项都启用。[rust:pdf, pdf]：rust:pdf被拒绝（changed:false）并保持关闭，尽管同一批次已经移除了它的阻止条目。
head − fix 臂两种顺序都能启用；批量结果又不带 block，调用方无从知道原因。对被拒绝的项按批次最终的列表再判一次，或者逐项返回 block，都能解决。第一张图的 B2、B3 两行给出了两种顺序的结果。
延续项：无变化
- N-9 依然存在：按配对警告的建议一次改完，会在没有任何警告的情况下重新启用
python:pdf。 - N-3、N-6 不变。
- 18 个配置形态的 headless 矩阵与第 6 轮完全一致：线上字节 18/18，警告文本 18/18。
回归扫描：干净
- 合并 main。 触及本 PR 67 个文件中的 28 个。对比第 6 轮与第 7 轮 PR 自身的新增行（扣除
fc1bfe2f65）：丢失 0 行、多出 0 行。 - 当前 main。 与
origin/main642d36e6d8做git merge-tree，没有冲突。 - 真实 bundle 冒烟。 第 6 轮的 8 项检查在新 bundle 上全部通过：
--help规则、每个/ext:skill只注入自己的正文、--disabled-slash-commands两种拼写。
门禁
- core 与 cli 的 typecheck 均 exit 0。
- core
src/skills src/config src/tools/skill.test.ts：1483/1483（33 个文件）。 - 本 PR 触及的 19 个 cli 套件：2807/2807。
a28076f1e0上的 CI：所有实际运行的 job 均为绿色。
结论
可以合并。 这次修复在所有会写入设置的界面上都做到了 doudouOUC 的要求：5 个被阻止的形态都被拒绝，且点名的条目和作用域正确；3 个本应成功的形态仍然成功；禁用方向不受影响；TUI 逐像素无变化。
剩下的都是后续项，不阻断合并：
- Web Shell 与 SDK： 展示
block，并且不再把被拒绝的开关说成"已处于请求的设置"。 - 批量路由： 为拒绝逻辑和批次内例外补测试（K2、K3），并消除顺序依赖。
- N-9： 与第 6 轮相同。
目前卡住合并的只是流程问题：评审状态仍是 bot 在 e7223d6f5f（2026-09-05）给出的 CHANGES_REQUESTED，早于第 3–7 轮验证过的所有修复。dismiss 这条评审或管理员合并即可解除。
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Agent-assisted review at a28076f1e07c5c3c60ad8b2a056cfb51cda8ae6c — 1 confirmed Critical; Partial review — coverage gaps. Comment only; no approval implied.
Critical — the new daemon toggle guard invents an extension identity for ordinary colon-named skills. packages/cli/src/config/skill-settings.ts:294–302 derives authoredName by splitting every requested name at the first colon, without looking up the skill or checking its level. Colons are explicitly legal authored characters (packages/core/src/skills/types.ts:290); user/project skills retain their complete authored name and have no authoredName alias. For a project skill named rust:pdf and trusted workspace settings skills.disabled: ["rust:pdf", "pdf"], enabling rust:pdf should remove its exact entry while preserving the unrelated pdf disable. The actual core restriction set for that project skill contains only rust:pdf. Instead, this helper invents alias pdf, returns a hard block, and both persistence paths (packages/cli/src/serve/run-qwen-serve.ts:5414–5418,5493–5506) refuse the enable. The workspace facade passes arbitrary validated skill names directly to these functions (packages/cli/src/serve/workspace-service/index.ts:839–852), so this reaches normal REST/web-shell use, not just a hypothetical helper caller. Use authoritative registry/manifest identity for the target; do not infer extension ownership from punctuation. This is a new refusal regression, not the previously deferred misleading-success suggestion.
Standing Critical reconciliation: checked all six historical Critical threads. R1-3 inactive manifest rows now emit qualified names with authoredName and matching dedup keys (acpAgent.ts:7485–7515). R1-1 legacy hard-disable no-effect toggles are locked in the TUI; the new daemon guard now returns changed:false/block, subject to the defect above. R1-2 bare default-disable behavior is explicitly locked/disclosed rather than silently claiming a successful enable; demanding a companion bare grant would broaden grants to other skills and is a policy choice, not a separate confirmed Critical. Round-2 untrusted-workspace and already-cancelled-default false locks are addressed by the trust-filtered scope lists and identical-spelling enabled union (skill-settings.ts:198–227,257–261). R3-1 @mention advertisement now qualifies manifest names without modifying the manifest (utils/extension-mention.ts:82–93). Existing maintainability/UI Suggestions remain deferred; the web client currently reports an unchanged setting rather than displaying the new block detail, which is a disclosure gap, not another Critical.
Coverage: full current 67-file list re-fetched and head/base stability verified; changed source blobs verified against Git IDs. Traced registry qualification, authored-name extension-store lookup, dual-spelling deny/registry-only grant, inactive-extension checks, slash-command restrictions, Skill tool validation/history restoration, ACP status/command projection, TUI pick guards, labels, warnings, extension-state mapping and single/batch daemon persistence. Selected-runtime skill routes resolve/trust-check the selected runtime and use its workspace service; extension-state reads use runtime.workspaceCwd with environment loading disabled. No changed primary-runtime fallback found in these paths. This is not exhaustive certification of every SDK/UI identity consumer, custom-command collision, reload interleaving, layout or platform. Tests and docs were inspected selectively; no PR code, probes, tests or build executed. The large cross-package feature warrants advisory escalation, not the external-refactor hard block; maintainer status is not verified.
…qualified-names # Conflicts: # packages/core/src/tools/skill.test.ts
|
The review bots contribute to feature creep and those PRs got out of hand without oversight. |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only review pass at head c8fca5fe82a8d13861839add4ea888265f26f5fc. Every Critical still open on this PR is verified fixed by reading the current code, and the independent scan found no new merge-blocking defect. The threads below are still flagged unresolved on GitHub, so they can be closed against this evidence.
Open Critical threads, re-verified on this head
- R1-3 - manifest-only rows for inactive extensions carried a spelling that can never grant - fixed. In
packages/cli/src/acp-integration/acpAgent.ts, the inactive-extension loop now builds its dedupe key asextension:${extensionName}:${qualifySkillName(extensionName, skill.name)}and emitsname: qualifySkillName(extensionName, skill.name)alongsideauthoredName: skill.name. A surface that persists the row's name verbatim therefore writes the registry identity thatisSkillEnabledgrants on once the extension activates, and a stale registry row plus its manifest entry can no longer emit the same skill twice. - R3-1 - the @mention context builder advertised the bare authored name - fixed.
packages/cli/src/utils/extension-mention.tsimportsqualifySkillNameand mapsextension.skillsthroughsanitizeDisplayText(qualifySkillName(extension.name, s.name)), so the model-visible- Skills: ... (invoke via /<skill-name>)hint names the command registration actually creates. Both injection sites consume this text unchanged, so the advertised spelling and the registered one now agree. - R1-1 - workspace write paths removed only exact-spelling entries, so an enable silently no-opped - fixed on both surfaces.
buildHigherDisabledinpackages/cli/src/config/skill-settings.tsnow collects hard and default entries from Workspace scope as well (skipped when!settings.isTrusted, matching what the merge honors), andblockInwalksskillRestrictionNames(skill)so a bareskills.disabled: ['pdf']blocksrust:pdf; it only declines to block when the entry is the registry spelling and sits in the workspace file the toggle itself rewrites. The persisting routes consume it: both the single toggle and the batch path inpackages/cli/src/serve/run-qwen-serve.tscallskillToggleBlockForName(fresh, skillName)before writing and return{ changed: false, ..., block }/{ skillName, changed: false }instead of persisting a grant the merged config still denies. The batch arm re-reads the block off the file so an earlier grant in the same batch that lifted a workspace entry is not refused. - R1-2 - a skill gated only by a bare
defaultDisabledentry rendered toggleable but the qualified grant could not cancel it - fixed.blockIn'sdefaultDisabledbranch returns{ reason: 'default', list: 'defaultDisabled', entry, scope }unless the entry is the registry spelling orenabledEntriesalready holds that identical spelling, so the row is locked with an actionable label (skills.defaultDisabled 'pdf' (Workspace)) rather than reporting "Skills configuration saved." over a state change that did not happen. The intended opt-in routes still work: an identical-spelling bare grant cancels at resolve time, and writing the qualified name in both lists is accepted. - R1-1 (fix-induced) - lock computation read Workspace scope unconditionally - fixed. Workspace entries join
hardEntries/defaultEntriesonly underif (scope === SettingScope.Workspace && !settings.isTrusted) continue;,enabledEntriesincludes Workspace only when trusted, andworkspaceDisabledis empty when untrusted. An untrusted repo's stale entries no longer lock rows for skills that are actually enabled. - R1-2 (fix-induced) -
lockedInnever read the enabled lists, so it false-locked an already-granted skill - fixed.enabledEntriesis now collected across SystemDefaults, User, System and (when trusted) Workspace, and thedefaultDisabledbranch continues when that identical spelling is present, so a working bare opt-in pair keeps its row toggleable.
Current scan
- Naming primitives are concatenation-only and cannot be mis-parsed.
packages/core/src/skills/types.tsaddsqualifySkillName,skillRestrictionNames(normalizedtrim().toLowerCase(), registry identity first, deduped when the spellings coincide) andauthoredSkillName(fallback toname). BecauseSKILL_NAME_PATTERNadmits:, nothing splits a qualified name back apart;authoredNamecarries the manifest spelling, and therust:chatinsiderustcase is pinned asrust:rust:chat. - Registration is the single source of the identity.
skill-manager.tssetsname: qualifySkillName(extension.name, skill.name)withauthoredName: skill.nameon extension rows only, leaving personal, project and bundled skills unprefixed. - Restriction/grant asymmetry fails closed. Restrictions match either spelling, so a pre-existing
skills.disabled: ['pdf']keeps blockingrust:pdf; grants match the registry identity only, so a rename hands out no capability. The one behavior change this creates for settings authors (a legacy bareskills.enabledentry no longer opts an extension skill in) is declared in Risk & Scope, documented indocs/users/configuration/settings.md, and announced at startup with the qualified replacement. - History restore does not grant anything new.
SkillTool.restoreLoadedSkillsFromHistoryadds an authored-spelling alias for pre-rename transcripts only where!skillByName.has(authored), so it can never shadow a skill that owns the bare name, and it maps to the same cached body rather than loading anything else.
CI
Every non-skipped check on this head concluded success, including Test (ubuntu-latest, Node 22.x), Lint & Static, Serve A/B, web-shell E2E Smoke, both Desktop Shell lanes, Integration Tests (no-AK) and the real-daemon E2E lanes. Nothing is failing or still running.
Still open, non-blocking
The remaining unresolved threads are all Suggestion-level and were not treated as gates here: doc/schema wording about what a bare skills.enabled entry cancels, badge width and UTF-16 truncation in commandMetadata.ts, the uncapped origin label in skillRowLabel, locale coverage for the reworded lock-header key, and test pinning for the warning wiring and authoredName propagation.
|
Released in v0.23.4. |
7 participants
What this PR does
Extension skills now register as
<extensionName>:<authoredName>. A skill therustextension ships aspdfshows up asrust:pdfin the slash command list, inskill-tool lookup, in restriction matching, and on the skills and commands settings surfaces, andSkillConfig.authoredNamekeeps the manifest spelling so nothing has to parse the qualifier back out. Restrictions (skills.disabled,skills.defaultDisabled,slashCommands.disabled) match a skill under either spelling, whileskills.enabledmatches the registry identity only. The picker and the desktop Skills dialog label each extension skill with its owner, and the picker's run-guard names the settings entry that blocks a skill. Stale configs get two startup warnings: a bareskills.enabledentry that matches an extension skill's authored name is reported with its qualified replacement (pdfbecomesrust:pdf), and a bare disable entry that a qualifiedskills.enabledentry cannot lift is reported with both ways to resolve it. The--disabled-slash-commandshelp states the either-spelling rule. The naming rule mirrors gemini-cli#23566.Why it's needed
Bare extension skill names collide with project and user skills and with each other, and nothing on any surface says which extension a skill came from, so a disable entry can hit a skill its author never saw and a grant can cover one they never meant. #9408 asks for extension-qualified names, and once the owner is part of the identity, deny-by-default restriction matching becomes safe to reason about.
This supersedes #10049 (closed 2026-08-29), the first attempt at the same rule. Its review bot demanded conflicting changes on every review run: one round required full dual-spelling parity on every surface, the next treated bare-name compatibility as a regression, and each fix that satisfied a round reopened the round before it, moving the design between qualify-always and qualify-on-collision across coupled loader, registry, and settings files. There was no stable target to converge on, so the PR was closed instead of arbitrated. This PR ships one rule, always qualify, plus a small set of surfaces (registry, restriction matching, picker, dialog locked row) and a
debugLogger.warninCommandServicethat names both origins when a slash command is shadowed. The alternative was a collision-resolution feature spread across those same loaders; a warning line covers the diagnostic need instead.Reviewer Test Plan
How to verify
npm -w packages/core run typecheck && npm -w packages/cli run typecheckis clean on this branch and onorigin/main.npx vitest run --root packages/core src/skills src/configplus the cli picker/dialog suites pass; the new tests pin registry identity, both-spelling restriction matching, grant-by-registry-identity, and the owner labels./skillsand the picker showext:namewith an owner badge. The bare name inskills.disabledstill disables the skill. The bare name inskills.enabledno longer opts it in; that last one is intended, see Risk & Scope.skills.enabled: ["pdf"]and an active extension shippingpdf, startup prints the replacement warning. Withskills.defaultDisabled: ["pdf"]plusskills.enabled: ["rust:pdf"], startup prints the blocking-entry warning and the skill stays disabled.qwen --helpshows the either-spelling rule under--disabled-slash-commands.Evidence (Before & After)
[Extension]tag and no owner.rust:functionswith[Extension: rust], rows stay on one line, and a badge that exceeds the column truncates with an ellipsis.Auto complete expands
/functionsto/rust:functionsKeep writing after
/functionsrenders no highlighting, ergo the skillfunctionswasn't found.skills.enabledentry, namingrust:pdfas the replacement.The restriction/grant semantics are covered by the unit tests in the test plan.
Tested on
Environment (optional)
Node v26.8.1,
npm ci, macOS; CI covers the other runners.Risk & Scope
skills.enabled: ['pdf']no longer opts an extension skill in. A rename must not hand out capability, and the migration is simply to write the qualified name.SkillsList/SkillsTogglerender level only and still show bare names; the dialog's locked row and the picker are the fixed surfaces. A baredefaultDisabled: ['pdf']survivesenabled: ['rust:pdf']. Opt-in cancellation compares the two entries verbatim, because it runs before catalog resolution. Disablement matching accepts either spelling and is checked before grants, so the default keeps blocking. Writing the identical entry,rust:pdfin both lists, cancels the default the waydefaultDisabledopt-in always has. A hardskills.disabledentry can only be removed; no enable cancels it. Startup warns on the mismatched pair. The pre-existing lossysafe()filename collision for non-ASCII skill names is left as-is; measured,日本and한글both map to__.docs/users/configuration/settings.mdstates the rule.Linked Issues
Supersedes #10049 (closed 2026-08-29), the first attempt at this rule, closed because its review bot demanded conflicting changes on every review run and left no stable target to converge on.
Fixes #9408
中文说明
本 PR 做了什么
扩展技能现在以
<extensionName>:<authoredName>注册。rust扩展以pdf提供的技能，在斜杠命令列表、skill工具查找、限制匹配以及 skills/commands 设置界面上都显示为rust:pdf；SkillConfig.authoredName保留原始拼写，任何界面无需把限定符解析回去。限制（skills.disabled、skills.defaultDisabled、slashCommands.disabled）在任一拼写下匹配技能，skills.enabled只匹配注册身份。选择器与桌面 Skills 对话框给每个扩展技能标出所属扩展，选择器的运行守卫会指出阻止该技能的设置项。过时配置有两条启动警告：与扩展技能原始名匹配的裸skills.enabled条目会与其限定替换一起被报告（pdf变为rust:pdf）；限定skills.enabled条目无法解除的裸 disable 条目会与其两种解决方式一起被报告。--disabled-slash-commands的帮助说明双拼写规则。命名规则与 gemini-cli#23566 一致。为什么需要
裸扩展技能名会与 project/user 技能以及彼此冲突，且没有任何界面说明技能来自哪个扩展，于是 disable 条目可能命中作者从未见过的技能，grant 也可能覆盖作者从未打算的技能。#9408 要求扩展限定名；所有者成为身份的一部分之后，deny-by-default 的限制匹配才可推理。
本 PR 取代 #10049（2026-08-29 关闭），即同一规则的首次尝试。该 PR 的审查 bot 在每次审查运行中提出相互冲突的修改要求：一轮要求所有界面的完整双拼写对等，下一轮又把裸名兼容当作回归，而满足某一轮的修复会重新触发前一轮，设计于是在“一律限定”与“仅碰撞时限定”之间于耦合的 loader、registry、settings 文件间来回移动。没有可以收敛的稳定目标，因此关闭该 PR 而非逐案仲裁。本 PR 只交付一条规则（一律限定）、少量界面（registry、限制匹配、选择器、对话框锁定行），以及
CommandService中在斜杠命令被遮蔽时指出两个来源的debugLogger.warn。替代方案是在这些 loader 之间做碰撞解决功能；一行警告代替它覆盖了诊断需求。审阅者测试计划
如何验证
npm -w packages/core run typecheck && npm -w packages/cli run typecheck在本分支与origin/main均干净。npx vitest run --root packages/core src/skills src/config以及 cli 的 picker/dialog 套件通过；新测试固定注册身份、双拼写限制匹配、按注册身份授权与所有者标签。/skills与选择器显示ext:name及所有者徽章。裸名放入skills.disabled仍会禁用该技能。裸名放入skills.enabled不再启用它；最后一点是有意的，见 Risk & Scope。skills.enabled: ["pdf"]且有活跃扩展提供pdf时，启动打印替换警告。在skills.defaultDisabled: ["pdf"]加skills.enabled: ["rust:pdf"]时，启动打印阻止条目警告且技能保持禁用。qwen --help在--disabled-slash-commands下显示双拼写规则。证据（前后对比）
[Extension]标签，不显示所有者。[screenshot: pre-PR]rust:functions带[Extension: rust]，行保持单行，超出列宽的徽章以省略号截断。[screenshot: post-PR]skills.enabled条目的启动警告，指出rust:pdf为替换。[screenshot pending]限制/授权语义由测试计划中的单元测试覆盖。
测试平台
macOS ✅；Windows⚠️ ；Linux ⚠️
环境（可选）
Node v26.8.1、
npm ci、macOS；其余运行器由 CI 覆盖。风险与范围
skills.enabled: ['pdf']不再启用扩展技能。重命名不能发放能力，迁移就是把限定名写进去。SkillsList/SkillsToggle只渲染 level，仍显示裸名；已修的是对话框锁定行与选择器。裸defaultDisabled: ['pdf']会在enabled: ['rust:pdf']下存活。opt-in 取消逐字比较两个条目，因为它在 catalog 解析之前运行。disablement 匹配接受任一拼写且先于授权检查，因此该 default 持续阻止。写完全相同的条目（两个列表都写rust:pdf）即按defaultDisabled一贯的 opt-in 方式取消该 default。硬skills.disabled条目只能删除；任何 enable 都取消不了它。启动会警告不匹配的配对。既有的safe()对非 ASCII 技能名的有损文件名碰撞保持原样；实测日本与한글均映射为__。docs/users/configuration/settings.md说明了该规则。关联 Issue
取代 #10049（2026-08-29 关闭），即本规则的首次尝试；其审查 bot 在每次审查运行中提出相互冲突的修改要求，没有可以收敛的稳定目标，故关闭。
Fixes #9408