mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 06:28:59 +02:00
fix(relink): never delete or link over a skill gstack does not own (#2119)
gstack-relink runs on every ./setup. Its cleanup did `rm -rf` on any same-name entry whose SKILL.md was a symlink, with no readlink check, and its link step did `mkdir -p` then `ln -snf` onto any existing SKILL.md — on Linux that replaces a user's real file with a symlink into gstack (macOS refused by accident). setup's Windows mode-flip cleanup deleted any real dir whose name matched a gstack skill. A personal `qa` skill, or a fork installed under another path, was destroyed by the installer of a tool it never asked for. Ownership is now proven, never assumed. An entry is ours when it is a symlink resolving into INSTALL_DIR or RENDER_DIR, a real dir whose SKILL.md is such a symlink, or a real dir carrying the .gstack-owned marker setup now writes for Windows copy installs (legacy copies count when byte-identical to the source or carrying gen-skill-docs' AUTO-GENERATED header). Anything else — including an entry whose readlink fails — is foreign: left untouched, reported on stderr, and listed in relink's summary line. The same rule replaces setup's Windows name-match deletion; setup:1040 and gstack-uninstall:204 already gated on readlink, so this closes the last unguarded deleter of the class. Tests: foreign real dir in flat mode, foreign flat entry on a prefix flip, foreign directory symlink, RENDER_DIR-targeted entry (ours), marker-carrying copy (ours), marker-less copy (foreign); the Windows cleanup test now proves provenance three ways and keeps the user's own same-name skill. Idea and two regression cases from PR #2119 (@smblight); implemented on the destination entry, not only the symlink target. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
co-authored by
Claude Fable 5.1
Claude Fable 5.1
parent
17733302cc
commit
584c2a44fb
+71
-2
@@ -42,12 +42,70 @@ PREFIX=$("$GSTACK_CONFIG" get skill_prefix 2>/dev/null || echo "false")
|
||||
# every skill back to the canonical (blockless) source.
|
||||
RENDER_DIR="${GSTACK_USER_RENDER_DIR:-${GSTACK_HOME:-$HOME/.gstack}/render/claude}"
|
||||
|
||||
# Helper: remove old skill entry (symlink or real directory with symlinked SKILL.md)
|
||||
# ─── Ownership gate ───────────────────────────────────────────────────────────
|
||||
# relink runs on every ./setup and used to `rm -rf` any same-name entry with a
|
||||
# symlinked SKILL.md and `ln -snf` over any existing SKILL.md — so a user's own
|
||||
# skill that happened to share a name (a personal `qa`, a fork under another
|
||||
# path) was deleted or had its SKILL.md replaced by a symlink into gstack
|
||||
# (#2119; Linux replaces a real file with `ln -snf`, macOS refuses by accident).
|
||||
# setup:1040 and gstack-uninstall:204 already gate on readlink; this is the
|
||||
# same rule for the one remaining unguarded deleter.
|
||||
#
|
||||
# An entry is OURS when:
|
||||
# - it is a symlink resolving into $INSTALL_DIR or $RENDER_DIR, or
|
||||
# - it is a real dir whose SKILL.md is a symlink resolving into either, or
|
||||
# - it is a real dir carrying the .gstack-owned marker setup writes for
|
||||
# Windows copy installs (no symlinks there to read).
|
||||
# Anything else — a foreign symlink, a real dir with a real SKILL.md and no
|
||||
# marker, or an entry whose readlink fails — is FOREIGN: never deleted, never
|
||||
# linked over, reported on stderr.
|
||||
_target_is_ours() {
|
||||
# $1 = a path that readlink resolved; ours when it lives under our roots.
|
||||
case "$1" in
|
||||
"$INSTALL_DIR"/*|"$RENDER_DIR"/*) return 0 ;;
|
||||
*) return 1 ;;
|
||||
esac
|
||||
}
|
||||
|
||||
_entry_is_ours() {
|
||||
local entry="$1" dest
|
||||
if [ -L "$entry" ]; then
|
||||
dest="$(readlink "$entry" 2>/dev/null || true)"
|
||||
[ -n "$dest" ] || return 1
|
||||
_target_is_ours "$dest"
|
||||
return $?
|
||||
fi
|
||||
if [ -d "$entry" ]; then
|
||||
[ -f "$entry/.gstack-owned" ] && return 0
|
||||
if [ -L "$entry/SKILL.md" ]; then
|
||||
dest="$(readlink "$entry/SKILL.md" 2>/dev/null || true)"
|
||||
[ -n "$dest" ] || return 1
|
||||
_target_is_ours "$dest"
|
||||
return $?
|
||||
fi
|
||||
return 1
|
||||
fi
|
||||
return 1
|
||||
}
|
||||
|
||||
FOREIGN_SKIPPED=()
|
||||
_report_foreign() {
|
||||
echo " skipped $1: not a gstack-managed entry (foreign skill with the same name) — left untouched" >&2
|
||||
FOREIGN_SKIPPED+=("$1")
|
||||
}
|
||||
|
||||
# Helper: remove an OLD skill entry from the opposite prefix mode. Only entries
|
||||
# we can prove are ours are removed; anything else is reported and kept.
|
||||
_cleanup_skill_entry() {
|
||||
local entry="$1"
|
||||
[ -e "$entry" ] || [ -L "$entry" ] || return 0
|
||||
if ! _entry_is_ours "$entry"; then
|
||||
_report_foreign "$entry"
|
||||
return 0
|
||||
fi
|
||||
if [ -L "$entry" ]; then
|
||||
rm -f "$entry"
|
||||
elif [ -d "$entry" ] && [ -L "$entry/SKILL.md" ]; then
|
||||
elif [ -d "$entry" ]; then
|
||||
rm -rf "$entry"
|
||||
fi
|
||||
}
|
||||
@@ -100,6 +158,14 @@ for skill_dir in "$INSTALL_DIR"/*/; do
|
||||
esac
|
||||
fi
|
||||
target="$SKILLS_DIR/$link_name"
|
||||
# A destination that already exists and is NOT ours is a foreign skill that
|
||||
# shares our name. Never `ln -snf` over its SKILL.md (on Linux that replaces
|
||||
# a real file with a symlink into gstack) and never mkdir into it — skip
|
||||
# loudly and leave registration of that one name to the user.
|
||||
if { [ -e "$target" ] || [ -L "$target" ]; } && ! _entry_is_ours "$target"; then
|
||||
_report_foreign "$target"
|
||||
continue
|
||||
fi
|
||||
# Upgrade old directory symlinks to real directories
|
||||
[ -L "$target" ] && rm -f "$target"
|
||||
# Create real directory with symlinked SKILL.md (absolute path)
|
||||
@@ -124,3 +190,6 @@ if [ "$PREFIX" = "true" ]; then
|
||||
else
|
||||
echo "Relinked $SKILL_COUNT skills as flat names"
|
||||
fi
|
||||
if [ ${#FOREIGN_SKIPPED[@]} -gt 0 ]; then
|
||||
echo "Skipped ${#FOREIGN_SKIPPED[@]} foreign entr$( [ ${#FOREIGN_SKIPPED[@]} -eq 1 ] && echo y || echo ies) (not gstack-managed, left untouched): ${FOREIGN_SKIPPED[*]}"
|
||||
fi
|
||||
|
||||
@@ -960,6 +960,10 @@ link_claude_skill_dirs() {
|
||||
_skill_md_src="$_render_dir/$dir_name/SKILL.md"
|
||||
fi
|
||||
_link_or_copy "$_skill_md_src" "$target/SKILL.md"
|
||||
# Ownership marker for Windows COPY installs (#2119): there is no symlink
|
||||
# to readlink, so gstack-relink and the mode-flip cleanup below prove
|
||||
# provenance by this marker instead of by name.
|
||||
if [ "$IS_WINDOWS" -eq 1 ]; then : > "$target/.gstack-owned" 2>/dev/null || true; fi
|
||||
# Link every runtime asset the skill ships next to its SKILL.md (#2317,
|
||||
# #2454): sections/ for carved skills, review's checklist.md +
|
||||
# specialists/, qa's templates/ + references/, gstack-upgrade's
|
||||
@@ -1062,10 +1066,13 @@ cleanup_old_claude_symlinks() {
|
||||
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.
|
||||
# available, so we can't readlink to verify provenance). A bare name match
|
||||
# deleted a user's own same-name skill (#2119); ownership is now proven by
|
||||
# the .gstack-owned marker link_claude_skill_dirs writes, or — for copies
|
||||
# made before the marker existed — by the copy being byte-identical to the
|
||||
# gstack source SKILL.md or carrying gen-skill-docs' AUTO-GENERATED header
|
||||
# (every generated SKILL.md does; a hand-written skill does not). Anything
|
||||
# else is foreign and is left alone.
|
||||
if [ "${IS_WINDOWS:-0}" -eq 1 ] && [ -d "$gstack_dir" ]; then
|
||||
for skill_dir in "$gstack_dir"/*/; do
|
||||
if [ -f "$skill_dir/SKILL.md" ]; then
|
||||
@@ -1074,7 +1081,10 @@ cleanup_old_claude_symlinks() {
|
||||
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
|
||||
&& [ -f "$old_target/SKILL.md" ] && [ ! -L "$old_target/SKILL.md" ] \
|
||||
&& { [ -f "$old_target/.gstack-owned" ] \
|
||||
|| cmp -s "$old_target/SKILL.md" "$skill_dir/SKILL.md" \
|
||||
|| grep -q 'AUTO-GENERATED from SKILL.md.tmpl' "$old_target/SKILL.md" 2>/dev/null; }; then
|
||||
rm -rf "$old_target"
|
||||
removed+=("$skill_name")
|
||||
fi
|
||||
|
||||
@@ -672,3 +672,105 @@ describe('gstack-patch-names (#620/#578)', () => {
|
||||
expect(content).toBe('# qa\nSome content.');
|
||||
});
|
||||
});
|
||||
|
||||
// ============================================================
|
||||
// Ownership gate (#2119): relink runs on every ./setup and must never delete
|
||||
// or link over a skill it does not own. Ownership = symlink into INSTALL_DIR
|
||||
// or RENDER_DIR, a real dir whose SKILL.md is such a symlink, or the
|
||||
// .gstack-owned marker setup writes for Windows copy installs.
|
||||
// ============================================================
|
||||
describe('gstack-relink ownership gate (#2119)', () => {
|
||||
const FOREIGN = '---\nname: qa\ndescription: my own qa skill\n---\n# not gstack';
|
||||
|
||||
function relink(env: Record<string, string> = {}): string {
|
||||
return run(`${path.join(installDir, 'bin', 'gstack-relink')} 2>&1`, {
|
||||
GSTACK_INSTALL_DIR: installDir,
|
||||
GSTACK_SKILLS_DIR: skillsDir,
|
||||
...env,
|
||||
});
|
||||
}
|
||||
function setPrefix(v: 'true' | 'false') {
|
||||
run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix ${v}`, {
|
||||
GSTACK_INSTALL_DIR: installDir, GSTACK_SKILLS_DIR: skillsDir,
|
||||
});
|
||||
}
|
||||
|
||||
test('flat mode: a foreign real dir with a real SKILL.md is never linked over (Linux ln -snf would replace the file)', () => {
|
||||
setupMockInstall(['qa', 'ship']);
|
||||
// The foreign skill exists before gstack ever runs (gstack-config `set`
|
||||
// auto-relinks, so fixtures go in first).
|
||||
fs.mkdirSync(path.join(skillsDir, 'qa'));
|
||||
fs.writeFileSync(path.join(skillsDir, 'qa', 'SKILL.md'), FOREIGN);
|
||||
setPrefix('false');
|
||||
const out = relink();
|
||||
const md = path.join(skillsDir, 'qa', 'SKILL.md');
|
||||
expect(fs.lstatSync(md).isSymbolicLink()).toBe(false);
|
||||
expect(fs.readFileSync(md, 'utf-8')).toBe(FOREIGN);
|
||||
expect(out).toContain('skipped');
|
||||
expect(out).toContain('Skipped 1 foreign entry');
|
||||
// The other skill still links normally.
|
||||
expect(fs.lstatSync(path.join(skillsDir, 'ship', 'SKILL.md')).isSymbolicLink()).toBe(true);
|
||||
});
|
||||
|
||||
test('prefix flip: a foreign flat entry sharing a skill name survives the cleanup pass', () => {
|
||||
setupMockInstall(['qa']);
|
||||
fs.mkdirSync(path.join(skillsDir, 'qa'));
|
||||
fs.writeFileSync(path.join(skillsDir, 'qa', 'SKILL.md'), FOREIGN);
|
||||
setPrefix('true');
|
||||
const out = relink();
|
||||
expect(fs.existsSync(path.join(skillsDir, 'qa', 'SKILL.md'))).toBe(true);
|
||||
expect(fs.readFileSync(path.join(skillsDir, 'qa', 'SKILL.md'), 'utf-8')).toBe(FOREIGN);
|
||||
expect(fs.existsSync(path.join(skillsDir, 'gstack-qa', 'SKILL.md'))).toBe(true);
|
||||
expect(out).toContain('skipped');
|
||||
});
|
||||
|
||||
test('a foreign directory symlink sharing a skill name is left in place', () => {
|
||||
setupMockInstall(['qa']);
|
||||
const elsewhere = path.join(tmpDir, 'elsewhere', 'qa');
|
||||
fs.mkdirSync(elsewhere, { recursive: true });
|
||||
fs.writeFileSync(path.join(elsewhere, 'SKILL.md'), FOREIGN);
|
||||
fs.symlinkSync(elsewhere, path.join(skillsDir, 'qa'));
|
||||
setPrefix('false');
|
||||
const out = relink();
|
||||
expect(fs.lstatSync(path.join(skillsDir, 'qa')).isSymbolicLink()).toBe(true);
|
||||
expect(fs.readlinkSync(path.join(skillsDir, 'qa'))).toBe(elsewhere);
|
||||
expect(fs.readFileSync(path.join(elsewhere, 'SKILL.md'), 'utf-8')).toBe(FOREIGN);
|
||||
expect(out).toContain('skipped');
|
||||
});
|
||||
|
||||
test('an entry whose SKILL.md links into RENDER_DIR is ours and is cleaned on a mode flip', () => {
|
||||
setupMockInstall(['qa']);
|
||||
setPrefix('false');
|
||||
const renderDir = path.join(tmpDir, 'render', 'claude');
|
||||
fs.mkdirSync(path.join(renderDir, 'qa'), { recursive: true });
|
||||
fs.writeFileSync(path.join(renderDir, 'qa', 'SKILL.md'), '---\nname: gstack-qa\ndescription: rendered\n---\n');
|
||||
// Stale prefixed entry from a prior prefix-mode run, pointing at the render.
|
||||
fs.mkdirSync(path.join(skillsDir, 'gstack-qa'));
|
||||
fs.symlinkSync(path.join(renderDir, 'qa', 'SKILL.md'), path.join(skillsDir, 'gstack-qa', 'SKILL.md'));
|
||||
const out = relink({ GSTACK_USER_RENDER_DIR: renderDir });
|
||||
expect(fs.existsSync(path.join(skillsDir, 'gstack-qa'))).toBe(false);
|
||||
expect(fs.readlinkSync(path.join(skillsDir, 'qa', 'SKILL.md'))).toBe(path.join(renderDir, 'qa', 'SKILL.md'));
|
||||
expect(out).not.toContain('skipped');
|
||||
});
|
||||
|
||||
test('a real-file copy carrying the .gstack-owned marker (Windows install shape) is ours', () => {
|
||||
setupMockInstall(['qa']);
|
||||
setPrefix('false');
|
||||
fs.mkdirSync(path.join(skillsDir, 'gstack-qa'));
|
||||
fs.writeFileSync(path.join(skillsDir, 'gstack-qa', 'SKILL.md'), '---\nname: gstack-qa\n---\n');
|
||||
fs.writeFileSync(path.join(skillsDir, 'gstack-qa', '.gstack-owned'), '');
|
||||
const out = relink();
|
||||
expect(fs.existsSync(path.join(skillsDir, 'gstack-qa'))).toBe(false);
|
||||
expect(out).not.toContain('skipped');
|
||||
});
|
||||
|
||||
test('a real-file copy WITHOUT the marker is foreign and survives', () => {
|
||||
setupMockInstall(['qa']);
|
||||
setPrefix('false');
|
||||
fs.mkdirSync(path.join(skillsDir, 'gstack-qa'));
|
||||
fs.writeFileSync(path.join(skillsDir, 'gstack-qa', 'SKILL.md'), FOREIGN);
|
||||
const out = relink();
|
||||
expect(fs.readFileSync(path.join(skillsDir, 'gstack-qa', 'SKILL.md'), 'utf-8')).toBe(FOREIGN);
|
||||
expect(out).toContain('skipped');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -259,21 +259,39 @@ describe.skipIf(process.platform === 'win32')('setup: cleanup_old_claude_symlink
|
||||
}
|
||||
});
|
||||
|
||||
test('Windows real-file leftover is removed when the payload still names it', () => {
|
||||
// #2119: a bare name match used to delete a USER's own skill that happened to
|
||||
// share a gstack skill name. Provenance must be proven: the .gstack-owned
|
||||
// marker, a byte-identical copy of the payload source, or gen-skill-docs'
|
||||
// AUTO-GENERATED header (legacy copies made before the marker existed).
|
||||
test('Windows real-file leftover is removed only when provably gstack-owned', () => {
|
||||
const generated = '---\nname: ship\n---\n<!-- AUTO-GENERATED from SKILL.md.tmpl — do not edit directly -->\n# ship\n';
|
||||
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');
|
||||
for (const name of ['qa', 'ship', 'review', 'browse']) {
|
||||
fs.mkdirSync(path.join(payload, name));
|
||||
fs.writeFileSync(path.join(payload, name, 'SKILL.md'), name === 'ship' ? generated : `---\nname: ${name}\n---\n`);
|
||||
}
|
||||
// Byte-identical copy of the payload source → ours.
|
||||
fs.mkdirSync(path.join(skills, 'qa'));
|
||||
fs.copyFileSync(path.join(payload, 'qa', 'SKILL.md'), path.join(skills, 'qa', 'SKILL.md'));
|
||||
// Legacy copy carrying the generated header but drifted from source → ours.
|
||||
fs.mkdirSync(path.join(skills, 'ship'));
|
||||
fs.writeFileSync(path.join(skills, 'ship', 'SKILL.md'), generated.replace('# ship', '# ship (older render)'));
|
||||
// Marker-carrying copy with arbitrary content → ours.
|
||||
fs.mkdirSync(path.join(skills, 'browse'));
|
||||
fs.writeFileSync(path.join(skills, 'browse', 'SKILL.md'), '---\nname: browse\n---\n# stale copy\n');
|
||||
fs.writeFileSync(path.join(skills, 'browse', '.gstack-owned'), '');
|
||||
// The user's OWN skill that shares a gstack name → foreign, must survive.
|
||||
plantUserSkill(skills, 'review');
|
||||
plantUserSkill(skills, 'my-own');
|
||||
},
|
||||
});
|
||||
try {
|
||||
expect(r.status).toBe(0);
|
||||
expect(r.names).toEqual(['gstack', 'my-own']);
|
||||
expect(r.names).toEqual(['gstack', 'my-own', 'review']);
|
||||
expect(fs.readFileSync(path.join(r.tmp, 'skills', 'review', 'SKILL.md'), 'utf-8')).toContain('user-owned');
|
||||
} finally {
|
||||
fs.rmSync(r.tmp, { recursive: true, force: true });
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user