fix(marketplace): key plugin skill dirs by id, not shortId - #308
Open
Peetee06 wants to merge 4 commits into
Open
fix(marketplace): key plugin skill dirs by id, not shortId#308Peetee06 wants to merge 4 commits into
Peetee06 wants to merge 4 commits into
Conversation
`shortId` is a skill's variant id within its group, so it is unique only per-group. A plugin aggregates many groups, so keying its skill dirs by `shortId` puts a group-scoped key into a plugin-scoped namespace. Two skills collide today: `omnibus/instrument-integration` and `omnibus/instrument-product-analytics` both declare `category: integration` with a single variant `id: all`, so both resolve to `plugins/posthog-integration/skills/all`. `copyDirSync` merges file-by-file without clearing, so the survivor also inherits the loser's leftover references. Use the globally-unique `id`, matching what the mega-plugin already does, and throw on a duplicate id the way `writeBundles` already does for duplicate variant ids. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SGVmnRUtrXyxgpwxUtoarr
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SGVmnRUtrXyxgpwxUtoarr
The `seen` set was scoped per plugin, so two skills sharing an id in different plugins never met the guard and both landed in the mega-plugin, which pools every plugin's skills — the same silent-overwrite failure as grouped plugins had, one directory over. Hoist the set to function scope so one id maps to one dir build-wide, and widen the tests: a second plugin in the fixture (keying by `shortId` for even one plugin now fails), plus an assertion on the mega-plugin's dirs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SGVmnRUtrXyxgpwxUtoarr
The per-plugin line reported the input array length, so a skill whose source dir was missing — skipped with a warning just above — still counted as shipped. That is what made PostHog#309 hard to spot: the build logged 40 skills for posthog-integration while writing 39. Count the copies instead. The mega-plugin line and the returned skillCount already derive from allSkillEntries, which is appended only after a successful copy, so they were already truthful. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SGVmnRUtrXyxgpwxUtoarr
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
1 participant
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.
Fixes #309.
shortIdis unique only within a skill group, but a plugin aggregates many groups — so two skills collapsed into one directory. Keyed byidinstead, which the mega-plugin already does.Also fixes the log line that hid it: it counted skills offered rather than copied, so the build reported 40 while writing 39.
Blast radius
Grouped-plugin skill dirs get their full id (
all→omnibus-instrument-integration,django→integration-django). Nothing reads those names:name:, which has always been the full id —integration/skills/django/SKILL.mdreadsname: integration-djangotoday. No invocation name changes; this fixes a skill going missing.plugin.jsoncarries no skill list, and nothing in the repo hardcodesskills/<name>posthog-allalready ships these exact names, so the two plugins convergeposthog-all/skills, already keyed byid— untouchedWon't reach PostHog/skills yet
PostHog/skillsis stale since March — the release workflow's push step is skipped afterGenerate skills repo tokenfails (latest run), and recent syncs came fromreleaser-skills[bot]instead.So #309's reproduction keeps returning the wrong
name:until that sync runs again — you'll know which path owns it.