fix(core): give plugins from the same repository distinct extension ids#7676
Conversation
getExtensionId() hashed only the normalized repo URL, so two plugins installed from one marketplace repo (e.g. dotnet and dotnet-test from github.com/dotnet/skills) collided on the same id and the second install failed with 'Extension id belongs to "dotnet", not "dotnet-test"'. The plugin name is now appended to the hash input when present. Because the formula change would strand existing installs (the store would mint a fresh default policy under the new id, resetting activation state, and the orphaned old policy would trip the name-conflict guard on update/uninstall), ExtensionStore.ensureInitialized now re-keys a stored policy to a loaded identity's id when the id is unknown but the unique name matches and no loaded extension still owns the old entry. Fixes QwenLM#7568 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with a clear reproduction — issue #7568 shows installing a second plugin from Direction: aligned. The extension system is core functionality, and multi-plugin repos colliding on a single ID is a real usability blocker. The maintainer's issue comment endorses exactly this approach. Size: 34 production lines (26 in Approach: the scope feels right — two focused changes (hash disambiguation + re-key migration) that directly address the bug and its upgrade path. No unrelated edits, no drive-by refactors. The re-key is well-constrained: it only claims orphaned policies by name, and only when no loaded extension still owns them. The maintainer's suggested fix and this PR are the same shape. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,有明确复现——issue #7568 显示从 方向:对齐。扩展系统是核心功能,多插件仓库 ID 碰撞是真实的可用性阻断。维护者在 issue 评论中背书了正是这个方案。 规模:34 行生产代码( 方案:范围合理——两个聚焦的改动(哈希消歧 + 重挂迁移)直接解决 bug 及其升级路径。无无关改动,无顺手重构。重挂逻辑约束良好:仅按名认领孤儿 policy,且仅在无已加载扩展持有时生效。维护者建议的修复与本 PR 形状一致。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the root cause (hash only uses repo URL), I would (1) append Findings: no critical blockers, no convention violations. The The re-key step in Downstream consumers: Tests cover the key scenarios: distinct IDs for same-repo plugins, repo-only ID preserved without Testing
All CI checks green on the reviewed commit. Windows and macOS tests were skipped because this is a fork PR; integration tests were also skipped. The ubuntu unit suite and precheck both passed. not verified: real-scenario E2E (CI run — the isolated 中文说明代码审查独立方案: 根据根因(哈希仅使用仓库 URL),我会 (1) 在 发现: 无关键阻断,无规范违反。
下游消费者: 测试覆盖关键场景:同仓库插件不同 ID、无 测试审查提交的所有 CI 检查全绿。Windows 和 macOS 测试因为是 fork PR 被跳过;集成测试也被跳过。ubuntu 单元测试套件和预检均通过。 未验证:真实场景 E2E(CI 运行——维护者可触发隔离的 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This is a textbook bugfix: the problem is real (issue #7568, maintainer-confirmed), the fix is exactly what the maintainer prescribed, and the migration path handles the ID formula change without user action. The diff is 34 production lines — two focused edits, no scope creep, no unrelated changes. The re-key logic is well-constrained by the store's existing name-uniqueness invariant, and the six new tests cover the happy path, the migration, case normalization, the orphan-only guard, and the no-theft guard. Going back to my independent proposal: the PR matches it exactly. I don't see a simpler path — both the hash disambiguation and the re-key are necessary, and the implementation is minimal. CI is fully green on the reviewed commit (ubuntu tests + precheck passed; Windows/macOS/integration skipped for fork PRs). Maintainer wenshao independently verified the fix locally with a compiled E2E and A/B test discrimination — the new tests genuinely fail on base. 中文说明置信度:5/5 —— 每个阶段都干净;毫不犹豫可以合并。 这是一个教科书式的 bugfix:问题真实存在(issue #7568,维护者确认),修复正是维护者开出的方子,迁移路径在用户无需操作的情况下处理了 ID 公式变更。diff 仅 34 行生产代码——两处聚焦编辑,无范围蔓延,无无关改动。重挂逻辑受 store 既有名字唯一性不变量良好约束,六条新测试覆盖了正常路径、迁移、大小写规范化、仅孤儿守卫和防盗守卫。 回到我的独立方案:PR 与之完全一致。我看不到更简的路径——哈希消歧和重挂都是必要的,实现是最小的。 审查提交上 CI 全绿(ubuntu 测试 + 预检通过;Windows/macOS/集成测试因 fork PR 跳过)。维护者 wenshao 已独立本地验证,使用编译产物 E2E 和 A/B 测试判别——新测试在 base 上确实失败。 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| const staleEntry = Object.entries(existing.extensions).find( | ||
| ([id, policy]) => | ||
| !loadedIds.has(id) && | ||
| policy.name.toLowerCase() === identity.name.toLowerCase(), |
There was a problem hiding this comment.
[Suggestion] The re-key migration uses case-insensitive name matching (toLowerCase()) but no test exercises a case mismatch between the stored policy name and the incoming identity name.
Failure scenario: a future refactor that simplifies to policy.name === identity.name would silently break migration for extensions stored under different casing — the stale policy would be orphaned and activation state reset with no error.
Consider adding a test that seeds the store with { id: oldId, name: 'DotNet' }, sets activation state, then calls ensureInitialized([{ id: newId, name: 'dotnet' }]) and asserts re-key with name: 'dotnet' and state preserved.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added in ec51b8a — re-keys across a case mismatch and normalizes the stored name: seeds { id: oldId, name: 'DotNet' } with defaultActivation: 'disabled', calls ensureInitialized([{ id: newId, name: 'dotnet' }]), and asserts the policy moves to the new id with state preserved and name normalized to 'dotnet'. Store suite 59/59.
中文
已在 ec51b8a 补上——re-keys across a case mismatch and normalizes the stored name:以 DotNet 名字播种 disabled 状态的 policy,用 dotnet 身份触发 re-key,断言 policy 迁移到新 id、状态保留、名字归一为 dotnet。store 套件 59/59。
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
— qwen3.7-max via Qwen Code /review
Seeds a policy under 'DotNet', re-keys it via an identity named 'dotnet', and asserts the policy moves with its activation state and the stored name normalizes — so a future simplification to case-sensitive matching fails a test instead of silently orphaning differently-cased installs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
— qwen3.7-max via Qwen Code /review
✅ Local verification (maintainer) — merge referenceBuilt and verified PR #7676 (head 1 · Independent before/after E2E — reproduces the issue, then fixes itA throwaway script drives the compiled
2 · Full extension suite green + A/B proves the tests pin the fix
3 · Quality gates & blast radius
4 · Code audit — the migration ordering is deliberate and correctI traced Note (not a blocker)In my E2E Part 3, two of the migration checks ( Verdict: LGTM / merge-ready. Correct, minimal, well-tested, backward-compatible (self-heals on first load, local/link installs unaffected), and the id-formula change has no fallout beyond the extension module. Verified locally by the maintainer at head 🇨🇳 中文版本(本地验证报告 · 供合并参考)✅ 本地验证(维护者)— 合并参考在隔离 worktree 中构建并验证了 PR #7676(head 1 · 独立 Before/After E2E — 先复现问题,再验证修复脚本驱动编译后的
2 · 扩展测试全绿 + A/B 证明测试确实钉住修复
3 · 质量关卡与影响范围
4 · 代码审查 — 迁移顺序是刻意且正确的追踪 说明(非阻塞)E2E Part 3 中,两个迁移检查( 结论:LGTM / 可以合并。 正确、最小、测试充分、向后兼容(首次加载自愈,local/link 安装不受影响),id 公式变更在扩展模块之外无任何波及。 由维护者在 head |
|
Thanks for the thorough independent verification — especially the sharp observation that |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅


What this PR does
Makes extensions installed from the same repository get distinct ids:
getExtensionId()now appendsinstallMetadata.pluginNameto the hash input when it is present, sodotnetanddotnet-testfromhttps://github.com/dotnet/skillsno longer collide. Because an id-formula change would otherwise strand every existing install — on the next load the store would mint a fresh default policy under the new id (silently resetting the user's activation state) and the orphaned old policy would then trip the name-conflict guard on any update or uninstall —ExtensionStore.ensureInitialized()gains a re-key step: a loaded identity whose id is unknown to the store claims the policy stored under its unique name, provided no other loaded extension still owns that entry. Activation state, workspace overrides, and artifact generation all move with the policy; the store's existing case-insensitive name-uniqueness guarantee is what makes the name-based claim safe.Why it's needed
#7568: installing a second plugin from a multi-plugin marketplace repo fails with
Extension id belongs to "dotnet", not "dotnet-test".—getExtensionId()hashed only the normalized repo URL, so every plugin from one repo produced the same id and the store's id-ownership check rejected the second install. The triage confirmed the root cause and maintainer doudouOUC endorsed exactly this fix shape (appendpluginNameto the hash input), explicitly calling out that a migration path or backward-compatible lookup must accompany the id change for users who already have one plugin installed from such a repo — that is the re-key step above.Reviewer Test Plan
How to verify
/extensions install https://github.com/dotnet/skills→ choosedotnet; repeat and choosedotnet-test. Before this PR the second install fails with the id-ownership error; after, both install and coexist.npx vitest run src/extension/inpackages/core(573/573).Evidence (Before & After)
E2E drives the compiled
getExtensionId+ realExtensionStore(temp dirs on disk, real install/uninstall commits), before/after viagit stash+ rebuild:Extension id belongs to "dotnet", not "dotnet-test".Tested on
Environment (optional)
macOS (Darwin 24.6), Node v22.23.1; vitest.
npm run typecheck,eslint --max-warnings 0,prettier --checkall clean.Risk & Scope
pluginNameare byte-for-byte unaffected.<repo>:<pluginName>install-source shorthand parsing (marketplace.ts) is untouched; Claude-marketplace conversions keep their own plugin hashing (claude-converter.ts).Linked Issues
Fixes #7568
中文说明
本 PR 做了什么
让同一仓库安装的多个扩展获得不同 id:
getExtensionId()在installMetadata.pluginName存在时把它追加进哈希输入,dotnet与dotnet-test不再碰撞。由于 id 公式变更会孤立所有既有安装——下次加载时 store 会在新 id 下铸造全新默认 policy(静默重置用户的启用状态),而旧 policy 变成孤儿、在 update/uninstall 时触发名字冲突守卫——ExtensionStore.ensureInitialized()新增重挂步骤:store 不认识其 id 的已加载 identity,可认领以其唯一名字存储的 policy(前提是没有其它已加载扩展仍持有该条目)。启用状态、workspace 覆盖、artifact generation 随 policy 一起迁移;store 既有的大小写不敏感名字唯一性保证了按名认领的安全性。为什么需要
#7568:从多插件市场仓库安装第二个插件时报
Extension id belongs to "dotnet", not "dotnet-test".——getExtensionId()只哈希规范化仓库 URL,同仓库所有插件产生相同 id,store 的 id 归属检查拒绝第二次安装。triage 已确认根因,维护者 doudouOUC 背书了正是本 PR 的修复形状(哈希输入追加pluginName),并明确指出 id 变更必须配迁移路径或向后兼容查找——即上述重挂步骤。审阅测试计划
如何验证
/extensions install https://github.com/dotnet/skills选dotnet,再来一次选dotnet-test:本 PR 之前第二次安装报 id 归属错误;之后两者共存。packages/core下npx vitest run src/extension/(573/573)。证据(Before & After)
E2E 驱动编译产物
getExtensionId+ 真实ExtensionStore(磁盘临时目录、真实 install/uninstall 提交),git stash+ 重建对照:未修复时两插件同 id、第二次安装报 issue 原始错误、无重挂;修复后全部通过。4 条新单测中 3 条在未修复源码上失败。测试平台
macOS 已本地验证(✅);Windows / Linux 依赖 CI(⚠️ )。
环境
macOS(Darwin 24.6)、Node v22.23.1;vitest;typecheck / eslint / prettier 全绿。
风险与范围
pluginName的 local/link 安装完全不受影响。<repo>:<pluginName>安装源简写解析(marketplace.ts)未改动;Claude marketplace 转换保留自己的插件哈希(claude-converter.ts)。关联 Issue
Fixes #7568
🤖 Generated with Claude Code