From 8fb01a17482c5ea4eb34b443afc2708466702cdf Mon Sep 17 00:00:00 2001 From: Peter Trost Date: Sun, 2 Aug 2026 12:07:03 +0200 Subject: [PATCH 1/4] 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/4] 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/4] 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/4] 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')]),