diff --git a/.changeset/wise-pumas-glide.md b/.changeset/wise-pumas-glide.md new file mode 100644 index 000000000..f7b198211 --- /dev/null +++ b/.changeset/wise-pumas-glide.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3766 +--- +**`applySurface` now prunes `~/.claude/skills/gsd-STEM/` directories on cluster disable** — matches the install/uninstall behavior; disabled clusters were leaving stale skill dirs on disk because the surface.md spec directed the AI to use the skills sub-directory as `runtimeConfigDir` instead of the base config dir. Extracts `pruneSkillDirs()` as the single point of truth for skill-dir removal. diff --git a/commands/gsd/surface.md b/commands/gsd/surface.md index 93aa3c45a..1f6217364 100644 --- a/commands/gsd/surface.md +++ b/commands/gsd/surface.md @@ -10,8 +10,9 @@ requires: [config, update] --- -Manage the runtime skill surface without reinstall. Reads/writes `~/.claude/skills/.gsd-surface.json` -(sibling to `.gsd-profile`) and re-stages the active commands/gsd directory in place. +Manage the runtime skill surface without reinstall. Reads/writes `~/.claude/.gsd-surface.json` +(sibling to `~/.claude/.gsd-profile`) and re-stages the active skills directory in place. +Skill dirs live at `~/.claude/skills/gsd-*/`. Sub-commands: list · status · profile · disable · enable · reset @@ -114,15 +115,27 @@ Valid cluster names: `core_loop`, `audit_review`, `milestone`, `research_ideate` ## runtimeConfigDir resolution +The `runtimeConfigDir` for `applySurface` is the **base Claude config directory** +(`~/.claude`), NOT the skills sub-directory (`~/.claude/skills`). + +This matches `installRuntimeArtifacts` and `uninstallRuntimeArtifacts`, which also +receive `~/.claude` as `configDir`. The skill dirs themselves live at +`~/.claude/skills/gsd-*/` because the `claude global` layout has `destSubpath = +'skills'` — they are derived from `configDir`, not the root for it. + ```bash -# Claude Code -RUNTIME_CONFIG_DIR=~/.claude/skills +# Claude Code — global install +RUNTIME_CONFIG_DIR="${CLAUDE_CONFIG_DIR:-$HOME/.claude}" +SCOPE="global" # Artifact destinations are derived from runtime layout -# via resolveRuntimeArtifactLayout(runtime, RUNTIME_CONFIG_DIR, scope) +# via resolveRuntimeArtifactLayout(runtime, RUNTIME_CONFIG_DIR, SCOPE) # then applySurface(RUNTIME_CONFIG_DIR, layout, manifest, CLUSTERS) ``` +Surface state is stored at `${RUNTIME_CONFIG_DIR}/.gsd-surface.json` +(i.e. `~/.claude/.gsd-surface.json`). + All paths can be overridden by reading the `CLAUDE_CONFIG_DIR` env var if set. --- @@ -134,8 +147,9 @@ All paths can be overridden by reading the `CLAUDE_CONFIG_DIR` env var if set. - Missing `surface.cjs` → prompt: "Run `npm i -g get-shit-done` to reinstall GSD." -Surface state file: `~/.claude/skills/.gsd-surface.json` -Install profile marker: `~/.claude/skills/.gsd-profile` +Surface state file: `~/.claude/.gsd-surface.json` +Install profile marker: `~/.claude/.gsd-profile` +Skill dirs: `~/.claude/skills/gsd-*/` Engine module: `~/.claude/get-shit-done/bin/lib/surface.cjs` Cluster definitions: `~/.claude/get-shit-done/bin/lib/clusters.cjs` diff --git a/get-shit-done/bin/lib/surface.cjs b/get-shit-done/bin/lib/surface.cjs index 3ca70d71d..83fb8ab21 100644 --- a/get-shit-done/bin/lib/surface.cjs +++ b/get-shit-done/bin/lib/surface.cjs @@ -3,7 +3,7 @@ * Runtime surface module — ADR-0011 Phase 2 (Option B). * * Manages the runtime enable/disable surface state (the `.gsd-surface.json` marker in - * each runtime's skills dir) independently of the install-time profile marker + * each runtime's config dir root (e.g., ~/.claude)) independently of the install-time profile marker * (`.gsd-profile`). Runtime config locations are resolved by callers. * * Effective skill set = base profile ∪ explicitAdds − disabledClusters − explicitRemoves, @@ -15,6 +15,7 @@ * resolveSurface(runtimeConfigDir, manifest, clusterMap) * applySurface(runtimeConfigDir, layout, manifest, clusterMap) * listSurface(runtimeConfigDir, layout, manifest, clusterMap) + * pruneSkillDirs(skillsDir, retainedNames, prefix, manifest) */ const fs = require('fs'); @@ -216,6 +217,89 @@ function applySurface(runtimeConfigDir, layout, manifest, clusterMap) { return resolved; } +/** + * Prune GSD-managed skill directories from a skills directory. + * + * Removes every directory in `skillsDir` that is GSD-owned but NOT listed + * in `retainedNames`. User-owned dirs (not matching the GSD ownership criteria) + * are always preserved. + * + * Ownership criteria: + * - Non-empty prefix (e.g. 'gsd-'): dir name starts with that prefix AND + * appears in the manifest (manifest membership is required). Dirs that match + * the prefix but are NOT in the manifest are treated as user-owned and + * preserved — this prevents data loss for user-created gsd-* directories. + * A warning is written to stderr when such a dir is encountered. + * - Empty prefix (Hermes): dir name appears as a canonical skill stem in the + * manifest. User dirs not in the manifest are preserved. + * - Empty prefix without manifest, or manifest not a Map: conservative; no + * dirs are removed. + * + * This is the single point of truth for skill-dir pruning. Both _syncGsdDir + * (surface apply) and callers that need stand-alone pruning use this function. + * + * @param {string} skillsDir directory that contains the gsd-STEM sub-dirs + * @param {Set} retainedNames set of directory names to keep (e.g. 'gsd-help') + * @param {string} prefix GSD dir prefix, e.g. 'gsd-' (or '' for Hermes) + * @param {Map} [manifest] optional; required for Hermes empty-prefix case + * and for manifest-membership gate in prefixed case. + * Must be a Map; any other type is treated as missing. + */ +function pruneSkillDirs(skillsDir, retainedNames, prefix, manifest) { + if (!fs.existsSync(skillsDir)) return; + + // Finding 2: guard against callers passing a truthy non-Map as manifest. + // A non-Map manifest would throw on .keys(); treat it as absent and be conservative. + const safeManifest = (manifest instanceof Map) ? manifest : null; + + // Build the canonical stem set from the manifest (used for both prefixed and Hermes paths). + // Deletion requires manifest membership — without a valid manifest, be conservative. + const canonicalStems = safeManifest + ? new Set([...safeManifest.keys()].filter(k => !k.startsWith('_calls_agents_'))) + : null; + + for (const entry of fs.readdirSync(skillsDir)) { + const entryPath = path.join(skillsDir, entry); + if (!fs.statSync(entryPath).isDirectory()) continue; + + let isGsdOwned; + if (prefix !== '') { + if (!entry.startsWith(prefix)) { + // Does not match prefix at all — user-owned, preserve. + continue; + } + if (!canonicalStems) { + // No manifest available: cannot confirm ownership — preserve conservatively. + continue; + } + // Finding 1 fix: prefix match is necessary but NOT sufficient. + // The dir must also be in the manifest to be considered GSD-owned. + // A user-created gsd-* dir that isn't in the manifest is preserved with a warning. + if (!canonicalStems.has(entry.slice(prefix.length))) { + process.stderr.write( + `[gsd] Warning: ${entry} matches GSD prefix '${prefix}' but is not in the manifest — preserving (user-owned or unknown)\n` + ); + continue; + } + isGsdOwned = true; + } else if (canonicalStems) { + // Hermes: GSD-owned iff the directory name appears in the canonical manifest. + isGsdOwned = canonicalStems.has(entry); + } else { + // No manifest available: be conservative, don't remove anything. + continue; + } + + if (!isGsdOwned) continue; // Hermes path only: preserve user-owned dirs not in manifest + if (retainedNames.has(entry)) continue; // GSD-owned and in retain set + try { + fs.rmSync(entryPath, { recursive: true, force: true }); + } catch (err) { + process.stderr.write(`surface: failed to prune ${entryPath}: ${err.message}\n`); + } + } +} + /** * Sync destination directory from staged source. * @@ -223,7 +307,7 @@ function applySurface(runtimeConfigDir, layout, manifest, clusterMap) { * For 'agents' kind: same, but only remove files starting with 'gsd-' prefix. * For 'skills' kind: iterate directories in destDir matching kind.prefix; add missing * by copying recursively; remove dirs not in staged set. Preserves dirs not matching - * the prefix (user-owned skills). + * the prefix (user-owned skills). Pruning is delegated to pruneSkillDirs(). * * For Hermes (empty prefix): uses manifest membership to discriminate GSD-owned vs * user-owned dirs. GSD-owned = stem in manifest; removal targets = in manifest AND @@ -251,47 +335,15 @@ function _syncGsdDir(stagedDir, destDir, kind, manifest) { }) ); - // Copy missing dirs from staged to dest + // Copy missing dirs from staged to dest (always overwrite to ensure content is current) for (const dirName of stagedDirs) { const destSubDir = path.join(destDir, dirName); - if (!fs.existsSync(destSubDir)) { - fs.cpSync(path.join(stagedDir, dirName), destSubDir, { recursive: true }); - } else { - // Overwrite to ensure content is current - fs.cpSync(path.join(stagedDir, dirName), destSubDir, { recursive: true }); - } + fs.cpSync(path.join(stagedDir, dirName), destSubDir, { recursive: true }); } - // Removal: discriminator depends on prefix shape. - // Non-empty prefix: GSD namespace IS the prefix; remove prefix-matching dirs not in staged set. - // Empty prefix (Hermes): GSD-owned = stem in manifest (i.e. canonically-shipped GSD skill). - // User-owned skills not in manifest are preserved. - // No manifest available: be conservative, don't remove anything. - const canonicalStems = manifest - ? new Set([...manifest.keys()].filter(k => !k.startsWith('_calls_agents_'))) - : null; - - const destEntries = fs.readdirSync(destDir); - for (const entry of destEntries) { - const entryPath = path.join(destDir, entry); - if (!fs.statSync(entryPath).isDirectory()) continue; - - let isGsdOwned; - if (kindPrefix !== '') { - isGsdOwned = entry.startsWith(kindPrefix); - } else if (canonicalStems) { - // Hermes: empty prefix, destSubpath is the namespace. - // GSD-owned iff the directory name (stem) appears in the canonical manifest. - isGsdOwned = canonicalStems.has(entry); - } else { - // No manifest available: be conservative, don't remove anything. - continue; - } - - if (!isGsdOwned) continue; // preserve user-owned - if (stagedDirs.has(entry)) continue; // current GSD-owned, keep - try { fs.rmSync(entryPath, { recursive: true, force: true }); } catch {} - } + // Prune GSD-owned dirs that are no longer in the staged set. + // pruneSkillDirs() is the single point of truth for this logic. + pruneSkillDirs(destDir, stagedDirs, kindPrefix, manifest); } else { // commands / agents kind: work with .md files const stagedFiles = new Set( @@ -372,6 +424,7 @@ module.exports = { resolveSurface, applySurface, listSurface, - // Exported for testing + // Exported for testing and for callers that need stand-alone pruning + pruneSkillDirs, _syncGsdDir, }; diff --git a/tests/bug-3659-applysurface-prune-skill-dirs.test.cjs b/tests/bug-3659-applysurface-prune-skill-dirs.test.cjs new file mode 100644 index 000000000..bb1b31a07 --- /dev/null +++ b/tests/bug-3659-applysurface-prune-skill-dirs.test.cjs @@ -0,0 +1,257 @@ +'use strict'; +/** + * Regression test for bug #3659 + * + * applySurface did not prune ~/.claude/skills/gsd-STEM dirs when a cluster + * was disabled. install/uninstall both prune correctly via _removeGsdEntries; + * applySurface called _syncGsdDir with the right logic but the surface.md spec + * directed the AI to use RUNTIME_CONFIG_DIR=~/.claude/skills (the skills dir + * itself) instead of the base Claude config dir (~/.claude). + * + * When runtimeConfigDir = ~/.claude/skills and scope = 'global': + * kind.destSubpath = 'skills' + * dest = path.join('~/.claude/skills', 'skills') = ~/.claude/skills/skills WRONG + * + * The pruning ran against the wrong (non-existent) dir so stale gsd-Y dirs + * were never removed from ~/.claude/skills/. + * + * Fix: + * 1. surface.md RUNTIME_CONFIG_DIR changed to use the base Claude config dir + * (getGlobalDir('claude') = ~/.claude), not ~/.claude/skills. + * 2. Surface state file moves to /.gsd-surface.json at the config + * root, matching install/uninstall conventions. + * 3. applySurface is called with scope='global' so the skills kind is active. + * + * Tests: + * a) disabled cluster gsd-STEM dirs are REMOVED from ~/.claude/skills/ + * b) gsd-STEM dirs in the retain set are preserved + * c) non-gsd dirs are UNTOUCHED (user-owned) + * d) idempotence: running applySurface twice produces the same on-disk state + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { writeSurface, applySurface } = require('../get-shit-done/bin/lib/surface.cjs'); +const { loadSkillsManifest } = require('../get-shit-done/bin/lib/install-profiles.cjs'); +const { CLUSTERS } = require('../get-shit-done/bin/lib/clusters.cjs'); +const { resolveRuntimeArtifactLayout } = require('../get-shit-done/bin/lib/runtime-artifact-layout.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const REAL_COMMANDS_DIR = path.join(__dirname, '..', 'commands', 'gsd'); + +/** + * Build a minimal fixture simulating a Claude global install. + * + * configDir — analogous to ~/.claude + * skillsDir — analogous to ~/.claude/skills (contains gsd-* dirs) + * + * Pre-populated with: + * gsd-explore/SKILL.md — in research_ideate cluster (will be disabled) + * gsd-help/SKILL.md — in core_loop cluster (will remain enabled) + * my-custom-skill/ — user-owned, not gsd-prefixed (must never be touched) + */ +function createFixture() { + const configDir = createTempDir('gsd-bug3659-'); + const skillsDir = path.join(configDir, 'skills'); + fs.mkdirSync(skillsDir, { recursive: true }); + + const gsdExplore = path.join(skillsDir, 'gsd-explore'); + const gsdHelp = path.join(skillsDir, 'gsd-help'); + const userSkill = path.join(skillsDir, 'my-custom-skill'); + + for (const d of [gsdExplore, gsdHelp, userSkill]) { + fs.mkdirSync(d, { recursive: true }); + fs.writeFileSync(path.join(d, 'SKILL.md'), '# skill\n', 'utf8'); + } + + return { configDir, skillsDir, gsdExplore, gsdHelp, userSkill }; +} + +/** + * Extended fixture that also includes a user-created gsd-* directory. + * Used by the all-clusters-disabled counter-test to prove the manifest-membership + * gate (Finding 1 fix) protects user-owned gsd-* dirs from data loss. + */ +function createFixtureWithUserGsdDir() { + const base = createFixture(); + const userGsdDir = path.join(base.skillsDir, 'gsd-mything'); + fs.mkdirSync(userGsdDir, { recursive: true }); + fs.writeFileSync(path.join(userGsdDir, 'SKILL.md'), '# user skill\n', 'utf8'); + return { ...base, userGsdDir }; +} + +describe('bug-3659: applySurface prunes ~/.claude/skills/gsd-*/ on cluster disable', () => { + test('(a) disabled cluster gsd-* dirs are removed from skills dir', (t) => { + const { configDir, skillsDir, gsdExplore, gsdHelp } = createFixture(); + t.after(() => cleanup(configDir)); + + // Surface state at configDir (= ~/.claude), NOT at skillsDir (= ~/.claude/skills). + // This is the corrected location after the fix. + writeSurface(configDir, { + baseProfile: 'full', + disabledClusters: ['research_ideate'], // contains 'explore' + explicitAdds: [], + explicitRemoves: [], + }); + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + // scope='global' gives the skills kind for Claude (destSubpath='skills', prefix='gsd-') + const layout = resolveRuntimeArtifactLayout('claude', configDir, 'global'); + applySurface(configDir, layout, manifest, CLUSTERS); + + // gsd-explore is in the research_ideate cluster which was disabled: + // it must be pruned from skillsDir. + assert.ok( + !fs.existsSync(gsdExplore), + 'gsd-explore/ must be removed from skills dir when research_ideate cluster is disabled' + ); + + // gsd-help is in core_loop (not disabled) and must survive. + assert.ok( + fs.existsSync(gsdHelp), + 'gsd-help/ must be preserved when its cluster is not disabled' + ); + }); + + test('(b) gsd-* dirs in retained clusters are preserved', (t) => { + const { configDir, skillsDir, gsdHelp } = createFixture(); + t.after(() => cleanup(configDir)); + + // Disable a cluster that does NOT include help (core_loop has help) + writeSurface(configDir, { + baseProfile: 'full', + disabledClusters: ['research_ideate'], + explicitAdds: [], + explicitRemoves: [], + }); + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + const layout = resolveRuntimeArtifactLayout('claude', configDir, 'global'); + applySurface(configDir, layout, manifest, CLUSTERS); + + assert.ok( + fs.existsSync(gsdHelp), + 'gsd-help/ must be preserved — core_loop cluster remains enabled' + ); + }); + + test('(c) non-gsd user dirs are untouched', (t) => { + const { configDir, skillsDir, userSkill } = createFixture(); + t.after(() => cleanup(configDir)); + + writeSurface(configDir, { + baseProfile: 'full', + disabledClusters: ['research_ideate'], + explicitAdds: [], + explicitRemoves: [], + }); + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + const layout = resolveRuntimeArtifactLayout('claude', configDir, 'global'); + applySurface(configDir, layout, manifest, CLUSTERS); + + assert.ok( + fs.existsSync(userSkill), + 'my-custom-skill/ (non-gsd user dir) must be preserved by applySurface' + ); + assert.ok( + fs.existsSync(path.join(userSkill, 'SKILL.md')), + 'user skill SKILL.md must be untouched' + ); + }); + + test('(d) idempotence: running applySurface twice produces identical on-disk state', (t) => { + const { configDir, skillsDir, gsdExplore, gsdHelp, userSkill } = createFixture(); + t.after(() => cleanup(configDir)); + + writeSurface(configDir, { + baseProfile: 'full', + disabledClusters: ['research_ideate'], + explicitAdds: [], + explicitRemoves: [], + }); + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + const layout = resolveRuntimeArtifactLayout('claude', configDir, 'global'); + + // First apply + applySurface(configDir, layout, manifest, CLUSTERS); + const afterFirst = fs.readdirSync(skillsDir).sort(); + + // Second apply — must produce exactly the same set + applySurface(configDir, layout, manifest, CLUSTERS); + const afterSecond = fs.readdirSync(skillsDir).sort(); + + assert.deepStrictEqual( + afterSecond, + afterFirst, + 'skills dir contents must be identical after two consecutive applySurface calls (idempotent)' + ); + + // Double-check the pruned dir is gone after both runs + assert.ok( + !fs.existsSync(gsdExplore), + 'gsd-explore/ must remain absent after second applySurface call' + ); + + // User dir must survive both runs + assert.ok( + fs.existsSync(userSkill), + 'my-custom-skill/ must survive both applySurface calls' + ); + }); + + test('(e) all-clusters-disabled: all gsd-owned dirs removed; user dirs and user gsd-* dirs survive', (t) => { + // Counter-test for Finding 1 (data-loss class) and Finding 3 (missing coverage). + // + // Disables EVERY cluster so the resolved skill set is empty. + // Assertions: + // 1. gsd-explore/ — GSD-owned, disabled cluster → REMOVED + // 2. gsd-help/ — GSD-owned, disabled cluster → REMOVED + // 3. my-custom-skill/ — user-owned, no gsd- prefix → PRESERVED + // 4. gsd-mything/ — prefix match but NOT in manifest → PRESERVED (Finding 1 fix) + const { configDir, skillsDir, gsdExplore, gsdHelp, userSkill, userGsdDir } = + createFixtureWithUserGsdDir(); + t.after(() => cleanup(configDir)); + + const allClusters = Object.keys(CLUSTERS); + + writeSurface(configDir, { + baseProfile: 'full', + disabledClusters: allClusters, + explicitAdds: [], + explicitRemoves: [], + }); + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + const layout = resolveRuntimeArtifactLayout('claude', configDir, 'global'); + applySurface(configDir, layout, manifest, CLUSTERS); + + // 1. GSD-owned dirs in now-disabled clusters must be removed. + assert.ok( + !fs.existsSync(gsdExplore), + 'gsd-explore/ must be removed when all clusters are disabled' + ); + assert.ok( + !fs.existsSync(gsdHelp), + 'gsd-help/ must be removed when all clusters are disabled' + ); + + // 2. Non-gsd user dir must be preserved regardless. + assert.ok( + fs.existsSync(userSkill), + 'my-custom-skill/ (non-gsd user dir) must survive when all clusters are disabled' + ); + + // 3. User-created gsd-* dir NOT in the manifest must be preserved. + // This is the critical Finding 1 regression guard: without the manifest-membership + // gate, gsd-mything/ would have been silently deleted. + assert.ok( + fs.existsSync(userGsdDir), + 'gsd-mything/ (user-created gsd-* dir not in manifest) must be preserved — ' + + 'prefix match alone must not trigger deletion (Finding 1 data-loss fix)' + ); + }); +}); diff --git a/tests/runtime-artifact-layout-surface.test.cjs b/tests/runtime-artifact-layout-surface.test.cjs index 51fbb101c..2436d21e1 100644 --- a/tests/runtime-artifact-layout-surface.test.cjs +++ b/tests/runtime-artifact-layout-surface.test.cjs @@ -196,10 +196,22 @@ describe('applySurface', () => { fs.writeFileSync(path.join(foreignDir, 'SKILL.md'), '# custom\n', 'utf8'); const skillsKind = { kind: 'skills', destSubpath: 'skills', prefix: 'gsd-', stage: () => stagedDir }; - _syncGsdDir(stagedDir, destDir, skillsKind); + + // Build a minimal manifest that includes the GSD-owned stems so that the + // manifest-membership gate (Finding 1 fix) correctly identifies gsd-old-skill + // as GSD-owned and prunes it. Without a manifest the new code conservatively + // preserves all gsd-* dirs it cannot confirm are GSD-owned. + const manifest = new Map([ + ['help', []], + ['update', []], + ['old-skill', []], // GSD-owned stale stem — must be pruned when not in staged set + ]); + + _syncGsdDir(stagedDir, destDir, skillsKind, manifest); assert.ok(fs.existsSync(path.join(destDir, stem1, 'SKILL.md')), 'gsd-help/SKILL.md should be copied'); assert.ok(fs.existsSync(path.join(destDir, stem2, 'SKILL.md')), 'gsd-update/SKILL.md should be copied'); + // stale gsd- dir removed (it's in the manifest so it is GSD-owned, but not in staged set) assert.ok(!fs.existsSync(staleDir), 'stale gsd-old-skill dir should be removed'); assert.ok(fs.existsSync(foreignDir), 'my-custom-skill dir should be preserved'); });