fix(#1615): applySurface rewrites commands kind, not just skills

Codex adversarial orthogonal review of PR #1622 surfaced that applySurface (src/surface.cts) only called rewriteStagedSkillBodies for kind='skills', skipping kind='commands'. The gap meant /gsd-surface profile changes on any runtime with commands kinds (windsurf, opencode, kilo, cursor, augment, codebuddy, gemini) wrote raw @~/.claude/... references into synced command/workflow bodies, which fail at invocation time on non-Claude runtimes.

For Windsurf specifically, this left workflow files containing @~/.claude/gsd-core/commands/gsd/X.md after a profile change — paths that don't exist on a Windsurf install. Verified by the new regression test which fails before the fix (workflow bodies contained @~/.claude/) and passes after (workflow bodies reference the install target).

Captures the return value of rewriteStagedCommandBodies (temp dir path — commands rewrite uses copy-then-rewrite to avoid mutating the package source), syncs from the temp dir, then cleans up. Type annotations satisfy typescript-eslint strict mode.

Findings 2 (install ordering) and 3 (legacy .devin cleanup) from the same review are tracked in #1629 — both real but out of scope for #1615.
This commit is contained in:
Tom Boucher
2026-06-23 14:52:32 -04:00
parent 4ed208e74b
commit 658ea33cb6
2 changed files with 92 additions and 10 deletions

View File

@@ -310,17 +310,44 @@ function applySurface(runtimeConfigDir: string, layout: Layout, manifest: Map<st
// module's deep seam (ADR-1508 / #1511 Phase 2) — no attribution resolver
// needed here (proven: Co-Authored-By never appears in staged content; see
// brief PROVEN KEY FACT). No getInstallExports() call required.
for (const kind of layout.kinds) {
const staged = kind.stage(resolved);
if (kind.kind === 'skills') {
runtimeArtifactConversion.rewriteStagedSkillBodies(staged, {
runtime: layout.runtime,
configDir: layout.configDir,
scope: layout.scope ?? 'global',
});
// #1615 adversarial review (PR #1622): commands kind was previously skipped,
// leaving raw @~/.claude/... references in Windsurf workflow bodies after a
// /gsd-surface profile change. Same gap affected any runtime with commands
// kinds (windsurf, opencode, kilo, cursor, augment, codebuddy, gemini).
//
// Asymmetry note: rewriteStagedSkillBodies mutates in place (returns void),
// but rewriteStagedCommandBodies copies to a fresh mkdtemp dir and returns
// its path (commands .md files are flat; mutating the staged source would
// corrupt the package source on full-profile runs). Caller MUST sync from
// the returned dir and clean it up.
const tempDirsToClean: string[] = [];
try {
for (const kind of layout.kinds) {
let staged: string = kind.stage(resolved);
if (kind.kind === 'skills') {
runtimeArtifactConversion.rewriteStagedSkillBodies(staged, {
runtime: layout.runtime,
configDir: layout.configDir,
scope: layout.scope ?? 'global',
});
} else if (kind.kind === 'commands') {
const rewritten: string | undefined = runtimeArtifactConversion.rewriteStagedCommandBodies(staged, {
runtime: layout.runtime,
configDir: layout.configDir,
scope: layout.scope ?? 'global',
}) as string | undefined;
if (rewritten && rewritten !== staged) {
staged = rewritten;
tempDirsToClean.push(rewritten);
}
}
const dest = path.join(layout.configDir, kind.destSubpath);
_syncGsdDir(staged, dest, kind, skillManifest);
}
} finally {
for (const dir of tempDirsToClean) {
try { fs.rmSync(dir, { recursive: true, force: true }); } catch { /* best-effort cleanup */ }
}
const dest = path.join(layout.configDir, kind.destSubpath);
_syncGsdDir(staged, dest, kind, skillManifest);
}
return resolved;
}

View File

@@ -1056,3 +1056,58 @@ describe('listSurface', () => {
}
});
});
// ─── #1615: applySurface must rewrite commands kind (Windsurf workflows) ─────
// Adversarial review of PR #1622 found that applySurface only rewrites 'skills'
// kinds, skipping 'commands'. Windsurf's capability now stages workflow files
// as kind='commands'; without the rewrite, /gsd-surface would write workflow
// bodies containing raw @~/.claude/... references that don't exist on a
// Windsurf install. The same gap affected any runtime with commands kinds.
describe('applySurface — commands kind path rewrite (#1615 adversarial review)', () => {
test('windsurf workflow bodies are rewritten to install target (no raw ~/.claude/)', (t) => {
const base = createTempDir('gsd-surface-cmds-windsurf-');
t.after(() => cleanup(base));
const runtimeConfigDir = base;
// Stage the canonical command body the workflow delegates to.
const canonicalDir = path.join(runtimeConfigDir, 'gsd-core', 'commands', 'gsd');
fs.mkdirSync(canonicalDir, { recursive: true });
fs.writeFileSync(path.join(canonicalDir, 'help.md'),
'---\nname: help\ndescription: Show help\n---\n\nHelp body\n');
const manifest = loadSkillsManifest(REAL_COMMANDS_DIR);
const layout = resolveRuntimeArtifactLayout('windsurf', runtimeConfigDir, 'local');
// Sanity: layout must have a commands kind (workflows) — pre-condition
// introduced by PR #1622; if a future refactor removes it, this test
// would silently pass without exercising the rewrite path.
const commandsKind = layout.kinds.find((k) => k.kind === 'commands');
assert.ok(commandsKind, 'pre-condition: windsurf layout has a commands kind');
applySurface(runtimeConfigDir, layout, manifest, CLUSTERS);
// Workflow files should be written to <configDir>/workflows/gsd-*.md
const workflowsDir = path.join(runtimeConfigDir, 'workflows');
const workflowFiles = fs.existsSync(workflowsDir)
? fs.readdirSync(workflowsDir).filter((f) => f.startsWith('gsd-') && f.endsWith('.md'))
: [];
assert.ok(workflowFiles.length > 0,
`expected at least one gsd-*.md workflow under ${workflowsDir}; got [${workflowFiles.join(', ')}]`);
// Every workflow body must reference the install target, NOT the raw
// ~/.claude/ path. This is the regression: pre-fix, the commands kind
// was skipped and raw @~/.claude/... survived into the synced file.
for (const fileName of workflowFiles) {
const workflowPath = path.join(workflowsDir, fileName);
const content = fs.readFileSync(workflowPath, 'utf8');
assert.ok(
!content.includes('~/.claude/'),
`${fileName} must not contain raw ~/.claude/ after applySurface rewrite (got: ${content.slice(0, 200)})`,
);
assert.ok(
!content.includes('$HOME/.claude/'),
`${fileName} must not contain raw $HOME/.claude/ after applySurface rewrite`,
);
}
});
});