From b8fa89b5b669afbbdc5f6a98d51547a1545aa5a7 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 17 May 2026 01:39:10 -0400 Subject: [PATCH] fix(3663): address CodeRabbit surface/layout follow-ups --- commands/gsd/surface.md | 6 ++-- .../3660-runtime-artifact-layout-module.md | 3 +- get-shit-done/bin/lib/surface.cjs | 5 +++- tests/surface-apply.test.cjs | 28 ++++++++++++------- 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/commands/gsd/surface.md b/commands/gsd/surface.md index 2d329b69d..93aa3c45a 100644 --- a/commands/gsd/surface.md +++ b/commands/gsd/surface.md @@ -118,9 +118,9 @@ Valid cluster names: `core_loop`, `audit_review`, `milestone`, `research_ideate` # Claude Code RUNTIME_CONFIG_DIR=~/.claude/skills -# Resolve commandsDir and agentsDir -COMMANDS_DIR=~/.claude/commands/gsd -AGENTS_DIR=~/.claude/agents +# Artifact destinations are derived from runtime layout +# via resolveRuntimeArtifactLayout(runtime, RUNTIME_CONFIG_DIR, scope) +# then applySurface(RUNTIME_CONFIG_DIR, layout, manifest, CLUSTERS) ``` All paths can be overridden by reading the `CLAUDE_CONFIG_DIR` env var if set. diff --git a/docs/adr/3660-runtime-artifact-layout-module.md b/docs/adr/3660-runtime-artifact-layout-module.md index 471dd5400..b3001090d 100644 --- a/docs/adr/3660-runtime-artifact-layout-module.md +++ b/docs/adr/3660-runtime-artifact-layout-module.md @@ -18,7 +18,7 @@ The root problem is the absence of a typed seam for "where does runtime R put ar - The `kinds` array is empty for runtimes with no GSD surface (a hypothetical future runtime with no integration). The `skills` kind is **absent** for runtimes that don't materialize skill directories (Cline; Gemini today). The `commands` kind is **absent** for runtimes that consume only the skills/agents layout (Claude global, Codex, etc.). - Per-runtime quirks live in the layout's record fields, not in caller branches: - **Hermes**: `{ kind: 'skills', destSubpath: 'skills/gsd', prefix: '' }` — preserves the nested namespace from #2841. - - **Cline**: `kinds: [ { kind: 'commands', … } ]` — no skills kind in the array. + - **Cline**: `kinds: []` — Cline resolves to zero kinds in Phase 1 (no `commands` kind). - **Gemini**: `kinds: [ { kind: 'commands', destSubpath: 'commands/gsd', prefix: 'gsd-' } ]` — no agents, no skills. - `applySurface` migrates from `(runtimeConfigDir, commandsDir, agentsDir, manifest, clusterMap)` to `(runtimeConfigDir, layout, manifest, clusterMap)`. Body collapses to `for (const kind of layout.kinds) _syncGsdDir(kind.stage(resolved), path.join(layout.configDir, kind.destSubpath), kind.kind)`. - `_findInstallSource` and `_findAgentsSource` in `surface.cjs` are removed. The layout owns source resolution. @@ -140,6 +140,7 @@ function applySurface(runtimeConfigDir, layout, manifest, clusterMap) { Phase 1 implementation landed on `feat/3663-runtime-artifact-layout-module-phase-1-m`: - `get-shit-done/bin/lib/runtime-artifact-layout.cjs` — 15-runtime layout table (grok intentionally excluded), `resolveRuntimeArtifactLayout(runtime, configDir, scope) → Layout`, walk-up `findInstallSourceRoot` helper. +- Clarification: in this Phase 1 implementation, **Cline resolves to zero kinds** (`kinds: []`), so it carries no `commands` kind in the layout table. - `get-shit-done/bin/lib/install-profiles.cjs` — new `stageSkillsForRuntimeAsSkills(srcCommandsDir, resolvedProfile, converter, prefix) → stagedDir` helper. - `get-shit-done/bin/lib/surface.cjs` — `applySurface(runtimeConfigDir, layout, manifest, clusterMap)` signature migration; `_findInstallSource` + `_findAgentsSource` deleted; `_syncGsdDir` extended to handle the `skills` kind via directory iteration. - Tests: `runtime-artifact-layout-resolve.test.cjs` (16), `runtime-artifact-layout-edge-cases.test.cjs` (10), `runtime-artifact-layout-stage.test.cjs` (5), `install-profiles-stage.test.cjs` (+7 new), `surface-apply.test.cjs` (updated 5 call sites + new skills-kind test). diff --git a/get-shit-done/bin/lib/surface.cjs b/get-shit-done/bin/lib/surface.cjs index af6ee17b3..3ca70d71d 100644 --- a/get-shit-done/bin/lib/surface.cjs +++ b/get-shit-done/bin/lib/surface.cjs @@ -204,7 +204,10 @@ function resolveSurface(runtimeConfigDir, manifest, clusterMap) { * @param {Object} [clusterMap] */ function applySurface(runtimeConfigDir, layout, manifest, clusterMap) { - const resolved = resolveSurface(runtimeConfigDir, manifest, clusterMap); + if (path.resolve(runtimeConfigDir) !== path.resolve(layout.configDir)) { + throw new TypeError('applySurface runtimeConfigDir must match layout.configDir'); + } + const resolved = resolveSurface(layout.configDir, manifest, clusterMap); for (const kind of layout.kinds) { const staged = kind.stage(resolved); const dest = path.join(layout.configDir, kind.destSubpath); diff --git a/tests/surface-apply.test.cjs b/tests/surface-apply.test.cjs index c785d4077..eab1d488e 100644 --- a/tests/surface-apply.test.cjs +++ b/tests/surface-apply.test.cjs @@ -46,18 +46,20 @@ describe('applySurface', () => { }); const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); const layout = resolveRuntimeArtifactLayout('claude', runtimeConfigDir, 'local'); - applySurface(runtimeConfigDir, layout, manifest, CLUSTERS); + const resolved = applySurface(runtimeConfigDir, layout, manifest, CLUSTERS); const files = fs.readdirSync(commandsDir).filter(f => f.endsWith('.md')); // Every file should be a real stem we know about for (const file of files) { assert.ok(fs.existsSync(path.join(REAL_COMMANDS_DIR, file)), `unexpected file: ${file}`); } - // At minimum core skills should be present - const coreStems = ['new-project', 'discuss-phase', 'plan-phase', 'execute-phase', 'help', 'update']; - for (const stem of coreStems) { - assert.ok(files.includes(`${stem}.md`), `core skill "${stem}" should be in commandsDir`); - } + // Core profile should materialize exactly the resolved core command set. + const expectedCore = [...resolved.skills].map(stem => `${stem}.md`).sort(); + assert.deepStrictEqual( + [...files].sort(), + expectedCore, + 'commandsDir should contain exactly core commands' + ); }); test('removes superseded files when profile shrinks', (t) => { @@ -84,17 +86,23 @@ describe('applySurface', () => { explicitAdds: [], explicitRemoves: [], }); - applySurface(runtimeConfigDir, layout, manifest, CLUSTERS); + const resolvedCore = applySurface(runtimeConfigDir, layout, manifest, CLUSTERS); const afterCore = new Set(fs.readdirSync(commandsDir).filter(f => f.endsWith('.md'))); // core should be a subset of standard assert.ok(afterCore.size <= afterStandard.size, 'core should have fewer or equal files than standard'); - // Files removed should not be in core set - const coreStems = new Set(['new-project', 'discuss-phase', 'plan-phase', 'execute-phase', 'help', 'update']); + // Core profile should materialize exactly the resolved core command set. + const expectedCore = [...resolvedCore.skills].map(stem => `${stem}.md`).sort(); + assert.deepStrictEqual( + [...afterCore].sort(), + expectedCore, + 'afterCore should contain exactly core commands' + ); + + // All files should still map to known real skills. for (const file of afterCore) { - const stem = file.slice(0, -3); assert.ok( fs.existsSync(path.join(REAL_COMMANDS_DIR, file)), `file in commandsDir not a real skill: ${file}`