diff --git a/setup b/setup index 51be8f13a..ab241bc7f 100755 --- a/setup +++ b/setup @@ -694,6 +694,41 @@ fi # 3. Ensure ~/.gstack global state directory exists mkdir -p "$HOME/.gstack/projects" +# ─── Helper: link a skill's runtime assets into its installed dir ──────────── +# Installs EVERY runtime asset a skill ships next to its SKILL.md (#2317, +# #2454): review/checklist.md + specialists/, qa/templates + references, +# gstack-upgrade/migrations, careful/bin, freeze/bin, sections/, etc. +# Exclusion list rather than inclusion list (F7) so a new asset file is +# installed by default instead of silently dropped: +# - SKILL.md linked separately by the caller (name-aware) +# - node_modules dependency trees, never a runtime read +# - dist compiled binaries; skills reference them repo-anchored +# (~/.claude/skills/gstack/browse/dist/...), never +# alias-relative, and fresh clones haven't built them +# - test test fixtures +# - *.tmpl generator sources; the generated file is the asset +# - hidden files excluded by the glob (no dotglob) +# Shared so any flattened-skill installer can reuse it (the Claude path is +# the first consumer; codex/factory/opencode install from generated trees). +_link_skill_runtime_assets() { + local src_dir="$1" + local dst_dir="$2" + local asset asset_name + for asset in "$src_dir"/*; do + [ -e "$asset" ] || continue # empty-glob guard + asset_name="$(basename "$asset")" + case "$asset_name" in + SKILL.md|node_modules|dist|test|*.tmpl) continue ;; + esac + # Refresh unconditionally: rm the old entry (symlink OR real copy — the + # Windows install pattern) so re-runs after `git pull` pick up changes. + if [ -e "$dst_dir/$asset_name" ] || [ -L "$dst_dir/$asset_name" ]; then + rm -rf "$dst_dir/$asset_name" + fi + _link_or_copy "$asset" "$dst_dir/$asset_name" + done +} + # ─── Helper: link Claude skill subdirectories into a skills parent directory ── # Creates real directories (not symlinks) at the top level with a SKILL.md symlink # inside. This ensures Claude discovers them as top-level skills, not nested under @@ -732,14 +767,14 @@ link_claude_skill_dirs() { # Validate target isn't a symlink before creating the link if [ -L "$target/SKILL.md" ]; then rm "$target/SKILL.md"; fi _link_or_copy "$gstack_dir/$dir_name/SKILL.md" "$target/SKILL.md" - # Link the sections/ subdir for carved skills (v2 plan T9). The prefixed - # Claude skill dir otherwise holds only SKILL.md, so a runtime - # "Read sections/.md" 404s. Route through _link_or_copy so Windows - # gets a fresh copy (and re-copies on every ./setup, refreshing staleness). - if [ -d "$gstack_dir/$dir_name/sections" ]; then - if [ -e "$target/sections" ] || [ -L "$target/sections" ]; then rm -rf "$target/sections"; fi - _link_or_copy "$gstack_dir/$dir_name/sections" "$target/sections" - fi + # Link every runtime asset the skill ships next to its SKILL.md (#2317, + # #2454): sections/ for carved skills, review's checklist.md + + # specialists/, qa's templates/ + references/, gstack-upgrade's + # migrations/, careful/freeze's bin/, ... Without this, only SKILL.md + # landed and /review 404'd at "Read .claude/skills/review/checklist.md" + # on every fresh Claude install. Routes through _link_or_copy so Windows + # gets real copies refreshed on every ./setup. + _link_skill_runtime_assets "$gstack_dir/$dir_name" "$target" linked+=("$link_name") fi done diff --git a/test/setup-claude-skill-assets.test.ts b/test/setup-claude-skill-assets.test.ts new file mode 100644 index 000000000..6e811bdca --- /dev/null +++ b/test/setup-claude-skill-assets.test.ts @@ -0,0 +1,229 @@ +/** + * Claude installer runtime-asset coverage (#2317 / #2454). + * + * `link_claude_skill_dirs` historically installed only SKILL.md (+ sections/) + * per skill, so every skill that reads a sibling runtime file at + * `.claude/skills//` — review's checklist.md + specialists/, qa's + * templates/ + references/, gstack-upgrade's migrations/, careful/freeze's + * bin/ — was broken on a fresh Claude install. This suite runs the REAL + * installer functions (extracted from `setup`) against the live repo into a + * temp skills dir and asserts the install is complete. + * + * Two-class referenced-paths assertion (eng review ENG-OV7): + * - Class 1 (alias-relative): a `.claude/skills//` reference + * in an INSTALLED SKILL.md must resolve under the install dir. These are + * runtime reads against the flattened alias — a miss is a broken skill. + * - Class 2 (repo-anchored): a `~/.claude/skills/gstack/` + * reference must exist in the source tree, EXCEPT built artifacts + * (browse/dist, design/dist, make-pdf/dist, the compiled + * bin/gstack-global-discover) — the free suite never builds binaries, so + * a naive "every path exists" either false-fails on dist or gets watered + * down to uselessness. + */ +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'); + +/** Built-at-setup artifacts: allowed to be absent from a fresh clone. */ +const BUILT_ARTIFACT_ALLOWLIST = [ + 'browse/dist/', + 'design/dist/', + 'make-pdf/dist/', + 'bin/gstack-global-discover', // compiled from bin/gstack-global-discover.ts at build time +]; + +/** + * Repo-anchored references that are KNOWN BROKEN on the current tree. + * Each entry must name the fix that removes it. An empty list is the goal — + * do not add entries without an issue + a scheduled fix. + */ +const KNOWN_BROKEN_CLASS2: Record = { + // #2250: setup-gbrain's docs call both scripts by bare name; only the .ts + // files exist. Fixed by the wave's c24 (PR #2409 re-derive) — remove these + // entries in that commit. + 'bin/gstack-memory-ingest': '#2250 — fixed by setup-gbrain .ts invocation-path commit', + 'bin/gstack-gbrain-sync': '#2250 — fixed by setup-gbrain .ts invocation-path commit', +}; + +/** Extract a named shell function body (through its closing brace) from setup. */ +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-claude-install-')); + +beforeAll(() => { + 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'), + `link_claude_skill_dirs "${ROOT}" "${installDir}"`, + ].join('\n'); + const result = spawnSync('bash', ['-c', script], { encoding: 'utf-8', timeout: 60_000 }); + if (result.status !== 0) { + throw new Error(`installer functions failed: ${result.stderr}\n${result.stdout}`); + } +}); + +afterAll(() => { + // rmSync does not follow symlinks — the repo sources the links point at survive. + fs.rmSync(installDir, { recursive: true, force: true }); +}); + +function installedSkillDirs(): string[] { + return fs + .readdirSync(installDir, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => e.name) + .filter((name) => fs.existsSync(path.join(installDir, name, 'SKILL.md'))); +} + +describe('link_claude_skill_dirs installs every runtime asset (#2317, #2454)', () => { + test('review skill ships its full runtime asset set', () => { + const review = path.join(installDir, 'review'); + for (const asset of [ + 'checklist.md', + 'design-checklist.md', + 'greptile-triage.md', + 'TODOS-format.md', + 'specialists', + ]) { + expect(fs.existsSync(path.join(review, asset))).toBe(true); + } + // specialists/ resolves to real content, not an empty shell + const specialists = fs.readdirSync(path.join(review, 'specialists')); + expect(specialists.length).toBeGreaterThan(0); + expect(specialists).toContain('testing.md'); + }); + + test('the #2454 affected-skills table is fully installed', () => { + const expected: Array<[string, string]> = [ + ['qa', 'references'], + ['qa', 'templates'], + ['plan-devex-review', 'dx-hall-of-fame.md'], + ['gstack-upgrade', 'migrations'], + ['careful', 'bin'], + ['freeze', 'bin'], + ]; + for (const [skill, asset] of expected) { + expect(fs.existsSync(path.join(installDir, skill, asset))).toBe(true); + } + }); + + test('sections/ still installs for carved skills', () => { + expect(fs.existsSync(path.join(installDir, 'ship', 'sections'))).toBe(true); + expect( + fs.readdirSync(path.join(installDir, 'ship', 'sections')).length, + ).toBeGreaterThan(0); + }); + + test('exclusion list holds: no node_modules, dist, test, or .tmpl installed', () => { + for (const skill of installedSkillDirs()) { + const entries = fs.readdirSync(path.join(installDir, skill)); + expect(entries).not.toContain('node_modules'); + expect(entries).not.toContain('dist'); + expect(entries).not.toContain('test'); + const tmpl = entries.filter((e) => e.endsWith('.tmpl')); + expect(tmpl).toEqual([]); + } + }); + + test('hidden files are not installed', () => { + for (const skill of installedSkillDirs()) { + const hidden = fs + .readdirSync(path.join(installDir, skill)) + .filter((e) => e.startsWith('.')); + expect(hidden).toEqual([]); + } + }); +}); + +// --------------------------------------------------------------------------- +// Two-class referenced-paths assertion (ENG-OV7) +// --------------------------------------------------------------------------- + +interface Ref { + fromSkill: string; + skillName: string; + rel: string; +} + +const REF_RE = /~?\.claude\/skills\/([A-Za-z0-9_-]+)\/([A-Za-z0-9_.\/-]+)/g; + +/** Placeholder-ish captures (globs, template vars, examples) are prose, not paths. */ +function isConcretePath(raw: string): boolean { + return !/[<>*$(){}|]/.test(raw) && !raw.includes('..'); +} + +function collectRefs(): Ref[] { + const refs: Ref[] = []; + for (const skill of installedSkillDirs()) { + const content = fs.readFileSync(path.join(installDir, skill, 'SKILL.md'), 'utf-8'); + for (const m of content.matchAll(REF_RE)) { + const rel = m[2].replace(/[.,:;/]+$/, ''); + if (!rel || !isConcretePath(rel)) continue; + refs.push({ fromSkill: skill, skillName: m[1], rel }); + } + } + return refs; +} + +describe('two-class referenced-paths (ENG-OV7)', () => { + test('class 1: alias-relative references resolve under the install dir', () => { + const missing: string[] = []; + for (const { fromSkill, skillName, rel } of collectRefs()) { + if (skillName === 'gstack') continue; // class 2 + // Prefix-mode prose may reference gstack-; the flat install dir + // is the unprefixed name. + const candidates = [skillName, skillName.replace(/^gstack-/, '')]; + const found = candidates.some((c) => fs.existsSync(path.join(installDir, c, rel))); + if (!found) missing.push(`${fromSkill}/SKILL.md → .claude/skills/${skillName}/${rel}`); + } + expect(missing).toEqual([]); + }); + + test('class 2: repo-anchored references exist in the tree (modulo built artifacts)', () => { + const missing: string[] = []; + for (const { fromSkill, skillName, rel } of collectRefs()) { + if (skillName !== 'gstack') continue; // class 1 + if (rel.startsWith('.')) continue; // runtime state markers (.feature-prompted-*, .git) + if (BUILT_ARTIFACT_ALLOWLIST.some((a) => rel === a || rel.startsWith(a))) continue; + if (KNOWN_BROKEN_CLASS2[rel]) continue; + if (!fs.existsSync(path.join(ROOT, rel))) { + missing.push(`${fromSkill}/SKILL.md → ~/.claude/skills/gstack/${rel}`); + } + } + expect(missing).toEqual([]); + }); + + test('the referenced-path scan actually sees the review checklist refs (self-check)', () => { + // Guard against the extraction regex silently rotting: the review skill is + // KNOWN to carry alias-relative refs; if the scanner stops seeing them the + // class-1 assertion is vacuous. + const class1 = collectRefs().filter((r) => r.skillName !== 'gstack'); + expect(class1.length).toBeGreaterThan(0); + expect(class1.some((r) => r.skillName === 'review' && r.rel === 'checklist.md')).toBe(true); + }); + + test('KNOWN_BROKEN_CLASS2 entries are still actually broken (ratchet)', () => { + // When a fix lands, its entry MUST be removed so the class-2 assertion + // guards the path again. + for (const rel of Object.keys(KNOWN_BROKEN_CLASS2)) { + expect(fs.existsSync(path.join(ROOT, rel))).toBe(false); + } + }); +});