From 6d75c30246c3f794f1f4044d49e31a56bfa16c82 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 11 May 2026 20:21:29 -0700 Subject: [PATCH] fix(setup): skip Claude skill registration when run from a worktree of the global install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a guard before `ln -snf "$SOURCE_GSTACK_DIR" "$HOME/.claude/skills/gstack"` that detects whether the target already exists as a separate real directory. On macOS/BSD, `ln -snf SRC DST` does not replace a real DST — it creates DST/$(basename SRC) → SRC inside it. Running ./setup from each Conductor worktree of the gstack repo was leaking per-worktree child symlinks into the global install, which Claude Code then picked up as separate top-level skills. The guard uses `cd ... && pwd -P` to resolve the existing real dir and compare against the source (mirroring setup's own `SOURCE_GSTACK_DIR` resolution). When they differ, prints a four-line remediation hint naming both paths and exits the Claude registration branch cleanly. Binaries still build locally. The four other code paths through this branch are unchanged: fresh install, retarget an existing symlink, self-rerun where the existing dir resolves to the same source, and --local installs. Includes 8 tests covering static guard placement, `pwd -P` resolution, the remediation message, a behavioral reproduction of the BSD `ln -snf` child- symlink bug, and every branch of the guard (skip on real-dir-elsewhere, allow on fresh, allow on existing symlink, allow on self-rerun). --- setup | 83 +++++++---- test/setup-conductor-worktree.test.ts | 200 ++++++++++++++++++++++++++ 2 files changed, 258 insertions(+), 25 deletions(-) create mode 100644 test/setup-conductor-worktree.test.ts diff --git a/setup b/setup index 4c1763f9f..f812511e4 100755 --- a/setup +++ b/setup @@ -806,35 +806,68 @@ if [ "$INSTALL_CLAUDE" -eq 1 ]; then fi log " browse: $BROWSE_BIN" else - # Not inside a skills/ directory — symlink into ~/.claude/skills/ and retry + # Not inside a skills/ directory — would symlink the source into + # ~/.claude/skills/gstack/ and register from there. CLAUDE_SKILLS_DIR="$HOME/.claude/skills" CLAUDE_GSTACK_LINK="$CLAUDE_SKILLS_DIR/gstack" - mkdir -p "$CLAUDE_SKILLS_DIR" - ln -snf "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK" - log " symlinked $CLAUDE_GSTACK_LINK -> $SOURCE_GSTACK_DIR" - INSTALL_SKILLS_DIR="$CLAUDE_SKILLS_DIR" - INSTALL_GSTACK_DIR="$CLAUDE_GSTACK_LINK" - # Clean up stale symlinks from the opposite prefix mode - if [ "$SKILL_PREFIX" -eq 1 ]; then - cleanup_old_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" + + # Conductor worktree guard: if ~/.claude/skills/gstack is already a real + # (non-symlink) directory pointing to a *different* install, refuse to plant + # a symlink there. On macOS/BSD, `ln -snf SRC DST` won't replace a real DST; + # it creates DST/$(basename SRC) → SRC inside it. The result is per-worktree + # symlinks leaking into the global install that Claude Code picks up as + # separate top-level skills (dublin-v1, lincoln-v2, ...). Typical trigger: + # running ./setup from a Conductor worktree of the gstack repo itself. + _SKIP_CLAUDE_REGISTER=0 + if [ -d "$CLAUDE_GSTACK_LINK" ] && [ ! -L "$CLAUDE_GSTACK_LINK" ]; then + _EXISTING_REAL=$(cd "$CLAUDE_GSTACK_LINK" 2>/dev/null && pwd -P || echo "") + if [ -n "$_EXISTING_REAL" ] && [ "$_EXISTING_REAL" != "$SOURCE_GSTACK_DIR" ]; then + _SKIP_CLAUDE_REGISTER=1 + fi + fi + + if [ "$_SKIP_CLAUDE_REGISTER" -eq 1 ]; then + log "" + log " $CLAUDE_GSTACK_LINK already exists as a separate global install." + log " Skipping Claude skill registration to avoid polluting it with" + log " per-worktree symlinks. (Binaries still built locally for dev.)" + log "" + log " Global install: $CLAUDE_GSTACK_LINK" + log " This worktree: $SOURCE_GSTACK_DIR" + log "" + log " To register this worktree as the active gstack, remove the global" + log " install first: rm -rf $CLAUDE_GSTACK_LINK" + log "" + log "gstack built (claude registration skipped)." + log " browse: $BROWSE_BIN" else - cleanup_prefixed_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" + mkdir -p "$CLAUDE_SKILLS_DIR" + ln -snf "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK" + log " symlinked $CLAUDE_GSTACK_LINK -> $SOURCE_GSTACK_DIR" + INSTALL_SKILLS_DIR="$CLAUDE_SKILLS_DIR" + INSTALL_GSTACK_DIR="$CLAUDE_GSTACK_LINK" + # Clean up stale symlinks from the opposite prefix mode + if [ "$SKILL_PREFIX" -eq 1 ]; then + cleanup_old_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" + else + cleanup_prefixed_claude_symlinks "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" + fi + "$SOURCE_GSTACK_DIR/bin/gstack-patch-names" "$SOURCE_GSTACK_DIR" "$SKILL_PREFIX" + link_claude_skill_dirs "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" + GSTACK_RELINK="$SOURCE_GSTACK_DIR/bin/gstack-relink" + if [ -x "$GSTACK_RELINK" ]; then + GSTACK_SKILLS_DIR="$INSTALL_SKILLS_DIR" GSTACK_INSTALL_DIR="$SOURCE_GSTACK_DIR" "$GSTACK_RELINK" >/dev/null 2>&1 || true + fi + _OGB_LINK="$INSTALL_SKILLS_DIR/connect-chrome" + if [ "$SKILL_PREFIX" -eq 1 ]; then + _OGB_LINK="$INSTALL_SKILLS_DIR/gstack-connect-chrome" + fi + if [ -L "$_OGB_LINK" ] || [ ! -e "$_OGB_LINK" ]; then + ln -snf "gstack/open-gstack-browser" "$_OGB_LINK" + fi + log "gstack ready (claude)." + log " browse: $BROWSE_BIN" fi - "$SOURCE_GSTACK_DIR/bin/gstack-patch-names" "$SOURCE_GSTACK_DIR" "$SKILL_PREFIX" - link_claude_skill_dirs "$SOURCE_GSTACK_DIR" "$INSTALL_SKILLS_DIR" - GSTACK_RELINK="$SOURCE_GSTACK_DIR/bin/gstack-relink" - if [ -x "$GSTACK_RELINK" ]; then - GSTACK_SKILLS_DIR="$INSTALL_SKILLS_DIR" GSTACK_INSTALL_DIR="$SOURCE_GSTACK_DIR" "$GSTACK_RELINK" >/dev/null 2>&1 || true - fi - _OGB_LINK="$INSTALL_SKILLS_DIR/connect-chrome" - if [ "$SKILL_PREFIX" -eq 1 ]; then - _OGB_LINK="$INSTALL_SKILLS_DIR/gstack-connect-chrome" - fi - if [ -L "$_OGB_LINK" ] || [ ! -e "$_OGB_LINK" ]; then - ln -snf "gstack/open-gstack-browser" "$_OGB_LINK" - fi - log "gstack ready (claude)." - log " browse: $BROWSE_BIN" fi fi diff --git a/test/setup-conductor-worktree.test.ts b/test/setup-conductor-worktree.test.ts new file mode 100644 index 000000000..6fb675afc --- /dev/null +++ b/test/setup-conductor-worktree.test.ts @@ -0,0 +1,200 @@ +import { describe, test, expect } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as path from 'path'; +import * as fs from 'fs'; +import * as os from 'os'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const SETUP_SCRIPT = path.join(ROOT, 'setup'); + +describe('setup: Conductor worktree guard', () => { + test('setup contains the real-dir guard before the ln -snf into ~/.claude/skills/', () => { + const content = fs.readFileSync(SETUP_SCRIPT, 'utf-8'); + const guardIdx = content.indexOf('_SKIP_CLAUDE_REGISTER=0'); + const lnIdx = content.indexOf('ln -snf "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK"'); + expect(guardIdx).toBeGreaterThan(-1); + expect(lnIdx).toBeGreaterThan(-1); + expect(guardIdx).toBeLessThan(lnIdx); + }); + + test('guard resolves the existing real dir with `pwd -P` and compares against source', () => { + const content = fs.readFileSync(SETUP_SCRIPT, 'utf-8'); + expect(content).toContain('[ -d "$CLAUDE_GSTACK_LINK" ] && [ ! -L "$CLAUDE_GSTACK_LINK" ]'); + expect(content).toContain('cd "$CLAUDE_GSTACK_LINK" 2>/dev/null && pwd -P'); + expect(content).toContain('"$_EXISTING_REAL" != "$SOURCE_GSTACK_DIR"'); + }); + + test('skip branch prints "registration skipped" + remediation hint', () => { + const content = fs.readFileSync(SETUP_SCRIPT, 'utf-8'); + expect(content).toContain('Skipping Claude skill registration'); + expect(content).toContain('claude registration skipped'); + expect(content).toContain('rm -rf $CLAUDE_GSTACK_LINK'); + }); + + // Reproduce the BSD/macOS `ln -snf` behavior that caused the bug, then + // confirm the guard avoids it. This is a behavioral test of the guard logic + // running in an isolated tmpdir — not the full setup script. + test('BSD ln -snf into an existing real dir creates a child symlink (bug reproduces)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-setup-guard-')); + try { + const source = path.join(tmp, 'source-worktree'); + const dest = path.join(tmp, 'dest-real-dir'); + fs.mkdirSync(source); + fs.mkdirSync(dest); + // The buggy invocation: target dest is an existing real dir. + const result = spawnSync('ln', ['-snf', source, dest], { encoding: 'utf-8' }); + expect(result.status).toBe(0); + // Child symlink leaked inside dest. + const leaked = path.join(dest, path.basename(source)); + expect(fs.existsSync(leaked)).toBe(true); + expect(fs.lstatSync(leaked).isSymbolicLink()).toBe(true); + expect(fs.readlinkSync(leaked)).toBe(source); + // dest itself stayed a real directory (not replaced). + expect(fs.lstatSync(dest).isSymbolicLink()).toBe(false); + expect(fs.lstatSync(dest).isDirectory()).toBe(true); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('guard logic refuses to ln when dest is a real dir pointing elsewhere', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-setup-guard-')); + try { + const source = path.join(tmp, 'source-worktree'); + const dest = path.join(tmp, 'dest-real-dir'); + fs.mkdirSync(source); + fs.mkdirSync(dest); + // Inline the guard logic from setup. If it triggers, $_SKIP=1 is echoed + // and no ln is performed; otherwise ln runs and we'd see the leak. + const script = ` + set -e + SOURCE_GSTACK_DIR='${source}' + CLAUDE_GSTACK_LINK='${dest}' + _SKIP_CLAUDE_REGISTER=0 + if [ -d "$CLAUDE_GSTACK_LINK" ] && [ ! -L "$CLAUDE_GSTACK_LINK" ]; then + _EXISTING_REAL=$(cd "$CLAUDE_GSTACK_LINK" 2>/dev/null && pwd -P || echo "") + if [ -n "$_EXISTING_REAL" ] && [ "$_EXISTING_REAL" != "$SOURCE_GSTACK_DIR" ]; then + _SKIP_CLAUDE_REGISTER=1 + fi + fi + if [ "$_SKIP_CLAUDE_REGISTER" -eq 1 ]; then + echo "SKIP" + else + ln -snf "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK" + echo "LINKED" + fi + `; + const result = spawnSync('bash', ['-c', script], { encoding: 'utf-8' }); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe('SKIP'); + // No child symlink leaked. + const leaked = path.join(dest, path.basename(source)); + expect(fs.existsSync(leaked)).toBe(false); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('guard allows ln when dest does not exist (fresh install path)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-setup-guard-')); + try { + const source = path.join(tmp, 'source-worktree'); + const dest = path.join(tmp, 'fresh-dest'); + fs.mkdirSync(source); + const script = ` + set -e + SOURCE_GSTACK_DIR='${source}' + CLAUDE_GSTACK_LINK='${dest}' + _SKIP_CLAUDE_REGISTER=0 + if [ -d "$CLAUDE_GSTACK_LINK" ] && [ ! -L "$CLAUDE_GSTACK_LINK" ]; then + _EXISTING_REAL=$(cd "$CLAUDE_GSTACK_LINK" 2>/dev/null && pwd -P || echo "") + if [ -n "$_EXISTING_REAL" ] && [ "$_EXISTING_REAL" != "$SOURCE_GSTACK_DIR" ]; then + _SKIP_CLAUDE_REGISTER=1 + fi + fi + if [ "$_SKIP_CLAUDE_REGISTER" -eq 1 ]; then + echo "SKIP" + else + ln -snf "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK" + echo "LINKED" + fi + `; + const result = spawnSync('bash', ['-c', script], { encoding: 'utf-8' }); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe('LINKED'); + expect(fs.lstatSync(dest).isSymbolicLink()).toBe(true); + expect(fs.readlinkSync(dest)).toBe(source); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('guard allows ln when dest is an existing symlink (upgrade-in-place path)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-setup-guard-')); + try { + const source = path.join(tmp, 'new-source'); + const oldSource = path.join(tmp, 'old-source'); + const dest = path.join(tmp, 'dest-symlink'); + fs.mkdirSync(source); + fs.mkdirSync(oldSource); + fs.symlinkSync(oldSource, dest); + // Existing symlink: -L is true, so the guard does NOT trigger. ln -snf + // should atomically retarget the symlink to the new source. + const script = ` + set -e + SOURCE_GSTACK_DIR='${source}' + CLAUDE_GSTACK_LINK='${dest}' + _SKIP_CLAUDE_REGISTER=0 + if [ -d "$CLAUDE_GSTACK_LINK" ] && [ ! -L "$CLAUDE_GSTACK_LINK" ]; then + _EXISTING_REAL=$(cd "$CLAUDE_GSTACK_LINK" 2>/dev/null && pwd -P || echo "") + if [ -n "$_EXISTING_REAL" ] && [ "$_EXISTING_REAL" != "$SOURCE_GSTACK_DIR" ]; then + _SKIP_CLAUDE_REGISTER=1 + fi + fi + if [ "$_SKIP_CLAUDE_REGISTER" -eq 1 ]; then + echo "SKIP" + else + ln -snf "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK" + echo "LINKED" + fi + `; + const result = spawnSync('bash', ['-c', script], { encoding: 'utf-8' }); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe('LINKED'); + expect(fs.readlinkSync(dest)).toBe(source); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('guard allows ln when dest is a real dir already pointing to source (self-rerun)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-setup-guard-')); + try { + const source = path.join(tmp, 'source-worktree'); + fs.mkdirSync(source); + // Mirror setup's SOURCE_GSTACK_DIR resolution (`pwd -P`) so the comparison + // is fair on macOS where /tmp itself is a symlink to /private/tmp. + const resolvedSource = fs.realpathSync(source); + // Degenerate case: existing real dir IS the source. + const dest = source; + const script = ` + set -e + SOURCE_GSTACK_DIR='${resolvedSource}' + CLAUDE_GSTACK_LINK='${dest}' + _SKIP_CLAUDE_REGISTER=0 + if [ -d "$CLAUDE_GSTACK_LINK" ] && [ ! -L "$CLAUDE_GSTACK_LINK" ]; then + _EXISTING_REAL=$(cd "$CLAUDE_GSTACK_LINK" 2>/dev/null && pwd -P || echo "") + if [ -n "$_EXISTING_REAL" ] && [ "$_EXISTING_REAL" != "$SOURCE_GSTACK_DIR" ]; then + _SKIP_CLAUDE_REGISTER=1 + fi + fi + echo "skip=$_SKIP_CLAUDE_REGISTER" + `; + const result = spawnSync('bash', ['-c', script], { encoding: 'utf-8' }); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe('skip=0'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); +});