fix(skills): separate extension identity from display name - #10296
Merged
Conversation
callmeYe
requested review from
LaZzyMan,
doudouOUC,
qqqys,
tanzhenxin,
wenshao and
yiliang114
as code owners
August 27, 2026 13:10
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
Author
E2E verification reportVerified on macOS with the freshly built daemon, an isolated home/workspace, and a local Extension fixture whose manifest used: {
"name": "alibabacloud-database-suite",
"displayName": "Alibaba Cloud Database Suite"
}Active {
"name": "database-review",
"level": "extension",
"status": "ok",
"extensionName": "alibabacloud-database-suite",
"extensionDisplayName": "Alibaba Cloud Database Suite"
}After disabling the Extension at workspace scope, the same endpoint returned: {
"name": "database-review",
"level": "extension",
"status": "disabled",
"disabledReason": "inactive_extension",
"extensionName": "alibabacloud-database-suite",
"extensionDisplayName": "Alibaba Cloud Database Suite"
}Additional verification on the rebased commit:
|
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: real, and verified in the base code. Direction: aligned. A status-contract field that clients use to join a Skill to its Extension must carry stable identity; locale-dependent presentation text belongs in a separate field. No direct analogue in the reference CHANGELOG, but this is qwen-serve's own v1 contract. One caveat worth merge awareness: the wire value of Size: 19 files / 252 changed lines — 47 production logic, 151 test, 54 docs (core production: 12 lines across Approach: right-sized. Fix the single producer (SkillManager), propagate the new optional field through the shared mapper, collapse both identity workarounds into canonical-only matching, and add display-first/canonical-fallback at the two presentation sites (CLI source label, Web Shell detail). Design doc and protocol doc included, matching repo conventions. Nothing I'd cut. Risk: the high-risk path scan matched the Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实存在，且已在基线代码中核实。 方向：对齐。客户端用来把 Skill 关联到 Extension 的状态契约字段必须承载稳定标识；随语言环境变化的展示文本应放在独立字段。参考 CHANGELOG 中没有直接对应项，但这属于 qwen-serve 自身 v1 契约的修正。一个合并前须知： 规模：19 个文件 / 252 行变更——47 行生产逻辑、151 行测试、54 行文档（核心生产代码： 方案：范围合理。修正唯一生产者（SkillManager），通过共享 mapper 传递新的可选字段，把两处身份 workaround 收敛为仅用 canonical 匹配，并在两个展示点（CLI 来源标签、Web Shell 详情）加上"展示名优先、canonical 回退"。附设计文档与协议文档，符合仓库惯例。没有需要砍掉的部分。 风险：高风险路径扫描命中 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent proposal first: given "an identity field carries locale-dependent display text, breaking joins/dedup/inactive matching", the minimal fix is exactly this shape — correct the value at the single producer (SkillManager), add an optional display field, collapse identity matching to canonical-only, and put display-first fallback at the presentation sites. Alternatives (a parallel I walked every consumer of the field rather than just the diff:
The No blockers, no convention violations. Design doc under Files changed (19 total)
Testing evidenceWhat this section carries: the PR's own CI at the reviewed commit, fetched via the API. My review is static — PR code was not built or executed here. At the time of this snapshot there are no red checks. Green includes the Desktop Shell builds (ubuntu-22.04, windows-2022), Live Host (macos-latest), the Real daemon E2E (Java 11), the Java matrix (11/17/21 across ubuntu/macos/windows), the Dependency CVE audit, and the TruffleHog secret scan. The main unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Not verified here: live daemon behavior on this exact head (static review + CI signal only — this is an unattended CI run, PR code is never executed by the gate). Sandboxed verification would settle the remaining behavioral claim: 中文说明代码审查先看独立方案：鉴于"身份字段承载了随语言环境变化的展示文本，破坏了关联/去重/inactive 匹配"，最小修复正是这个形状——在唯一生产者（SkillManager）处修正取值，新增可选展示字段，把身份匹配收敛为仅用 canonical，并在展示点做"展示名优先、回退 canonical"。备选方案（并行的 我逐一核对了字段的所有消费者，而不只是 diff 本身：
无阻断问题，无惯例违规。 按仓库惯例附设计文档（ 测试证据本节内容：被审提交上 PR 自身 CI（经 API 获取）。本次审查为静态——未在此构建或运行 PR 代码。 快照时刻无红色检查。绿色包括 Desktop Shell 构建（ubuntu-22.04、windows-2022）、Live Host（macos-latest）、Real daemon E2E（Java 11）、Java 矩阵（ubuntu/macos/windows 上的 11/17/21）、依赖 CVE 审计与 TruffleHog 密钥扫描。主单元测试套件（ 未在此验证：该提交上的真实 daemon 行为（仅静态审查 + CI 信号——这是无人值守 CI 运行，门禁从不执行 PR 代码）。 沙箱验证可以了结剩余的行为性结论： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — a clean, minimal contract fix that matches the smallest change I would have written myself; the gap to five is the runtime behavior CI alone can't pin, plus one third-party caveat. Stepping back: the problem was verified in the base code before any code review — a contract field carrying locale-dependent display text, with two visible workarounds papering over it and concrete failure modes (merged snapshot rows, an active extension's skill hidden by a disabled namesake). The fix corrects the value at the single producer, deletes both workarounds rather than adding machinery, and every one of the 19 files earns its place: producer, types, shared mapper, two identity sites, two presentation sites, tests, design doc, protocol doc. Nothing unrelated rode along. If I'm maintaining this in six months I'll thank the author — the identity/display split is documented in the protocol doc and in field-level JSDoc that warns against misuse. Reservations, none of them blocking:
Approval deferred until CI lands green on 中文说明置信度：4/5 —— 干净、最小化的契约修复，与我会写的最小改动一致；没到 5 分是因为 CI 本身无法固定的运行时行为，外加一个第三方注意事项。 退一步看：问题在任何代码审查之前就已在基线代码中核实——契约字段承载随语言环境变化的展示文本，两处可见的 workaround 在掩盖它，并有具体故障模式（快照行被合并、启用中 Extension 的 skill 被同名的已禁用 Extension 隐藏）。修复在唯一生产者处修正取值，删除了两处 workaround 而不是新增机制，19 个文件各司其职：生产者、类型、共享 mapper、两处身份匹配点、两处展示点、测试、设计文档、协议文档。没有夹带无关改动。半年后维护这段代码我会感谢作者——身份/展示的拆分在协议文档和字段级 JSDoc 中都有记录，且明确警告不得误用。 有所保留，但均不阻断：
批准推迟至该提交 CI 全绿后执行。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Aug 27, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
ytahdn
approved these changes
Aug 27, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
There was a problem hiding this comment.
✅ APPROVE
身份与展示的拆分贯穿一致，关键路径全部交叉验证通过，未发现 Critical / Important 问题。
The identity/display split is consistent end to end; all key paths were cross-verified in the head tree. No critical or important issues found.
值得肯定 / Highlights
extensionName（canonical manifest name）成为单一事实来源：active 路径（skill-manager）与非活跃合成路径（acpAgent）产生的键形完全对齐，原先针对 displayName 的双重去重检查安全收敛为单次查询。- 顺带修复了一个真实缺陷：活跃扩展与不活跃扩展共享 displayName 时，活跃扩展的 skill / slash command 曾被误判为非活跃而隐藏；改为 canonical 匹配后不再误伤（Session 测试中的 collision 用例即此场景）。
- 契约兼容处理完整：
extensionDisplayName为纯增量可选字段，schema 保持 v1；新客户端对接旧 daemon 时回退extensionName仍有正确显示；设计文档与协议文档同步更新。 mapSkillConfigToStatus将两个字段统一 gate 在level === 'extension'，避免身份信息泄漏到非 extension skill，并附回归测试。
(1. extensionName (the canonical manifest name) is now the single source of truth: the active path (skill-manager) and the inactive-synthesis path (acpAgent) produce identical key shapes, so the former double dedup check against displayName safely collapses to a single lookup. 2. This also fixes a real defect: when an active and an inactive extension shared a displayName, the active extension's skill / slash command was misclassified as inactive and hidden; canonical matching removes the false positive (the collision case in the Session tests covers exactly this). 3. Contract compatibility is fully handled: extensionDisplayName is a purely additive optional field, the schema stays at v1, new clients talking to older daemons fall back to extensionName with correct display, and the design and protocol docs are updated in sync. 4. mapSkillConfigToStatus gates both fields on level === 'extension', preventing owner identity from leaking onto non-extension skills, with regression tests.)
🟢 Nit（非阻塞，可后续跟进）/ Nit (non-blocking)
packages/web-shell/client/components/skills/SkillsManagerPage.tsx:692
本 PR 有意让"同名但来自不同 canonical 扩展"的 skill 各占一行（测试断言同名条目 ×2）。但列表行键为 key={skill.name}，选中态也是纯名字键（selectedName + find(name)）。出现两行同名时：React 会报重复 key 警告；点击第二行永远展示第一行的详情；statusOverrides / busySkill 也按名字互相串扰。建议行键与选中身份改为复合标识，例如 ${skill.level}:${skill.extensionName ?? ''}:${skill.name}。（备注：daemon 侧 enable/disable 本就按 skill 名、不区分扩展，该歧义为既有设计，不在本 PR 范围内。）
This PR intentionally gives each same-named skill from different canonical extensions its own row (the test asserts the duplicate entries). However, list rows use key={skill.name} and selection is keyed by name alone (selectedName + find(name)), so with two same-named rows React emits duplicate-key warnings, clicking the second row always shows the first row's details, and statusOverrides / busySkill cross-contaminate by name. Consider keying rows and selection by a composite identity such as ${skill.level}:${skill.extensionName ?? ''}:${skill.name}. (Note: daemon-side enable/disable is already by skill name only regardless of extension; that ambiguity is pre-existing and out of scope here.)
callmeYe
enabled auto-merge
August 27, 2026 13:59
yiliang114
approved these changes
Aug 27, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The canonical extension identity stays separate from presentation metadata across the workspace-skills path, and the current head has no failed checks.
yiliang114
pushed a commit
that referenced
this pull request
Aug 28, 2026
…l-cutover The branch had gone conflicting against main, which stops GitHub from building the merge ref — so `Qwen Code CI` (triggered on `pull_request`) had not run since the previous main merge, leaving recent commits with no test signal at all. The single conflict was in skills-manager-logic.test.ts: this branch moved the daemon React import from `@qwen-code/webui/daemon-react-sdk` to `@qwen-code/web-shell/daemon-react-sdk`, while #10296 on main added `skillExtensionLabel` to the same import. Kept the branch's module path and main's new symbol.
Collaborator
|
Released in v0.22.3. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR separates Extension Skill ownership identity from presentation metadata in the workspace skills status contract. Extension-owned Skills now expose the canonical manifest name through
extensionNameand the optional localized label throughextensionDisplayName, consistently for active and inactive Extensions.All inactive-state matching and snapshot deduplication use only the canonical owner name. The CLI and Web Shell continue to show a friendly label by preferring
extensionDisplayNameand falling back toextensionName, including when connected to an older daemon that does not provide the new field.The public ACP Bridge and TypeScript SDK status types and the daemon protocol documentation declare the same additive v1 contract.
Why it's needed
extensionNamepreviously contained a localized display name. Display names can change with locale and are not unique, so consumers could not reliably join a Skill to its owning Extension, and two Extensions sharing a label could affect each other's inactive-state or deduplication behavior.Reviewer Test Plan
How to verify
GET /workspace/skillswhile the Extension is active; the Skill should return the manifest name inextensionName, the friendly label inextensionDisplayName, andstatus: "ok".status: "disabled"anddisabledReason: "inactive_extension".Evidence (Before & After)
Before:
{ "extensionName": "Alibaba Cloud Database Suite" }After:
{ "extensionName": "alibabacloud-database-suite", "extensionDisplayName": "Alibaba Cloud Database Suite" }Local verification passed with the freshly built daemon in both active and inactive states. Focused Core, CLI, ACP workspace snapshot, Session, and Web Shell tests passed, as did
npm run build,npm run typecheck, and lint validation.Tested on
Environment (optional)
The real daemon was started on loopback with an isolated home and workspace. A local Extension fixture was installed through the daemon API, queried while active, disabled at workspace scope, and queried again.
Risk & Scope
extensionNamedirectly will now show the technical manifest name. Official clients preserve the friendly label through the new optional display field and canonical fallback.extensionDisplayNameis additive and optional;extensionNamekeeps its existing field shape but now carries its documented canonical identity.Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 在 workspace skills 状态契约中拆分 Extension Skill 的归属标识与展示元数据。Extension 所属 Skill 现在统一通过
extensionName返回 manifest canonical name，并通过可选的extensionDisplayName返回本地化展示名称，active 与 inactive Extension 保持一致。所有 inactive 状态匹配和快照去重只使用 canonical owner name。CLI 与 Web Shell 继续优先展示
extensionDisplayName，并回退到extensionName；连接不返回新字段的旧 daemon 时也能正常展示。ACP Bridge、TypeScript SDK 的公共状态类型以及 daemon 协议文档同步声明了这一 additive v1 契约。
为什么需要
extensionName之前承载的是本地化展示名称。展示名称会随语言环境变化，也不保证唯一，因此消费者无法可靠地将 Skill 关联到归属 Extension；两个共享展示名称的 Extension 还可能在 inactive 判断或去重时互相影响。Reviewer 验证计划
如何验证
GET /workspace/skills；Skill 应在extensionName返回 manifest name，在extensionDisplayName返回友好名称，并返回status: "ok"。status: "disabled"和disabledReason: "inactive_extension"。Before / After 证据
Before：
{ "extensionName": "Alibaba Cloud Database Suite" }After：
{ "extensionName": "alibabacloud-database-suite", "extensionDisplayName": "Alibaba Cloud Database Suite" }本地使用最新构建的 daemon 验证了 active 与 inactive 两种状态。Core、CLI、ACP workspace snapshot、Session 和 Web Shell 定向测试均通过，
npm run build、npm run typecheck与 lint 验证也通过。测试平台
环境（可选）
真实 daemon 在隔离的 home 与 workspace 中通过 loopback 启动。通过 daemon API 安装本地 Extension fixture，分别在启用状态查询、以 workspace scope 禁用后再次查询。
风险与范围
extensionName的第三方客户端现在会显示技术性的 manifest name。官方客户端通过新增的可选展示字段与 canonical fallback 保持友好展示。extensionDisplayName是 additive optional 字段；extensionName的字段形态不变，但其值修正为文档定义的 canonical identity。关联 Issue
N/A