diff --git a/setup b/setup index 8c5d6c077..08bf540a7 100755 --- a/setup +++ b/setup @@ -973,45 +973,67 @@ link_claude_root_skill_alias() { # ─── Helper: remove old unprefixed Claude skill entries ─────────────────────── # Migration: when switching from flat names to gstack- prefixed names, # clean up stale symlinks or directories that point into the gstack directory. +# Scan $skills_dir (not $gstack_dir): orphans live next to the payload, so a +# missing payload must still be able to reap leftover flat names (#2204). cleanup_old_claude_symlinks() { local gstack_dir="$1" local skills_dir="$2" local removed=() - for skill_dir in "$gstack_dir"/*/; do - if [ -f "$skill_dir/SKILL.md" ]; then - skill_name="$(basename "$skill_dir")" - [ "$skill_name" = "node_modules" ] && continue - # Skip already-prefixed dirs (gstack-upgrade) — no old symlink to clean - case "$skill_name" in gstack-*) continue ;; esac - old_target="$skills_dir/$skill_name" - # Remove directory symlinks pointing into gstack/ - if [ -L "$old_target" ]; then - link_dest="$(readlink "$old_target" 2>/dev/null || true)" - case "$link_dest" in - gstack/*|*/gstack/*) - rm -f "$old_target" - removed+=("$skill_name") - ;; - esac - # Remove real directories with symlinked SKILL.md pointing into gstack/ - elif [ -d "$old_target" ] && [ -L "$old_target/SKILL.md" ]; then - link_dest="$(readlink "$old_target/SKILL.md" 2>/dev/null || true)" - case "$link_dest" in - *gstack*) - rm -rf "$old_target" - removed+=("$skill_name") - ;; - esac - # Windows install pattern: real dir with real-file SKILL.md (no symlink - # available, so we can't readlink to verify provenance). The outer loop - # iterates known gstack skill names from "$gstack_dir"/*, so a name match - # plus IS_WINDOWS is safe to treat as gstack-managed during a mode flip. - elif [ "$IS_WINDOWS" -eq 1 ] && [ -d "$old_target" ] && [ -f "$old_target/SKILL.md" ]; then - rm -rf "$old_target" - removed+=("$skill_name") - fi + local old_target skill_name link_dest skill_dir + # Destination scan. The glob already yields dangling dir symlinks; [ -e ] + # alone would skip them, so [ -L ] keeps those entries. An unmatched `*` + # literal (empty skills_dir) is rejected by the same guard. + for old_target in "$skills_dir"/*; do + [ -e "$old_target" ] || [ -L "$old_target" ] || continue + skill_name="$(basename "$old_target")" + [ "$skill_name" = "node_modules" ] && continue + [ "$skill_name" = "gstack" ] && continue + # Skip already-prefixed dirs (gstack-upgrade) — no old symlink to clean + case "$skill_name" in gstack-*) continue ;; esac + # Remove directory symlinks pointing into gstack/ + if [ -L "$old_target" ]; then + link_dest="$(readlink "$old_target" 2>/dev/null || true)" + case "$link_dest" in + gstack/*|*/gstack/*) + rm -f "$old_target" + removed+=("$skill_name") + ;; + esac + # Remove real directories with symlinked SKILL.md pointing into gstack/ + elif [ -d "$old_target" ] && [ -L "$old_target/SKILL.md" ]; then + link_dest="$(readlink "$old_target/SKILL.md" 2>/dev/null || true)" + # Anchored path segments (same as the dir-symlink arm and + # gstack-uninstall #2563). A bare *gstack* substring would wipe a + # user skill under e.g. ~/tools/gstack-fork/. Also accept the #2569 + # render prefix (~/.gstack/render/claude/...), which is not `/gstack/`. + case "$link_dest" in + gstack/*|*/gstack/*|*/.gstack/render/claude/*) + rm -rf "$old_target" + removed+=("$skill_name") + ;; + esac fi done + # Windows install pattern: real dir with real-file SKILL.md (no symlink + # available, so we can't readlink to verify provenance). Iterate known + # gstack skill names from "$gstack_dir"/*, so a name match plus IS_WINDOWS + # is safe to treat as gstack-managed during a mode flip. When the payload + # is gone this branch is a no-op — a real file has no proven owner. + if [ "${IS_WINDOWS:-0}" -eq 1 ] && [ -d "$gstack_dir" ]; then + for skill_dir in "$gstack_dir"/*/; do + if [ -f "$skill_dir/SKILL.md" ]; then + skill_name="$(basename "$skill_dir")" + [ "$skill_name" = "node_modules" ] && continue + case "$skill_name" in gstack-*) continue ;; esac + old_target="$skills_dir/$skill_name" + if [ -d "$old_target" ] && [ ! -L "$old_target" ] \ + && [ -f "$old_target/SKILL.md" ] && [ ! -L "$old_target/SKILL.md" ]; then + rm -rf "$old_target" + removed+=("$skill_name") + fi + fi + done + fi if [ ${#removed[@]} -gt 0 ]; then echo " cleaned up old entries: ${removed[*]}" fi diff --git a/test/setup-cleanup-orphans.test.ts b/test/setup-cleanup-orphans.test.ts new file mode 100644 index 000000000..4869e24fd --- /dev/null +++ b/test/setup-cleanup-orphans.test.ts @@ -0,0 +1,281 @@ +/** + * cleanup_old_claude_symlinks destination scan (#2204). + * + * The helper used to iterate the payload skill dirs. When the payload is + * gone the glob matches nothing, so leftover flat skill dirs in $skills_dir + * stay forever. This suite extracts the REAL function from setup and drives + * it against a temp skills tree — payload-missing orphans must go, user + * skills must stay. + */ +import { describe, test, expect } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const SETUP_SRC = fs.readFileSync(path.join(ROOT, 'setup'), 'utf-8'); + +function extractFn(name: string): string { + const start = SETUP_SRC.indexOf(`${name}() {`); + const end = SETUP_SRC.indexOf('\n}\n', start); + if (start < 0 || end < 0) throw new Error(`Could not locate ${name}() in setup`); + return SETUP_SRC.slice(start, end + 2); +} + +function cleanupBody(): string { + return extractFn('cleanup_old_claude_symlinks'); +} + +describe('setup: cleanup_old_claude_symlinks — static (#2204)', () => { + test('scans the skills dir, not only the payload', () => { + const body = cleanupBody(); + expect(body).toContain('for old_target in "$skills_dir"/*'); + expect(body).toContain('[ "$skill_name" = "gstack" ] && continue'); + expect(body).toContain('readlink'); + expect(body).toContain('gstack/*'); + expect(body).toContain('gstack-*) continue'); + expect(body).toContain('-d "$old_target"'); + expect(body).toContain('-L "$old_target/SKILL.md"'); + expect(body).toContain('rm -rf "$old_target"'); + // SKILL.md arm must use path-segment provenance, not a bare substring. + expect(body).toContain('gstack/*|*/gstack/*|*/.gstack/render/claude/*'); + expect(body).not.toMatch(/\*gstack\*\)/); + }); + + test('Windows real-file reap still requires a live payload name list', () => { + const body = cleanupBody(); + expect(body).toContain('for skill_dir in "$gstack_dir"/*/'); + expect(body).toContain('[ "${IS_WINDOWS:-0}" -eq 1 ] && [ -d "$gstack_dir" ]'); + }); +}); + +describe.skipIf(process.platform === 'win32')('setup: cleanup_old_claude_symlinks — behavior (#2204)', () => { + function runCleanup(opts: { + isWindows?: '0' | '1'; + payload?: boolean; + plant: (skills: string, payload: string) => void; + }): { status: number; stdout: string; stderr: string; names: string[]; tmp: string } { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'cleanup-orphans-')); + const skills = path.join(tmp, 'skills'); + const payload = path.join(skills, 'gstack'); + fs.mkdirSync(skills, { recursive: true }); + if (opts.payload) { + fs.mkdirSync(payload, { recursive: true }); + } + opts.plant(skills, payload); + const gstackArg = opts.payload ? payload : path.join(skills, 'missing-payload'); + const script = [ + 'set -e', + `IS_WINDOWS=${opts.isWindows ?? '0'}`, + extractFn('cleanup_old_claude_symlinks'), + `cleanup_old_claude_symlinks "${gstackArg}" "${skills}"`, + ].join('\n'); + const result = spawnSync('bash', ['-c', script], { + encoding: 'utf-8', + timeout: 5000, + }); + const names = fs.existsSync(skills) + ? fs.readdirSync(skills).sort() + : []; + return { + status: result.status ?? -1, + stdout: result.stdout ?? '', + stderr: result.stderr ?? '', + names, + tmp, + }; + } + + function plantDanglingSkillMd(skills: string, name: string) { + const dir = path.join(skills, name); + fs.mkdirSync(dir); + fs.symlinkSync(`gstack/${name}/SKILL.md`, path.join(dir, 'SKILL.md')); + } + + function plantUserSkill(skills: string, name: string) { + const dir = path.join(skills, name); + fs.mkdirSync(dir); + fs.writeFileSync(path.join(dir, 'SKILL.md'), '---\nname: user-owned\n---\n'); + } + + test('payload gone: dangling SKILL.md orphan is removed, user skill stays', () => { + const r = runCleanup({ + payload: false, + plant(skills) { + plantDanglingSkillMd(skills, 'qa'); + plantUserSkill(skills, 'my-own'); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.stderr).toBe(''); + expect(r.stdout).toContain('cleaned up old entries: qa'); + expect(r.names).toEqual(['my-own']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('payload gone: whole-dir symlink into gstack/ is removed', () => { + const r = runCleanup({ + payload: false, + plant(skills) { + fs.symlinkSync('gstack/qa', path.join(skills, 'qa')); + plantUserSkill(skills, 'my-own'); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.names).toEqual(['my-own']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('payload present: leftover flat name pointing at gstack is still removed', () => { + const r = runCleanup({ + payload: true, + plant(skills, payload) { + const src = path.join(payload, 'qa'); + fs.mkdirSync(src); + fs.writeFileSync(path.join(src, 'SKILL.md'), '---\nname: qa\n---\n'); + plantDanglingSkillMd(skills, 'qa'); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.names).toEqual(['gstack']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('payload present: dangling name absent from the payload is still removed', () => { + // Unique dest-scan win: the old "$gstack_dir"/*/ loop only considered + // names that still exist in the payload. A retired leftover must go. + const r = runCleanup({ + payload: true, + plant(skills, payload) { + const src = path.join(payload, 'ship'); + fs.mkdirSync(src); + fs.writeFileSync(path.join(src, 'SKILL.md'), '---\nname: ship\n---\n'); + plantDanglingSkillMd(skills, 'qa'); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.names).toEqual(['gstack']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('does not remove a SKILL.md symlink that does not point at gstack', () => { + const r = runCleanup({ + payload: false, + plant(skills) { + const dir = path.join(skills, 'elsewhere'); + fs.mkdirSync(dir); + fs.symlinkSync('other/SKILL.md', path.join(dir, 'SKILL.md')); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.stdout).toBe(''); + expect(r.names).toEqual(['elsewhere']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('does not remove a SKILL.md whose target merely contains the substring gstack', () => { + const r = runCleanup({ + payload: false, + plant(skills) { + const dir = path.join(skills, 'notes'); + fs.mkdirSync(dir); + fs.symlinkSync('../../archive/my-gstack-backup/SKILL.md', path.join(dir, 'SKILL.md')); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.stdout).toBe(''); + expect(r.names).toEqual(['notes']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('reaps a leftover whose SKILL.md points at the user render dir', () => { + const r = runCleanup({ + payload: false, + plant(skills) { + const dir = path.join(skills, 'qa'); + fs.mkdirSync(dir); + fs.symlinkSync('../../.gstack/render/claude/qa/SKILL.md', path.join(dir, 'SKILL.md')); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.stdout).toContain('cleaned up old entries: qa'); + expect(r.names).toEqual([]); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('does not remove prefixed gstack-* names or the payload dir', () => { + const r = runCleanup({ + payload: true, + plant(skills, payload) { + fs.writeFileSync(path.join(payload, 'SKILL.md'), '---\nname: gstack\n---\n'); + const prefixed = path.join(skills, 'gstack-qa'); + fs.mkdirSync(prefixed); + fs.symlinkSync('gstack/qa/SKILL.md', path.join(prefixed, 'SKILL.md')); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.names).toEqual(['gstack', 'gstack-qa']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('Windows real-file orphan is left alone when the payload is gone', () => { + const r = runCleanup({ + isWindows: '1', + payload: false, + plant(skills) { + plantUserSkill(skills, 'qa'); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.names).toEqual(['qa']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); + + test('Windows real-file leftover is removed when the payload still names it', () => { + const r = runCleanup({ + isWindows: '1', + payload: true, + plant(skills, payload) { + const src = path.join(payload, 'qa'); + fs.mkdirSync(src); + fs.writeFileSync(path.join(src, 'SKILL.md'), '---\nname: qa\n---\n'); + plantUserSkill(skills, 'qa'); + plantUserSkill(skills, 'my-own'); + }, + }); + try { + expect(r.status).toBe(0); + expect(r.names).toEqual(['gstack', 'my-own']); + } finally { + fs.rmSync(r.tmp, { recursive: true, force: true }); + } + }); +});