diff --git a/bin/gstack-relink b/bin/gstack-relink index dd2a681fa..75d3337e2 100755 --- a/bin/gstack-relink +++ b/bin/gstack-relink @@ -52,7 +52,13 @@ _link_root_skill_alias() { [ -f "$INSTALL_DIR/SKILL.md" ] || return 0 [ -L "$target" ] && rm -f "$target" mkdir -p "$target" - ln -snf "$INSTALL_DIR/SKILL.md" "$target/SKILL.md" + # Copy-then-rewrite, never a symlink (#2511): a symlinked alias re-serves + # the canonical `name: gstack`, Claude Code sees a duplicate skill name, + # and drops the ENTIRE personal-skills set. sed reads the source and writes + # a fresh copy — remove any prior symlink first so the redirect can never + # write through it into the generated source. + rm -f "$target/SKILL.md" + sed "1,/^---\$/ s/^name:[[:space:]].*/name: _gstack-command/" "$INSTALL_DIR/SKILL.md" > "$target/SKILL.md" } _link_root_skill_alias @@ -61,6 +67,11 @@ _link_root_skill_alias SKILL_COUNT=0 for skill_dir in "$INSTALL_DIR"/*/; do [ -d "$skill_dir" ] || continue + # Skip symlinked skill dirs (connect-chrome → open-gstack-browser): linking + # one under the symlink's basename would duplicate the canonical frontmatter + # name and collide in Claude Code's skill registry (#2201). setup owns the + # rewritten-copy alias for those. + [ -L "${skill_dir%/}" ] && continue skill=$(basename "$skill_dir") # Skip non-skill directories case "$skill" in bin|browse|design|docs|extension|lib|node_modules|scripts|test|.git|.github) continue ;; esac diff --git a/setup b/setup index ab241bc7f..def88155a 100755 --- a/setup +++ b/setup @@ -784,24 +784,40 @@ link_claude_skill_dirs() { fi } +# ─── Helper: install an alias SKILL.md as a rewritten COPY ─────────────────── +# Alias dirs (_gstack-command, connect-chrome) must NOT symlink the canonical +# SKILL.md: the alias then carries the canonical frontmatter name:, Claude Code +# sees two skills with the same name, and drops the ENTIRE personal-skills set +# (#2511, #2201). Copy-then-rewrite instead: sed reads the SOURCE and writes a +# fresh copy with name: set to the alias. It must never edit through an +# existing symlink — that would rewrite the generated source file itself. +_install_alias_skill_md() { + local src_skill_md="$1" + local dst_dir="$2" + local alias_name="$3" + [ -f "$src_skill_md" ] || return 0 + # Old installs left the alias as a whole-dir symlink — replace it. + if [ -L "$dst_dir" ]; then rm -f "$dst_dir"; fi + mkdir -p "$dst_dir" + # Remove any prior symlinked SKILL.md so the redirect below cannot write + # through it into the generated source. + rm -f "$dst_dir/SKILL.md" + sed "1,/^---\$/ s/^name:[[:space:]].*/name: $alias_name/" "$src_skill_md" > "$dst_dir/SKILL.md" +} + # Claude Code skips the repo-shaped ~/.claude/skills/gstack directory when # building the user-facing slash-command list. Keep the repo path for runtime -# assets, and add a separate thin wrapper whose frontmatter name remains -# `gstack` so `/gstack` can autocomplete. +# assets, and add a separate thin wrapper. Its frontmatter name is rewritten to +# `_gstack-command` (the dir name) so it never collides with the canonical +# `gstack` name (#2511). link_claude_root_skill_alias() { local gstack_dir="$1" local skills_dir="$2" local target="$skills_dir/_gstack-command" [ -f "$gstack_dir/SKILL.md" ] || return 0 - if [ -L "$target" ]; then - rm -f "$target" - fi - mkdir -p "$target" - if [ -L "$target/SKILL.md" ]; then rm "$target/SKILL.md"; fi - _link_or_copy "$gstack_dir/SKILL.md" "$target/SKILL.md" + _install_alias_skill_md "$gstack_dir/SKILL.md" "$target" "_gstack-command" echo " linked root skill alias: gstack" - _print_windows_copy_note_once } # ─── Helper: remove old unprefixed Claude skill entries ─────────────────────── @@ -1231,13 +1247,16 @@ if [ "$INSTALL_CLAUDE" -eq 1 ]; then GSTACK_SKILLS_DIR="$INSTALL_SKILLS_DIR" GSTACK_INSTALL_DIR="$SOURCE_GSTACK_DIR" "$GSTACK_RELINK" >/dev/null 2>&1 || true fi # Backwards-compat alias: /connect-chrome → /open-gstack-browser + # Rewritten copy, not a symlink: a symlinked alias re-serves the canonical + # name: open-gstack-browser, so one of the two silently shadows the other + # (#2201) — and duplicate names can drop the whole skill set (#2511). _OGB_LINK="$INSTALL_SKILLS_DIR/connect-chrome" + _OGB_ALIAS_NAME="connect-chrome" if [ "$SKILL_PREFIX" -eq 1 ]; then _OGB_LINK="$INSTALL_SKILLS_DIR/gstack-connect-chrome" + _OGB_ALIAS_NAME="gstack-connect-chrome" fi - if [ -L "$_OGB_LINK" ] || [ ! -e "$_OGB_LINK" ]; then - _link_or_copy "gstack/open-gstack-browser" "$_OGB_LINK" - fi + _install_alias_skill_md "$SOURCE_GSTACK_DIR/open-gstack-browser/SKILL.md" "$_OGB_LINK" "$_OGB_ALIAS_NAME" if [ "$LOCAL_INSTALL" -eq 1 ]; then log "gstack ready (project-local)." log " skills: $INSTALL_SKILLS_DIR" @@ -1299,13 +1318,17 @@ if [ "$INSTALL_CLAUDE" -eq 1 ]; then 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 + # Rewritten copy, not a symlink: a symlinked alias re-serves the + # canonical name: open-gstack-browser, so one of the two silently + # shadows the other (#2201) — and duplicate names can drop the whole + # skill set (#2511). _OGB_LINK="$INSTALL_SKILLS_DIR/connect-chrome" + _OGB_ALIAS_NAME="connect-chrome" if [ "$SKILL_PREFIX" -eq 1 ]; then _OGB_LINK="$INSTALL_SKILLS_DIR/gstack-connect-chrome" + _OGB_ALIAS_NAME="gstack-connect-chrome" fi - if [ -L "$_OGB_LINK" ] || [ ! -e "$_OGB_LINK" ]; then - _link_or_copy "gstack/open-gstack-browser" "$_OGB_LINK" - fi + _install_alias_skill_md "$SOURCE_GSTACK_DIR/open-gstack-browser/SKILL.md" "$_OGB_LINK" "$_OGB_ALIAS_NAME" log "gstack ready (claude)." log " browse: $BROWSE_BIN" fi diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index 1c9bf1fb4..e75571e6f 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -2435,7 +2435,10 @@ describe('setup script validation', () => { const fnEnd = setupContent.indexOf('# ─── Helper: remove old unprefixed Claude skill entries', fnStart); const fnBody = setupContent.slice(fnStart, fnEnd); expect(fnBody).toContain('_gstack-command'); - expect(fnBody).toContain('_link_or_copy "$gstack_dir/SKILL.md" "$target/SKILL.md"'); + // #2511: the alias must be a rewritten COPY (unique frontmatter name), + // never a verbatim symlink of the canonical SKILL.md. + expect(fnBody).toContain('_install_alias_skill_md "$gstack_dir/SKILL.md" "$target" "_gstack-command"'); + expect(fnBody).not.toContain('_link_or_copy "$gstack_dir/SKILL.md"'); const claudeSection = setupContent.slice( setupContent.indexOf('# 4. Install for Claude'), diff --git a/test/relink.test.ts b/test/relink.test.ts index 5e7ec809c..5af335c50 100644 --- a/test/relink.test.ts +++ b/test/relink.test.ts @@ -215,9 +215,15 @@ describe('gstack-relink (#578)', () => { const aliasSkill = path.join(aliasDir, 'SKILL.md'); expect(fs.lstatSync(aliasDir).isDirectory()).toBe(true); expect(fs.lstatSync(aliasDir).isSymbolicLink()).toBe(false); - expect(fs.lstatSync(aliasSkill).isSymbolicLink()).toBe(true); - expect(fs.readlinkSync(aliasSkill)).toBe(path.join(installDir, 'SKILL.md')); - expect(fs.readFileSync(aliasSkill, 'utf-8')).toContain('name: gstack'); + // #2511: the alias is a rewritten COPY, never a symlink. A symlinked + // alias re-serves the canonical `name: gstack`; Claude Code refuses + // duplicate skill names and drops the entire personal-skills set. + expect(fs.lstatSync(aliasSkill).isSymbolicLink()).toBe(false); + const aliasContent = fs.readFileSync(aliasSkill, 'utf-8'); + expect(aliasContent).toContain('name: _gstack-command'); + expect(aliasContent).not.toContain('name: gstack\n'); + // The rewrite happened on the COPY: the canonical source keeps its name. + expect(fs.readFileSync(path.join(installDir, 'SKILL.md'), 'utf-8')).toContain('name: gstack'); run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix true`, { GSTACK_INSTALL_DIR: installDir, @@ -226,6 +232,40 @@ describe('gstack-relink (#578)', () => { expect(fs.existsSync(aliasSkill)).toBe(true); }); + // #2201: connect-chrome ships as a dir SYMLINK to open-gstack-browser. The + // discovery loop used to link it under its own basename while its SKILL.md + // carried `name: open-gstack-browser` — a duplicate name that silently + // shadows the real skill (readdir-order roulette). Symlinked source dirs + // must be skipped; setup owns the rewritten-copy alias. + test('symlinked skill dirs are skipped, so no duplicate frontmatter names (#2201)', () => { + setupMockInstall(['open-gstack-browser', 'qa']); + fs.symlinkSync( + path.join(installDir, 'open-gstack-browser'), + path.join(installDir, 'connect-chrome'), + ); + run(`${path.join(installDir, 'bin', 'gstack-config')} set skill_prefix false`, { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + run(`${path.join(installDir, 'bin', 'gstack-relink')}`, { + GSTACK_INSTALL_DIR: installDir, + GSTACK_SKILLS_DIR: skillsDir, + }); + + expect(fs.existsSync(path.join(skillsDir, 'open-gstack-browser'))).toBe(true); + expect(fs.existsSync(path.join(skillsDir, 'connect-chrome'))).toBe(false); + + // No two installed SKILL.md files may share a frontmatter name. + const names: string[] = []; + for (const entry of fs.readdirSync(skillsDir)) { + const skillMd = path.join(skillsDir, entry, 'SKILL.md'); + if (!fs.existsSync(skillMd)) continue; + const m = fs.readFileSync(skillMd, 'utf-8').match(/^name:\s*(\S+)/m); + if (m) names.push(m[1]); + } + expect(new Set(names).size).toBe(names.length); + }); + // FIRST INSTALL: --no-prefix must create ONLY flat names, zero gstack-* pollution test('first install --no-prefix: only flat names exist, zero gstack-* entries', () => { setupMockInstall(['qa', 'ship', 'review', 'plan-ceo-review', 'gstack-upgrade']); diff --git a/test/setup-alias-name-uniqueness.test.ts b/test/setup-alias-name-uniqueness.test.ts new file mode 100644 index 000000000..c05d1d6d6 --- /dev/null +++ b/test/setup-alias-name-uniqueness.test.ts @@ -0,0 +1,164 @@ +/** + * Alias name uniqueness (#2511 / #2201). + * + * setup installs two back-compat alias dirs — `_gstack-command` (root router) + * and `connect-chrome` (→ open-gstack-browser). Both used to symlink the + * canonical SKILL.md verbatim, so the alias carried the canonical frontmatter + * `name:`. Claude Code keys skills on that name and requires global + * uniqueness: the `connect-chrome` duplicate silently shadowed + * /open-gstack-browser (readdir-order roulette), and the `_gstack-command` + * duplicate could drop the ENTIRE personal-skills set. + * + * The fix is copy-then-rewrite: sed reads the SOURCE and writes a fresh copy + * with `name:` set to the alias dir's own name. Eng review E2 pinned the + * hazard this suite guards hardest: on Unix the old install path was a + * SYMLINK to the repo source, so an in-place sed through it would have + * corrupted the generated SKILL.md — the source files must stay byte-intact. + */ +import { describe, test, expect, beforeAll, afterAll } 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 SETUP_SRC = fs.readFileSync(path.join(ROOT, 'setup'), 'utf-8'); + +function extractFn(name: string): string { + const start = SETUP_SRC.indexOf(`${name}() {`); + const end = SETUP_SRC.indexOf('\n}\n', start); + if (start < 0 || end < 0) throw new Error(`Could not locate ${name}() in setup`); + return SETUP_SRC.slice(start, end + 2); +} + +const installDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-alias-install-')); + +const sourceRootSkill = fs.readFileSync(path.join(ROOT, 'SKILL.md'), 'utf-8'); +const sourceOgbSkill = fs.readFileSync( + path.join(ROOT, 'open-gstack-browser', 'SKILL.md'), + 'utf-8', +); + +beforeAll(() => { + const installOnce = [ + `link_claude_skill_dirs "${ROOT}" "${installDir}"`, + `link_claude_root_skill_alias "${ROOT}" "${installDir}"`, + // The connect-chrome back-compat alias, exactly as the install section does it. + `_install_alias_skill_md "${ROOT}/open-gstack-browser/SKILL.md" "${installDir}/connect-chrome" "connect-chrome"`, + ].join('\n'); + const script = [ + 'set -e', + 'IS_WINDOWS=0', + 'SKILL_PREFIX=0', + 'QUIET=1', + '_WINDOWS_COPY_NOTE_PRINTED=1', + extractFn('_link_or_copy'), + extractFn('_print_windows_copy_note_once'), + extractFn('_link_skill_runtime_assets'), + extractFn('link_claude_skill_dirs'), + extractFn('_install_alias_skill_md'), + extractFn('link_claude_root_skill_alias'), + // Run TWICE: the second pass proves re-runs refresh instead of corrupting + // (the historical failure mode was sed'ing through a symlink on re-run). + installOnce, + installOnce, + ].join('\n'); + const result = spawnSync('bash', ['-c', script], { encoding: 'utf-8', timeout: 60_000 }); + if (result.status !== 0) { + throw new Error(`alias install failed: ${result.stderr}\n${result.stdout}`); + } +}, 30_000); + +afterAll(() => { + fs.rmSync(installDir, { recursive: true, force: true }); +}); + +function frontmatterName(skillMdPath: string): string | null { + const m = fs.readFileSync(skillMdPath, 'utf-8').match(/^name:\s*(\S+)/m); + return m ? m[1] : null; +} + +describe('alias installs are rewritten copies (#2511, #2201)', () => { + test('_gstack-command alias is NOT a symlink and carries its own name', () => { + const aliasDir = path.join(installDir, '_gstack-command'); + const aliasSkill = path.join(aliasDir, 'SKILL.md'); + expect(fs.lstatSync(aliasDir).isSymbolicLink()).toBe(false); + expect(fs.lstatSync(aliasSkill).isSymbolicLink()).toBe(false); + expect(frontmatterName(aliasSkill)).toBe('_gstack-command'); + }); + + test('connect-chrome alias is NOT a symlink and carries its own name', () => { + const aliasDir = path.join(installDir, 'connect-chrome'); + const aliasSkill = path.join(aliasDir, 'SKILL.md'); + expect(fs.lstatSync(aliasDir).isSymbolicLink()).toBe(false); + expect(fs.lstatSync(aliasSkill).isSymbolicLink()).toBe(false); + expect(frontmatterName(aliasSkill)).toBe('connect-chrome'); + }); + + test('alias body is the canonical content — only the name: line differs', () => { + const alias = fs.readFileSync( + path.join(installDir, '_gstack-command', 'SKILL.md'), + 'utf-8', + ); + expect(alias.replace(/^name:.*$/m, 'name: gstack')).toBe(sourceRootSkill); + + const ogbAlias = fs.readFileSync( + path.join(installDir, 'connect-chrome', 'SKILL.md'), + 'utf-8', + ); + expect(ogbAlias.replace(/^name:.*$/m, 'name: open-gstack-browser')).toBe(sourceOgbSkill); + }); + + test('the SOURCE files are byte-intact (E2: sed never wrote through a symlink)', () => { + expect(fs.readFileSync(path.join(ROOT, 'SKILL.md'), 'utf-8')).toBe(sourceRootSkill); + expect( + fs.readFileSync(path.join(ROOT, 'open-gstack-browser', 'SKILL.md'), 'utf-8'), + ).toBe(sourceOgbSkill); + expect(frontmatterName(path.join(ROOT, 'SKILL.md'))).toBe('gstack'); + expect(frontmatterName(path.join(ROOT, 'open-gstack-browser', 'SKILL.md'))).toBe( + 'open-gstack-browser', + ); + }); + + test('every installed skill name is globally unique', () => { + const names: string[] = []; + for (const entry of fs.readdirSync(installDir)) { + const skillMd = path.join(installDir, entry, 'SKILL.md'); + if (!fs.existsSync(skillMd)) continue; + const name = frontmatterName(skillMd); + if (name) names.push(name); + } + expect(names.length).toBeGreaterThan(10); + const dupes = names.filter((n, i) => names.indexOf(n) !== i); + expect(dupes).toEqual([]); + }); + + test('a legacy symlinked alias is replaced, not written through', () => { + // Simulate a pre-fix install: alias SKILL.md is a symlink to the source. + const legacyDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-alias-legacy-')); + try { + const aliasDir = path.join(legacyDir, '_gstack-command'); + fs.mkdirSync(aliasDir); + fs.symlinkSync(path.join(ROOT, 'SKILL.md'), path.join(aliasDir, 'SKILL.md')); + + const script = [ + 'set -e', + 'IS_WINDOWS=0', + extractFn('_link_or_copy'), + extractFn('_install_alias_skill_md'), + extractFn('link_claude_root_skill_alias'), + `link_claude_root_skill_alias "${ROOT}" "${legacyDir}"`, + ].join('\n'); + const result = spawnSync('bash', ['-c', script], { encoding: 'utf-8', timeout: 30_000 }); + expect(result.status).toBe(0); + + const aliasSkill = path.join(aliasDir, 'SKILL.md'); + expect(fs.lstatSync(aliasSkill).isSymbolicLink()).toBe(false); + expect(frontmatterName(aliasSkill)).toBe('_gstack-command'); + // The source the legacy symlink pointed at is untouched. + expect(fs.readFileSync(path.join(ROOT, 'SKILL.md'), 'utf-8')).toBe(sourceRootSkill); + } finally { + fs.rmSync(legacyDir, { recursive: true, force: true }); + } + }); +});