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 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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 `<router>/skills/<name>/`) or the flat `skills/gsd-<stem>/` 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.
|
||||
|
||||
@@ -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).
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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`);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user