From 67e76e2b80c916fe7d11467369b97ec48d2b8722 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 21 Jun 2026 02:23:59 -0700 Subject: [PATCH] Address PR review feedback (#1701) - Plugin skills now respect project-settings enable/disable toggles (discoverSkills consults getSkillSettingState, falling back to the plugin's declared default) so toggleExecutionSkill writes persist - readSkillContent returns synthesized content for plugin-contributed skills (name + description + provenance) instead of a silent blank panel, since their path is a virtual runtime-materialized path Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/__tests__/skills-adapter.test.ts | 58 ++++++++++++++ packages/dashboard/src/skills-adapter.ts | 77 +++++++++++++++---- 2 files changed, 122 insertions(+), 13 deletions(-) diff --git a/packages/dashboard/src/__tests__/skills-adapter.test.ts b/packages/dashboard/src/__tests__/skills-adapter.test.ts index 7631d63d49..e06ce68d7a 100644 --- a/packages/dashboard/src/__tests__/skills-adapter.test.ts +++ b/packages/dashboard/src/__tests__/skills-adapter.test.ts @@ -826,6 +826,64 @@ describe("createSkillsAdapter - plugin skill merge", () => { const skills = await adapter.discoverSkills("/tmp/project"); expect(skills).toEqual([]); }); + + it("lets a project-settings toggle override a plugin skill's default enabled", async () => { + const dir = join(tmpdir(), `skills-adapter-plugin-toggle-${process.pid}-${Date.now()}`); + const settingsPath = join(dir, "settings.json"); + await mkdir(dir, { recursive: true }); + // ce-plan defaults to enabled, but a "-" entry under its plugin package + // source must disable it; without the settings lookup the toggle is lost. + const relativePath = "skills/ce-plan/SKILL.md"; + await writeFile( + settingsPath, + JSON.stringify({ + packages: [{ source: "plugin:fusion-plugin-compound-engineering", skills: [`-${relativePath}`] }], + }), + ); + + try { + const adapter = createSkillsAdapter({ + packageManager: { resolve: vi.fn().mockResolvedValue({ skills: [] }) }, + getSettingsPath: () => settingsPath, + getPluginSkills: () => [ + { pluginId: "fusion-plugin-compound-engineering", skill: { name: "ce-plan", enabled: true } }, + ], + }); + + const skills = await adapter.discoverSkills(dir); + const cePlan = skills.find((s) => s.name === "ce-plan"); + expect(cePlan).toBeDefined(); + expect(cePlan!.enabled).toBe(false); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); +}); + +describe("createSkillsAdapter - readSkillContent for plugin skills", () => { + it("returns synthesized content (not a blank panel) for plugin-contributed skills", async () => { + const adapter = createSkillsAdapter({ + packageManager: { resolve: vi.fn().mockResolvedValue({ skills: [] }) }, + getSettingsPath: () => "/tmp/does-not-exist-settings.json", + getPluginSkills: () => [ + { + pluginId: "fusion-plugin-compound-engineering", + skill: { name: "ce-plan", description: "Create structured plans." }, + }, + ], + }); + + const skills = await adapter.discoverSkills("/tmp/project"); + const cePlan = skills.find((s) => s.name === "ce-plan")!; + + const content = await adapter.readSkillContent("/tmp/project", cePlan.id); + expect(content.name).toBe("ce-plan"); + expect(content.skillMd).toContain("ce-plan"); + expect(content.skillMd).toContain("Create structured plans."); + expect(content.skillMd).toContain("fusion-plugin-compound-engineering"); + expect(content.skillMd).not.toBe(""); + expect(content.files).toEqual([]); + }); }); describe("bareSkillName", () => { diff --git a/packages/dashboard/src/skills-adapter.ts b/packages/dashboard/src/skills-adapter.ts index 54cb1f31b7..283b1b30dd 100644 --- a/packages/dashboard/src/skills-adapter.ts +++ b/packages/dashboard/src/skills-adapter.ts @@ -53,6 +53,8 @@ export interface DiscoveredSkill { path: string; relativePath: string; enabled: boolean; + /** Optional human-readable description (currently set for plugin skills). */ + description?: string; metadata: { source: string; scope: "user" | "project" | "temporary"; @@ -256,17 +258,18 @@ async function waitForSupervisedExit( } /** - * Check if a skill path is enabled in the settings. - * Checks both top-level skills and package-scoped skills. + * Resolve a skill's explicit enable/disable state from settings. + * Checks both top-level skills and package-scoped skills. Returns "enabled" or + * "disabled" when a settings entry matches, or undefined when the settings file + * says nothing about this skill -- so callers can apply their own default. */ -function isSkillEnabled( +function getSkillSettingState( skillId: string, settings: { skills?: string[]; packages?: Array<{ source: string; skills?: string[] }> }, -): boolean { - // Check top-level skills +): "enabled" | "disabled" | undefined { const parsedSkillId = parseSkillId(skillId); if (!parsedSkillId) { - return false; + return undefined; } const normalizedSkillPath = normalizeStoredSkillPath(parsedSkillId.relativePath); @@ -278,7 +281,7 @@ function isSkillEnabled( ); const entryId = computeSkillId("*", `skills/${entryPath}`); if (entryId === skillId || entryPath === normalizedSkillPath) { - return entry.startsWith("+"); + return entry.startsWith("+") ? "enabled" : "disabled"; } } @@ -295,13 +298,24 @@ function isSkillEnabled( ); const entryId = computeSkillId(source, `skills/${entryPath}`); if (entryId === skillId || entryPath === normalizedSkillPath) { - return entry.startsWith("+"); + return entry.startsWith("+") ? "enabled" : "disabled"; } } } - // Default to disabled if not found - return false; + return undefined; +} + +/** + * Check if a skill path is enabled in the settings. + * Checks both top-level skills and package-scoped skills. + * Defaults to disabled when the settings file says nothing about the skill. + */ +function isSkillEnabled( + skillId: string, + settings: { skills?: string[]; packages?: Array<{ source: string; skills?: string[] }> }, +): boolean { + return getSkillSettingState(skillId, settings) === "enabled"; } /** @@ -324,7 +338,10 @@ export function createSkillsAdapter(options: { * skill catalog omits them. Lazy thunk: plugins may load after the adapter is * created, so it is invoked per discovery rather than captured eagerly. */ - getPluginSkills?: () => Array<{ pluginId: string; skill: { name: string; enabled?: boolean } }>; + getPluginSkills?: () => Array<{ + pluginId: string; + skill: { name: string; description?: string; enabled?: boolean }; + }>; /** Optional superviseSpawn seam for tests */ superviseSpawn?: typeof superviseSpawn; }): SkillsAdapter { @@ -389,12 +406,25 @@ export function createSkillsAdapter(options: { if (seenBareNames.has(bare)) continue; seenBareNames.add(bare); const relativePath = `skills/${name}/SKILL.md`; + const id = computeSkillId(`plugin:${pluginId}`, relativePath); + // Respect an explicit enable/disable written to project settings by + // toggleExecutionSkill, falling back to the plugin's declared default. + // Without consulting settings here, a user toggle on a plugin skill + // would be silently reverted on the very next discovery. + const settingState = getSkillSettingState( + id, + settings as Parameters[1], + ); + const enabled = settingState === undefined + ? skill.enabled !== false + : settingState === "enabled"; discoveredSkills.push({ - id: computeSkillId(`plugin:${pluginId}`, relativePath), + id, name, path: relativePath, relativePath, - enabled: skill.enabled !== false, + enabled, + description: skill.description, metadata: { source: `plugin:${pluginId}`, scope: "user", @@ -682,6 +712,27 @@ export function createSkillsAdapter(options: { throw new Error(`Skill not found: ${skillId}`); } + // Plugin-contributed skills have no on-disk representation in this + // catalog: the engine materializes them for executor sessions at runtime, + // so `path` is a virtual relative path with no filesystem backing. Reading + // it would silently return a blank panel, so surface what we know (name + + // description) and explain where the definition lives instead. + if (skill.metadata.source.startsWith("plugin:")) { + const pluginId = skill.metadata.source.slice("plugin:".length); + const lines = [`# ${skill.name}`, ""]; + if (skill.description) { + lines.push(skill.description, ""); + } + lines.push( + `_Contributed by the \`${pluginId}\` plugin. Its definition is materialized at runtime and has no editable file in this project._`, + ); + return { + name: skill.name, + skillMd: lines.join("\n"), + files: [], + }; + } + let skillDir = skill.path; try { const skillPathStat = await stat(skill.path);