From 907cd01aa316dfd16320a8483bbbb9e478dc3bdd Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 7 Jun 2026 16:26:14 -0400 Subject: [PATCH] fix(#813): apply per-runtime skill path rewrites in applySurface (#817) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#813): apply per-runtime skill path rewrites in applySurface applySurface() re-staged skill artifacts but, unlike installRuntimeArtifacts(), never applied the per-runtime path rewrites. So /gsd:surface (profile/enable/disable/reset) overwrote installed SKILL.md bodies with the converter's default ~/.claude paths instead of the install target (pathPrefix), silently regressing skill path references for every skillsKind runtime until the next reinstall. applySurface now mirrors installRuntimeArtifacts: for kind.kind === 'skills' it derives pathPrefix the same way and applies applyRuntimeContentRewritesInPlace on the staged dir before syncing. - bin/install.js: export applyRuntimeContentRewritesInPlace - runtime-artifact-layout.cts: carry resolved scope on Layout; export getInstallExports; type computePathPrefix/applyRuntimeContentRewritesInPlace on InstallExports - surface.cts: lazily derive pathPrefix (only when a skills kind exists) and apply the rewrite via the shared getInstallExports accessor — single source of truth with install, only skills kinds rewritten (matches install) - tests: regression test parameterized over cursor + codex asserting post-applySurface bodies carry the install pathPrefix, not ~/.claude - CONTEXT.md: glossary updated for the applySurface rewrite parity + scope seam Closes #813 Co-Authored-By: Claude Opus 4.8 * chore(#813): add changeset fragment for PR #817 Co-Authored-By: Claude Opus 4.8 * test(#813): normalize configDir prefix to forward slashes for Windows CI The #813 regression assertion compared skill bodies against a raw ${configDir}/ prefix, but production derives pathPrefix via path.resolve(configDir).replace(/\\/g, '/'). On Windows, mkdtempSync returns backslash paths while the rewritten body uses forward slashes, so the assertion would fail Windows-only (not covered by local gsd-test). Normalize the expected prefix the same way production does. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/silly-zebras-forage.md | 5 ++ CONTEXT.md | 2 +- bin/install.js | 1 + src/runtime-artifact-layout.cts | 7 +- src/surface.cts | 25 +++++- .../runtime-artifact-layout-surface.test.cjs | 83 +++++++++++++++++++ 6 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 .changeset/silly-zebras-forage.md diff --git a/.changeset/silly-zebras-forage.md b/.changeset/silly-zebras-forage.md new file mode 100644 index 000000000..7eabd459a --- /dev/null +++ b/.changeset/silly-zebras-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 817 +--- +**`/gsd:surface` no longer corrupts installed skill paths** — re-surfacing (profile/enable/disable/reset) now applies the same per-runtime path rewrites as install, so SKILL.md bodies keep the correct install target instead of reverting to the converter's default `~/.claude` paths. diff --git a/CONTEXT.md b/CONTEXT.md index baea885cd..4f97160ed 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -116,7 +116,7 @@ Module owning install detection for `/gsd:update`. `resolveUpdateContext({ home, Module owning which skills and agents are written to runtime config directories at install time (Phase 1) and at runtime via cluster-level toggles (Phase 2). Phase 1: `gsd-core/bin/lib/install-profiles.cjs` defines named profiles (`core`, `standard`, `full`), computes transitive closure over `requires:` frontmatter, stages skills/agents to runtime config dirs, and persists the chosen profile in a `.gsd-profile` marker. Profile resolution precedence: explicit `--profile=` flag > `.gsd-profile` marker > `full`. `--minimal`/`--core-only` are back-compat aliases for `--profile=core`. Phase 2: `gsd-core/bin/lib/surface.cjs` implements the `/gsd:surface` slash command for cluster-level enable/disable without reinstall; cluster definitions live in `gsd-core/bin/lib/clusters.cjs`; per-runtime state persists in `/.gsd-surface.json` independent from the `.gsd-profile` marker. See ADR-0011. ### Runtime Artifact Layout Module -Module owning the per-runtime mapping from artifact kind to filesystem placement. ADR-3660 defines the typed `kinds` per runtime (`commands`, `agents`, `skills`) with destination subpath, prefix, and stage adapter (with per-runtime converters in `bin/install.js`: `convertClaudeCommandToClaudeSkill`, `…CodexSkill`, `…CopilotSkill`, `…AntigravitySkill`). Phase 1 applies this seam to the Runtime Surface Module (`surface.cjs:applySurface`). Phase 2 is planned to migrate install/uninstall in `bin/install.js` so all lifecycle sites iterate one shared layout table instead of re-encoding runtime layout logic. This design is intended to remove the #3659 class of omissions. Migrations remain under the Installer Migration Module (ADR-0008). See ADR-3660. +Module owning the per-runtime mapping from artifact kind to filesystem placement. ADR-3660 defines the typed `kinds` per runtime (`commands`, `agents`, `skills`) with destination subpath, prefix, and stage adapter (with per-runtime converters in `bin/install.js`: `convertClaudeCommandToClaudeSkill`, `…CodexSkill`, `…CopilotSkill`, `…AntigravitySkill`). Phase 1 applies this seam to the Runtime Surface Module (`surface.cjs:applySurface`); as of #813, `applySurface` applies the same per-runtime skill-body path rewrites as `installRuntimeArtifacts` for `skills` kinds — re-surfacing no longer overwrites installed SKILL.md bodies with converter-default `~/.claude` paths. The shared accessor `getInstallExports` (exported from `runtime-artifact-layout.cjs`) is the single-source seam through which `surface.cjs` reaches `computePathPrefix` and `applyRuntimeContentRewritesInPlace`; the resolved `scope` (`'local'`|`'global'`) is now carried on the `Layout` object returned by `resolveRuntimeArtifactLayout` so `applySurface` derives the same `pathPrefix` (global `$HOME` form vs. absolute) as a fresh install. Phase 2 is planned to migrate install/uninstall in `bin/install.js` so all lifecycle sites iterate one shared layout table instead of re-encoding runtime layout logic. This design is intended to remove the #3659 class of omissions. Migrations remain under the Installer Migration Module (ADR-0008). See ADR-3660. ### Runtime Install Policy Module Projects a pure, typed install plan for a given runtime by composing artifact placements (Runtime Artifact Layout Module), command text (Shell Command Projection Module), and per-runtime config intentions — with no filesystem IO or format-specific serialization. Runtime-specific adapters consume the plan and execute concrete file mutations and config rendering. See ADR-58. diff --git a/bin/install.js b/bin/install.js index 169b8c700..2d039002c 100755 --- a/bin/install.js +++ b/bin/install.js @@ -11536,6 +11536,7 @@ module.exports = { normalizeAgentBodyForRuntime, yamlIdentifier, computePathPrefix, + applyRuntimeContentRewritesInPlace, getCodexSkillAdapterHeader, convertClaudeCommandToCursorSkill, convertClaudeCommandToCursorCommand, diff --git a/src/runtime-artifact-layout.cts b/src/runtime-artifact-layout.cts index 1b2a495fe..949a57e3c 100644 --- a/src/runtime-artifact-layout.cts +++ b/src/runtime-artifact-layout.cts @@ -33,6 +33,8 @@ const _require: NodeRequire = require; interface InstallExports { readGsdCommandNames: () => string[]; + computePathPrefix: (opts: { isGlobal: boolean; isOpencode: boolean; isWindowsHost: boolean; resolvedTarget: string; homeDir: string }) => string; + applyRuntimeContentRewritesInPlace: (stagedDir: string, runtime: string, pathPrefix: string) => void; [converterName: string]: unknown; } @@ -84,6 +86,7 @@ interface ArtifactKind { interface Layout { runtime: string; configDir: string; + scope?: 'local' | 'global'; kinds: ArtifactKind[]; } @@ -371,7 +374,7 @@ function resolveRuntimeArtifactLayout(runtime: string, configDir: string, scope: throw new TypeError(`Unknown runtime: '${runtime}' — add to runtime-artifact-layout.cjs table`); } - return { runtime, configDir, kinds }; + return { runtime, configDir, scope, kinds }; } -export = { resolveRuntimeArtifactLayout, findInstallSourceRoot }; +export = { resolveRuntimeArtifactLayout, findInstallSourceRoot, getInstallExports }; diff --git a/src/surface.cts b/src/surface.cts index 430395e95..bd8cccc94 100644 --- a/src/surface.cts +++ b/src/surface.cts @@ -23,6 +23,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import os from 'node:os'; import { platformWriteSync } from './shell-command-projection.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import installProfiles = require('./install-profiles.cjs'); @@ -35,7 +36,7 @@ import { CLUSTERS } from './clusters.cjs'; import type { ClusterMap } from './clusters.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import runtimeArtifactLayout = require('./runtime-artifact-layout.cjs'); -const { findInstallSourceRoot } = runtimeArtifactLayout; +const { findInstallSourceRoot, getInstallExports } = runtimeArtifactLayout; const SURFACE_FILE_NAME = '.gsd-surface.json'; @@ -53,6 +54,7 @@ interface ArtifactKind { interface Layout { runtime: string; configDir: string; + scope?: 'local' | 'global'; kinds: ArtifactKind[]; } @@ -256,8 +258,29 @@ function applySurface(runtimeConfigDir: string, layout: Layout, manifest: Map { assert.ok(fs.existsSync(userDir), 'user-custom-skill dir must be preserved when kindPrefix is empty (Hermes)'); assert.ok(fs.existsSync(path.join(destDir, stem1, 'SKILL.md')), 'GSD help/SKILL.md must be copied'); }); + + // Regression test for #813: applySurface must apply per-runtime path rewrites + // (applyRuntimeContentRewritesInPlace) just as installRuntimeArtifacts does. + // Without the fix, skill bodies retain the converter's default ~/.claude/ paths + // instead of being rewritten to the install target (pathPrefix). + // + // Both 'cursor' and 'codex' use skillsKind AND have a path-rewrite case in + // _applyRuntimeRewrites — so the regression guard covers both. + for (const runtime of ['cursor', 'codex']) { + test(`applySurface rewrites ${runtime} skill bodies to the install pathPrefix, not the converter default ~/.claude path (#813)`, (t) => { + // Use mkdtempSync under os.tmpdir() — NOT under the user's home dir — so that + // computePathPrefix returns an ABSOLUTE prefix `${configDir}/` for local installs, + // clearly distinguishable from the `~/.claude/` converter default. + const configDir = fs.mkdtempSync(path.join(os.tmpdir(), `gsd-surface-813-${runtime}-`)); + t.after(() => cleanup(configDir)); + + writeActiveProfile(configDir, 'standard'); + writeSurface(configDir, { + baseProfile: 'standard', + disabledClusters: [], + explicitAdds: [], + explicitRemoves: [], + }); + + const manifest = loadSkillsManifest(REAL_COMMANDS_DIR); + // skills/gsd-/SKILL.md (destSubpath 'skills', prefix 'gsd-') + // scope='local' ensures computePathPrefix returns an absolute configDir prefix + // rather than the home-relative default (e.g. ~/.cursor/ or ~/.codex/). + const layout = resolveRuntimeArtifactLayout(runtime, configDir, 'local'); + applySurface(configDir, layout, manifest, CLUSTERS); + + // Collect every SKILL.md body under ${configDir}/skills/ + const skillsRoot = path.join(configDir, 'skills'); + const skillBodies = []; + if (fs.existsSync(skillsRoot)) { + for (const dirEntry of fs.readdirSync(skillsRoot)) { + const skillMd = path.join(skillsRoot, dirEntry, 'SKILL.md'); + if (fs.existsSync(skillMd)) { + skillBodies.push(fs.readFileSync(skillMd, 'utf8')); + } + } + } + + // (a) Sanity: at least one SKILL.md must have been staged + assert.ok( + skillBodies.length > 0, + `applySurface must stage at least one ${runtime} SKILL.md under ${skillsRoot}/ but found none` + ); + + // (b) BUG SYMPTOM (#813): after the rewrite, no body should contain '~/.claude/' + // or '$HOME/.claude/' — both are converter-default forms that must be eliminated. + // This assertion FAILS on unpatched code — applySurface does not call + // applyRuntimeContentRewritesInPlace, so the converter's default ~/.claude/ + // paths are left verbatim in the staged files. + const bodiesWithTildeClaude = skillBodies.filter(b => b.includes('~/.claude/') || b.includes('$HOME/.claude/')); + assert.strictEqual( + bodiesWithTildeClaude.length, + 0, + `#813 regression: ${bodiesWithTildeClaude.length} ${runtime} SKILL.md(s) still contain '~/.claude/' or '$HOME/.claude/' after applySurface — ` + + `applyRuntimeContentRewritesInPlace was not applied (mirrors installRuntimeArtifacts' rewrite step)` + ); + + // (c) The rewrite must inject the real install target path, not just remove the tilde. + // This also fails on unpatched code for the same reason. + // NOTE: this assertion depends on the command corpus emitting rewritable + // '~/.claude/'-style paths in at least one skill body. If a future corpus + // change removes all such paths, this assertion will become vacuously true + // (no body will contain configDirPrefix either) — update the test rather than + // treating a silent zero-match as a pass. + // Production derives pathPrefix as `path.resolve(configDir).replace(/\\/g, '/')` + // (mirrors installRuntimeArtifacts), so on Windows the rewritten body uses + // forward slashes. Normalize the expected prefix the same way so this assertion + // is cross-platform (Windows CI leg is not covered by local gsd-test) (#813). + const configDirPrefix = `${path.resolve(configDir).replace(/\\/g, '/')}/`; + const bodiesWithAbsolutePrefix = skillBodies.filter(b => b.includes(configDirPrefix)); + assert.ok( + bodiesWithAbsolutePrefix.length > 0, + `#813 regression: no ${runtime} SKILL.md contains the absolute configDir prefix '${configDirPrefix}' — ` + + `the path rewrite was not applied by applySurface. ` + + `(If the command corpus no longer emits any '~/.claude/'-style paths, update this test.)` + ); + }); + } }); // ─── resolveSurface ──────────────────────────────────────────────────────────