mirror of
https://github.com/garrytan/gstack.git
synced 2026-05-01 19:25:10 +02:00
fix: top-level skill dirs so Claude discovers unprefixed names (#761)
* fix: top-level skill dirs so Claude discovers unprefixed names Replace directory symlinks (gstack/qa → qa) with real directories containing a SKILL.md symlink. Claude Code auto-prefixes skills nested under a parent dir symlink, so /plan-ceo-review became "Unknown skill" even with skill_prefix=false. Real dirs fix this. Also syncs package.json version to match VERSION file and updates test assertions to match the new mkdir + ln approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: update symlink references to new top-level directory pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: regression tests for top-level skill directory structure Verifies the invariant that setup/relink creates real directories (not symlinks) at the top level, with SKILL.md symlinks inside. This prevents Claude Code from auto-prefixing skills with gstack- when using --no-prefix. Tests added: - unprefixed skills must be real dirs with SKILL.md symlinks - prefixed skills must also be real dirs with SKILL.md symlinks - old directory symlinks get upgraded to real directories - cleanup functions handle both old symlinks and new dir pattern - link function removes old directory symlinks before mkdir Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: namespace isolation tests for first install + mode switching Verifies the core invariant: when you pick a prefix mode, ONLY that mode's entries exist. Zero pollution from the other mode. - first install --no-prefix: only flat names, zero gstack-* leaks - first install --prefix: only gstack-* names, zero flat leaks - non-TTY defaults to flat names - switching prefix→no-prefix removes ALL gstack-* entries - switching no-prefix→prefix removes ALL flat entries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: upgrade migration system — versioned fix scripts for broken state Adds gstack-upgrade/migrations/ directory with version-keyed bash scripts that run automatically during /gstack-upgrade (Step 4.75, after ./setup). Each script is idempotent and handles state fixes that setup alone can't cover. First migration: v0.15.2.0.sh runs gstack-relink to fix stale directory symlinks from pre-v0.15.2.0 installs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: migration script validation + v0.15.2.0 end-to-end fix test Tests that migration scripts are executable, parse without syntax errors, follow the v{VERSION}.sh naming convention, and that v0.15.2.0 actually fixes stale directory symlinks by converting them to real directories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: upgrade migration guide in CONTRIBUTING.md + CLAUDE.md pointer CONTRIBUTING.md: new "Upgrade migrations" section documenting when and how to add migration scripts for broken on-disk state. CLAUDE.md: added note under vendored symlink awareness pointing to CONTRIBUTING.md's migration section when worried about broken installs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
@@ -1969,13 +1969,43 @@ describe('setup script validation', () => {
|
||||
expect(fnBody).toContain('gstack*');
|
||||
});
|
||||
|
||||
test('link_claude_skill_dirs creates relative symlinks', () => {
|
||||
// Claude links should be relative: ln -snf "gstack/$dir_name"
|
||||
// Uses dir_name (not skill_name) because symlink target must point to the physical directory
|
||||
test('link_claude_skill_dirs creates real directories with absolute SKILL.md symlinks', () => {
|
||||
// Claude links should be real directories with absolute SKILL.md symlinks
|
||||
// to ensure Claude Code discovers them as top-level skills (not nested under gstack/)
|
||||
const fnStart = setupContent.indexOf('link_claude_skill_dirs()');
|
||||
const fnEnd = setupContent.indexOf('}', setupContent.indexOf('linked[@]}', fnStart));
|
||||
const fnBody = setupContent.slice(fnStart, fnEnd);
|
||||
expect(fnBody).toContain('ln -snf "gstack/$dir_name"');
|
||||
expect(fnBody).toContain('mkdir -p "$target"');
|
||||
expect(fnBody).toContain('ln -snf "$gstack_dir/$dir_name/SKILL.md" "$target/SKILL.md"');
|
||||
});
|
||||
|
||||
// REGRESSION: cleanup functions must handle both old symlinks AND new real-directory pattern
|
||||
test('cleanup functions handle real directories with symlinked SKILL.md', () => {
|
||||
// cleanup_old_claude_symlinks must detect and remove real dirs with SKILL.md symlinks
|
||||
const cleanupOldStart = setupContent.indexOf('cleanup_old_claude_symlinks()');
|
||||
const cleanupOldEnd = setupContent.indexOf('}', setupContent.indexOf('cleaned up old', cleanupOldStart));
|
||||
const cleanupOldBody = setupContent.slice(cleanupOldStart, cleanupOldEnd);
|
||||
expect(cleanupOldBody).toContain('-d "$old_target"');
|
||||
expect(cleanupOldBody).toContain('-L "$old_target/SKILL.md"');
|
||||
expect(cleanupOldBody).toContain('rm -rf "$old_target"');
|
||||
|
||||
// cleanup_prefixed_claude_symlinks must also handle the new pattern
|
||||
const cleanupPrefixedStart = setupContent.indexOf('cleanup_prefixed_claude_symlinks()');
|
||||
const cleanupPrefixedEnd = setupContent.indexOf('}', setupContent.indexOf('cleaned up prefixed', cleanupPrefixedStart));
|
||||
const cleanupPrefixedBody = setupContent.slice(cleanupPrefixedStart, cleanupPrefixedEnd);
|
||||
expect(cleanupPrefixedBody).toContain('-d "$prefixed_target"');
|
||||
expect(cleanupPrefixedBody).toContain('-L "$prefixed_target/SKILL.md"');
|
||||
expect(cleanupPrefixedBody).toContain('rm -rf "$prefixed_target"');
|
||||
});
|
||||
|
||||
// REGRESSION: link function must upgrade old directory symlinks
|
||||
test('link_claude_skill_dirs removes old directory symlinks before creating real dirs', () => {
|
||||
const fnStart = setupContent.indexOf('link_claude_skill_dirs()');
|
||||
const fnEnd = setupContent.indexOf('}', setupContent.indexOf('linked[@]}', fnStart));
|
||||
const fnBody = setupContent.slice(fnStart, fnEnd);
|
||||
// Must check for and remove old symlinks before mkdir
|
||||
expect(fnBody).toContain('if [ -L "$target" ]');
|
||||
expect(fnBody).toContain('rm -f "$target"');
|
||||
});
|
||||
|
||||
test('setup supports --host auto|claude|codex|kiro', () => {
|
||||
|
||||
@@ -97,6 +97,173 @@ describe('gstack-relink (#578)', () => {
|
||||
expect(output).toContain('flat');
|
||||
});
|
||||
|
||||
// REGRESSION: unprefixed skills must be real directories, not symlinks (#761)
|
||||
// Claude Code auto-prefixes skills nested under a parent dir symlink.
|
||||
// e.g., `qa -> gstack/qa` gets discovered as "gstack-qa", not "qa".
|
||||
// The fix: create real directories with SKILL.md symlinks inside.
|
||||
test('unprefixed skills are real directories with SKILL.md symlinks, not dir symlinks', () => {
|
||||
setupMockInstall(['qa', 'ship', 'review', 'plan-ceo-review']);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
for (const skill of ['qa', 'ship', 'review', 'plan-ceo-review']) {
|
||||
const skillPath = path.join(skillsDir, skill);
|
||||
const skillMdPath = path.join(skillPath, 'SKILL.md');
|
||||
// Must be a real directory, NOT a symlink
|
||||
expect(fs.lstatSync(skillPath).isDirectory()).toBe(true);
|
||||
expect(fs.lstatSync(skillPath).isSymbolicLink()).toBe(false);
|
||||
// Must contain a SKILL.md that IS a symlink
|
||||
expect(fs.existsSync(skillMdPath)).toBe(true);
|
||||
expect(fs.lstatSync(skillMdPath).isSymbolicLink()).toBe(true);
|
||||
// The SKILL.md symlink must point to the source skill's SKILL.md
|
||||
const target = fs.readlinkSync(skillMdPath);
|
||||
expect(target).toContain(skill);
|
||||
expect(target).toEndWith('/SKILL.md');
|
||||
}
|
||||
});
|
||||
|
||||
// Same invariant for prefixed mode
|
||||
test('prefixed skills are real directories with SKILL.md symlinks, not dir symlinks', () => {
|
||||
setupMockInstall(['qa', 'ship']);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
for (const skill of ['gstack-qa', 'gstack-ship']) {
|
||||
const skillPath = path.join(skillsDir, skill);
|
||||
const skillMdPath = path.join(skillPath, 'SKILL.md');
|
||||
expect(fs.lstatSync(skillPath).isDirectory()).toBe(true);
|
||||
expect(fs.lstatSync(skillPath).isSymbolicLink()).toBe(false);
|
||||
expect(fs.lstatSync(skillMdPath).isSymbolicLink()).toBe(true);
|
||||
}
|
||||
});
|
||||
|
||||
// Upgrade: old directory symlinks get replaced with real directories
|
||||
test('upgrades old directory symlinks to real directories', () => {
|
||||
setupMockInstall(['qa', 'ship']);
|
||||
// Simulate old behavior: create directory symlinks (the old pattern)
|
||||
fs.symlinkSync(path.join(installDir, 'qa'), path.join(skillsDir, 'qa'));
|
||||
fs.symlinkSync(path.join(installDir, 'ship'), path.join(skillsDir, 'ship'));
|
||||
// Verify they start as symlinks
|
||||
expect(fs.lstatSync(path.join(skillsDir, 'qa')).isSymbolicLink()).toBe(true);
|
||||
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
|
||||
// After relink: must be real directories, not symlinks
|
||||
expect(fs.lstatSync(path.join(skillsDir, 'qa')).isSymbolicLink()).toBe(false);
|
||||
expect(fs.lstatSync(path.join(skillsDir, 'qa')).isDirectory()).toBe(true);
|
||||
expect(fs.lstatSync(path.join(skillsDir, 'qa', 'SKILL.md')).isSymbolicLink()).toBe(true);
|
||||
});
|
||||
|
||||
// FIRST INSTALL: --no-prefix must create ONLY flat names, zero gstack-* pollution
|
||||
test('first install --no-prefix: only flat names exist, zero gstack-* entries', () => {
|
||||
setupMockInstall(['qa', 'ship', 'review', 'plan-ceo-review', 'gstack-upgrade']);
|
||||
// Simulate first install: no saved config, pass --no-prefix equivalent
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
// Enumerate everything in skills dir
|
||||
const entries = fs.readdirSync(skillsDir);
|
||||
// Expected: qa, ship, review, plan-ceo-review, gstack-upgrade (its real name)
|
||||
expect(entries.sort()).toEqual(['gstack-upgrade', 'plan-ceo-review', 'qa', 'review', 'ship']);
|
||||
// No gstack-qa, gstack-ship, gstack-review, gstack-plan-ceo-review
|
||||
const leaked = entries.filter(e => e.startsWith('gstack-') && e !== 'gstack-upgrade');
|
||||
expect(leaked).toEqual([]);
|
||||
});
|
||||
|
||||
// FIRST INSTALL: --prefix must create ONLY gstack-* names, zero flat-name pollution
|
||||
test('first install --prefix: only gstack-* entries exist, zero flat names', () => {
|
||||
setupMockInstall(['qa', 'ship', 'review', 'plan-ceo-review', 'gstack-upgrade']);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
const entries = fs.readdirSync(skillsDir);
|
||||
// Expected: gstack-qa, gstack-ship, gstack-review, gstack-plan-ceo-review, gstack-upgrade
|
||||
expect(entries.sort()).toEqual([
|
||||
'gstack-plan-ceo-review', 'gstack-qa', 'gstack-review', 'gstack-ship', 'gstack-upgrade',
|
||||
]);
|
||||
// No unprefixed qa, ship, review, plan-ceo-review
|
||||
const leaked = entries.filter(e => !e.startsWith('gstack-'));
|
||||
expect(leaked).toEqual([]);
|
||||
});
|
||||
|
||||
// FIRST INSTALL: non-TTY (no saved config, piped stdin) defaults to flat names
|
||||
test('non-TTY first install defaults to flat names via relink', () => {
|
||||
setupMockInstall(['qa', 'ship']);
|
||||
// Don't set any config — simulate fresh install
|
||||
// gstack-relink reads config; on fresh install config returns empty → defaults to false
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
const entries = fs.readdirSync(skillsDir);
|
||||
// Should be flat names (relink defaults to false when config returns empty)
|
||||
expect(entries.sort()).toEqual(['qa', 'ship']);
|
||||
});
|
||||
|
||||
// SWITCH: prefix → no-prefix must clean up ALL gstack-* entries
|
||||
test('switching prefix to no-prefix removes all gstack-* entries completely', () => {
|
||||
setupMockInstall(['qa', 'ship', 'review', 'plan-ceo-review', 'gstack-upgrade']);
|
||||
// Start in prefix mode
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
let entries = fs.readdirSync(skillsDir);
|
||||
expect(entries.filter(e => !e.startsWith('gstack-'))).toEqual([]);
|
||||
|
||||
// Switch to no-prefix
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
entries = fs.readdirSync(skillsDir);
|
||||
// Only flat names + gstack-upgrade (its real name)
|
||||
expect(entries.sort()).toEqual(['gstack-upgrade', 'plan-ceo-review', 'qa', 'review', 'ship']);
|
||||
const leaked = entries.filter(e => e.startsWith('gstack-') && e !== 'gstack-upgrade');
|
||||
expect(leaked).toEqual([]);
|
||||
});
|
||||
|
||||
// SWITCH: no-prefix → prefix must clean up ALL flat entries
|
||||
test('switching no-prefix to prefix removes all flat entries completely', () => {
|
||||
setupMockInstall(['qa', 'ship', 'review', 'gstack-upgrade']);
|
||||
// Start in no-prefix mode
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
let entries = fs.readdirSync(skillsDir);
|
||||
expect(entries.filter(e => e.startsWith('gstack-') && e !== 'gstack-upgrade')).toEqual([]);
|
||||
|
||||
// Switch to prefix
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`);
|
||||
run(`${path.join(installDir, 'bin', 'gstack-relink')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
entries = fs.readdirSync(skillsDir);
|
||||
// Only gstack-* names
|
||||
expect(entries.sort()).toEqual([
|
||||
'gstack-qa', 'gstack-review', 'gstack-ship', 'gstack-upgrade',
|
||||
]);
|
||||
const leaked = entries.filter(e => !e.startsWith('gstack-'));
|
||||
expect(leaked).toEqual([]);
|
||||
});
|
||||
|
||||
// Test 13: cleans stale symlinks from opposite mode
|
||||
test('cleans up stale symlinks from opposite mode', () => {
|
||||
setupMockInstall(['qa', 'ship']);
|
||||
@@ -158,6 +325,66 @@ describe('gstack-relink (#578)', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('upgrade migrations', () => {
|
||||
const MIGRATIONS_DIR = path.join(ROOT, 'gstack-upgrade', 'migrations');
|
||||
|
||||
test('migrations directory exists', () => {
|
||||
expect(fs.existsSync(MIGRATIONS_DIR)).toBe(true);
|
||||
});
|
||||
|
||||
test('all migration scripts are executable and parse without syntax errors', () => {
|
||||
const scripts = fs.readdirSync(MIGRATIONS_DIR).filter(f => f.endsWith('.sh'));
|
||||
expect(scripts.length).toBeGreaterThan(0);
|
||||
for (const script of scripts) {
|
||||
const fullPath = path.join(MIGRATIONS_DIR, script);
|
||||
// Must be executable
|
||||
const stat = fs.statSync(fullPath);
|
||||
expect(stat.mode & 0o111).toBeGreaterThan(0);
|
||||
// Must parse without syntax errors (bash -n is a syntax check, doesn't execute)
|
||||
const result = execSync(`bash -n "${fullPath}" 2>&1`, { encoding: 'utf-8', timeout: 5000 });
|
||||
// bash -n outputs nothing on success
|
||||
}
|
||||
});
|
||||
|
||||
test('migration filenames follow v{VERSION}.sh pattern', () => {
|
||||
const scripts = fs.readdirSync(MIGRATIONS_DIR).filter(f => f.endsWith('.sh'));
|
||||
for (const script of scripts) {
|
||||
expect(script).toMatch(/^v\d+\.\d+\.\d+\.\d+\.sh$/);
|
||||
}
|
||||
});
|
||||
|
||||
test('v0.15.2.0 migration runs gstack-relink', () => {
|
||||
const content = fs.readFileSync(path.join(MIGRATIONS_DIR, 'v0.15.2.0.sh'), 'utf-8');
|
||||
expect(content).toContain('gstack-relink');
|
||||
});
|
||||
|
||||
test('v0.15.2.0 migration fixes stale directory symlinks', () => {
|
||||
setupMockInstall(['qa', 'ship', 'review']);
|
||||
// Simulate old state: directory symlinks (pre-v0.15.2.0 pattern)
|
||||
fs.symlinkSync(path.join(installDir, 'qa'), path.join(skillsDir, 'qa'));
|
||||
fs.symlinkSync(path.join(installDir, 'ship'), path.join(skillsDir, 'ship'));
|
||||
fs.symlinkSync(path.join(installDir, 'review'), path.join(skillsDir, 'review'));
|
||||
// Set no-prefix mode
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`);
|
||||
// Verify old state: symlinks
|
||||
expect(fs.lstatSync(path.join(skillsDir, 'qa')).isSymbolicLink()).toBe(true);
|
||||
|
||||
// Run the migration (it calls gstack-relink internally)
|
||||
run(`bash ${path.join(MIGRATIONS_DIR, 'v0.15.2.0.sh')}`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
|
||||
// After migration: real directories with SKILL.md symlinks
|
||||
for (const skill of ['qa', 'ship', 'review']) {
|
||||
const skillPath = path.join(skillsDir, skill);
|
||||
expect(fs.lstatSync(skillPath).isSymbolicLink()).toBe(false);
|
||||
expect(fs.lstatSync(skillPath).isDirectory()).toBe(true);
|
||||
expect(fs.lstatSync(path.join(skillPath, 'SKILL.md')).isSymbolicLink()).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('gstack-patch-names (#620/#578)', () => {
|
||||
// Helper to read name: from SKILL.md frontmatter
|
||||
function readSkillName(skillDir: string): string | null {
|
||||
|
||||
Reference in New Issue
Block a user