diff --git a/src/cli/commands/plugin-skills.ts b/src/cli/commands/plugin-skills.ts index 5c3d986..bffb669 100644 --- a/src/cli/commands/plugin-skills.ts +++ b/src/cli/commands/plugin-skills.ts @@ -5,9 +5,7 @@ import chalk from 'chalk'; import { command, positional, option, flag, string, optional, restPositionals } from 'cmd-ts'; import { syncWorkspace, syncUserWorkspace } from '../../core/sync.js'; import { - addDisabledSkill, removeDisabledSkill, - removeEnabledSkill, addEnabledSkill, addPlugin, hasPlugin, @@ -16,9 +14,7 @@ import { upsertGitHubPluginSourceAllowlist, } from '../../core/workspace-modify.js'; import { - addUserDisabledSkill, removeUserDisabledSkill, - removeUserEnabledSkill, addUserEnabledSkill, isUserConfigPath, addUserPlugin, @@ -55,6 +51,7 @@ import { } from '../../core/marketplace.js'; import { parseMarketplaceManifest, resolvePluginSourcePath } from '../../utils/marketplace-manifest-parser.js'; import { formatSyncHeader, formatSyncSummary, formatVerboseSyncLines } from '../format-sync.js'; +import { removeInstalledSkill } from '../skill-removal.js'; import type { SyncResult } from '../../core/sync.js'; /** @@ -336,10 +333,10 @@ const removeCmd = command({ const workspacePath = isUser ? getHomeDir() : process.cwd(); // Find the skill - const matches = await findSkillByName(skill, workspacePath); + const allSkills = await getAllSkillsFromPlugins(workspacePath); + const matches = allSkills.filter((candidate) => candidate.name === skill); if (matches.length === 0) { - const allSkills = await getAllSkillsFromPlugins(workspacePath); const skillNames = [...new Set(allSkills.map((s) => s.name))].join(', '); const error = `Skill '${skill}' not found in any installed plugin.\n\nAvailable skills: ${skillNames || 'none'}`; if (isJsonMode()) { @@ -391,12 +388,12 @@ const removeCmd = command({ return; } - const skillKey = `${targetSkill.pluginName}:${skill}`; - - const result = targetSkill.pluginSkillsMode === 'allowlist' - ? isUser ? await removeUserEnabledSkill(skillKey) : await removeEnabledSkill(skillKey, workspacePath) - : isUser ? await addUserDisabledSkill(skillKey) : await addDisabledSkill(skillKey, workspacePath); - + const result = await removeInstalledSkill({ + targetSkill, + isUser, + workspacePath, + allSkills, + }); if (!result.success) { if (isJsonMode()) { jsonOutput({ success: false, command: 'skill remove', error: result.error ?? 'Unknown error' }); @@ -407,7 +404,13 @@ const removeCmd = command({ } if (!isJsonMode()) { - console.log(`\u2713 Disabled skill: ${skill} (${targetSkill.pluginName})`); + if (result.action === 'removed-plugin') { + console.log(`\u2713 Removed plugin: ${targetSkill.pluginSource}`); + } else if (result.action === 'removed-skill') { + console.log(`\u2713 Removed skill: ${skill} (${targetSkill.pluginName})`); + } else { + console.log(`\u2713 Disabled skill: ${skill} (${targetSkill.pluginName})`); + } } const syncResult = isUser ? await syncUserWorkspace() : await syncWorkspace(workspacePath); diff --git a/src/cli/skill-removal.ts b/src/cli/skill-removal.ts new file mode 100644 index 0000000..e1cedb8 --- /dev/null +++ b/src/cli/skill-removal.ts @@ -0,0 +1,55 @@ +import { addDisabledSkill, removeEnabledSkill, removePlugin } from '../core/workspace-modify.js'; +import { getAllSkillsFromPlugins, type SkillInfo } from '../core/skills.js'; +import { + addUserDisabledSkill, + removeUserEnabledSkill, + removeUserPlugin, +} from '../core/user-workspace.js'; + +export interface RemoveInstalledSkillOptions { + targetSkill: Pick; + isUser: boolean; + workspacePath: string; + allSkills?: SkillInfo[]; +} + +export interface RemoveInstalledSkillResult { + success: boolean; + error?: string; + action?: 'removed-plugin' | 'removed-skill' | 'disabled-skill'; +} + +export async function removeInstalledSkill( + options: RemoveInstalledSkillOptions, +): Promise { + const { targetSkill, isUser, workspacePath } = options; + const allSkills = options.allSkills ?? await getAllSkillsFromPlugins(workspacePath); + const pluginSkills = allSkills.filter((skill) => skill.pluginSource === targetSkill.pluginSource); + const remainingEnabledSkills = pluginSkills.filter( + (skill) => !skill.disabled && skill.name !== targetSkill.name, + ); + + if (remainingEnabledSkills.length === 0) { + const result = isUser + ? await removeUserPlugin(targetSkill.pluginSource) + : await removePlugin(targetSkill.pluginSource, workspacePath); + + return result.success + ? { success: true, action: 'removed-plugin' } + : { success: false, error: result.error ?? 'Unknown error' }; + } + + const skillKey = `${targetSkill.pluginName}:${targetSkill.name}`; + const result = targetSkill.pluginSkillsMode === 'allowlist' + ? isUser ? await removeUserEnabledSkill(skillKey) : await removeEnabledSkill(skillKey, workspacePath) + : isUser ? await addUserDisabledSkill(skillKey) : await addDisabledSkill(skillKey, workspacePath); + + if (!result.success) { + return { success: false, error: result.error ?? 'Unknown error' }; + } + + return { + success: true, + action: targetSkill.pluginSkillsMode === 'allowlist' ? 'removed-skill' : 'disabled-skill', + }; +} diff --git a/src/cli/tui/actions/plugins.ts b/src/cli/tui/actions/plugins.ts index b308ef1..ce80aa9 100644 --- a/src/cli/tui/actions/plugins.ts +++ b/src/cli/tui/actions/plugins.ts @@ -1,12 +1,10 @@ import * as p from '@clack/prompts'; -import { addPlugin, removePlugin, addDisabledSkill, removeDisabledSkill, addEnabledSkill, removeEnabledSkill, setPluginSkillsMode } from '../../../core/workspace-modify.js'; +import { addPlugin, removePlugin, removeDisabledSkill, addEnabledSkill, setPluginSkillsMode } from '../../../core/workspace-modify.js'; import { addUserPlugin, removeUserPlugin, - addUserDisabledSkill, removeUserDisabledSkill, addUserEnabledSkill, - removeUserEnabledSkill, setUserPluginSkillsMode, getInstalledUserPlugins, getInstalledProjectPlugins, @@ -32,6 +30,7 @@ import { getAllSkillsFromPlugins, discoverSkillNames } from '../../../core/skill import { getHomeDir } from '../../../constants.js'; import type { TuiContext } from '../context.js'; import type { TuiCache } from '../cache.js'; +import { removeInstalledSkill } from '../../skill-removal.js'; const { select, text, confirm, multiselect, autocomplete } = p; @@ -565,20 +564,11 @@ export async function runBrowsePluginSkills( // Disable newly unchecked skills for (const skill of toDisable) { - const skillKey = `${skill.pluginName}:${skill.name}`; - if (skill.pluginSkillsMode === 'allowlist') { - if (scope === 'user') { - await removeUserEnabledSkill(skillKey); - } else if (context.workspacePath) { - await removeEnabledSkill(skillKey, context.workspacePath); - } - } else { - if (scope === 'user') { - await addUserDisabledSkill(skillKey); - } else if (context.workspacePath) { - await addDisabledSkill(skillKey, context.workspacePath); - } - } + await removeInstalledSkill({ + targetSkill: skill, + isUser: scope === 'user', + workspacePath, + }); } // Enable newly checked skills diff --git a/src/cli/tui/actions/skills.ts b/src/cli/tui/actions/skills.ts index fbe2808..faec2b7 100644 --- a/src/cli/tui/actions/skills.ts +++ b/src/cli/tui/actions/skills.ts @@ -1,17 +1,13 @@ import * as p from '@clack/prompts'; import { getAllSkillsFromPlugins, discoverSkillNames, type SkillInfo } from '../../../core/skills.js'; import { - addDisabledSkill, removeDisabledSkill, addEnabledSkill, - removeEnabledSkill, hasPlugin, } from '../../../core/workspace-modify.js'; import { - addUserDisabledSkill, removeUserDisabledSkill, addUserEnabledSkill, - removeUserEnabledSkill, isUserConfigPath, hasUserPlugin, } from '../../../core/user-workspace.js'; @@ -25,6 +21,7 @@ import { getHomeDir } from '../../../constants.js'; import type { TuiContext } from '../context.js'; import type { TuiCache } from '../cache.js'; import { installSelectedPlugin, runBrowsePluginSkills } from './plugins.js'; +import { removeInstalledSkill } from '../../skill-removal.js'; const { multiselect, select, autocomplete, text } = p; @@ -223,18 +220,13 @@ async function runToggleSkills( // Disable newly unchecked skills for (const skill of toDisable) { - if (skill.pluginSkillsMode === 'allowlist') { - if (skill.scope === 'user') { - await removeUserEnabledSkill(skill.skillKey); - } else if (context.workspacePath) { - await removeEnabledSkill(skill.skillKey, context.workspacePath); - } - } else { - if (skill.scope === 'user') { - await addUserDisabledSkill(skill.skillKey); - } else if (context.workspacePath) { - await addDisabledSkill(skill.skillKey, context.workspacePath); - } + const effectivePath = skill.scope === 'user' ? getHomeDir() : context.workspacePath; + if (effectivePath) { + await removeInstalledSkill({ + targetSkill: skill, + isUser: skill.scope === 'user', + workspacePath: effectivePath, + }); } if (skill.scope === 'user') changedUser = true; else changedProject = true; diff --git a/tests/unit/cli/skill-removal.test.ts b/tests/unit/cli/skill-removal.test.ts new file mode 100644 index 0000000..ec8be60 --- /dev/null +++ b/tests/unit/cli/skill-removal.test.ts @@ -0,0 +1,213 @@ +import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; +import { mkdtemp, mkdir, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { load, dump } from 'js-yaml'; +import { removeInstalledSkill } from '../../../src/cli/skill-removal.js'; +import type { WorkspaceConfig } from '../../../src/models/workspace-config.js'; +import type { SkillInfo } from '../../../src/core/skills.js'; +import { resetFetchCache } from '../../../src/core/plugin.js'; + +describe('removeInstalledSkill', () => { + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), 'allagents-skill-removal-')); + await mkdir(join(tmpDir, '.allagents'), { recursive: true }); + }); + + afterEach(async () => { + resetFetchCache(); + await rm(tmpDir, { recursive: true, force: true }); + }); + + it('removes the plugin when removing its only installed skill', async () => { + const pluginDir = join(tmpDir, 'solo-plugin'); + await mkdir(pluginDir, { recursive: true }); + await writeFile( + join(pluginDir, 'SKILL.md'), + '---\nname: solo-skill\ndescription: Solo skill\n---\n# Solo skill\n', + ); + + const config: WorkspaceConfig = { + version: 2, + repositories: [], + clients: ['copilot'], + plugins: [{ source: pluginDir, skills: ['solo-skill'] }], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + + const result = await removeInstalledSkill({ + targetSkill: { + name: 'solo-skill', + pluginName: 'solo-plugin', + pluginSource: pluginDir, + pluginSkillsMode: 'allowlist', + }, + isUser: false, + workspacePath: tmpDir, + }); + + expect(result).toEqual({ success: true, action: 'removed-plugin' }); + + const content = await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'); + const updated = load(content) as WorkspaceConfig; + expect(updated.plugins).toEqual([]); + }); + + it('removes only the selected skill when the plugin exposes multiple skills', async () => { + const pluginDir = join(tmpDir, 'multi-plugin'); + await mkdir(join(pluginDir, 'skills/skill-a'), { recursive: true }); + await mkdir(join(pluginDir, 'skills/skill-b'), { recursive: true }); + await writeFile( + join(pluginDir, 'skills/skill-a/SKILL.md'), + '---\nname: skill-a\ndescription: Skill A\n---\n# Skill A\n', + ); + await writeFile( + join(pluginDir, 'skills/skill-b/SKILL.md'), + '---\nname: skill-b\ndescription: Skill B\n---\n# Skill B\n', + ); + + const config: WorkspaceConfig = { + version: 2, + repositories: [], + clients: ['copilot'], + plugins: [{ source: pluginDir, skills: ['skill-a', 'skill-b'] }], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + + const allSkills: SkillInfo[] = [ + { + name: 'skill-a', + pluginName: 'multi-plugin', + pluginSource: pluginDir, + path: join(pluginDir, 'skills/skill-a'), + disabled: false, + pluginSkillsMode: 'allowlist', + }, + { + name: 'skill-b', + pluginName: 'multi-plugin', + pluginSource: pluginDir, + path: join(pluginDir, 'skills/skill-b'), + disabled: false, + pluginSkillsMode: 'allowlist', + }, + ]; + + const result = await removeInstalledSkill({ + targetSkill: allSkills[0]!, + isUser: false, + workspacePath: tmpDir, + allSkills, + }); + + expect(result).toEqual({ success: true, action: 'removed-skill' }); + + const content = await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'); + const updated = load(content) as WorkspaceConfig; + expect(updated.plugins).toEqual([{ source: pluginDir, skills: ['skill-b'] }]); + }); + + it('removes the plugin when removing the last enabled skill from a multi-skill plugin', async () => { + const pluginDir = join(tmpDir, 'mixed-plugin'); + await mkdir(join(pluginDir, 'skills/skill-a'), { recursive: true }); + await mkdir(join(pluginDir, 'skills/skill-b'), { recursive: true }); + await writeFile( + join(pluginDir, 'skills/skill-a/SKILL.md'), + '---\nname: skill-a\ndescription: Skill A\n---\n# Skill A\n', + ); + await writeFile( + join(pluginDir, 'skills/skill-b/SKILL.md'), + '---\nname: skill-b\ndescription: Skill B\n---\n# Skill B\n', + ); + + const config: WorkspaceConfig = { + version: 2, + repositories: [], + clients: ['copilot'], + plugins: [{ source: pluginDir, skills: ['skill-a'] }], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + + const allSkills: SkillInfo[] = [ + { + name: 'skill-a', + pluginName: 'mixed-plugin', + pluginSource: pluginDir, + path: join(pluginDir, 'skills/skill-a'), + disabled: false, + pluginSkillsMode: 'allowlist', + }, + { + name: 'skill-b', + pluginName: 'mixed-plugin', + pluginSource: pluginDir, + path: join(pluginDir, 'skills/skill-b'), + disabled: true, + pluginSkillsMode: 'allowlist', + }, + ]; + + const result = await removeInstalledSkill({ + targetSkill: allSkills[0]!, + isUser: false, + workspacePath: tmpDir, + allSkills, + }); + + expect(result).toEqual({ success: true, action: 'removed-plugin' }); + + const content = await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'); + const updated = load(content) as WorkspaceConfig; + expect(updated.plugins).toEqual([]); + }); + + it('removes a single-skill GitHub source instead of leaving an empty allowlist', async () => { + const originalHome = process.env.HOME; + const fakeHome = join(tmpDir, 'home'); + const pluginDir = join( + fakeHome, + '.allagents/plugins/marketplaces/NousResearch-hermes-agent@main/skills/research/llm-wiki', + ); + await mkdir(pluginDir, { recursive: true }); + await writeFile( + join(pluginDir, 'SKILL.md'), + '---\nname: llm-wiki\ndescription: Wiki skill\n---\n# llm-wiki\n', + ); + + const source = 'https://github.com/NousResearch/hermes-agent/tree/main/skills/research/llm-wiki'; + const config: WorkspaceConfig = { + version: 2, + repositories: [], + clients: ['copilot'], + plugins: [{ source, skills: ['llm-wiki'] }], + }; + await writeFile(join(tmpDir, '.allagents/workspace.yaml'), dump(config), 'utf-8'); + + process.env.HOME = fakeHome; + resetFetchCache(); + + try { + const result = await removeInstalledSkill({ + targetSkill: { + name: 'llm-wiki', + pluginName: 'llm-wiki', + pluginSource: source, + pluginSkillsMode: 'allowlist', + }, + isUser: false, + workspacePath: tmpDir, + }); + + expect(result).toEqual({ success: true, action: 'removed-plugin' }); + + const content = await readFile(join(tmpDir, '.allagents/workspace.yaml'), 'utf-8'); + const updated = load(content) as WorkspaceConfig; + expect(updated.plugins).toEqual([]); + } finally { + process.env.HOME = originalHome; + resetFetchCache(); + } + }); +});