From c84246845eb52798bc2127d71662a3a2ddca2f40 Mon Sep 17 00:00:00 2001
From: Garry Tan <garrytan@gmail.com>
Date: Sun, 16 Aug 2026 09:11:25 -0700
Subject: [PATCH] fix(uninstall): remove real-directory skill installs, gated
 on provenance
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
---
 bin/gstack-uninstall                  |  72 ++++++++--
 scripts/test-free-shards.ts           |  10 ++
 test/uninstall-windows-copies.test.ts | 195 ++++++++++++++++++++++++++
 3 files changed, 269 insertions(+), 8 deletions(-)
 create mode 100644 test/uninstall-windows-copies.test.ts

diff --git a/bin/gstack-uninstall b/bin/gstack-uninstall
index 17d7d30bc..c379c2265 100755
--- a/bin/gstack-uninstall
+++ b/bin/gstack-uninstall
@@ -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
diff --git a/scripts/test-free-shards.ts b/scripts/test-free-shards.ts
index a164af2a3..dbc9186a5 100755
--- a/scripts/test-free-shards.ts
+++ b/scripts/test-free-shards.ts
@@ -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
diff --git a/test/uninstall-windows-copies.test.ts b/test/uninstall-windows-copies.test.ts
new file mode 100644
index 000000000..e72fe962f
--- /dev/null
+++ b/test/uninstall-windows-copies.test.ts
@@ -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',
+    );
+  });
+});