mirror of https://github.com/garrytan/gstack.git
fix(relink): don't clobber foreign skills that share a gstack name
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) <noreply@anthropic.com>
This commit is contained in:
parent
11de390be1
commit
5d5a73ae10
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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`, {
|
||||
|
|
|
|||
Loading…
Reference in New Issue