fix(installer): address automator review feedback

This commit is contained in:
bmad 2026-04-28 22:03:45 -03:00
parent 95941d7768
commit 6b60599e3b
5 changed files with 92 additions and 6 deletions

View File

@ -38,6 +38,10 @@ The interactive flow asks you five things:
Accept the defaults and you land on the latest stable release of every module, configured for your chosen tool. Accept the defaults and you land on the latest stable release of every module, configured for your chosen tool.
:::caution[BMad Automator constraints]
`bma` installs runnable Automator skills only for the Claude Code entrypoint. Codex is supported as a worker target only, and worker sessions currently require `tmux` on macOS.
:::
:::tip[Just want the newest prerelease?] :::tip[Just want the newest prerelease?]
```bash ```bash

View File

@ -116,6 +116,7 @@ async function createAutomatorBmadFixture() {
path.join(skillDir, 'SKILL.md'), path.join(skillDir, 'SKILL.md'),
['---', `name: ${skillName}`, 'description: Automator skill', '---', '', 'Automator body.'].join('\n'), ['---', `name: ${skillName}`, 'description: Automator skill', '---', '', 'Automator body.'].join('\n'),
); );
await fs.writeFile(path.join(skillDir, 'workflow.md'), `# ${skillName}\n\nAutomator workflow body.\n`);
} }
return fixtureDir; return fixtureDir;
@ -3290,6 +3291,28 @@ async function runTests() {
automatorInfo42.installTargets.length === 1 && automatorInfo42.installTargets.includes('claude-code'), automatorInfo42.installTargets.length === 1 && automatorInfo42.installTargets.includes('claude-code'),
'BMad Automator is limited to Claude Code skill installation', 'BMad Automator is limited to Claude Code skill installation',
); );
const normalizedInfo42 = externalManager42._normalizeModule({
name: 'bad-shapes',
code: 'bad',
repository: 'https://example.com/bad.git',
install_targets: 'claude-code',
worker_targets: { bad: true },
requirements: ['tmux', { bad: true }, false],
});
assert(
Array.isArray(normalizedInfo42.installTargets) && normalizedInfo42.installTargets.includes('claude-code'),
'External module install targets normalize scalar values to arrays',
);
assert(
Array.isArray(normalizedInfo42.workerTargets) && normalizedInfo42.workerTargets.length === 0,
'External module worker targets drop invalid shapes',
);
assert(
normalizedInfo42.requirements.length === 2 &&
normalizedInfo42.requirements.includes('tmux') &&
normalizedInfo42.requirements.includes('false'),
'External module requirements normalize scalar array entries',
);
tempProjectDir42 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-automator-target-')); tempProjectDir42 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-automator-target-'));
installedBmadDir42 = await createAutomatorBmadFixture(); installedBmadDir42 = await createAutomatorBmadFixture();
@ -3310,6 +3333,30 @@ async function runTests() {
!(await fs.pathExists(path.join(tempProjectDir42, '.agents', 'skills', 'bmad-story-automator', 'SKILL.md'))), !(await fs.pathExists(path.join(tempProjectDir42, '.agents', 'skills', 'bmad-story-automator', 'SKILL.md'))),
'Codex setup skips Claude Code-only automator skill', 'Codex setup skips Claude Code-only automator skill',
); );
assert(
!(await fs.pathExists(path.join(tempProjectDir42, '.agents', 'skills', 'bmad-story-automator-review', 'SKILL.md'))),
'Codex setup skips Claude Code-only automator review skill',
);
const escapeRoot42 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-source-root-'));
const escapeRepo42 = path.join(escapeRoot42, 'repo');
await fs.ensureDir(escapeRepo42);
const escapeManager42 = new ExternalModuleManager();
escapeManager42.getModuleByCode = async () => ({
code: 'escape',
builtIn: false,
sourceRoot: '../outside',
});
escapeManager42.cloneExternalModule = async () => escapeRepo42;
let rejectedEscapingSourceRoot42 = false;
try {
await escapeManager42.findExternalModuleSource('escape');
} catch (error) {
rejectedEscapingSourceRoot42 = error.message.includes('source-root escapes repository');
} finally {
await fs.remove(escapeRoot42).catch(() => {});
}
assert(rejectedEscapingSourceRoot42, 'External module source-root cannot escape cloned repository');
const claudeResult42 = await ideManager42.setup('claude-code', tempProjectDir42, installedBmadDir42, { const claudeResult42 = await ideManager42.setup('claude-code', tempProjectDir42, installedBmadDir42, {
silent: true, silent: true,
@ -3324,8 +3371,12 @@ async function runTests() {
await fs.pathExists(path.join(tempProjectDir42, '.claude', 'skills', 'bmad-story-automator-review', 'SKILL.md')), await fs.pathExists(path.join(tempProjectDir42, '.claude', 'skills', 'bmad-story-automator-review', 'SKILL.md')),
'Claude Code setup installs automator review skill', 'Claude Code setup installs automator review skill',
); );
assert(
await fs.pathExists(path.join(tempProjectDir42, '.claude', 'skills', 'bmad-story-automator-review', 'workflow.md')),
'Claude Code setup copies automator workflow files',
);
} catch (error) { } catch (error) {
assert(false, 'Automator external skill-only module test succeeds', error.message); assert(false, `Automator external skill-only module test succeeds: ${error.message}`);
} finally { } finally {
if (tempProjectDir42) await fs.remove(tempProjectDir42).catch(() => {}); if (tempProjectDir42) await fs.remove(tempProjectDir42).catch(() => {});
if (installedBmadDir42) await fs.remove(path.dirname(installedBmadDir42)).catch(() => {}); if (installedBmadDir42) await fs.remove(path.dirname(installedBmadDir42)).catch(() => {});

View File

@ -810,8 +810,22 @@ class ManifestGenerator {
const modulePath = path.join(this.bmadDir, moduleName); const modulePath = path.join(this.bmadDir, moduleName);
if (!(await fs.pathExists(modulePath))) return false; if (!(await fs.pathExists(modulePath))) return false;
if (await fs.pathExists(path.join(modulePath, 'module.yaml'))) return false; if (await fs.pathExists(path.join(modulePath, 'module.yaml'))) return false;
if (!(await this._moduleUsesSourceRoot(moduleName))) return false;
return this._hasSkillMdRecursive(modulePath); return this._hasSkillMdRecursive(modulePath);
} }
async _moduleUsesSourceRoot(moduleName) {
if (!this.sourceRootModuleCodes) {
try {
const { ExternalModuleManager } = require('../modules/external-manager');
const externalModules = await new ExternalModuleManager().listAvailable();
this.sourceRootModuleCodes = new Set(externalModules.filter((mod) => mod.sourceRoot).map((mod) => mod.code));
} catch {
this.sourceRootModuleCodes = new Set();
}
}
return this.sourceRootModuleCodes.has(moduleName);
}
} }
/** /**

View File

@ -218,7 +218,7 @@ class ConfigDrivenIdeSetup {
const { ExternalModuleManager } = require('../modules/external-manager'); const { ExternalModuleManager } = require('../modules/external-manager');
this.externalModuleManager = this.externalModuleManager || new ExternalModuleManager(); this.externalModuleManager = this.externalModuleManager || new ExternalModuleManager();
const moduleInfo = await this.externalModuleManager.getModuleByCode(moduleName); const moduleInfo = await this.externalModuleManager.getModuleByCode(moduleName);
const targets = moduleInfo?.installTargets || null; const targets = moduleInfo?.installTargets?.length ? moduleInfo.installTargets : null;
this.moduleTargetCache.set(moduleName, targets); this.moduleTargetCache.set(moduleName, targets);
return !targets || targets.includes(this.name); return !targets || targets.includes(this.name);

View File

@ -16,6 +16,15 @@ function normalizeChannelName(raw) {
return VALID_CHANNELS.has(lower) ? lower : null; return VALID_CHANNELS.has(lower) ? lower : null;
} }
function normalizeStringList(raw) {
if (raw == null || raw === '') return [];
const values = Array.isArray(raw) ? raw : [raw];
return values
.filter((value) => ['string', 'number', 'boolean'].includes(typeof value))
.map((value) => String(value).trim())
.filter(Boolean);
}
/** /**
* Conservative quoting for tag names passed to git commands. Tags are * Conservative quoting for tag names passed to git commands. Tags are
* user-typed (--pin) or come from the GitHub API. Only allow the semver * user-typed (--pin) or come from the GitHub API. Only allow the semver
@ -120,6 +129,9 @@ class ExternalModuleManager {
* @returns {Object} Normalized module info * @returns {Object} Normalized module info
*/ */
_normalizeModule(mod, key) { _normalizeModule(mod, key) {
const installTargets = mod.install_targets ?? mod['install-targets'] ?? mod.installTargets;
const workerTargets = mod.worker_targets ?? mod['worker-targets'] ?? mod.workerTargets;
return { return {
key: key || mod.name, key: key || mod.name,
url: mod.repository || mod.url, url: mod.repository || mod.url,
@ -131,9 +143,9 @@ class ExternalModuleManager {
defaultSelected: mod.default_selected === true || mod.defaultSelected === true, defaultSelected: mod.default_selected === true || mod.defaultSelected === true,
type: mod.type || 'bmad-org', type: mod.type || 'bmad-org',
npmPackage: mod.npm_package || mod.npmPackage || null, npmPackage: mod.npm_package || mod.npmPackage || null,
installTargets: mod.install_targets || mod['install-targets'] || mod.installTargets || null, installTargets: normalizeStringList(installTargets),
workerTargets: mod.worker_targets || mod['worker-targets'] || mod.workerTargets || [], workerTargets: normalizeStringList(workerTargets),
requirements: mod.requirements || [], requirements: normalizeStringList(mod.requirements),
installNote: mod.install_note || mod['install-note'] || mod.installNote || null, installNote: mod.install_note || mod['install-note'] || mod.installNote || null,
defaultChannel: normalizeChannelName(mod.default_channel || mod.defaultChannel) || 'stable', defaultChannel: normalizeChannelName(mod.default_channel || mod.defaultChannel) || 'stable',
builtIn: mod.built_in === true, builtIn: mod.built_in === true,
@ -513,7 +525,12 @@ class ExternalModuleManager {
const cloneDir = await this.cloneExternalModule(moduleCode, options); const cloneDir = await this.cloneExternalModule(moduleCode, options);
if (moduleInfo.sourceRoot) { if (moduleInfo.sourceRoot) {
const sourceRoot = path.join(cloneDir, moduleInfo.sourceRoot); const repoRoot = path.resolve(cloneDir);
const sourceRoot = path.resolve(repoRoot, moduleInfo.sourceRoot);
const relativeSourceRoot = path.relative(repoRoot, sourceRoot);
if (relativeSourceRoot === '..' || relativeSourceRoot.startsWith(`..${path.sep}`) || path.isAbsolute(relativeSourceRoot)) {
throw new Error(`External module '${moduleCode}' source-root escapes repository: ${moduleInfo.sourceRoot}`);
}
if (!(await fs.pathExists(sourceRoot))) { if (!(await fs.pathExists(sourceRoot))) {
throw new Error(`External module '${moduleCode}' source-root not found: ${moduleInfo.sourceRoot}`); throw new Error(`External module '${moduleCode}' source-root not found: ${moduleInfo.sourceRoot}`);
} }