From 21fe9d362716489b629f5975a148e635e0672615 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 21 Jun 2026 15:20:46 -0400 Subject: [PATCH] chore(#1544): test-quality + doc cleanups from the #1507 conversion-module review (#1547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hygiene from the #1507 epic review (release blocker fixed in #1537): - path-replacement.test.cjs: delete the hand-reimplemented computePathPrefix copy (ADR-1508 said to); route all cases (incl. Windows + outside-home) through the real _computePathPrefix so the test can no longer drift from the function. - enh-1511: add an isWindowsHost no-op tripwire characterization test. - enh-1511: add a deterministic (fs-method monkeypatch, root/OS-independent) error-path test for applyRuntimeContentRewritesForCommandsInPlace — asserts the temp dir is rm'd on a read failure with no orphaned gsd-cmd-rewrites-* leak. - runtime-artifact-conversion.cts: @internal note on rewriteStagedCommandBodies (deep-seam companion to rewriteStagedSkillBodies; no production caller today). - ADR-1508 + CONTEXT.md: qualify the "single owner" claim with the deliberate opencode/kilo applyOpencodeFamilyPathPrefix pre-conversion carve-out (#784). No user-facing behavior change (tests + internal docs + one code comment). Refs #1507. Co-authored-by: Claude Opus 4.8 --- CONTEXT.md | 2 +- ...1508-runtime-artifact-conversion-module.md | 2 +- src/runtime-artifact-conversion.cts | 5 ++ ...nh-1511-rewrite-engine-relocation.test.cjs | 69 +++++++++++++++++++ tests/path-replacement.test.cjs | 54 ++++++++------- 5 files changed, 105 insertions(+), 27 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 9d1e5672a..aaded44ec 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -155,7 +155,7 @@ Module owning which skills and agents are written to runtime config directories 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`). Owns the per-runtime `nested` skill-bundle decision (#69): a `skillsKind` flag in `src/runtime-artifact-layout.cts` drives whether a runtime receives the nested router layout (6 `gsd-ns-*` routers + concrete skills under `/skills//`) or the flat `skills/gsd-/` layout; the evidence/doc-link matrix is recorded in a comment above `resolveRuntimeArtifactLayout`. 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. Per ADR-1508 / #1511 the former `getInstallExports`/`loadInstallExports` relay (a `GSD_TEST_MODE`-guarded `require('bin/install.js')` by which `surface.cjs` reached `computePathPrefix`/`applyRuntimeContentRewritesInPlace`) was DELETED from this module; content rewriting now lives in the Runtime Artifact Conversion Module and `surface.cjs:applySurface` calls its `rewriteStagedSkillBodies` directly. The resolved `scope` is still carried on the `Layout` object 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 Artifact Conversion Module -Sibling Module to Runtime Artifact Layout Module. Owns projection from canonical Claude-authored command/agent/skill markdown into runtime-specific artifact bodies, including converter selection, frontmatter/body normalization, runtime path rewrites, and staged artifact generation. Runtime Artifact Layout remains responsible for filesystem placement (`kind`, destination subpath, prefix, nesting); Runtime Artifact Conversion owns the content Implementation behind that placement seam so install, uninstall/surface parity, and future plugin/package projections stop reaching back through `bin/install.js` for converter functions or `GSD_TEST_MODE`-guarded installer exports. Chosen direction: sibling Module, not an expanded Layout Module, to preserve ADR-3660's narrow placement responsibility while deepening artifact content locality. First slice: relocate only the layout-reached conversion family (`convertClaudeCommandTo*Skill`, converted command-file emitters, `buildKimiAgentArtifacts`) plus the minimal helper closure they need; do not leave helper dependencies in `bin/install.js` because that would preserve the same shallow seam under a new filename. Installer integration decision: `bin/install.js` imports the conversion Module at top level and re-exports the moved names for compatibility; the conversion Module must not import `bin/install.js` or Runtime Artifact Layout, so the dependency direction becomes installer/layout Adapters -> conversion Module, never conversion -> installer. First-slice Interface decision: export the existing compatibility names only; do not introduce a grouped `convertRuntimeArtifact` Interface until after relocation proves byte-for-byte behavior. SHIPPED (ADR-1508): the converter family relocated in #1510 Phase 1 (`getDirName`→runtime-name-policy, `processAttribution` here); #1511 Phase 2 moved the content-rewrite engine here in full — `_applyRuntimeRewrites` (per-runtime switch, injected attribution), the staged-content walkers `applyRuntimeContentRewritesInPlace`/`applyRuntimeContentRewritesForCommandsInPlace`, `computePathPrefix` (private; `_computePathPrefix` for tests), and the deep public seam `rewriteStagedSkillBodies`/`rewriteStagedCommandBodies({runtime,configDir,scope,homedir?,platform?,resolveAttribution?})`. `bin/install.js` binds these back (single owner, exports preserved); `getCommitAttribution` stays in `bin/install.js` (impure install-time config I/O) and is injected. The `getInstallExports` relay in Runtime Artifact Layout Module was deleted; the dependency direction installer/layout → conversion (never upward) is now enforced. Source: `gsd-core/bin/lib/runtime-artifact-conversion.cjs` (generated from `src/runtime-artifact-conversion.cts`). +Sibling Module to Runtime Artifact Layout Module. Owns projection from canonical Claude-authored command/agent/skill markdown into runtime-specific artifact bodies, including converter selection, frontmatter/body normalization, runtime path rewrites, and staged artifact generation. Runtime Artifact Layout remains responsible for filesystem placement (`kind`, destination subpath, prefix, nesting); Runtime Artifact Conversion owns the content Implementation behind that placement seam so install, uninstall/surface parity, and future plugin/package projections stop reaching back through `bin/install.js` for converter functions or `GSD_TEST_MODE`-guarded installer exports. Chosen direction: sibling Module, not an expanded Layout Module, to preserve ADR-3660's narrow placement responsibility while deepening artifact content locality. First slice: relocate only the layout-reached conversion family (`convertClaudeCommandTo*Skill`, converted command-file emitters, `buildKimiAgentArtifacts`) plus the minimal helper closure they need; do not leave helper dependencies in `bin/install.js` because that would preserve the same shallow seam under a new filename. Installer integration decision: `bin/install.js` imports the conversion Module at top level and re-exports the moved names for compatibility; the conversion Module must not import `bin/install.js` or Runtime Artifact Layout, so the dependency direction becomes installer/layout Adapters -> conversion Module, never conversion -> installer. First-slice Interface decision: export the existing compatibility names only; do not introduce a grouped `convertRuntimeArtifact` Interface until after relocation proves byte-for-byte behavior. SHIPPED (ADR-1508): the converter family relocated in #1510 Phase 1 (`getDirName`→runtime-name-policy, `processAttribution` here); #1511 Phase 2 moved the content-rewrite engine here in full — `_applyRuntimeRewrites` (per-runtime switch, injected attribution), the staged-content walkers `applyRuntimeContentRewritesInPlace`/`applyRuntimeContentRewritesForCommandsInPlace`, `computePathPrefix` (private; `_computePathPrefix` for tests), and the deep public seam `rewriteStagedSkillBodies`/`rewriteStagedCommandBodies({runtime,configDir,scope,homedir?,platform?,resolveAttribution?})`. `bin/install.js` binds these back (single owner, exports preserved); `getCommitAttribution` stays in `bin/install.js` (impure install-time config I/O) and is injected. The `getInstallExports` relay in Runtime Artifact Layout Module was deleted; the dependency direction installer/layout → conversion (never upward) is now enforced. Exception: opencode and kilo path-prefix rewriting is a deliberate `bin/install.js`-owned pre-conversion step (`applyOpencodeFamilyPathPrefix`) per #784, not a violation of the single-owner rule. Source: `gsd-core/bin/lib/runtime-artifact-conversion.cjs` (generated from `src/runtime-artifact-conversion.cts`). ### Command Roster Module Tiny read-only helper Module owning discovery of canonical `commands/gsd/*.md` command stems for artifact conversion and runtime projection. It is a sibling dependency of Runtime Artifact Conversion Module, not part of conversion itself: conversion consumes a roster to safely rewrite `gsd:` / `/gsd-` references, while roster discovery owns filesystem/catalog knowledge. First slice: extract existing `readGsdCommandNames` behavior behind this Module instead of moving it into Runtime Artifact Conversion Module or keeping it as installer-owned state. diff --git a/docs/adr/1508-runtime-artifact-conversion-module.md b/docs/adr/1508-runtime-artifact-conversion-module.md index ceba5ddfb..df49ca1ff 100644 --- a/docs/adr/1508-runtime-artifact-conversion-module.md +++ b/docs/adr/1508-runtime-artifact-conversion-module.md @@ -14,7 +14,7 @@ This is the **last upward dependency from the `.cts` source tree into the hand-a ## Decision -- Promote the `[Planned]` **Runtime Artifact Conversion Module** (`src/runtime-artifact-conversion.cts`) to the single owner of per-runtime **content rewriting**: the per-runtime converters (already relocated as ADR-3660's "first slice", #1099), **plus** the rewrite engine `_applyRuntimeRewrites`, the staged-content walkers, path-prefix derivation, and commit attribution. The **Runtime Artifact Layout Module** keeps owning **placement** only. +- Promote the `[Planned]` **Runtime Artifact Conversion Module** (`src/runtime-artifact-conversion.cts`) to the single owner of per-runtime **content rewriting**: the per-runtime converters (already relocated as ADR-3660's "first slice", #1099), **plus** the rewrite engine `_applyRuntimeRewrites`, the staged-content walkers, path-prefix derivation, and commit attribution. The **Runtime Artifact Layout Module** keeps owning **placement** only. **Exception:** opencode and kilo path-prefix rewriting remains a deliberate `bin/install.js`-owned pre-conversion step (see `applyOpencodeFamilyPathPrefix`); this is intentional per #784 and is not a violation of the single-owner rule. - **Public seam** — two deep calls; the caller passes only what it has, the module derives the rest: - `rewriteStagedSkillBodies(stagedDir, { runtime, configDir, scope }, env?)` — in-place walk (skills / kimi-agents). - `rewriteStagedCommandBodies(stagedDir, { runtime, configDir, scope }, env?) → tempDir` — copy-to-temp (commands). diff --git a/src/runtime-artifact-conversion.cts b/src/runtime-artifact-conversion.cts index f37102bb8..7136316e6 100644 --- a/src/runtime-artifact-conversion.cts +++ b/src/runtime-artifact-conversion.cts @@ -2444,6 +2444,11 @@ function rewriteStagedSkillBodies(stagedDir, opts) { * attribution from opts, then delegates to applyRuntimeContentRewritesForCommandsInPlace * (single copy+rewrite owner). * + * @internal — symmetric companion to rewriteStagedSkillBodies; retained as the deep-seam + * API for command bodies. No production caller today (install rewrites commands via + * copyWithPathReplacement → applyRuntimeContentRewritesForCommandsInPlace). Kept for + * API symmetry + test coverage. + * * @returns {string} path to the temp dir (caller is responsible for cleanup) */ function rewriteStagedCommandBodies(stagedDir, opts) { diff --git a/tests/enh-1511-rewrite-engine-relocation.test.cjs b/tests/enh-1511-rewrite-engine-relocation.test.cjs index 74f60f394..6321ed053 100644 --- a/tests/enh-1511-rewrite-engine-relocation.test.cjs +++ b/tests/enh-1511-rewrite-engine-relocation.test.cjs @@ -68,6 +68,29 @@ describe('_computePathPrefix', () => { }); assert.equal(prefix, '/opt/custom-cursor/'); }); + + test('isWindowsHost tripwire — Windows paths collapse to $HOME/ same as POSIX (no-op today)', () => { + // Documents CURRENT behavior: isWindowsHost is accepted but not branched on. + // Both win32=true and win32=false return '$HOME/.cursor/' for a home-relative target. + // If a future Windows-specific branch is added, this tripwire fails and forces + // an explicit decision about what to return on Windows. + const withWindows = conversion._computePathPrefix({ + isGlobal: true, + isOpencode: false, + isWindowsHost: true, + resolvedTarget: 'C:/Users/matte/.cursor', + homeDir: 'C:/Users/matte', + }); + const withoutWindows = conversion._computePathPrefix({ + isGlobal: true, + isOpencode: false, + isWindowsHost: false, + resolvedTarget: 'C:/Users/matte/.cursor', + homeDir: 'C:/Users/matte', + }); + assert.equal(withWindows, '$HOME/.cursor/'); + assert.strictEqual(withWindows, withoutWindows); + }); }); // --------------------------------------------------------------------------- @@ -231,6 +254,52 @@ describe('rewriteStagedCommandBodies', () => { }); }); +// --------------------------------------------------------------------------- +// Error-path: applyRuntimeContentRewritesForCommandsInPlace must rm the tempDir +// on any exception and NOT leave an orphaned gsd-cmd-rewrites-* directory. +// --------------------------------------------------------------------------- + +describe('applyRuntimeContentRewritesForCommandsInPlace — error-path tempDir cleanup', () => { + test('rmSync is called on the tempDir when readFileSync throws (deterministic monkeypatch)', () => { + // Asserting the injected error propagates proves the throw happens AFTER the tempDir is + // created (the function creates tempDir, then reads .md), so the catch's rmSync cleanup + // is genuinely exercised — deterministic on every platform/uid. + const stagedDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-test-error-path-')); + fs.writeFileSync(path.join(stagedDir, 'x.md'), '# test\n'); + + const before = new Set( + fs.readdirSync(os.tmpdir()).filter(n => n.startsWith('gsd-cmd-rewrites-')) + ); + + const origReadFileSync = fs.readFileSync; + let leaked = []; + try { + fs.readFileSync = () => { throw new Error('injected read failure'); }; + + assert.throws( + () => conversion.applyRuntimeContentRewritesForCommandsInPlace(stagedDir, 'cursor', '/tmp/x/', false), + /injected read failure/, + ); + + // Restore before any further fs use so the snapshot read is trustworthy. + fs.readFileSync = origReadFileSync; + + const after = fs.readdirSync(os.tmpdir()).filter(n => n.startsWith('gsd-cmd-rewrites-')); + leaked = after.filter(n => !before.has(n)); + assert.deepStrictEqual(leaked, [], `tempDir not cleaned up on error: ${leaked.join(',')}`); + } finally { + // Idempotent restore — guard against early-throw paths above. + fs.readFileSync = origReadFileSync; + // Clean up the staged dir created for this test. + cleanup(stagedDir); + // Clean up any genuinely leaked gsd-cmd-rewrites-* dirs so the runner stays clean. + for (const n of leaked) { + cleanup(path.join(os.tmpdir(), n)); + } + } + }); +}); + // --------------------------------------------------------------------------- // Guard: runtime-artifact-layout no longer exports getInstallExports // --------------------------------------------------------------------------- diff --git a/tests/path-replacement.test.cjs b/tests/path-replacement.test.cjs index 2f34348ec..ef6df8336 100644 --- a/tests/path-replacement.test.cjs +++ b/tests/path-replacement.test.cjs @@ -20,14 +20,19 @@ const os = require('os'); const repoRoot = path.join(__dirname, '..'); -// Simulate the pathPrefix computation from install.js (global install) +// Thin adapter over the REAL _computePathPrefix (ADR-1508 Phase 2: deleted hand-copy). +// Old signature: computePathPrefix(homedir, targetDir) assumed isGlobal=true, isOpencode=false. +// This adapter preserves that contract so existing call-sites stay unchanged. +process.env['GSD_TEST_MODE'] = '1'; +const { _computePathPrefix } = require('../gsd-core/bin/lib/runtime-artifact-conversion.cjs'); function computePathPrefix(homedir, targetDir) { - const resolvedTarget = path.resolve(targetDir).replace(/\\/g, '/'); - const homeDir = homedir.replace(/\\/g, '/'); - if (resolvedTarget.startsWith(homeDir)) { - return '$HOME' + resolvedTarget.slice(homeDir.length) + '/'; - } - return resolvedTarget + '/'; + return _computePathPrefix({ + isGlobal: true, + isOpencode: false, + isWindowsHost: process.platform === 'win32', + resolvedTarget: path.resolve(targetDir).replace(/\\/g, '/'), + homeDir: homedir.replace(/\\/g, '/'), + }); } // Detect whether `content` leaks a resolved absolute homedir path (e.g. @@ -65,29 +70,28 @@ describe('pathPrefix computation', () => { }); test('Windows-style paths produce $HOME/ not C:/', () => { - // On Windows, path.resolve returns the input unchanged when it's already absolute. - // Simulate the string operation directly (can't use path.resolve for Windows paths on macOS/Linux). - const winHomedir = 'C:\\Users\\matte'; - const winTargetDir = 'C:\\Users\\matte\\.claude'; - const resolvedTarget = winTargetDir.replace(/\\/g, '/'); - const homeDir = winHomedir.replace(/\\/g, '/'); - const prefix = resolvedTarget.startsWith(homeDir) - ? '$HOME' + resolvedTarget.slice(homeDir.length) + '/' - : resolvedTarget + '/'; + // Call the REAL _computePathPrefix with Windows-style paths. + // isWindowsHost=true is passed; today the function ignores it (no-op) and + // the $HOME shorthand is determined by the startsWith(homeDir) check alone. + const prefix = _computePathPrefix({ + isGlobal: true, + isOpencode: false, + isWindowsHost: true, + resolvedTarget: 'C:/Users/matte/.claude', + homeDir: 'C:/Users/matte', + }); assert.strictEqual(prefix, '$HOME/.claude/'); assert.ok(!prefix.includes('C:'), `Should not contain drive letter, got: ${prefix}`); }); test('target outside home uses absolute path', () => { - const homedir = '/home/user'; - const targetDir = '/opt/gsd/.claude'; - // path.resolve won't change an already-absolute path on the same OS, - // so simulate the string operation directly - const resolvedTarget = targetDir.replace(/\\/g, '/'); - const homeDir = homedir.replace(/\\/g, '/'); - const prefix = resolvedTarget.startsWith(homeDir) - ? '$HOME' + resolvedTarget.slice(homeDir.length) + '/' - : resolvedTarget + '/'; + const prefix = _computePathPrefix({ + isGlobal: true, + isOpencode: false, + isWindowsHost: false, + resolvedTarget: '/opt/gsd/.claude', + homeDir: '/home/user', + }); assert.strictEqual(prefix, '/opt/gsd/.claude/'); assert.ok(!prefix.includes('$HOME'), `Should not contain $HOME for non-home paths`); });