From 81badb92cddc9cd1cf73ec107b923d187f754239 Mon Sep 17 00:00:00 2001 From: jheyworth <8269695+jheyworth@users.noreply.github.com> Date: Sun, 26 Apr 2026 21:45:45 +0100 Subject: [PATCH] fix(installer): address PR #2324 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- test/test-installation-components.js | 123 +++++++++++++++++++++ tools/installer/ide/_config-driven.js | 153 ++++++++++++++++++++++---- 2 files changed, 253 insertions(+), 23 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 6a7909ca3..17379a20b 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -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) // ============================================================ diff --git a/tools/installer/ide/_config-driven.js b/tools/installer/ide/_config-driven.js index 0d12983e5..0ec652aae 100644 --- a/tools/installer/ide/_config-driven.js +++ b/tools/installer/ide/_config-driven.js @@ -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} { created, skippedExisting, skippedCollision, fallbackDescription } + * @param {Object} options - Setup options. forceCommands overwrites existing + * files unconditionally (including hand-modified ones). + * @returns {Promise} { 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 / 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 / commands pointing at missing skills. - if (this.installerConfig?.commands_target_dir) { - await this.cleanupCommandPointers(projectDir, this.installerConfig.commands_target_dir, options, removalSet); - } } /**