mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix(uninstall): remove real-directory skill installs, gated on provenance
On Windows, setup installs skills as REAL directory copies (cp -R via _link_or_copy). gstack-uninstall's per-skill loop filtered on [ -L ], so every copy was skipped: --force exited 0 and printed 'gstack uninstalled.' while leaving ~52 gstack-* directories plus _gstack-command/ behind in ~/.claude/skills. The same filter also missed the standard Unix shape (real dir + symlinked SKILL.md), which was left as a dangling-symlink husk. Fix: the loop now handles all three install shapes. Symlink entries keep the existing readlink check. Real dirs with a SYMLINKED SKILL.md are removed when the link points into gstack (same semantics as setup's cleanup helpers). Real dirs with a REAL-FILE SKILL.md — the Windows copy shape — are removed ONLY when both provenance gates pass (F8): (a) the directory name is in gstack's skill inventory (source dir names, frontmatter names, gstack- prefixed variants, and the alias dirs), and (b) the SKILL.md carries the existing generated banner '<!-- AUTO-GENERATED from' (ENG-OV10: every pre-v1.67 copy already carries it; a NEW marker would refuse to delete legitimate old installs, recreating the bug). Anything failing a gate is listed to stderr and never deleted — a user's own skill that happens to share a name with a gstack skill survives. Tests: a fake-tree fixture covers removed/kept/listed for every shape (including the F8 name-collision row), and a census test asserts every installable skill's generated SKILL.md carries the banner so the gate can't strand a bannerless skill. Registered in the Windows-safe curated list — the copy shape is exactly what windows-latest exercises. Fixes #2563 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
co-authored by
Claude Fable 5
Claude Fable 5
parent
52006feac4
commit
c84246845e
+64
-8
@@ -130,22 +130,78 @@ fi
|
||||
|
||||
# ─── Remove global Claude skills ────────────────────────────
|
||||
CLAUDE_SKILLS="$HOME/.claude/skills"
|
||||
|
||||
# Skill-name inventory (#2563 gate a): every name setup could have installed —
|
||||
# each source skill's directory name, its frontmatter name, their gstack-
|
||||
# prefixed variants, and the alias dirs. Built BEFORE the install root is
|
||||
# removed. A real directory in ~/.claude/skills is only deletable when its
|
||||
# name is in this inventory AND its SKILL.md carries the generated banner.
|
||||
_INVENTORY=" _gstack-command connect-chrome gstack-connect-chrome "
|
||||
if [ -d "$GSTACK_DIR" ]; then
|
||||
for _SRC in "$GSTACK_DIR"/*/; do
|
||||
[ -f "$_SRC/SKILL.md" ] || continue
|
||||
_SRC_NAME="$(basename "$_SRC")"
|
||||
_FM_NAME=$(grep -m1 '^name:' "$_SRC/SKILL.md" 2>/dev/null | sed 's/^name:[[:space:]]*//' | tr -d '[:space:]' || true)
|
||||
for _N in "$_SRC_NAME" "$_FM_NAME"; do
|
||||
[ -n "$_N" ] || continue
|
||||
case "$_INVENTORY" in *" $_N "*) ;; *) _INVENTORY="$_INVENTORY$_N gstack-$_N " ;; esac
|
||||
done
|
||||
done
|
||||
fi
|
||||
_in_skill_inventory() { case "$_INVENTORY" in *" $1 "*) return 0 ;; *) return 1 ;; esac; }
|
||||
|
||||
_SKIPPED_DIRS=()
|
||||
if [ -d "$CLAUDE_SKILLS/gstack" ] || [ -L "$CLAUDE_SKILLS/gstack" ]; then
|
||||
# Remove per-skill symlinks that point into gstack/
|
||||
for _LINK in "$CLAUDE_SKILLS"/*; do
|
||||
[ -L "$_LINK" ] || continue
|
||||
_NAME="$(basename "$_LINK")"
|
||||
# Remove per-skill entries created by setup. Three install shapes exist:
|
||||
# 1. symlink entry (oldest installs)
|
||||
# 2. real dir + SYMLINKED SKILL.md (standard Unix install)
|
||||
# 3. real dir + REAL-FILE SKILL.md (Windows copy install, #2563)
|
||||
# Shape 3 was skipped entirely — gstack-uninstall exited 0 and reported
|
||||
# success while leaving ~52 gstack-* directories behind on Windows.
|
||||
for _ENTRY in "$CLAUDE_SKILLS"/*; do
|
||||
_NAME="$(basename "$_ENTRY")"
|
||||
[ "$_NAME" = "gstack" ] && continue
|
||||
_TARGET="$(readlink "$_LINK" 2>/dev/null || true)"
|
||||
case "$_TARGET" in
|
||||
gstack/*|*/gstack/*) rm -f "$_LINK"; REMOVED+=("claude/$_NAME") ;;
|
||||
esac
|
||||
if [ -L "$_ENTRY" ]; then
|
||||
_TARGET="$(readlink "$_ENTRY" 2>/dev/null || true)"
|
||||
case "$_TARGET" in
|
||||
gstack/*|*/gstack/*) rm -f "$_ENTRY"; REMOVED+=("claude/$_NAME") ;;
|
||||
esac
|
||||
elif [ -d "$_ENTRY" ] && { [ -f "$_ENTRY/SKILL.md" ] || [ -L "$_ENTRY/SKILL.md" ]; }; then
|
||||
if [ -L "$_ENTRY/SKILL.md" ]; then
|
||||
# Shape 2: provenance readable from the symlink target itself
|
||||
# (mirrors setup's cleanup_old_claude_symlinks semantics).
|
||||
_TARGET="$(readlink "$_ENTRY/SKILL.md" 2>/dev/null || true)"
|
||||
case "$_TARGET" in
|
||||
*gstack*) rm -rf "$_ENTRY"; REMOVED+=("claude/$_NAME") ;;
|
||||
*) _SKIPPED_DIRS+=("$_ENTRY") ;;
|
||||
esac
|
||||
elif _in_skill_inventory "$_NAME" && grep -q '<!-- AUTO-GENERATED from' "$_ENTRY/SKILL.md" 2>/dev/null; then
|
||||
# Shape 3: delete ONLY when BOTH gates pass (F8) — the name is in
|
||||
# gstack's skill inventory AND the SKILL.md carries the existing
|
||||
# generated banner. ENG-OV10: the banner IS the provenance marker —
|
||||
# every pre-v1.67 copy already carries it; inventing a new marker
|
||||
# would refuse to delete legitimate old installs, recreating #2563.
|
||||
rm -rf "$_ENTRY"
|
||||
REMOVED+=("claude/$_NAME")
|
||||
else
|
||||
# A real dir we cannot prove is gstack-managed (name collision with a
|
||||
# user's own skill, or a hand-written SKILL.md): NEVER delete — list.
|
||||
_SKIPPED_DIRS+=("$_ENTRY")
|
||||
fi
|
||||
fi
|
||||
done
|
||||
|
||||
rm -rf "$CLAUDE_SKILLS/gstack"
|
||||
REMOVED+=("~/.claude/skills/gstack")
|
||||
fi
|
||||
|
||||
if [ ${#_SKIPPED_DIRS[@]} -gt 0 ]; then
|
||||
echo "left in place (not provably gstack-managed — remove by hand if they are yours):" >&2
|
||||
for _D in "${_SKIPPED_DIRS[@]}"; do
|
||||
echo " $_D" >&2
|
||||
done
|
||||
fi
|
||||
|
||||
# ─── Remove project-local Claude skills (--local installs) ──
|
||||
if [ -n "$_GIT_ROOT" ] && [ -d "$_GIT_ROOT/.claude/skills" ]; then
|
||||
for _LINK in "$_GIT_ROOT/.claude/skills"/*; do
|
||||
|
||||
@@ -265,6 +265,16 @@ const KNOWN_WINDOWS_SAFE: Array<{ file: string; reason: string }> = [
|
||||
// platform it bites.
|
||||
reason: 'bin/ hits are fixture path segments; spawns bash explicitly — the IS_WINDOWS=1 refresh path must run on windows-latest',
|
||||
},
|
||||
{
|
||||
file: 'test/uninstall-windows-copies.test.ts',
|
||||
// Trips the "spawns bin/ shebang script" pattern via the
|
||||
// path.join(ROOT, 'bin', 'gstack-uninstall') constant, but the script is
|
||||
// always spawned through spawnSync('bash', [UNINSTALL, ...]). This file
|
||||
// carries the #2563 Windows real-dir-copy uninstall coverage — the bug
|
||||
// ONLY reproduces on the copy install shape windows-latest exercises.
|
||||
// The symlink-shape describe block self-skips on win32.
|
||||
reason: 'bin/ hit is a bash-spawned script path; #2563 real-dir uninstall coverage must run on windows-latest',
|
||||
},
|
||||
{
|
||||
file: 'browse/test/file-permissions.test.ts',
|
||||
// Trips the POSIX-mode-bitmask pattern, but every `mode & 0o777` assertion
|
||||
|
||||
@@ -0,0 +1,195 @@
|
||||
/**
|
||||
* gstack-uninstall: real-directory installs are removed, gated on provenance
|
||||
* (#2563, F8, ENG-OV10).
|
||||
*
|
||||
* On Windows, setup installs skills as REAL directory copies (no symlinks).
|
||||
* gstack-uninstall's per-skill loop filtered on `[ -L ]`, so every copy was
|
||||
* skipped: the tool exited 0, printed "gstack uninstalled.", and left ~52
|
||||
* gstack-* directories behind. The same filter also missed the standard Unix
|
||||
* shape (real dir + symlinked SKILL.md).
|
||||
*
|
||||
* Deletion gate for real-file installs (F8): the directory name must be in
|
||||
* gstack's skill inventory AND its SKILL.md must carry the existing generated
|
||||
* banner `<!-- AUTO-GENERATED from` (ENG-OV10 — every pre-v1.67 copy already
|
||||
* carries it; a NEW marker would refuse legitimate old installs). Anything
|
||||
* that fails a gate is listed to stderr and NEVER deleted.
|
||||
*/
|
||||
import { describe, test, expect, beforeEach, afterEach } 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 UNINSTALL = path.join(ROOT, 'bin', 'gstack-uninstall');
|
||||
|
||||
const BANNER = '<!-- AUTO-GENERATED from SKILL.md.tmpl - DO NOT EDIT DIRECTLY -->\n';
|
||||
|
||||
function skillMd(name: string, withBanner = true): string {
|
||||
return `---\nname: ${name}\ndescription: test\n---\n${withBanner ? BANNER : ''}# ${name}\n`;
|
||||
}
|
||||
|
||||
let tmpDir: string;
|
||||
let mockHome: string;
|
||||
let skillsDir: string;
|
||||
let installRoot: string;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-uninstall-copies-'));
|
||||
mockHome = path.join(tmpDir, 'home');
|
||||
skillsDir = path.join(mockHome, '.claude', 'skills');
|
||||
installRoot = path.join(skillsDir, 'gstack');
|
||||
|
||||
// Mock install root: the source-of-truth skill dirs the inventory reads.
|
||||
for (const skill of ['review', 'ship', 'qa']) {
|
||||
fs.mkdirSync(path.join(installRoot, skill), { recursive: true });
|
||||
fs.writeFileSync(path.join(installRoot, skill, 'SKILL.md'), skillMd(skill));
|
||||
}
|
||||
fs.writeFileSync(path.join(installRoot, 'SKILL.md'), skillMd('gstack'));
|
||||
fs.mkdirSync(path.join(mockHome, '.gstack'), { recursive: true });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
fs.rmSync(tmpDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
function runUninstall(): { status: number | null; stdout: string; stderr: string } {
|
||||
const r = spawnSync('bash', [UNINSTALL, '--force'], {
|
||||
stdio: 'pipe',
|
||||
encoding: 'utf-8',
|
||||
env: {
|
||||
...process.env,
|
||||
HOME: mockHome,
|
||||
GSTACK_DIR: installRoot,
|
||||
GSTACK_STATE_DIR: path.join(mockHome, '.gstack'),
|
||||
},
|
||||
cwd: tmpDir, // not a git repo — per-project paths inert
|
||||
timeout: 20_000,
|
||||
});
|
||||
return { status: r.status, stdout: r.stdout, stderr: r.stderr };
|
||||
}
|
||||
|
||||
/** Create a Windows-shape install entry: real dir + real-file SKILL.md. */
|
||||
function realDirEntry(name: string, content: string): string {
|
||||
const dir = path.join(skillsDir, name);
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
fs.writeFileSync(path.join(dir, 'SKILL.md'), content);
|
||||
return dir;
|
||||
}
|
||||
|
||||
describe('gstack-uninstall removes Windows real-dir copies (#2563)', () => {
|
||||
test('inventory name + banner → removed (flat, prefixed, and alias forms)', () => {
|
||||
const review = realDirEntry('review', skillMd('review'));
|
||||
const prefixedShip = realDirEntry('gstack-ship', skillMd('gstack-ship'));
|
||||
const alias = realDirEntry('_gstack-command', skillMd('_gstack-command'));
|
||||
const ogbAlias = realDirEntry('connect-chrome', skillMd('connect-chrome'));
|
||||
|
||||
const r = runUninstall();
|
||||
expect(r.status).toBe(0);
|
||||
expect(fs.existsSync(review)).toBe(false);
|
||||
expect(fs.existsSync(prefixedShip)).toBe(false);
|
||||
expect(fs.existsSync(alias)).toBe(false);
|
||||
expect(fs.existsSync(ogbAlias)).toBe(false);
|
||||
expect(fs.existsSync(installRoot)).toBe(false);
|
||||
});
|
||||
|
||||
test('name NOT in inventory → kept and listed to stderr, even with a banner', () => {
|
||||
const foreign = realDirEntry('my-notes', skillMd('my-notes'));
|
||||
|
||||
const r = runUninstall();
|
||||
expect(r.status).toBe(0);
|
||||
expect(fs.existsSync(foreign)).toBe(true);
|
||||
expect(r.stderr).toContain('my-notes');
|
||||
expect(r.stderr).toContain('left in place');
|
||||
});
|
||||
|
||||
test('no banner → kept and listed, even when the name collides with a gstack skill', () => {
|
||||
// F8's name-collision row: a user's own hand-written ~/.claude/skills/ship.
|
||||
const usersOwn = realDirEntry('ship', skillMd('ship', false));
|
||||
|
||||
const r = runUninstall();
|
||||
expect(r.status).toBe(0);
|
||||
expect(fs.existsSync(usersOwn)).toBe(true);
|
||||
expect(fs.readFileSync(path.join(usersOwn, 'SKILL.md'), 'utf-8')).toContain('name: ship');
|
||||
expect(r.stderr).toContain(path.join('skills', 'ship'));
|
||||
});
|
||||
|
||||
test('real dir without any SKILL.md is untouched and unlisted', () => {
|
||||
const plain = path.join(skillsDir, 'other-tool');
|
||||
fs.mkdirSync(plain, { recursive: true });
|
||||
|
||||
const r = runUninstall();
|
||||
expect(r.status).toBe(0);
|
||||
expect(fs.existsSync(plain)).toBe(true);
|
||||
expect(r.stderr).not.toContain('other-tool');
|
||||
});
|
||||
|
||||
test('a clean sweep reports the removed entries', () => {
|
||||
realDirEntry('review', skillMd('review'));
|
||||
const r = runUninstall();
|
||||
expect(r.status).toBe(0);
|
||||
expect(r.stdout).toContain('claude/review');
|
||||
expect(r.stdout).toContain('gstack uninstalled.');
|
||||
});
|
||||
});
|
||||
|
||||
// symlinkSync needs Developer Mode on Windows runners; the Unix install shape
|
||||
// can't be constructed there. The shape is Unix-only in practice anyway.
|
||||
describe.skipIf(process.platform === 'win32')(
|
||||
'gstack-uninstall removes the Unix real-dir + symlinked-SKILL.md shape',
|
||||
() => {
|
||||
test('SKILL.md symlink pointing into gstack → removed', () => {
|
||||
const dir = path.join(skillsDir, 'qa');
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
fs.symlinkSync(path.join(installRoot, 'qa', 'SKILL.md'), path.join(dir, 'SKILL.md'));
|
||||
|
||||
const r = runUninstall();
|
||||
expect(r.status).toBe(0);
|
||||
expect(fs.existsSync(dir)).toBe(false);
|
||||
});
|
||||
|
||||
test('SKILL.md symlink pointing elsewhere → kept and listed', () => {
|
||||
// Target path must not contain "gstack" anywhere (the provenance match
|
||||
// is a substring check, mirroring setup's cleanup helpers) — the suite
|
||||
// tmpdir prefix does, so use a separate neutral tmpdir.
|
||||
const neutral = fs.mkdtempSync(path.join(os.tmpdir(), 'other-skill-src-'));
|
||||
const elsewhere = path.join(neutral, 'elsewhere.md');
|
||||
fs.writeFileSync(elsewhere, '# not ours\n');
|
||||
const dir = path.join(skillsDir, 'someone-elses');
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
fs.symlinkSync(elsewhere, path.join(dir, 'SKILL.md'));
|
||||
|
||||
try {
|
||||
const r = runUninstall();
|
||||
expect(r.status).toBe(0);
|
||||
expect(fs.existsSync(dir)).toBe(true);
|
||||
expect(r.stderr).toContain('someone-elses');
|
||||
} finally {
|
||||
fs.rmSync(neutral, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
describe('every installable skill SKILL.md carries the generated banner (ENG-OV10)', () => {
|
||||
// The uninstall provenance gate is only sound if the banner is universal:
|
||||
// a bannerless generated skill would be stranded on Windows forever.
|
||||
test('all top-level skill SKILL.md files contain the AUTO-GENERATED banner', () => {
|
||||
const missing: string[] = [];
|
||||
for (const entry of fs.readdirSync(ROOT, { withFileTypes: true })) {
|
||||
if (!entry.isDirectory() && !entry.isSymbolicLink()) continue;
|
||||
const md = path.join(ROOT, entry.name, 'SKILL.md');
|
||||
if (!fs.existsSync(md)) continue;
|
||||
if (!fs.readFileSync(md, 'utf-8').includes('<!-- AUTO-GENERATED from')) {
|
||||
missing.push(entry.name);
|
||||
}
|
||||
}
|
||||
expect(missing).toEqual([]);
|
||||
});
|
||||
|
||||
test('the root router SKILL.md carries the banner too (alias copies inherit it)', () => {
|
||||
expect(fs.readFileSync(path.join(ROOT, 'SKILL.md'), 'utf-8')).toContain(
|
||||
'<!-- AUTO-GENERATED from',
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user