From 9850af63bd1bdbaf59f8a752a71f277bc6dc6dcd Mon Sep 17 00:00:00 2001 From: Jonathan Zhang Date: Tue, 14 Jul 2026 16:33:01 -0700 Subject: [PATCH] fix(relink): discover skills structurally so `browse` is materialized MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gstack-relink` gated skill discovery on a hardcoded meta-directory name list (`bin|browse|design|docs|extension|lib|node_modules|scripts|test|.git|.github`). `browse` is a real, live skill — `browse/SKILL.md` (name: browse, v1.1.0), depended on by `benchmark` and `setup-browser-cookies` — so the name collision meant relink never materialized it. It only appeared to work historically because a tracked `skills/browse` symlink in a downstream consumer papered over the gap; once that was removed, `browse` went permanently missing on fresh installs and could not self-heal via relink. `browse` was the only one of the nine non-dot excluded names that actually carries a SKILL.md; the structural guard `[ -f "$skill_dir/SKILL.md" ]` on the next line already handles every genuinely SKILL-less support dir. Drop the redundant name list and let that structural test be the sole gate. This matches gstack's canonical discovery (`scripts/discover-skills.ts`, whose SKIP set is only node_modules/.git/dist) and the companion `gstack-patch-names` (structural, no name list), and future-proofs `design`/`docs`/`test` should any ever become real skills. Adds regression tests: a real-payload assertion that every top-level `*/SKILL.md` materializes (fails on exactly `browse` pre-fix), a future-proofing test over all former-exclusion names, and a companion test that SKILL-less dirs stay skipped. --- bin/gstack-relink | 6 ++-- test/relink.test.ts | 70 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/bin/gstack-relink b/bin/gstack-relink index dd2a681fa..2d839b33b 100755 --- a/bin/gstack-relink +++ b/bin/gstack-relink @@ -62,8 +62,10 @@ SKILL_COUNT=0 for skill_dir in "$INSTALL_DIR"/*/; do [ -d "$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 + # A directory is a skill iff it carries a SKILL.md. This structural test — not a + # hardcoded name list — is the sole gate, so real skills whose names happen to + # collide with support-dir names (e.g. browse) are never dropped, and support dirs + # (bin, lib, node_modules, …) are skipped because they have no SKILL.md. [ -f "$skill_dir/SKILL.md" ] || continue if [ "$PREFIX" = "true" ]; then diff --git a/test/relink.test.ts b/test/relink.test.ts index d83c4cd37..de0d416df 100644 --- a/test/relink.test.ts +++ b/test/relink.test.ts @@ -406,6 +406,76 @@ describe('gstack-relink (#578)', () => { expect(fs.existsSync(path.join(skillsDir, 'gstack-qa'))).toBe(true); expect(fs.existsSync(path.join(skillsDir, 'gstack-ship'))).toBe(true); }); + + // REGRESSION: skill discovery must be structural (has SKILL.md), never name-based. + // A hardcoded meta-dir exclusion list silently dropped `browse` — a real, live skill + // (skills/gstack/browse/SKILL.md, name: browse) that benchmark + setup-browser-cookies + // depend on — because its name collided with a support-dir name in the exclusion list. + // The fix: any top-level dir with a SKILL.md is a skill, regardless of its name. + test('materializes every dir carrying a SKILL.md, even names once meta-excluded (browse)', () => { + // These names were all in the old hardcoded exclusion list. Give each a real + // SKILL.md: every one must now materialize. + const formerlyExcluded = ['bin', 'browse', 'design', 'docs', 'extension', 'lib', 'node_modules', 'scripts', 'test']; + setupMockInstall(['qa', ...formerlyExcluded]); + 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, + }); + for (const skill of ['qa', ...formerlyExcluded]) { + expect({ skill, exists: fs.existsSync(path.join(skillsDir, skill, 'SKILL.md')) }) + .toEqual({ skill, exists: true }); + } + }); + + // Companion invariant: dirs WITHOUT a SKILL.md are still skipped (the structural gate). + test('skips top-level dirs that have no SKILL.md', () => { + setupMockInstall(['qa']); + // Real support dirs carry no SKILL.md — they must not materialize as skills. + for (const meta of ['bin', 'node_modules', 'lib']) { + fs.mkdirSync(path.join(installDir, meta), { recursive: true }); + fs.writeFileSync(path.join(installDir, meta, 'placeholder.txt'), 'not a skill'); + } + 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, 'qa', 'SKILL.md'))).toBe(true); + for (const meta of ['bin', 'node_modules', 'lib']) { + expect({ meta, materialized: fs.existsSync(path.join(skillsDir, meta)) }) + .toEqual({ meta, materialized: false }); + } + }); + + // REGRESSION: against the REAL gstack payload, every skills/gstack/*/SKILL.md must + // materialize. Read-only: relinks into a temp skills dir, never touches the real tree. + // skill_prefix=false → gstack-patch-names is a no-op on already-flat names. + test('real payload: every top-level */SKILL.md materializes (incl. browse)', () => { + const realSkills = fs.readdirSync(ROOT, { withFileTypes: true }) + .filter((d) => d.isDirectory() && !d.name.startsWith('.')) + .filter((d) => fs.existsSync(path.join(ROOT, d.name, 'SKILL.md'))) + .map((d) => d.name); + // Sanity: the payload actually ships browse as a real skill. + expect(realSkills).toContain('browse'); + + const realSkillsDir = path.join(tmpDir, 'real-skills'); + fs.mkdirSync(realSkillsDir, { recursive: true }); + run(`${BIN}/gstack-relink`, { + GSTACK_INSTALL_DIR: ROOT, + GSTACK_SKILLS_DIR: realSkillsDir, + }); + const missing = realSkills.filter( + (s) => !fs.existsSync(path.join(realSkillsDir, s, 'SKILL.md')), + ); + expect(missing).toEqual([]); + }); }); describe('upgrade migrations', () => {