From 5a12fb963209a111cb22e5270a0f0e4d2a26d7b7 Mon Sep 17 00:00:00 2001 From: Ankit Ranjan Date: Mon, 7 Sep 2026 17:57:42 +0530 Subject: [PATCH] fix: scope skills remove to one provider/scope, like skills add webcmd skills remove previously accepted no --provider/--scope and never prompted: it blind-scanned every provider (agents/codex/claude) and scope (user/project) combination plus the internal stable-link root, removing any matching symlink it found anywhere. Passing an agent name like `codex` had no effect since there was no such flag. skills remove now mirrors skills add: it resolves one provider+scope (or --path) via the same destinationFor() helper, interactively prompting with the same two questions as add when stdin is a TTY and no flags are given. It touches only that single destination per skill and leaves the ~/.webcmd/skills stable links (add/update's concern) alone. Fixes #490. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_013KuPnjTXhrBoY2KtPV2R89 --- src/cli.test.ts | 32 ++++++++++++++++++++++++++ src/cli.ts | 53 ++++++++++++++++++++++++++++++++++--------- src/skills.test.ts | 56 +++++++++++++++++++++++++++++++++++----------- src/skills.ts | 32 ++++++++++---------------- 4 files changed, 129 insertions(+), 44 deletions(-) diff --git a/src/cli.test.ts b/src/cli.test.ts index 9cf8e244..039cfd5b 100644 --- a/src/cli.test.ts +++ b/src/cli.test.ts @@ -2080,6 +2080,38 @@ describe('structured output for data-returning built-ins', () => { expect(JSON.parse(stdout())).toEqual(bare); }); + it('removes skills for --provider and --scope without prompting', async () => { + const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'webcmd-skills-project-')); + const previousCwd = process.cwd(); + process.chdir(projectDir); + try { + await createProgram('', '').parseAsync(['node', 'webcmd', 'skills', 'add', '--provider', 'codex', '--scope', 'project', '--json']); + const added = JSON.parse(stdout()) as { skills: Array<{ destination: string }> }; + consoleLogSpy.mockClear(); + + await createProgram('', '').parseAsync(['node', 'webcmd', 'skills', 'remove', '--provider', 'codex', '--scope', 'project', '--json']); + const removed = JSON.parse(stdout()); + + expect(removed).toMatchObject({ provider: 'codex', scope: 'project' }); + expect(removed.removed).toEqual(added.skills.map((skill) => skill.destination)); + for (const linkPath of removed.removed) { + expect(() => fs.lstatSync(linkPath)).toThrow(); + } + } finally { + process.chdir(previousCwd); + fs.rmSync(projectDir, { recursive: true, force: true }); + } + }); + + it('rejects skills remove --provider custom without --path as a JSON error', async () => { + const stderr = await captureStderr(async () => { + await createProgram('', '').parseAsync(['node', 'webcmd', 'skills', 'remove', '--provider', 'custom', '--json']); + }); + + expect(process.exitCode).toBe(EXIT_CODES.USAGE_ERROR); + expect(stderr).toContain('Custom skill provider requires --path.'); + }); + it('renders daemon status as JSON', async () => { vi.mocked(fetch).mockResolvedValue(daemonStatusResponse()); diff --git a/src/cli.ts b/src/cli.ts index 3710cd79..8b01a936 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -25,7 +25,7 @@ import { handleProgramParseError } from './cli-error-report.js'; import { PKG_VERSION } from './version.js'; import { printCompletionScript } from './completion.js'; import { loadExternalClis, executeExternalCli, installExternalCli, registerExternalCli, isBinaryInstalled, formatExternalCliLabel } from './external.js'; -import { addWebcmdSkills, listWebcmdSkills, removeWebcmdSkills, updateWebcmdSkill, type WebcmdSkillAddResult } from './skills.js'; +import { addWebcmdSkills, listWebcmdSkills, removeWebcmdSkills, updateWebcmdSkill, type WebcmdSkillAddResult, type WebcmdSkillRemoveResult } from './skills.js'; import { registerAllCommands } from './commanderAdapter.js'; import { buildRootHelpPresentation, classifyAdapter, commanderCommandHelpData, installCommanderNamespaceStructuredHelp, installRootPresentationHelp, installStructuredHelp, leadingPositionalFromUsage, rootHelpData, type RootAdapterGroups } from './help.js'; import { EXIT_CODES, getErrorMessage, BrowserConnectError, CliError, ArgumentError } from './errors.js'; @@ -130,20 +130,23 @@ type SkillLinkCommandOptions = { json?: boolean; }; -function isInteractiveSkillAdd(opts: SkillLinkCommandOptions): boolean { +function isInteractiveSkillCommand(opts: SkillLinkCommandOptions): boolean { return !opts.json && process.stdin.isTTY === true && process.stdout.isTTY === true; } -async function resolveSkillAddOptions(opts: SkillLinkCommandOptions): Promise { - if (!isInteractiveSkillAdd(opts)) return opts; +async function resolveSkillCommandOptions( + opts: SkillLinkCommandOptions, + questions: { scope: string; provider: string }, +): Promise { + if (!isInteractiveSkillCommand(opts)) return opts; const rl = readline.createInterface({ input: process.stdin, output: process.stdout }); try { - const scope = opts.scope ?? await choosePrompt(rl, 'Where should Webcmd add skills?', [ + const scope = opts.scope ?? await choosePrompt(rl, questions.scope, [ { key: '1', label: 'Global', value: 'user', aliases: ['global', 'user', 'g'] }, { key: '2', label: 'Local project', value: 'project', aliases: ['local', 'project', 'l'] }, ], '1'); - const provider = opts.provider ?? (opts.path ? undefined : await choosePrompt(rl, 'Which coding agent should use them?', [ + const provider = opts.provider ?? (opts.path ? undefined : await choosePrompt(rl, questions.provider, [ { key: '1', label: 'Agents', value: 'agents', aliases: ['agents', 'agent', 'a'] }, { key: '2', label: 'Codex', value: 'codex', aliases: ['codex', 'c'] }, { key: '3', label: 'Claude', value: 'claude', aliases: ['claude', 'claude-code'] }, @@ -156,6 +159,20 @@ async function resolveSkillAddOptions(opts: SkillLinkCommandOptions): Promise { + return resolveSkillCommandOptions(opts, { + scope: 'Where should Webcmd add skills?', + provider: 'Which coding agent should use them?', + }); +} + +async function resolveSkillRemoveOptions(opts: SkillLinkCommandOptions): Promise { + return resolveSkillCommandOptions(opts, { + scope: 'Where should Webcmd remove skills from?', + provider: "Which coding agent's skills should be removed?", + }); +} + async function choosePrompt( rl: readline.Interface, question: string, @@ -203,9 +220,9 @@ function wantsJsonEnvelope(opts: { json?: boolean; format?: string }): boolean { return opts.json === true || opts.format === 'json'; } -function handleSkillRemoveCommand(customPath: string | undefined, json: boolean): void { +async function handleSkillRemoveCommand(action: () => WebcmdSkillRemoveResult | Promise, json: boolean): Promise { try { - const result = removeWebcmdSkills({ customPath }); + const result = await action(); if (json) { console.log(JSON.stringify(result, null, 2)); return; @@ -711,10 +728,24 @@ export function createProgram(BUILTIN_CLIS: string, USER_CLIS: string, pluginsDi skillsCmd .command('remove') - .description('Remove bundled Webcmd skill symlinks from supported locations') - .option('--path ', 'Also remove links from a custom agent skills directory') + .description('Remove bundled Webcmd skill symlinks from an agent skills folder') + .option('-p, --provider ', 'Agent provider: agents, codex, claude') + .option('-s, --scope ', 'Remove scope: user/global or project/local') + .option('--path ', 'Custom agent skills directory') .option('--json', 'Output a JSON envelope', false) - .action((opts) => handleSkillRemoveCommand(opts.path, wantsJsonEnvelope(opts))); + .action(async (opts) => { + await handleSkillRemoveCommand(async () => { + const resolved = await resolveSkillRemoveOptions(opts); + if (resolved.provider === 'custom' && !resolved.path) { + throw new ArgumentError('Custom skill provider requires --path.', 'Pass --path or run interactively.'); + } + return removeWebcmdSkills({ + provider: resolved.provider, + scope: resolved.scope, + customPath: resolved.path, + }); + }, wantsJsonEnvelope(opts)); + }); program .command('update') diff --git a/src/skills.test.ts b/src/skills.test.ts index 89e29307..35898305 100644 --- a/src/skills.test.ts +++ b/src/skills.test.ts @@ -451,25 +451,55 @@ describe('webcmd skills content', () => { expect(() => updateWebcmdSkill({ packageRoot, homeDir })).toThrow(ArgumentError); }); - it('removes bundled skill links from every supported location', () => { + it('removes bundled skill links from one provider and scope at a time', () => { const packageRoot = makePackageRoot(); const homeDir = fs.mkdtempSync(path.join(os.tmpdir(), 'webcmd-home-')); const cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'webcmd-project-')); const customPath = fs.mkdtempSync(path.join(os.tmpdir(), 'webcmd-custom-skills-')); + const destinations = { + agentsUser: path.join(homeDir, '.agents', 'skills', 'webcmd-browser'), + agentsProject: path.join(cwd, '.agents', 'skills', 'webcmd-browser'), + codexUser: path.join(homeDir, '.codex', 'skills', 'webcmd-browser'), + custom: path.join(customPath, 'webcmd-browser'), + stable: path.join(homeDir, '.webcmd', 'skills', 'webcmd-browser'), + }; - for (const provider of ['agents', 'codex', 'claude']) { - addWebcmdSkills({ packageRoot, homeDir, cwd, provider, scope: 'user' }); - addWebcmdSkills({ packageRoot, homeDir, cwd, provider, scope: 'project' }); - } + addWebcmdSkills({ packageRoot, homeDir, cwd, provider: 'agents', scope: 'user' }); + addWebcmdSkills({ packageRoot, homeDir, cwd, provider: 'agents', scope: 'project' }); + addWebcmdSkills({ packageRoot, homeDir, cwd, provider: 'codex', scope: 'user' }); addWebcmdSkills({ packageRoot, homeDir, cwd, customPath }); - const result = removeWebcmdSkills({ packageRoot, homeDir, cwd, customPath }); - - expect(result.removed).toHaveLength(8); - for (const linkPath of result.removed) { - expect(() => fs.lstatSync(linkPath)).toThrow(); - } - expect(removeWebcmdSkills({ packageRoot, homeDir, cwd, customPath })).toEqual({ removed: [] }); + const removedUser = removeWebcmdSkills({ packageRoot, homeDir, cwd, provider: 'agents', scope: 'user' }); + expect(removedUser).toMatchObject({ provider: 'agents', scope: 'user' }); + expect(removedUser.removed).toEqual([destinations.agentsUser]); + expect(() => fs.lstatSync(destinations.agentsUser)).toThrow(); + expect(fs.lstatSync(destinations.agentsProject).isSymbolicLink()).toBe(true); + expect(fs.lstatSync(destinations.codexUser).isSymbolicLink()).toBe(true); + expect(fs.lstatSync(destinations.custom).isSymbolicLink()).toBe(true); + expect(fs.lstatSync(destinations.stable).isSymbolicLink()).toBe(true); + + const removedProject = removeWebcmdSkills({ packageRoot, homeDir, cwd, provider: 'agents', scope: 'project' }); + expect(removedProject).toEqual({ + provider: 'agents', + scope: 'project', + removed: [destinations.agentsProject], + }); + expect(fs.lstatSync(destinations.codexUser).isSymbolicLink()).toBe(true); + expect(fs.lstatSync(destinations.custom).isSymbolicLink()).toBe(true); + expect(fs.lstatSync(destinations.stable).isSymbolicLink()).toBe(true); + + const removedCustom = removeWebcmdSkills({ packageRoot, homeDir, cwd, customPath }); + expect(removedCustom.provider).toBeUndefined(); + expect(removedCustom.scope).toBe('user'); + expect(removedCustom.removed).toEqual([destinations.custom]); + expect(fs.lstatSync(destinations.codexUser).isSymbolicLink()).toBe(true); + expect(fs.lstatSync(destinations.stable).isSymbolicLink()).toBe(true); + + expect(removeWebcmdSkills({ packageRoot, homeDir, cwd, provider: 'agents', scope: 'user' })).toEqual({ + provider: 'agents', + scope: 'user', + removed: [], + }); }); it('refuses removal before deleting any links when a destination is not a symlink', () => { @@ -480,7 +510,7 @@ describe('webcmd skills content', () => { const blocker = path.join(cwd, '.codex', 'skills', 'webcmd-browser'); fs.mkdirSync(blocker, { recursive: true }); - expect(() => removeWebcmdSkills({ packageRoot, homeDir, cwd })).toThrow(ArgumentError); + expect(() => removeWebcmdSkills({ packageRoot, homeDir, cwd, provider: 'codex', scope: 'project' })).toThrow(ArgumentError); expect(fs.lstatSync(added.skills[0].destination!).isSymbolicLink()).toBe(true); expect(fs.lstatSync(blocker).isDirectory()).toBe(true); }); diff --git a/src/skills.ts b/src/skills.ts index 5ea61686..96d33701 100644 --- a/src/skills.ts +++ b/src/skills.ts @@ -38,6 +38,8 @@ export interface WebcmdSkillAddResult { } export interface WebcmdSkillRemoveResult { + provider?: SkillProvider; + scope?: SkillScope; removed: string[]; } @@ -95,33 +97,23 @@ export function updateWebcmdSkill(options: WebcmdSkillOptions = {}): WebcmdSkill } export function removeWebcmdSkills(options: WebcmdSkillOptions = {}): WebcmdSkillRemoveResult { - const homeDir = options.homeDir ?? os.homedir(); - const cwd = options.cwd ?? process.cwd(); - const roots = new Set([ - ...['.agents', '.codex', '.claude'].flatMap((dir) => [ - path.join(homeDir, dir, 'skills'), - path.join(cwd, dir, 'skills'), - ]), - ...(options.customPath === undefined ? [] : [expandHomePath(options.customPath)]), - path.join(homeDir, '.webcmd', 'skills'), - ]); + const provider = options.customPath === undefined ? normalizeProvider(options.provider) : undefined; + const scope = normalizeScope(options.scope); const skills = listWebcmdSkills(options.packageRoot); const removed: string[] = []; - for (const root of roots) { - for (const skill of skills) { - const linkPath = path.join(root, skill.name); - const current = safeLstat(linkPath); - if (!current) continue; - if (!current.isSymbolicLink()) { - throw new ArgumentError(`Refusing to remove non-symlink path: ${linkPath}`, 'Remove it manually if it is no longer needed.'); - } - removed.push(linkPath); + for (const skill of skills) { + const linkPath = destinationFor(skill.name, provider, scope, options); + const current = safeLstat(linkPath); + if (!current) continue; + if (!current.isSymbolicLink()) { + throw new ArgumentError(`Refusing to remove non-symlink path: ${linkPath}`, 'Remove it manually if it is no longer needed.'); } + removed.push(linkPath); } for (const linkPath of removed) fs.unlinkSync(linkPath); - return { removed }; + return { provider, scope, removed }; } function updateStableSkillLinks(options: WebcmdSkillOptions): WebcmdSkillLink[] {