fix(extension): populate resources when Claude marketplace points at whole folder#4497
Merged
Merged
Conversation
…older `collectResources` in the Claude→Qwen converter silently dropped every file when a marketplace entry referenced the *whole* resource folder (e.g. `commands: ["./commands/"]`) instead of individual sub-entries (`skills: ["./skills/xlsx"]`). The cause was a stale skip-branch: `convertClaudePluginPackage` deletes `tmpDir/<destFolderName>` before calling `collectResources` (so it can honor selective sub-entry lists), but `collectResources` still believed the prior `copyDirectory` had already placed the files at the destination and short-circuited. The freshly-`mkdir`'d folder was left empty and the install reported success. Fix: - Directory branch: when `dirName === destFolderName`, flatten contents into `destDir` (instead of nesting under `destDir/<dirName>/`); for sub-entries (`./skills/xlsx`) keep nesting so `tmpDir/skills/xlsx/` still works for anthropics/skills shape. - File branch: drop the skip — always copy. `destFile = destDir/<fileName>` is correct for both layouts. Verified end-to-end against `microsoft/skills:deep-wiki` (13 commands, 10 skills, 3 agents now all install and surface in `extensions list`) and against `anthropics/skills:document-skills` (4 sub-skills still install correctly). Fixes #4452
Contributor
📋 Review SummaryThis PR fixes issue #4452 where installing Claude plugins that reference resources by whole folder (e.g., 🔍 General Feedback
🎯 Specific Feedback🟢 Medium
🔵 Low
✅ Highlights
|
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. |
wenshao
reviewed
May 25, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Clean fix — the root cause analysis is solid and the regression tests are thorough.
[Suggestion] packages/core/src/extension/claude-converter.ts:552 — The JSDoc for collectResources still says "If a resource is already in the destination folder, it will be skipped", but this PR removes all skip logic. The function now copies unconditionally (the caller clears destDir first). Consider updating the doc to match, e.g.:
* Resources are always copied unconditionally — the caller clears destDir beforehand.
— claude-opus-4-7 via Claude Code /qreview
Collaborator
|
<tool_call> Reviewed by |
Collaborator
Qwen Code Review (STANDARD)What this PR does (2–3 sentences synthesized from the diff and PR title/body): 🟢 P3 — Low
✅ Highlights (optional)
Validation EvidencePRESENT — The PR body includes:
Qwen Code |
Collaborator
Qwen Code Review (STANDARD)What this PR does: Fixes a bug in 🟠 P1 — High
✅ Highlights
Validation Evidence
Qwen Code |
The JSDoc still claimed "If a resource is already in the destination folder, it will be skipped" — true under the prior implementation but stale after #4497's fix, which removed all skip logic so the function copies unconditionally (the caller clears destDir beforehand). Spotted by wenshao on #4497.
Collaborator
Author
wenshao
reviewed
May 25, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running. — qwen3.7-max via Qwen Code /review
Collaborator
🧪 维护者本地构建 + 测试报告（基于 PR HEAD
|
| # | 步骤 | 结果 | 详情 |
|---|---|---|---|
| 1 | npm install（root + workspaces） |
✅ PASS | 1372 packages + post-install builds 全绿（≈1:22） |
| 2 | tsc --noEmit（@qwen-code/qwen-code-core） |
✅ PASS | 0 type errors（3.10s） |
| 3 | eslint on 2 个 PR 变更文件 |
✅ PASS | 0 lint errors（2.41s） |
| 4 | claude-converter.test.ts |
✅ PASS | 19/19 in 3.47s |
| 5 | 回滚回归检验：revert 修复后重跑 | ✅ AS EXPECTED | 2 个新测试 fail，17 旧测试 pass；恢复修复后 19/19 全绿 |
| 6 | core 全量测试（env-clean） | ✅ PASS | 9326 passed / 0 failed / 3 skipped，342 文件 / 67.43s |
| 7 | E2E install microsoft/skills:deep-wiki |
✅ PASS | 13 commands / 10 skills / 3 agents，与 PR 描述完全一致 |
| 8 | E2E 回归 anthropics/skills:document-skills |
✅ PASS | 4 个嵌套子 skill（docx/pdf/pptx/xlsx）保留 |
回滚回归检验（最强信号）
PR 描述声称 "new tests fail without the fix"，我把 claude-converter.ts checkout 回 ed14a3306（修复前）独立重跑：
FAIL src/extension/claude-converter.test.ts
> should populate commands/skills/agents when marketplace references the whole folder (deep-wiki shape)
AssertionError: expected [] to deeply equal [ 'index.md', 'wiki.md' ]
> should populate resources when marketplace references whole folder with trailing slash variants
AssertionError: expected false to be true (file 'a.md' does not exist)
Tests 2 failed | 17 passed (19)
恢复修复后 19/19 全绿。证明：
- 2 个新测试精准覆盖
collectResources在刚mkdir出的空目录上短路的 bug - 修复不动任何已有 case 的行为（17 个旧测试在 revert 状态下也全绿）
E2E 真机验证
#4452 故障重现 → 修复后正常（microsoft/skills:deep-wiki，正是 issue 报的那个）：
$ HOME=$(mktemp -d) node dist/cli.js extensions install \
"https://github.com/microsoft/skills:deep-wiki" --consent
Extension "deep-wiki" installed successfully and enabled.
$ node dist/cli.js extensions list
✓ deep-wiki (1.0.0)
Commands: /research /page /onboard /llms /generate /deploy /crisp
/changelog /catalogue /build /ask /agents /ado (13 ✓)
Skills: wiki-ado-convert wiki-agents-md wiki-architect
wiki-changelog wiki-llms-txt wiki-onboarding
wiki-page-writer wiki-qa wiki-researcher
wiki-vitepress (10 ✓)
Agents: wiki-architect wiki-researcher wiki-writer ( 3 ✓)
数量 13/10/3 与 PR 描述完全吻合，以前那个"装好但 list 是空"的症状消失。
回归基线（anthropics/skills:document-skills，验证嵌套 sub-entry 不被打断）：
$ HOME=$(mktemp -d) node dist/cli.js extensions install \
"https://github.com/anthropics/skills:document-skills" --consent
Extension "document-skills" installed successfully and enabled.
$ node dist/cli.js extensions list
✓ document-skills
Skills: docx pdf pptx xlsx (4 ✓)
skills/xlsx/ 嵌套结构如常 —— 修复只对 whole-folder 短路那条分支起作用，per-sub-entry 路径不动。
风险评估
- 范围：单文件单函数（
collectResources）。gh pr diff确认仅 2 文件，且claude-converter.ts净减 -5 LOC —— 删的是不该有的早退代码，很健康。 - 3 种 plugin 形态全覆盖：
dirName === destFolderName（deep-wiki，平铺入destDir，新行为）dirName !== destFolderName（anthropics 嵌套，不变，E2E 已验证）- 单文件 entry（不变，旧测试覆盖）
- 副作用：原先
src-agents重命名（PR 描述里 "renamed to src-agents to avoid skip-logic bug"）不再是 load-bearing，JSDoc 已在 follow-up 提交897077f69修正。 - 操作面：行为只对原本就坏掉的 case（装完是空 extension）改变。对原先能正常装的用户，二进制等价。
推荐：Merge。 修复精准、新测试可独立证伪 bug、E2E 真机验证两种 plugin 形态都通过、核心 9326 测试零回归。
wenshao
approved these changes
May 25, 2026
pomelo-nwu
approved these changes
May 26, 2026
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.
Summary
microsoft/skills:deep-wiki(and any otherClaude plugin that references resources by the whole folder) reported
success but produced an empty extension.
collectResources(packages/core/src/extension/claude-converter.ts).convertClaudePluginPackagedeletestmpDir/<commands|skills|agents>before invoking
collectResources(so it can honor selectivesub-entry lists like
skills: ["./skills/xlsx"]), butcollectResourceswas still short-circuiting on the assumption thatthe prior
copyDirectoryhad already placed the files at thedestination. The freshly-
mkdir'd folder was left empty.Fix
collectResourcesno longer short-circuits when the resource pathalready lives at
pluginRoot/<destFolderName>. Two cases:dirName === destFolderName(deep-wiki shape—
commands: ["./commands/"]): copy contents flat intodestDirinstead of nesting under
destDir/<dirName>/.dirName !== destFolderName(anthropics/skillsshape —
skills: ["./skills/xlsx"]): keep nesting attmpDir/skills/xlsx/— unchanged.agents: ["./agents/foo.md"]):always copy; the existing
destFile = destDir/<fileName>alreadyproduces the right path.
Tests
should populate commands/skills/agents when marketplace references the whole folder (deep-wiki shape)exercisesthe exact failure mode (mixed whole-folder + file-entry shape).
trailing slash variantstest covers both./commands/and./commands.renamed to src-agents to avoid skip-logic bug) — comment updated; the directoryrename is now incidental, not load-bearing.
Verified all 19 tests in
claude-converter.test.tspass with the fix andthat the new test fails without it (
git stash-ed the fix, re-ran thetest, observed
expected [] to deeply equal [ 'index.md', 'wiki.md' ]).End-to-end verification
Built the CLI from this branch and ran against a fresh
$HOME:Regression check against the previously-working
anthropics/skills:Sub-folder nesting (
skills/xlsx/) is preserved as before.Test plan
npx vitest run packages/core/src/extension/claude-converter.test.ts— 19/19 passgit stash)microsoft/skills:deep-wikiinstalls with 13 commands, 10 skills, 3 agents andextensions listsurfaces themanthropics/skills:document-skillsstill installs with 4 nested sub-skills