fix(parser): deduplicate component dirs when manifest declares the default - #1260
fix(parser): deduplicate component dirs when manifest declares the default#1260danikdanik wants to merge 1 commit into
Conversation
|
Thanks @danikdanik for the contribution! I'd like to reconcile the stated problem with what I measured, because I couldn't reproduce the recursive-scan behavior. What I tested (Claude Code I built a throwaway local marketplace plugin Installed it project-scoped, then asked a fresh session what it could see:
Corroborating evidence from a real install: my Question 1: how did you observe the 6 fixture skills loading? The test plan says "Verified the 6 fixture skills no longer load with the manifest field present" — that's consistent with them also not loading without it. If you have a before-state capture (a Question 2 — the parser change is the part I can reproduce, and it's worth landing. On
On this branch all three return 2. Note the ordering though: that double-scan is latent on Question 3 — validation claims. The branch is based on Separately, Merging the PR into current |
…fault resolveComponentDirs always included the default dir (root/skills) AND any manifest-declared dir. When a manifest declares the default dir explicitly (e.g. `"skills": "./skills/"`), this scanned it twice and double-counted skills. Deduplicate by resolved path. Adds a regression fixture (default-dir-declared) with a manifest that declares `"skills": "./skills/"` and two skills, asserting the parser returns 2 skills, not 4.
912faa9 to
bdda198
Compare
danikdanik
changed the title
|
Thanks @tmchow for the thorough review - you were right on all three points, and I appreciate the time you took to reproduce and measure. Q1 - you're correct, the recursive-scan claim was wrong. I verified on Claude Code 2.1.220 with What changed in this PR: I dropped the manifest change entirely. The PR now contains only the parser dedup (Q2) plus the regression test you asked for. Rebased onto current Q2 - regression test added. New fixture Q3 - rebased and re-validated.
PR title and description updated to match the reframed scope. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdda198a1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| test("does not double-count skills when the manifest declares the default dir", async () => { | ||
| const plugin = await loadClaudePlugin(defaultDirDeclaredRoot) | ||
| expect(plugin.skills.map((skill) => skill.name).sort()).toEqual(["skill-a", "skill-b"]) |
There was a problem hiding this comment.
This new regression only exercises a temporary fixture with skills: "./skills/"; it never checks the actual .claude-plugin/plugin.json, which still has no skills field in this commit. If the goal is to stop Claude installs of the root plugin from discovering fixture SKILL.md files, users remain on the old manifest behavior until the real Claude manifest declares the skills directory, so please update that manifest or assert it here against compoundPluginRoot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging this; it was correct for the original PR scope, but that scope has since been dropped.
The original PR added "skills": "./skills/" to .claude-plugin/plugin.json to prevent fixture SKILL.md files from loading as real skills. That change (and the recursive-scan claim behind it) was removed after @tmchow's review proved the Claude Code loader scans skills/<name>/SKILL.md one level deep regardless of whether the skills field is present, so fixture SKILL.md files under tests/fixtures/ are never discovered. I verified this on Claude Code 2.1.220 with compound-engineering@3.20.0 installed (no skills field; all 6 fixture SKILL.md files present in the Claude Code plugin cache): a fresh claude -p session listed 24 real CE skills and zero fixtures.
The current PR is purely the parser dedup: when a manifest does declare the default dir explicitly, resolveComponentDirs no longer scans it twice. The regression test exercises that via a fixture because the real .claude-plugin/plugin.json intentionally has no skills field and doesn't need one. A contract test asserting the real manifest omits skills isn't applicable here: that absence is an intentional manifest design choice (rely on default loading), not a parser behavior, and the loader's one-level scan makes it irrelevant to skill discovery either way.
|
This finding is based on the original PR scope, which has been dropped. The manifest change ( The PR is now purely the parser dedup fix: when a manifest does declare the default dir explicitly, |
2 participants
Summary
resolveComponentDirsso a manifest that declares the default directory explicitly (e.g."skills": "./skills/") does not scan it twice and double-count skillsdefault-dir-declared) with a manifest declaring"skills": "./skills/"and two skills, asserting the parser returns 2 skills, not 4Context
resolveComponentDirsalways included the default dir (root/skills) AND any manifest-declared dir. When a manifest declares the default dir explicitly, the default dir ended up in the array twice, producing 2N skills in conversion output. This was latent onmainbecause the Claude manifest did not declare theskillsfield, but it is reachable for any plugin that does.The original version of this PR also added
"skills": "./skills/"to.claude-plugin/plugin.jsonbased on a claim that the Claude Code loader recursively scanned the entire repo tree without it. That claim was wrong — the loader scansskills/<name>/SKILL.mdone level deep and does not pick up fixtures undertests/fixtures/. The manifest change has been dropped. Thanks to @tmchow for the thorough review that caught this.Test plan
bun run test— 2747 pass, 0 failbun run plugin:validate— both manifests pass--strictbun run release:validate— in sync, 32 skillsSecurity Disclosure
No security-relevant changes.
Agent Disclosure
Claude Code · claude-sonnet-4-5 (verification and code changes); review and reframing informed by prior sessions on Claude Code 2.1.220.