fix(setup): install every skill runtime asset for the Claude host

On a fresh Claude install, link_claude_skill_dirs installed only SKILL.md (+ sections/) per skill. Every skill that reads a sibling runtime file at .claude/skills/<name>/<file> was broken out of the box: /review stopped at 'Read .claude/skills/review/checklist.md' (file never installed), and qa's templates/references, plan-devex-review's dx-hall-of-fame.md, gstack-upgrade's migrations/, and careful/freeze's bin/ hooks were all silently missing. Codex/Factory/OpenCode/Kiro installers already copied these; the primary host never did. Fix: a shared _link_skill_runtime_assets helper installs EVERYTHING a skill ships next to its SKILL.md, with an explicit exclusion list (F7): node_modules, dist, test, *.tmpl, hidden files. Exclusion-list polarity means a newly added asset installs by default instead of being silently dropped. Assets refresh unconditionally on re-run (rm + relink/copy), so Windows real-dir copies pick up changes after git pull. New free test runs the real installer functions against the live repo into a temp skills dir with a TWO-CLASS referenced-paths assertion (ENG-OV7): alias-relative refs (.claude/skills/<name>/<path>) must exist under the install; repo-anchored refs (~/.claude/skills/gstack/<path>) must exist in the tree modulo an explicit built-artifact allowlist (browse/design/ make-pdf dist + the compiled gstack-global-discover). Known-broken class-2 refs (#2250 bare bin names) are ratcheted: the test fails if they quietly start existing without the entry being removed. Fixes #2317 Fixes #2454 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-09-09 14:38:59 +02:00 · 2026-08-16 09:49:30 -07:00
parent 1a1dab2c5a
commit 0f38feee78
2 changed files with 272 additions and 8 deletions
@@ -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/<name>.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
@@ -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/<name>/<file>` — 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/<name>/<relpath>` 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/<relpath>`
+ *     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<string, string> = {
+  // #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, <angle> 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-<name>; 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);
+    }
+  });
+});