fix(installer): address PR #2324 review feedback
Six fixes from CodeRabbit + Augment review on the OpenCode command
pointer generation:
- skipTarget no longer suppresses installCommandPointers in multi-IDE
shared-target_dir batches. Pointers live in a per-IDE directory and
are not deduped across peers, so OpenCode must still generate them
even when a peer (e.g. openhands) won the .agents/skills write race.
- skipTarget no longer suppresses cleanupCommandPointers either, so
partial uninstalls leave no stale pointers when a peer remains.
- canonicalId is validated as a safe basename before being interpolated
into a file path (defense in depth against a malformed manifest entry
writing outside commands_target_dir).
- yamlSafeSingleLine now quotes descriptions starting with `[` or `{`
so YAML doesn't parse them as a sequence/map.
- Per-record fs.writeFile failures are caught and counted (writeFailures)
rather than aborting the whole IDE install — pointer files are a
non-essential adjunct to the skill copy.
- Generator-shaped pointer files are refreshed when the manifest
description changes; hand-modified files (body diverges from the
generator pattern) are still preserved unless forceCommands is set.
Tests: extends Suite 8 with description-update propagation; adds new
Suite 40c covering OpenCode + openhands batches in both orderings plus
partial-IDE uninstall pointer cleanup. 308 tests pass (was 296).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
jheyworth
parent
9086546e2f
commit
81badb92cd
|
|
@ -323,6 +323,22 @@ async function runTests() {
|
|||
assert(result2.success === true, 'Second OpenCode install succeeds (idempotent)');
|
||||
assert(await fs.pathExists(commandFile), 'Command pointer survives a second install pass');
|
||||
|
||||
// Description-update propagation: when the manifest description changes
|
||||
// and the on-disk pointer still matches the generator pattern, refresh
|
||||
// the file so users see the updated description.
|
||||
const csvPath = path.join(installedBmadDir, '_config', 'skill-manifest.csv');
|
||||
const updatedCsv =
|
||||
'canonicalId,name,description,module,path\n' +
|
||||
'"bmad-master","bmad-master","UPDATED description for the test agent","core","_bmad/core/bmad-master/SKILL.md"\n';
|
||||
await fs.writeFile(csvPath, updatedCsv);
|
||||
const result3 = await ideManager.setup('opencode', tempProjectDir, installedBmadDir, {
|
||||
silent: true,
|
||||
selectedModules: ['bmm'],
|
||||
});
|
||||
assert(result3.success === true, 'Third OpenCode install succeeds after description update');
|
||||
const refreshed = await fs.readFile(commandFile, 'utf8');
|
||||
assert(refreshed.includes('UPDATED description'), 'Generator-shaped pointer is refreshed when manifest description changes');
|
||||
|
||||
await fs.remove(tempProjectDir);
|
||||
await fs.remove(path.dirname(installedBmadDir));
|
||||
} catch (error) {
|
||||
|
|
@ -2753,6 +2769,113 @@ async function runTests() {
|
|||
|
||||
console.log('');
|
||||
|
||||
// ============================================================
|
||||
// Test Suite 40c: OpenCode command pointers in multi-IDE batches
|
||||
// ============================================================
|
||||
// Regression: when OpenCode is the *peer* in a setupBatch sharing
|
||||
// .agents/skills (e.g. with openhands), the skill write is dedup-skipped
|
||||
// but the per-IDE .opencode/commands/ pointers must still be generated.
|
||||
// Symmetrically, partial uninstall while a peer remains must still clean
|
||||
// up OpenCode's own command pointers.
|
||||
console.log(`${colors.yellow}Test Suite 40c: OpenCode command pointers in shared-target batches${colors.reset}\n`);
|
||||
|
||||
try {
|
||||
clearCache();
|
||||
const platformCodes40c = await loadPlatformCodes();
|
||||
const opencodeTarget40c = platformCodes40c.platforms.opencode?.installer?.target_dir;
|
||||
const openhandsTarget40c = platformCodes40c.platforms.openhands?.installer?.target_dir;
|
||||
assert(
|
||||
opencodeTarget40c === '.agents/skills' && openhandsTarget40c === '.agents/skills',
|
||||
'OpenCode and OpenHands share .agents/skills target_dir',
|
||||
);
|
||||
|
||||
// Order A: opencode first → opencode is the writer.
|
||||
const projA = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-opencode-batch-a-'));
|
||||
const bmadA = await createTestBmadFixture();
|
||||
const mgrA = new IdeManager();
|
||||
await mgrA.ensureInitialized();
|
||||
const resultsA = await mgrA.setupBatch(['opencode', 'openhands'], projA, bmadA, {
|
||||
silent: true,
|
||||
selectedModules: ['core'],
|
||||
});
|
||||
const cmdA = path.join(projA, '.opencode', 'commands', 'bmad-master.md');
|
||||
assert(
|
||||
resultsA.every((r) => r.success === true),
|
||||
'opencode-first batch: all platforms succeed',
|
||||
);
|
||||
assert(await fs.pathExists(cmdA), 'opencode-first batch: command pointer is created');
|
||||
|
||||
// Order B: openhands first → opencode is the peer (skipTarget=true).
|
||||
// Without the fix, the early-return would bypass installCommandPointers.
|
||||
const projB = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-opencode-batch-b-'));
|
||||
const bmadB = await createTestBmadFixture();
|
||||
const mgrB = new IdeManager();
|
||||
await mgrB.ensureInitialized();
|
||||
const resultsB = await mgrB.setupBatch(['openhands', 'opencode'], projB, bmadB, {
|
||||
silent: true,
|
||||
selectedModules: ['core'],
|
||||
});
|
||||
const cmdB = path.join(projB, '.opencode', 'commands', 'bmad-master.md');
|
||||
const opencodeResultB = resultsB.find((r) => r.ide === 'opencode');
|
||||
assert(
|
||||
resultsB.every((r) => r.success === true),
|
||||
'openhands-first batch: all platforms succeed',
|
||||
);
|
||||
assert(
|
||||
opencodeResultB?.handlerResult?.results?.sharedTargetHandledByPeer === true,
|
||||
'openhands-first batch: opencode is marked sharedTargetHandledByPeer (skill write deduped)',
|
||||
);
|
||||
assert(await fs.pathExists(cmdB), 'openhands-first batch: command pointer is generated even when skill write is deduped');
|
||||
|
||||
// Cleanup symmetry: uninstall opencode while openhands remains.
|
||||
// Uses an in-project bmadDir so the cleanup path can compute removalSet
|
||||
// from the manifest (the production layout). The cross-temp-dir fixture
|
||||
// above can't exercise this — same constraint Test Suite 40 documents.
|
||||
const projC = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-opencode-batch-c-'));
|
||||
const bmadC = path.join(projC, '_bmad');
|
||||
await fs.ensureDir(path.join(bmadC, '_config'));
|
||||
await fs.writeFile(
|
||||
path.join(bmadC, '_config', 'skill-manifest.csv'),
|
||||
'canonicalId,name,description,module,path\n' +
|
||||
'"bmad-master","bmad-master","Minimal test agent fixture","core","_bmad/core/bmad-master/SKILL.md"\n',
|
||||
);
|
||||
const skillC = path.join(bmadC, 'core', 'bmad-master');
|
||||
await fs.ensureDir(skillC);
|
||||
await fs.writeFile(
|
||||
path.join(skillC, 'SKILL.md'),
|
||||
['---', 'name: bmad-master', 'description: Minimal test agent fixture', '---', '', 'You are a test agent.'].join('\n'),
|
||||
);
|
||||
|
||||
const mgrC = new IdeManager();
|
||||
await mgrC.ensureInitialized();
|
||||
await mgrC.setupBatch(['openhands', 'opencode'], projC, bmadC, {
|
||||
silent: true,
|
||||
selectedModules: ['core'],
|
||||
});
|
||||
const cmdC = path.join(projC, '.opencode', 'commands', 'bmad-master.md');
|
||||
assert(await fs.pathExists(cmdC), 'in-project fixture: pointer is generated for opencode peer');
|
||||
|
||||
const cleanupResultsC = await mgrC.cleanupByList(projC, ['opencode'], {
|
||||
silent: true,
|
||||
remainingIdes: ['openhands'],
|
||||
});
|
||||
assert(cleanupResultsC[0].success !== false, 'opencode partial-uninstall reports success');
|
||||
const sharedSurvivesC = await fs.pathExists(path.join(projC, '.agents', 'skills', 'bmad-master', 'SKILL.md'));
|
||||
assert(sharedSurvivesC, 'shared .agents/skills/ survives partial uninstall (peer still uses it)');
|
||||
assert(!(await fs.pathExists(cmdC)), 'opencode command pointer is removed on partial uninstall even when peer remains');
|
||||
|
||||
await fs.remove(projA).catch(() => {});
|
||||
await fs.remove(path.dirname(bmadA)).catch(() => {});
|
||||
await fs.remove(projB).catch(() => {});
|
||||
await fs.remove(path.dirname(bmadB)).catch(() => {});
|
||||
await fs.remove(projC).catch(() => {});
|
||||
} catch (error) {
|
||||
console.log(`${colors.red}Test Suite 40c setup failed: ${error.message}${colors.reset}`);
|
||||
failed++;
|
||||
}
|
||||
|
||||
console.log('');
|
||||
|
||||
// ============================================================
|
||||
// Test Suite 41: Custom-module skill ownership (non-bmad prefix)
|
||||
// ============================================================
|
||||
|
|
|
|||
|
|
@ -37,12 +37,47 @@ function yamlSafeSingleLine(value) {
|
|||
const collapsed = String(value)
|
||||
.replaceAll(/[\r\n]+/g, ' ')
|
||||
.trim();
|
||||
const needsQuoting = /[:#'"\\]/.test(collapsed) || /^[!&*?|>%@`]/.test(collapsed);
|
||||
const needsQuoting = /[:#'"\\]/.test(collapsed) || /^[!&*?|>%@`[{]/.test(collapsed);
|
||||
if (!needsQuoting) return collapsed;
|
||||
const escaped = collapsed.replaceAll('\\', '\\\\').replaceAll('"', String.raw`\"`);
|
||||
return `"${escaped}"`;
|
||||
}
|
||||
|
||||
// Validate that a canonicalId is a safe basename — no path separators, no
|
||||
// parent-dir traversal, no leading dots, only the character set we expect.
|
||||
// Defense-in-depth: the manifest is trusted today, but the value flows
|
||||
// directly into a file path and a malformed entry should not write outside
|
||||
// the commands directory.
|
||||
function isSafeCanonicalId(value) {
|
||||
return typeof value === 'string' && /^[a-zA-Z0-9][a-zA-Z0-9_.-]*$/.test(value) && !value.includes('..');
|
||||
}
|
||||
|
||||
// The exact body the installer would generate for a given description and
|
||||
// canonicalId. Centralised so both the write and the freshness-check paths
|
||||
// agree on the canonical form.
|
||||
function buildCommandPointerBody(description, canonicalId) {
|
||||
return `---\ndescription: ${yamlSafeSingleLine(description)}\n---\n\n@skills/${canonicalId}\n`;
|
||||
}
|
||||
|
||||
// Heuristic: does an existing pointer file look like our generator's output
|
||||
// (and therefore safe to refresh) versus a user-modified file (which we
|
||||
// preserve)? We check the body shape rather than full equality so that
|
||||
// description-only edits in the manifest can propagate without trampling
|
||||
// hand edits to the body.
|
||||
function looksLikeGeneratorOutput(content, canonicalId) {
|
||||
if (typeof content !== 'string') return false;
|
||||
const trimmed = content.trim();
|
||||
// Must end with the exact reference line our generator writes.
|
||||
if (!trimmed.endsWith(`@skills/${canonicalId}`)) return false;
|
||||
// Must start with frontmatter containing exactly one description: line.
|
||||
const fmMatch = trimmed.match(/^---\n([\S\s]*?)\n---\n/);
|
||||
if (!fmMatch) return false;
|
||||
const fmLines = fmMatch[1].split('\n').filter((l) => l.length > 0);
|
||||
if (fmLines.length !== 1) return false;
|
||||
if (!fmLines[0].startsWith('description:')) return false;
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Config-driven IDE setup handler
|
||||
*
|
||||
|
|
@ -134,9 +169,15 @@ class ConfigDrivenIdeSetup {
|
|||
}
|
||||
|
||||
// When a peer platform in the same install batch owns this target_dir,
|
||||
// skip the skill write — the peer has already populated it.
|
||||
// skip the skill write — the peer has already populated it. Command
|
||||
// pointers, however, write to a separate per-IDE directory and must
|
||||
// still be generated for this IDE; they are not deduped across peers.
|
||||
if (options.skipTarget) {
|
||||
return { success: true, results: { skills: 0, sharedTargetHandledByPeer: true } };
|
||||
const results = { skills: 0, sharedTargetHandledByPeer: true };
|
||||
if (this.installerConfig.commands_target_dir) {
|
||||
results.commands = await this.installCommandPointers(projectDir, bmadDir, this.installerConfig, options);
|
||||
}
|
||||
return { success: true, results };
|
||||
}
|
||||
|
||||
if (this.installerConfig.target_dir) {
|
||||
|
|
@ -184,16 +225,33 @@ class ConfigDrivenIdeSetup {
|
|||
*
|
||||
* Skips:
|
||||
* - Names that collide with reserved built-in slash commands.
|
||||
* - Existing files (treated as hand-tuned) unless options.forceCommands.
|
||||
* - canonicalIds that aren't safe basename-only identifiers (defense
|
||||
* against path traversal even though the manifest is currently trusted).
|
||||
* - Existing files whose body looks user-modified (preserves hand edits);
|
||||
* pointer files matching the generator pattern get overwritten so that
|
||||
* description changes in skill-manifest.csv propagate on re-install.
|
||||
*
|
||||
* Per-file write failures are recorded and reported but do not abort the
|
||||
* rest of the install — pointer files are a non-essential adjunct to the
|
||||
* skill copy that already succeeded.
|
||||
*
|
||||
* @param {string} projectDir
|
||||
* @param {string} bmadDir
|
||||
* @param {Object} config - Installer config; reads commands_target_dir.
|
||||
* @param {Object} options - Setup options. forceCommands overwrites existing files.
|
||||
* @returns {Promise<Object>} { created, skippedExisting, skippedCollision, fallbackDescription }
|
||||
* @param {Object} options - Setup options. forceCommands overwrites existing
|
||||
* files unconditionally (including hand-modified ones).
|
||||
* @returns {Promise<Object>} { created, updated, skippedExisting, skippedCollision, skippedInvalidId, writeFailures, fallbackDescription }
|
||||
*/
|
||||
async installCommandPointers(projectDir, bmadDir, config, options = {}) {
|
||||
const result = { created: 0, skippedExisting: 0, skippedCollision: 0, fallbackDescription: 0 };
|
||||
const result = {
|
||||
created: 0,
|
||||
updated: 0,
|
||||
skippedExisting: 0,
|
||||
skippedCollision: 0,
|
||||
skippedInvalidId: 0,
|
||||
writeFailures: 0,
|
||||
fallbackDescription: 0,
|
||||
};
|
||||
|
||||
const csvPath = path.join(bmadDir, '_config', 'skill-manifest.csv');
|
||||
if (!(await fs.pathExists(csvPath))) return result;
|
||||
|
|
@ -208,15 +266,16 @@ class ConfigDrivenIdeSetup {
|
|||
const canonicalId = record.canonicalId;
|
||||
if (!canonicalId) continue;
|
||||
|
||||
if (RESERVED_OPENCODE_COMMANDS.has(canonicalId)) {
|
||||
result.skippedCollision++;
|
||||
// Defensive basename validation. canonicalId comes from a trusted
|
||||
// manifest today, but the value flows directly into a file path —
|
||||
// reject anything that could escape commands_target_dir.
|
||||
if (!isSafeCanonicalId(canonicalId)) {
|
||||
result.skippedInvalidId++;
|
||||
continue;
|
||||
}
|
||||
|
||||
const commandFile = path.join(commandsPath, `${canonicalId}.md`);
|
||||
|
||||
if ((await fs.pathExists(commandFile)) && !options.forceCommands) {
|
||||
result.skippedExisting++;
|
||||
if (RESERVED_OPENCODE_COMMANDS.has(canonicalId)) {
|
||||
result.skippedCollision++;
|
||||
continue;
|
||||
}
|
||||
|
||||
|
|
@ -226,10 +285,54 @@ class ConfigDrivenIdeSetup {
|
|||
result.fallbackDescription++;
|
||||
}
|
||||
|
||||
const body = `---\ndescription: ${yamlSafeSingleLine(description)}\n---\n\n@skills/${canonicalId}\n`;
|
||||
const body = buildCommandPointerBody(description, canonicalId);
|
||||
const commandFile = path.join(commandsPath, `${canonicalId}.md`);
|
||||
|
||||
await fs.writeFile(commandFile, body, 'utf8');
|
||||
result.created++;
|
||||
// If a pointer file already exists, decide whether to overwrite based
|
||||
// on whether it looks like generator output (description-only diff) or
|
||||
// a user-modified file. forceCommands overrides this protection.
|
||||
if (!options.forceCommands && (await fs.pathExists(commandFile))) {
|
||||
let existing;
|
||||
try {
|
||||
existing = await fs.readFile(commandFile, 'utf8');
|
||||
} catch {
|
||||
// Treat unreadable as user-owned and skip — safer than overwriting.
|
||||
result.skippedExisting++;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (existing === body) {
|
||||
// No-op idempotent re-run.
|
||||
result.skippedExisting++;
|
||||
continue;
|
||||
}
|
||||
if (looksLikeGeneratorOutput(existing, canonicalId)) {
|
||||
// Description (or other generated bit) has changed; refresh in place.
|
||||
try {
|
||||
await fs.writeFile(commandFile, body, 'utf8');
|
||||
result.updated++;
|
||||
} catch (error) {
|
||||
result.writeFailures++;
|
||||
if (!options.silent) {
|
||||
await prompts.log.warn(`Failed to update command pointer ${canonicalId}.md: ${error.message}`);
|
||||
}
|
||||
}
|
||||
continue;
|
||||
}
|
||||
// Hand-modified pointer — preserve it.
|
||||
result.skippedExisting++;
|
||||
continue;
|
||||
}
|
||||
|
||||
try {
|
||||
await fs.writeFile(commandFile, body, 'utf8');
|
||||
result.created++;
|
||||
} catch (error) {
|
||||
result.writeFailures++;
|
||||
if (!options.silent) {
|
||||
await prompts.log.warn(`Failed to write command pointer ${canonicalId}.md: ${error.message}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return result;
|
||||
|
|
@ -349,6 +452,17 @@ class ConfigDrivenIdeSetup {
|
|||
await this.cleanupRovoDevPrompts(projectDir, options);
|
||||
}
|
||||
|
||||
// Clean generated command pointer files in commands_target_dir.
|
||||
// Mirrors target_dir cleanup so uninstalls and skill removals don't
|
||||
// leave dangling /<canonicalId> commands pointing at missing skills.
|
||||
// Runs regardless of skipTarget — command pointers live in a per-IDE
|
||||
// directory and are not deduped across peers, so a peer-owned shared
|
||||
// skills directory does not protect this IDE's command pointers from
|
||||
// cleanup.
|
||||
if (this.installerConfig?.commands_target_dir) {
|
||||
await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet);
|
||||
}
|
||||
|
||||
// Skip target_dir cleanup when a peer platform owns this directory
|
||||
// (set during dedup'd install or when uninstalling one of several
|
||||
// platforms that share the same target_dir).
|
||||
|
|
@ -358,13 +472,6 @@ class ConfigDrivenIdeSetup {
|
|||
if (this.installerConfig?.target_dir) {
|
||||
await this.cleanupTarget(projectDir, this.installerConfig.target_dir, options, removalSet);
|
||||
}
|
||||
|
||||
// Clean generated command pointer files in commands_target_dir.
|
||||
// Mirrors target_dir cleanup so uninstalls and skill removals don't
|
||||
// leave dangling /<canonicalId> commands pointing at missing skills.
|
||||
if (this.installerConfig?.commands_target_dir) {
|
||||
await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Reference in New Issue