diff --git a/bin/gstack-relink b/bin/gstack-relink index 7a3b4b0e6..6ee0d367b 100755 --- a/bin/gstack-relink +++ b/bin/gstack-relink @@ -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 diff --git a/setup b/setup index 4f28bd49d..7b9dd6764 100755 --- a/setup +++ b/setup @@ -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 diff --git a/test/relink.test.ts b/test/relink.test.ts index 8f18d7281..f1174bfc6 100644 --- a/test/relink.test.ts +++ b/test/relink.test.ts @@ -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 { + 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'); + }); +}); diff --git a/test/setup-cleanup-orphans.test.ts b/test/setup-cleanup-orphans.test.ts index 4869e24fd..26a1793eb 100644 --- a/test/setup-cleanup-orphans.test.ts +++ b/test/setup-cleanup-orphans.test.ts @@ -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\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 }); }