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) <noreply@anthropic.com>
This commit is contained in:
@@ -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", () => {
|
||||
|
||||
@@ -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<typeof getSkillSettingState>[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);
|
||||
|
||||
Reference in New Issue
Block a user