From 5d5a73ae10b323d7b9c335eeb7dfe47c4e408c59 Mon Sep 17 00:00:00 2001 From: smblight Date: Fri, 26 Jun 2026 10:56:43 -0500 Subject: [PATCH] fix(relink): don't clobber foreign skills that share a gstack name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gstack-relink's _cleanup_skill_entry deleted ANY flat-named skill entry matching a gstack skill name when switching prefix modes — including symlinks that point outside the gstack install. A user with a personal /qa or /review skill (e.g. symlinked into ~/.agents/skills/) would have it silently removed on every ./setup, because setup runs gstack-relink as a self-healing step. In prefix mode the deletion isn't even needed: gstack installs under gstack-* names, so the flat entry it removed was pure collateral. Scope the cleanup to gstack-owned entries only: delete a flat entry just when its symlink (or its dir's SKILL.md symlink) resolves into the gstack install dir. Foreign skills sharing a gstack name are now preserved, while genuinely stale flat-mode gstack entries are still cleaned. This mirrors the provenance check setup's cleanup_old_claude_symlinks() already uses. Adds two regression tests to test/relink.test.ts: one asserting a foreign flat skill (both the dir-symlink and real-dir-with-foreign-SKILL.md shapes) survives a prefix-mode relink, and a companion asserting a gstack-owned stale flat entry is still cleaned. Co-Authored-By: Claude Opus 4.8 (1M context) --- bin/gstack-relink | 16 +++++++++-- test/relink.test.ts | 66 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/bin/gstack-relink b/bin/gstack-relink index dd2a681fa..748456d7f 100755 --- a/bin/gstack-relink +++ b/bin/gstack-relink @@ -37,12 +37,24 @@ SKILLS_DIR="${GSTACK_SKILLS_DIR:-$(dirname "$INSTALL_DIR")}" PREFIX=$("$GSTACK_CONFIG" get skill_prefix 2>/dev/null || echo "false") # Helper: remove old skill entry (symlink or real directory with symlinked SKILL.md) +# ONLY removes gstack-owned entries — i.e. ones whose symlink (or whose dir's +# SKILL.md symlink) resolves into the gstack install dir. A flat entry of the +# same name that points elsewhere (e.g. a personal /qa or /review symlink into +# ~/.agents/skills/) is left untouched, so switching prefix modes never clobbers +# unrelated user skills that happen to share a gstack skill name. _cleanup_skill_entry() { local entry="$1" + local dest if [ -L "$entry" ]; then - rm -f "$entry" + dest="$(readlink "$entry" 2>/dev/null || true)" + case "$dest" in + "$INSTALL_DIR"/*|gstack/*|*/gstack/*) rm -f "$entry" ;; + esac elif [ -d "$entry" ] && [ -L "$entry/SKILL.md" ]; then - rm -rf "$entry" + dest="$(readlink "$entry/SKILL.md" 2>/dev/null || true)" + case "$dest" in + "$INSTALL_DIR"/*|gstack/*|*/gstack/*) rm -rf "$entry" ;; + esac fi } diff --git a/test/relink.test.ts b/test/relink.test.ts index d83c4cd37..96e241b66 100644 --- a/test/relink.test.ts +++ b/test/relink.test.ts @@ -367,6 +367,72 @@ describe('gstack-relink (#578)', () => { expect(fs.existsSync(path.join(skillsDir, 'gstack-qa'))).toBe(false); }); + // REGRESSION: prefix-mode relink must NOT clobber a *foreign* flat skill that + // happens to share a gstack skill name. Only gstack-owned flat entries (whose + // symlink resolves into the install dir) may be cleaned — a personal `/qa` or + // `/review` symlink pointing into e.g. ~/.agents/skills/ must survive. + test('prefix mode preserves foreign flat skills sharing a gstack name', () => { + setupMockInstall(['qa', 'ship']); + + // A user's personal skills living OUTSIDE the gstack install. + const foreignDir = path.join(tmpDir, 'foreign-skills'); + fs.mkdirSync(path.join(foreignDir, 'qa'), { recursive: true }); + fs.writeFileSync( + path.join(foreignDir, 'qa', 'SKILL.md'), + '---\nname: qa\ndescription: my own qa skill\n---\n# my qa', + ); + + // Case A: foreign entry is a directory symlink (e.g. `qa -> ~/.agents/skills/qa`). + fs.symlinkSync(path.join(foreignDir, 'qa'), path.join(skillsDir, 'qa')); + // Case B: foreign entry is a real dir whose SKILL.md symlinks OUTSIDE the install. + fs.mkdirSync(path.join(skillsDir, 'ship'), { recursive: true }); + fs.symlinkSync( + path.join(foreignDir, 'qa', 'SKILL.md'), + path.join(skillsDir, 'ship', 'SKILL.md'), + ); + + run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`, { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + run(`${path.join(installDir, 'bin', 'gstack-relink')}`, { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + + // Foreign /qa symlink survives, still pointing outside the install. + expect(fs.lstatSync(path.join(skillsDir, 'qa')).isSymbolicLink()).toBe(true); + expect(fs.readlinkSync(path.join(skillsDir, 'qa'))).toBe(path.join(foreignDir, 'qa')); + // Foreign /ship (real dir, foreign SKILL.md) survives too. + expect(fs.readlinkSync(path.join(skillsDir, 'ship', 'SKILL.md'))) + .toBe(path.join(foreignDir, 'qa', 'SKILL.md')); + // gstack still installs under its prefixed names alongside them. + expect(fs.existsSync(path.join(skillsDir, 'gstack-qa'))).toBe(true); + expect(fs.existsSync(path.join(skillsDir, 'gstack-ship'))).toBe(true); + }); + + // REGRESSION companion: a genuinely gstack-owned stale flat entry IS still removed. + test('prefix mode still removes gstack-owned stale flat entries', () => { + setupMockInstall(['qa']); + // Leftover flat-mode gstack entry: real dir whose SKILL.md → into the install. + fs.mkdirSync(path.join(skillsDir, 'qa'), { recursive: true }); + fs.symlinkSync( + path.join(installDir, 'qa', 'SKILL.md'), + path.join(skillsDir, 'qa', 'SKILL.md'), + ); + run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`, { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + run(`${path.join(installDir, 'bin', 'gstack-relink')}`, { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + // The stale flat /qa (gstack-owned) is gone; only gstack-qa remains. + expect(fs.existsSync(path.join(skillsDir, 'gstack-qa'))).toBe(true); + expect(fs.readdirSync(skillsDir)).not.toContain('qa'); + }); + // Test 14: error when install dir missing test('prints error when install dir missing', () => { const output = run(`${BIN}/gstack-relink`, {