From 8fb01a17482c5ea4eb34b443afc2708466702cdf Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 2 Aug 2026 12:07:03 +0200 Subject: [PATCH 1/6] fix(marketplace): key plugin skill dirs by id, not shortId `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) Claude-Session: https://claude.ai/code/session_01SGVmnRUtrXyxgpwxUtoarr --- scripts/lib/marketplace-generator.js | 11 +- .../lib/tests/marketplace-generator.test.js | 102 ++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 scripts/lib/tests/marketplace-generator.test.js diff --git a/scripts/lib/marketplace-generator.js b/scripts/lib/marketplace-generator.js index 74ca1ec6..fc36f05e 100644 --- a/scripts/lib/marketplace-generator.js +++ b/scripts/lib/marketplace-generator.js @@ -170,6 +170,7 @@ function generateMarketplace({ skills, tempDir, version, outputDir, configDir }) // Generate grouped plugins for (const [pluginName, groupSkills] of Object.entries(pluginGroups)) { const pluginDir = path.join(pluginsDir, pluginName); + const seen = new Set(); for (const skill of groupSkills) { const srcDir = path.join(tempDir, skill.id); @@ -178,7 +179,15 @@ function generateMarketplace({ skills, tempDir, version, outputDir, configDir }) continue; } - const destDir = path.join(pluginDir, 'skills', skill.shortId); + // `shortId` is only unique within a skill group, but a plugin aggregates + // many groups — keying dirs by it let two skills overwrite each other. + // Use the globally-unique `id`, as the mega-plugin below already does. + if (seen.has(skill.id)) { + throw new Error(`Plugin "${pluginName}" has duplicate skill id "${skill.id}"`); + } + seen.add(skill.id); + + const destDir = path.join(pluginDir, 'skills', skill.id); copyDirSync(srcDir, destDir); allSkillEntries.push({ diff --git a/scripts/lib/tests/marketplace-generator.test.js b/scripts/lib/tests/marketplace-generator.test.js new file mode 100644 index 00000000..6c8c7fea --- /dev/null +++ b/scripts/lib/tests/marketplace-generator.test.js @@ -0,0 +1,102 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { generateMarketplace } from '../marketplace-generator.js'; + +// Two skills from different groups sharing one category — the real shape of the +// collision: `omnibus/instrument-integration` and `omnibus/instrument-product-analytics` +// both declare `category: integration` with a single variant `id: all`. +const skill = (id, extra = {}) => ({ + id, + shortId: 'all', + category: 'integration', + displayName: id, + description: `${id} description`, + ...extra, +}); + +let dir; +const tempDir = () => path.join(dir, 'built'); +const configDir = () => path.join(dir, 'context'); +const outputDir = () => path.join(dir, 'dist'); +const pluginSkills = plugin => + fs.readdirSync(path.join(outputDir(), 'marketplace', 'plugins', plugin, 'skills')); + +function writeSkillSource(id) { + const skillDir = path.join(tempDir(), id); + fs.mkdirSync(path.join(skillDir, 'references'), { recursive: true }); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), `name: ${id}`); + fs.writeFileSync(path.join(skillDir, 'references', `${id}.md`), `${id} docs`); +} + +const run = skills => + generateMarketplace({ + skills, + tempDir: tempDir(), + version: 'test', + outputDir: outputDir(), + configDir: configDir(), + }); + +beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'marketplace-generator-')); + fs.mkdirSync(configDir(), { recursive: true }); + fs.writeFileSync( + path.join(configDir(), 'marketplace.yaml'), + [ + 'target_repo: PostHog/skills', + 'mega_plugin:', + ' name: posthog-all', + ' destination: skills/posthog/all', + 'plugins:', + ' integration:', + ' name: posthog-integration', + ' destination: skills/posthog/integration', + ].join('\n'), + ); + writeSkillSource('omnibus-instrument-integration'); + writeSkillSource('omnibus-instrument-product-analytics'); +}); + +afterEach(() => fs.rmSync(dir, { recursive: true, force: true })); + +describe('generateMarketplace', () => { + it('gives every skill in a plugin its own directory, keyed by full id', () => { + const skills = [ + skill('omnibus-instrument-integration'), + skill('omnibus-instrument-product-analytics'), + ]; + + const result = run(skills); + + // Keying by `shortId` collapsed both skills into `skills/all`, so one was + // silently dropped and the survivor inherited the loser's leftover files. + expect(pluginSkills('posthog-integration').sort()).toEqual([ + 'omnibus-instrument-integration', + 'omnibus-instrument-product-analytics', + ]); + expect(result.skillCount).toBe(skills.length); + }); + + it('copies each skill intact, with no files bleeding across siblings', () => { + run([ + skill('omnibus-instrument-integration'), + skill('omnibus-instrument-product-analytics'), + ]); + + const dirOf = id => + path.join(outputDir(), 'marketplace', 'plugins', 'posthog-integration', 'skills', id); + + for (const id of ['omnibus-instrument-integration', 'omnibus-instrument-product-analytics']) { + expect(fs.readFileSync(path.join(dirOf(id), 'SKILL.md'), 'utf8')).toBe(`name: ${id}`); + expect(fs.readdirSync(path.join(dirOf(id), 'references'))).toEqual([`${id}.md`]); + } + }); + + it('throws rather than overwriting when two skills share an id', () => { + expect(() => + run([skill('omnibus-instrument-integration'), skill('omnibus-instrument-integration')]), + ).toThrow(/duplicate skill id "omnibus-instrument-integration"/); + }); +}); From ef8a19c48fddce7b1bdca97df8a02b977e843d24 Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 2 Aug 2026 12:18:37 +0200 Subject: [PATCH 2/6] test(marketplace): reference #309 as the regression these cover Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SGVmnRUtrXyxgpwxUtoarr --- scripts/lib/tests/marketplace-generator.test.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/scripts/lib/tests/marketplace-generator.test.js b/scripts/lib/tests/marketplace-generator.test.js index 6c8c7fea..d09fbdae 100644 --- a/scripts/lib/tests/marketplace-generator.test.js +++ b/scripts/lib/tests/marketplace-generator.test.js @@ -5,8 +5,9 @@ import path from 'path'; import { generateMarketplace } from '../marketplace-generator.js'; // Two skills from different groups sharing one category — the real shape of the -// collision: `omnibus/instrument-integration` and `omnibus/instrument-product-analytics` -// both declare `category: integration` with a single variant `id: all`. +// #309 collision: `omnibus/instrument-integration` and +// `omnibus/instrument-product-analytics` both declare `category: integration` +// with a single variant `id: all`. const skill = (id, extra = {}) => ({ id, shortId: 'all', @@ -61,6 +62,10 @@ beforeEach(() => { afterEach(() => fs.rmSync(dir, { recursive: true, force: true })); +// Regression tests for #309 — `posthog-integration` published +// `omnibus-instrument-product-analytics` under `skills/all` while the +// integration omnibus went missing, because plugin skill dirs were keyed by the +// group-scoped `shortId` instead of the globally-unique `id`. describe('generateMarketplace', () => { it('gives every skill in a plugin its own directory, keyed by full id', () => { const skills = [ From 2901afbcd043886d7c9b424e7b0957dbb40ef871 Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 2 Aug 2026 12:33:27 +0200 Subject: [PATCH 3/6] fix(marketplace): guard duplicate ids across the whole build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01SGVmnRUtrXyxgpwxUtoarr --- scripts/lib/marketplace-generator.js | 11 ++++-- .../lib/tests/marketplace-generator.test.js | 36 ++++++++++++++++++- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/scripts/lib/marketplace-generator.js b/scripts/lib/marketplace-generator.js index fc36f05e..28c20e32 100644 --- a/scripts/lib/marketplace-generator.js +++ b/scripts/lib/marketplace-generator.js @@ -166,11 +166,14 @@ function generateMarketplace({ skills, tempDir, version, outputDir, configDir }) } const allSkillEntries = []; + // Every skill dir written below is keyed by `id`, so one id must map to one + // dir across the whole build — the mega-plugin pools skills from every plugin, + // so a per-plugin guard would miss a collision between two of them. + const seen = new Set(); // Generate grouped plugins for (const [pluginName, groupSkills] of Object.entries(pluginGroups)) { const pluginDir = path.join(pluginsDir, pluginName); - const seen = new Set(); for (const skill of groupSkills) { const srcDir = path.join(tempDir, skill.id); @@ -181,9 +184,11 @@ function generateMarketplace({ skills, tempDir, version, outputDir, configDir }) // `shortId` is only unique within a skill group, but a plugin aggregates // many groups — keying dirs by it let two skills overwrite each other. - // Use the globally-unique `id`, as the mega-plugin below already does. + // Use `id`, as the mega-plugin below already does. if (seen.has(skill.id)) { - throw new Error(`Plugin "${pluginName}" has duplicate skill id "${skill.id}"`); + throw new Error( + `Duplicate skill id "${skill.id}" — plugin skill dirs would overwrite each other`, + ); } seen.add(skill.id); diff --git a/scripts/lib/tests/marketplace-generator.test.js b/scripts/lib/tests/marketplace-generator.test.js index d09fbdae..b56a1181 100644 --- a/scripts/lib/tests/marketplace-generator.test.js +++ b/scripts/lib/tests/marketplace-generator.test.js @@ -54,10 +54,16 @@ beforeEach(() => { ' integration:', ' name: posthog-integration', ' destination: skills/posthog/integration', + // A second plugin keeps the suite honest: keyed by `shortId` for even + // one plugin, the assertions below fail. + ' logs:', + ' name: posthog-logs', + ' destination: skills/posthog/logs', ].join('\n'), ); writeSkillSource('omnibus-instrument-integration'); writeSkillSource('omnibus-instrument-product-analytics'); + writeSkillSource('logs-setup'); }); afterEach(() => fs.rmSync(dir, { recursive: true, force: true })); @@ -71,6 +77,7 @@ describe('generateMarketplace', () => { const skills = [ skill('omnibus-instrument-integration'), skill('omnibus-instrument-product-analytics'), + skill('logs-setup', { category: 'logs' }), ]; const result = run(skills); @@ -81,9 +88,28 @@ describe('generateMarketplace', () => { 'omnibus-instrument-integration', 'omnibus-instrument-product-analytics', ]); + // Every plugin is keyed the same way — `logs-setup` would land in + // `skills/all` too if any plugin still used `shortId`. + expect(pluginSkills('posthog-logs')).toEqual(['logs-setup']); expect(result.skillCount).toBe(skills.length); }); + it('pools every skill into the mega-plugin under its own id', () => { + const skills = [ + skill('omnibus-instrument-integration'), + skill('omnibus-instrument-product-analytics'), + skill('logs-setup', { category: 'logs' }), + ]; + + run(skills); + + expect(pluginSkills('posthog-all').sort()).toEqual([ + 'logs-setup', + 'omnibus-instrument-integration', + 'omnibus-instrument-product-analytics', + ]); + }); + it('copies each skill intact, with no files bleeding across siblings', () => { run([ skill('omnibus-instrument-integration'), @@ -102,6 +128,14 @@ describe('generateMarketplace', () => { it('throws rather than overwriting when two skills share an id', () => { expect(() => run([skill('omnibus-instrument-integration'), skill('omnibus-instrument-integration')]), - ).toThrow(/duplicate skill id "omnibus-instrument-integration"/); + ).toThrow(/Duplicate skill id "omnibus-instrument-integration"/); + }); + + // The mega-plugin pools every plugin's skills, so a collision across two + // plugins reaches it even though neither plugin collides on its own. + it('throws when two skills in different plugins share an id', () => { + expect(() => + run([skill('logs-setup'), skill('logs-setup', { category: 'logs' })]), + ).toThrow(/Duplicate skill id "logs-setup"/); }); }); From b83dd27762cfa5b1c0c3aa6398234050d1c6ac98 Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 2 Aug 2026 12:54:20 +0200 Subject: [PATCH 4/6] fix(marketplace): log skills copied, not skills offered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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) Claude-Session: https://claude.ai/code/session_01SGVmnRUtrXyxgpwxUtoarr --- scripts/lib/marketplace-generator.js | 6 +++++- scripts/lib/tests/marketplace-generator.test.js | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/scripts/lib/marketplace-generator.js b/scripts/lib/marketplace-generator.js index 28c20e32..30db56a6 100644 --- a/scripts/lib/marketplace-generator.js +++ b/scripts/lib/marketplace-generator.js @@ -174,6 +174,7 @@ function generateMarketplace({ skills, tempDir, version, outputDir, configDir }) // Generate grouped plugins for (const [pluginName, groupSkills] of Object.entries(pluginGroups)) { const pluginDir = path.join(pluginsDir, pluginName); + let written = 0; for (const skill of groupSkills) { const srcDir = path.join(tempDir, skill.id); @@ -194,6 +195,7 @@ function generateMarketplace({ skills, tempDir, version, outputDir, configDir }) const destDir = path.join(pluginDir, 'skills', skill.id); copyDirSync(srcDir, destDir); + written++; allSkillEntries.push({ dirName: skill.id, @@ -204,7 +206,9 @@ function generateMarketplace({ skills, tempDir, version, outputDir, configDir }) } writePluginJson(pluginDir, pluginName, version, maps); - console.log(` ✓ ${pluginName} (${groupSkills.length} skills)`); + // Count what was copied, not what was offered — a skipped source dir above + // would otherwise be reported as shipped. + console.log(` ✓ ${pluginName} (${written} skills)`); } // Generate mega-plugin diff --git a/scripts/lib/tests/marketplace-generator.test.js b/scripts/lib/tests/marketplace-generator.test.js index b56a1181..58d7fefa 100644 --- a/scripts/lib/tests/marketplace-generator.test.js +++ b/scripts/lib/tests/marketplace-generator.test.js @@ -125,6 +125,20 @@ describe('generateMarketplace', () => { } }); + it('logs the number of skills copied, not the number offered', () => { + const logged = []; + const log = console.log; + console.log = msg => logged.push(msg); + try { + // `missing-skill` has no source dir, so it is skipped with a warning. + run([skill('omnibus-instrument-integration'), skill('missing-skill')]); + } finally { + console.log = log; + } + + expect(logged).toContain(' ✓ posthog-integration (1 skills)'); + }); + it('throws rather than overwriting when two skills share an id', () => { expect(() => run([skill('omnibus-instrument-integration'), skill('omnibus-instrument-integration')]), From 82238b2259ea026ee1ff80e2ae097a0075e052c2 Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Mon, 17 Aug 2026 08:54:26 +0200 Subject: [PATCH 5/6] fix(marketplace): name the cause in the duplicate-id error Whoever trips this guard is editing config.yaml, not the generator, so "plugin skill dirs would overwrite each other" points at the symptom. Spell out that ids are the group key flattened plus the variant id, give the colliding shape, and say what to do about it. Co-Authored-By: Claude Opus 5 --- scripts/lib/marketplace-generator.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/lib/marketplace-generator.js b/scripts/lib/marketplace-generator.js index 30db56a6..14574615 100644 --- a/scripts/lib/marketplace-generator.js +++ b/scripts/lib/marketplace-generator.js @@ -188,7 +188,10 @@ function generateMarketplace({ skills, tempDir, version, outputDir, configDir }) // Use `id`, as the mega-plugin below already does. if (seen.has(skill.id)) { throw new Error( - `Duplicate skill id "${skill.id}" — plugin skill dirs would overwrite each other`, + `Duplicate skill id "${skill.id}" — two skill groups in config.yaml flatten to it. ` + + `An id is the group key with "/" replaced by "-", plus the variant id ` + + `(dropped when the variant is "all"), so "a/b" + variant "c" collides with ` + + `"a" + variant "b-c". Rename one group or variant.`, ); } seen.add(skill.id); From 60d1b8a5a1dffe1bbc05b163b1bdb150d19172a5 Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Mon, 17 Aug 2026 08:54:32 +0200 Subject: [PATCH 6/6] test(marketplace): assert the skip, not the log line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The count in "✓ posthog-integration (40 skills)" was the tell for #309, but pinning the exact string tested the sentence rather than the behaviour — two leading spaces, a checkmark, and the "(1 skills)" grammar bug all had to survive any rewording. Assert the plugin's skill dirs instead; verified by mutation that dropping the generator's `continue` still fails it. Also drop the same-plugin collision test. The cross-plugin case is strictly stronger — it only passes if the guard is build-scoped. Co-Authored-By: Claude Opus 5 --- .../lib/tests/marketplace-generator.test.js | 25 ++++++------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/scripts/lib/tests/marketplace-generator.test.js b/scripts/lib/tests/marketplace-generator.test.js index 58d7fefa..16210304 100644 --- a/scripts/lib/tests/marketplace-generator.test.js +++ b/scripts/lib/tests/marketplace-generator.test.js @@ -125,28 +125,17 @@ describe('generateMarketplace', () => { } }); - it('logs the number of skills copied, not the number offered', () => { - const logged = []; - const log = console.log; - console.log = msg => logged.push(msg); - try { - // `missing-skill` has no source dir, so it is skipped with a warning. - run([skill('omnibus-instrument-integration'), skill('missing-skill')]); - } finally { - console.log = log; - } - - expect(logged).toContain(' ✓ posthog-integration (1 skills)'); - }); + it('skips a skill with no source dir instead of writing an empty one', () => { + // `missing-skill` has no source dir, so it is skipped with a warning. The + // build's own log line counts what was copied off the back of this. + run([skill('omnibus-instrument-integration'), skill('missing-skill')]); - it('throws rather than overwriting when two skills share an id', () => { - expect(() => - run([skill('omnibus-instrument-integration'), skill('omnibus-instrument-integration')]), - ).toThrow(/Duplicate skill id "omnibus-instrument-integration"/); + expect(pluginSkills('posthog-integration')).toEqual(['omnibus-instrument-integration']); }); // The mega-plugin pools every plugin's skills, so a collision across two - // plugins reaches it even though neither plugin collides on its own. + // plugins reaches it even though neither plugin collides on its own. This + // subsumes the same-plugin case: it only passes if the guard is build-scoped. it('throws when two skills in different plugins share an id', () => { expect(() => run([skill('logs-setup'), skill('logs-setup', { category: 'logs' })]),