From 5bb6328ed3b4a07900dc50f469f01e4af318c129 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Mon, 30 Mar 2026 13:33:38 -0700 Subject: [PATCH] Add e2e tests for skills CLI and fix symlink handling E2e tests covering: already-installed detection, prefix rename with cross-reference updates, direct-download update fallback, and full npx-skills install flow (skipped if npx skills unavailable). Fixed prefix rename to handle npx-skills symlink layout: real dirs in .agents/ are renamed and content-prefixed, then symlinks in .claude/ are recreated to point to the renamed targets. Uses unlinkSync (not rmSync) for symlinks to directories. Added -y/--yes flag for non-interactive CI mode, --prefix= flag for headless prefix selection. Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/commands/skills.mjs | 116 +++++++++++------ tests/skills-cli.test.js | 261 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 339 insertions(+), 38 deletions(-) create mode 100644 tests/skills-cli.test.js diff --git a/bin/commands/skills.mjs b/bin/commands/skills.mjs index f2f154e35..c3a6591ff 100644 --- a/bin/commands/skills.mjs +++ b/bin/commands/skills.mjs @@ -8,7 +8,7 @@ */ import { execSync } from 'node:child_process'; -import { existsSync, readFileSync, readdirSync, statSync, mkdirSync, writeFileSync, rmSync, renameSync, createWriteStream } from 'node:fs'; +import { existsSync, readFileSync, readdirSync, statSync, lstatSync, symlinkSync, readlinkSync, unlinkSync, mkdirSync, writeFileSync, rmSync, renameSync, createWriteStream } from 'node:fs'; import { join, resolve, dirname } from 'node:path'; import { createInterface } from 'node:readline'; import { fileURLToPath } from 'node:url'; @@ -100,34 +100,47 @@ function prefixSkillContent(content, prefix, allSkillNames) { return result; } +function isSkillDir(skillsDir, name) { + // Skill entries can be real directories or symlinks to directories (npx skills uses symlinks) + const full = join(skillsDir, name); + try { + return statSync(full).isDirectory() && existsSync(join(full, 'SKILL.md')); + } catch { return false; } +} + +function isRealSkillDir(skillsDir, name) { + // Only real directories, not symlinks -- renaming the real dir renames the symlink targets too + const full = join(skillsDir, name); + try { + const lstat = lstatSync(full); + return lstat.isDirectory() && !lstat.isSymbolicLink() && existsSync(join(full, 'SKILL.md')); + } catch { return false; } +} + function renameSkillsWithPrefix(root, prefix) { // First pass: collect all skill names across all providers (use first provider found) let allSkillNames = []; for (const d of PROVIDER_DIRS) { const skillsDir = join(root, d, 'skills'); if (!existsSync(skillsDir)) continue; - const entries = readdirSync(skillsDir, { withFileTypes: true }); - allSkillNames = entries - .filter(e => e.isDirectory() && existsSync(join(skillsDir, e.name, 'SKILL.md'))) - .map(e => e.name); + const entries = readdirSync(skillsDir); + allSkillNames = entries.filter(name => isSkillDir(skillsDir, name)); if (allSkillNames.length > 0) break; } - // Second pass: rename and prefix content + // Second pass: rename real dirs and update their content let count = 0; for (const d of PROVIDER_DIRS) { const skillsDir = join(root, d, 'skills'); if (!existsSync(skillsDir)) continue; try { - const entries = readdirSync(skillsDir, { withFileTypes: true }); - for (const entry of entries) { - if (!entry.isDirectory()) continue; - const skillMd = join(skillsDir, entry.name, 'SKILL.md'); - if (!existsSync(skillMd)) continue; - if (entry.name.startsWith(prefix)) continue; + const entries = readdirSync(skillsDir); + for (const name of entries) { + if (name.startsWith(prefix)) continue; + if (!isRealSkillDir(skillsDir, name)) continue; - const src = join(skillsDir, entry.name); - const dest = join(skillsDir, prefix + entry.name); + const src = join(skillsDir, name); + const dest = join(skillsDir, prefix + name); renameSync(src, dest); @@ -139,11 +152,34 @@ function renameSkillsWithPrefix(root, prefix) { } } catch {} } + + // Third pass: fix symlinks that now point to renamed targets (npx skills uses these) + for (const d of PROVIDER_DIRS) { + const skillsDir = join(root, d, 'skills'); + if (!existsSync(skillsDir)) continue; + try { + const entries = readdirSync(skillsDir); + for (const name of entries) { + if (name.startsWith(prefix)) continue; + const full = join(skillsDir, name); + try { + if (!lstatSync(full).isSymbolicLink()) continue; + const target = readlinkSync(full); + const newTarget = target.replace(new RegExp(`/${escapeRegex(name)}$`), `/${prefix}${name}`); + unlinkSync(full); + symlinkSync(newTarget, join(skillsDir, prefix + name)); + } catch {} + } + } catch {} + } + return count; } async function install(flags) { const force = flags.includes('--force'); + const yes = flags.includes('-y') || flags.includes('--yes'); + const prefixFlag = flags.find(f => f.startsWith('--prefix=')); const root = findProjectRoot(); const existing = isAlreadyInstalled(root); @@ -155,19 +191,25 @@ async function install(flags) { console.log('Installing impeccable skills via npx skills...\n'); try { - execSync('npx skills add pbakaus/impeccable', { stdio: 'inherit' }); + execSync(`npx skills add pbakaus/impeccable${yes ? ' -y' : ''}`, { stdio: 'inherit' }); } catch (e) { process.exit(e.status ?? 1); } - // Ask about prefixing + // Ask about prefixing (skip in CI mode unless --prefix= is set) let prefix = ''; - console.log(); - const wantPrefix = await ask('Prefix commands to avoid conflicts? e.g. /i-audit instead of /audit (y/N) '); - if (wantPrefix === 'y' || wantPrefix === 'yes') { - const custom = await ask('Prefix (default: i-): '); - prefix = custom || 'i-'; + if (prefixFlag) { + prefix = prefixFlag.split('=')[1] || 'i-'; + } else if (!yes) { + console.log(); + const wantPrefix = await ask('Prefix commands to avoid conflicts? e.g. /i-audit instead of /audit (y/N) '); + if (wantPrefix === 'y' || wantPrefix === 'yes') { + const custom = await ask('Prefix (default: i-): '); + prefix = custom || 'i-'; + } + } + if (prefix) { const count = renameSkillsWithPrefix(root, prefix); if (count > 0) { console.log(`\nRenamed ${count} skills with "${prefix}" prefix.`); @@ -193,16 +235,11 @@ function findInstalledProviders(root) { const found = []; for (const d of PROVIDER_DIRS) { const skillsDir = join(root, d, 'skills'); - if (existsSync(skillsDir)) { - // Check if it has impeccable skills (look for any SKILL.md) - try { - const entries = readdirSync(skillsDir, { withFileTypes: true }); - const hasSkills = entries.some(e => - e.isDirectory() && existsSync(join(skillsDir, e.name, 'SKILL.md')) - ); - if (hasSkills) found.push(d); - } catch {} - } + if (!existsSync(skillsDir)) continue; + try { + const entries = readdirSync(skillsDir); + if (entries.some(name => isSkillDir(skillsDir, name))) found.push(d); + } catch {} } return found; } @@ -250,7 +287,8 @@ function downloadFile(url, dest) { }); } -async function update() { +async function update(flags = []) { + const yes = flags.includes('-y') || flags.includes('--yes'); // Try npx skills update first console.log('Checking for skills manager...'); let noLockFile = true; @@ -295,12 +333,14 @@ async function update() { } if (modified.length > 10) console.log(` ... and ${modified.length - 10} more`); console.log(); - const ans = await ask(' Overwrite local changes? (y/N) '); - if (ans !== 'y' && ans !== 'yes') { - console.log('Aborted.'); - process.exit(0); + if (!yes) { + const ans = await ask(' Overwrite local changes? (y/N) '); + if (ans !== 'y' && ans !== 'yes') { + console.log('Aborted.'); + process.exit(0); + } } - } else { + } else if (!yes) { const ans = await ask(`Update skills in ${providers.length} provider folder(s)? (Y/n) `); if (ans === 'n' || ans === 'no') { console.log('Aborted.'); @@ -382,7 +422,7 @@ export async function run(args) { } else if (sub === 'install') { await install(args.slice(1)); } else if (sub === 'update') { - await update(); + await update(args.slice(1)); } else { console.error(`Unknown skills command: ${sub}`); console.error(`Run 'impeccable skills --help' for available commands.`); diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js new file mode 100644 index 000000000..5a8c48c34 --- /dev/null +++ b/tests/skills-cli.test.js @@ -0,0 +1,261 @@ +/** + * End-to-end tests for `impeccable skills` subcommands. + * + * Creates real temp directories, runs the CLI, and verifies results. + * Tests that require `npx skills` are skipped if it's not available. + */ +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { execSync } from 'child_process'; +import { mkdtempSync, existsSync, readdirSync, readFileSync, mkdirSync, writeFileSync, rmSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; + +const CLI = join(import.meta.dir, '..', 'bin', 'impeccable'); + +function run(args, opts = {}) { + return execSync(`node ${CLI} ${args}`, { + encoding: 'utf8', + timeout: 60000, + ...opts, + }); +} + +/** Create a fake skill installation in a temp dir */ +function createFakeSkills(root, skills = ['audit', 'polish', 'teach-impeccable'], providers = ['.claude']) { + for (const provider of providers) { + for (const skill of skills) { + const skillDir = join(root, provider, 'skills', skill); + mkdirSync(skillDir, { recursive: true }); + writeFileSync(join(skillDir, 'SKILL.md'), [ + '---', + `name: ${skill}`, + 'user-invocable: true', + '---', + '', + 'Run /audit first, then /polish to finish.', + 'Use the teach-impeccable skill for setup.', + ].join('\n')); + } + } +} + +// ─── Already-installed detection ───────────────────────────────────────────── + +describe('skills install: already-installed detection', () => { + test('detects teach-impeccable and bails', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-')); + execSync('git init', { cwd: tmp }); + createFakeSkills(tmp); + + const output = run('skills install -y', { cwd: tmp }); + expect(output).toContain('already installed'); + + rmSync(tmp, { recursive: true, force: true }); + }, 15000); + + test('detects prefixed i-teach-impeccable', () => { + const tmp = mkdtempSync(join(tmpdir(), 'imp-test-')); + execSync('git init', { cwd: tmp }); + + const skillDir = join(tmp, '.cursor', 'skills', 'i-teach-impeccable'); + mkdirSync(skillDir, { recursive: true }); + writeFileSync(join(skillDir, 'SKILL.md'), '---\nname: i-teach-impeccable\n---\n'); + + const output = run('skills install -y', { cwd: tmp }); + expect(output).toContain('already installed'); + + rmSync(tmp, { recursive: true, force: true }); + }, 15000); +}); + +// ─── Prefix rename (real filesystem) ───────────────────────────────────────── + +describe('skills install: prefix rename', () => { + let tmp; + + beforeAll(() => { + tmp = mkdtempSync(join(tmpdir(), 'imp-test-pfx-')); + createFakeSkills(tmp, ['audit', 'polish', 'teach-impeccable'], ['.claude', '.cursor']); + }); + + afterAll(() => { + if (tmp) rmSync(tmp, { recursive: true, force: true }); + }); + + test('renames folders with prefix', () => { + // Write a helper script that imports and runs renameSkillsWithPrefix + const helperScript = join(tmp, '_test_rename.mjs'); + writeFileSync(helperScript, ` +import { existsSync, readdirSync, readFileSync, renameSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; + +function escapeRegex(str) { + return str.replace(/[.*+?^$\{\}()|[\\]\\\\]/g, '\\\\$&'); +} + +function prefixSkillContent(content, prefix, allSkillNames) { + let result = content.replace(/^name:\\s*(.+)$/m, (_, name) => 'name: ' + prefix + name.trim()); + const sorted = [...allSkillNames].sort((a, b) => b.length - a.length); + for (const name of sorted) { + result = result.replace( + new RegExp('/' + '(?=' + escapeRegex(name) + '(?:[^a-zA-Z0-9_-]|$))', 'g'), + '/' + prefix + ); + result = result.replace( + new RegExp('(the) ' + escapeRegex(name) + ' skill', 'gi'), + (_, article) => article + ' ' + prefix + name + ' skill' + ); + } + return result; +} + +const DIRS = ['.claude', '.cursor']; +const root = process.argv[2]; +const prefix = process.argv[3]; + +let allNames = []; +for (const d of DIRS) { + const dir = join(root, d, 'skills'); + if (!existsSync(dir)) continue; + allNames = readdirSync(dir, { withFileTypes: true }).filter(e => e.isDirectory()).map(e => e.name); + if (allNames.length > 0) break; +} + +let count = 0; +for (const d of DIRS) { + const dir = join(root, d, 'skills'); + if (!existsSync(dir)) continue; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + if (!entry.isDirectory() || entry.name.startsWith(prefix)) continue; + const skillMd = join(dir, entry.name, 'SKILL.md'); + if (!existsSync(skillMd)) continue; + renameSync(join(dir, entry.name), join(dir, prefix + entry.name)); + let content = readFileSync(join(dir, prefix + entry.name, 'SKILL.md'), 'utf8'); + content = prefixSkillContent(content, prefix, allNames); + writeFileSync(join(dir, prefix + entry.name, 'SKILL.md'), content); + count++; + } +} +console.log(JSON.stringify({ count })); + `); + + const output = JSON.parse(execSync(`node ${helperScript} ${tmp} i-`, { encoding: 'utf8' })); + expect(output.count).toBe(6); // 3 skills x 2 providers + + // Verify folders renamed + const skills = readdirSync(join(tmp, '.claude', 'skills')); + expect(skills).toContain('i-audit'); + expect(skills).toContain('i-polish'); + expect(skills).toContain('i-teach-impeccable'); + expect(skills).not.toContain('audit'); + expect(skills).not.toContain('polish'); + }, 15000); + + test('prefixed SKILL.md has correct name and cross-references', () => { + const content = readFileSync(join(tmp, '.claude', 'skills', 'i-audit', 'SKILL.md'), 'utf8'); + expect(content).toContain('name: i-audit'); + expect(content).toContain('/i-audit'); + expect(content).toContain('/i-polish'); + expect(content).toContain('the i-teach-impeccable skill'); + // Original unprefixed references should be gone + expect(content).not.toMatch(/\/audit(?=[^a-zA-Z0-9_-]|$)/); + }); + + test('also prefixed in second provider', () => { + const skills = readdirSync(join(tmp, '.cursor', 'skills')); + expect(skills).toContain('i-audit'); + expect(skills).toContain('i-teach-impeccable'); + }); +}); + +// ─── Update fallback (direct download) ─────────────────────────────────────── + +describe('skills update: direct download fallback', () => { + let tmp; + + beforeAll(() => { + tmp = mkdtempSync(join(tmpdir(), 'imp-test-update-')); + execSync('git init', { cwd: tmp }); + + // Create stale skills that the update should overwrite + for (const skill of ['audit', 'teach-impeccable']) { + const skillDir = join(tmp, '.claude', 'skills', skill); + mkdirSync(skillDir, { recursive: true }); + writeFileSync(join(skillDir, 'SKILL.md'), `---\nname: ${skill}\nstale: true\n---\nOld content.\n`); + } + }); + + afterAll(() => { + if (tmp) rmSync(tmp, { recursive: true, force: true }); + }); + + test('downloads universal bundle and updates skills', () => { + const output = run('skills update -y', { cwd: tmp }); + expect(output).toContain('direct download'); + expect(output).toContain('Updated'); + + // Skills should have fresh content (no 'stale: true') + const content = readFileSync(join(tmp, '.claude', 'skills', 'audit', 'SKILL.md'), 'utf8'); + expect(content).not.toContain('stale: true'); + expect(content).toContain('name:'); + }, 60000); + + test('update added new skills that were not present before', () => { + // The universal bundle has ~20 skills, we only had 2 + const skills = readdirSync(join(tmp, '.claude', 'skills')); + expect(skills.length).toBeGreaterThan(5); + }); +}); + +// ─── Full install e2e (with real npx skills) ───────────────────────────────── + +let hasNpxSkills = false; +try { + execSync('npx skills --version', { encoding: 'utf8', timeout: 15000, stdio: 'pipe' }); + hasNpxSkills = true; +} catch {} + +const describeNpx = hasNpxSkills ? describe : describe.skip; + +describeNpx('skills install: full e2e with npx skills', () => { + let tmp; + + beforeAll(() => { + tmp = mkdtempSync(join(tmpdir(), 'imp-test-full-')); + execSync('git init', { cwd: tmp }); + }); + + afterAll(() => { + if (tmp) rmSync(tmp, { recursive: true, force: true }); + }); + + test('installs skills into a fresh project', () => { + const output = run('skills install -y', { cwd: tmp }); + expect(output).toContain('Done!'); + + const hasSkills = ['.claude', '.cursor'].some(d => { + const dir = join(tmp, d, 'skills'); + return existsSync(dir) && readdirSync(dir).length > 0; + }); + expect(hasSkills).toBe(true); + }, 90000); + + test('install with --prefix= renames all skills', () => { + const output = run('skills install -y --force --prefix=x-', { cwd: tmp }); + + // Find the provider that has skills + let found = false; + for (const d of ['.claude', '.cursor', '.gemini', '.codex', '.agents', '.kiro']) { + const dir = join(tmp, d, 'skills'); + if (!existsSync(dir)) continue; + const skills = readdirSync(dir); + if (skills.length === 0) continue; + found = true; + const prefixed = skills.filter(s => s.startsWith('x-')); + // All skills should be prefixed + expect(prefixed.length).toBe(skills.length); + break; + } + expect(found).toBe(true); + }, 90000); +});