External/BMAD-METHOD
1
0
Fork 0
mirror of https://github.com/bmadcode/BMAD-METHOD.git synced 2025-12-17 09:45:25 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: address code review issues from alpha.14 to alpha.15 (#1068)

Browse Source 
* fix: remove debug console.log statements from ui.js

* fix: add error handling and rollback for temp directory cleanup

* fix: use streaming for hash calculation to reduce memory usage

* refactor: hoist CustomHandler require to top of installer.js and ui.js

* fix: fail fast on malformed custom module YAML

User customizations must be valid - silent skip hides broken configs.

* refactor: use consistent return type in handleMissingCustomSources

* refactor: clone config at install() entry to prevent mutation
This commit is contained in:
Alex Verkhovsky 2025-12-08 12:24:30 -07:00 committed by GitHub
parent 55cb4681bc
commit cf50f4935d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 66 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
tools/cli/installers/lib/core/custom-module-cache.js
View File

@ -51,7 +51,19 @@ class CustomModuleCache {
}
/**
* Calculate hash of a file or directory
* Stream a file into the hash to avoid loading entire file into memory
*/
async hashFileStream(filePath, hash) {
return new Promise((resolve, reject) => {
const stream = require('node:fs').createReadStream(filePath);
stream.on('data', (chunk) => hash.update(chunk));
stream.on('end', resolve);
stream.on('error', reject);
});
}
/**
* Calculate hash of a file or directory using streaming to minimize memory usage
*/
async calculateHash(sourcePath) {
const hash = crypto.createHash('sha256');
@ -76,14 +88,14 @@ class CustomModuleCache {
files.sort(); // Ensure consistent order
for (const file of files) {
const content = await fs.readFile(file);
const relativePath = path.relative(sourcePath, file);
hash.update(relativePath + '|' + content.toString('base64'));
// Hash the path first, then stream file contents
hash.update(relativePath + '|');
await this.hashFileStream(file, hash);
}
} else {
// For single files
const content = await fs.readFile(sourcePath);
hash.update(content);
// For single files, stream directly into hash
await this.hashFileStream(sourcePath, hash);
}
return hash.digest('hex');

72
tools/cli/installers/lib/core/installer.js
View File

@ -39,6 +39,7 @@ const { CLIUtils } = require('../../../lib/cli-utils');
const { ManifestGenerator } = require('./manifest-generator');
const { IdeConfigManager } = require('./ide-config-manager');
const { replaceAgentSidecarFolders } = require('./post-install-sidecar-replacement');
const { CustomHandler } = require('../custom/handler');
class Installer {
constructor() {
@ -407,7 +408,10 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
* @param {string[]} config.ides - IDEs to configure
* @param {boolean} config.skipIde - Skip IDE configuration
*/
async install(config) {
async install(originalConfig) {
// Clone config to avoid mutating the caller's object
const config = { ...originalConfig };
// Display BMAD logo
CLIUtils.displayLogo();
@ -440,7 +444,6 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
// Handle selectedFiles (from existing install path or manual directory input)
if (config.customContent && config.customContent.selected && config.customContent.selectedFiles) {
const { CustomHandler } = require('../custom/handler');
const customHandler = new CustomHandler();
for (const customFile of config.customContent.selectedFiles) {
const customInfo = await customHandler.getCustomInfo(customFile, path.resolve(config.directory));
@ -837,9 +840,8 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
// Regular custom content from user input (non-cached)
if (finalCustomContent && finalCustomContent.selected && finalCustomContent.selectedFiles) {
// Add custom modules to the installation list
const customHandler = new CustomHandler();
for (const customFile of finalCustomContent.selectedFiles) {
const { CustomHandler } = require('../custom/handler');
const customHandler = new CustomHandler();
const customInfo = await customHandler.getCustomInfo(customFile, projectDir);
if (customInfo && customInfo.id) {
allModules.push(customInfo.id);
@ -929,7 +931,6 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
// Finally check regular custom content
if (!isCustomModule && finalCustomContent && finalCustomContent.selected && finalCustomContent.selectedFiles) {
const { CustomHandler } = require('../custom/handler');
const customHandler = new CustomHandler();
for (const customFile of finalCustomContent.selectedFiles) {
const info = await customHandler.getCustomInfo(customFile, projectDir);
@ -943,7 +944,6 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
if (isCustomModule && customInfo) {
// Install custom module using CustomHandler but as a proper module
const { CustomHandler } = require('../custom/handler');
const customHandler = new CustomHandler();
// Install to module directory instead of custom directory
@ -972,19 +972,39 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
if (await fs.pathExists(customDir)) {
// Move contents to module directory
const items = await fs.readdir(customDir);
for (const item of items) {
const srcPath = path.join(customDir, item);
const destPath = path.join(moduleTargetPath, item);
const movedItems = [];
try {
for (const item of items) {
const srcPath = path.join(customDir, item);
const destPath = path.join(moduleTargetPath, item);
// If destination exists, remove it first (or we could merge)
if (await fs.pathExists(destPath)) {
await fs.remove(destPath);
// If destination exists, remove it first (or we could merge)
if (await fs.pathExists(destPath)) {
await fs.remove(destPath);
}
await fs.move(srcPath, destPath);
movedItems.push({ src: srcPath, dest: destPath });
}
await fs.move(srcPath, destPath);
} catch (moveError) {
// Rollback: restore any successfully moved items
for (const moved of movedItems) {
try {
await fs.move(moved.dest, moved.src);
} catch {
// Best-effort rollback - log if it fails
console.error(`Failed to rollback ${moved.dest} during cleanup`);
}
}
throw new Error(`Failed to move custom module files: ${moveError.message}`);
}
}
await fs.remove(tempCustomPath);
try {
await fs.remove(tempCustomPath);
} catch (cleanupError) {
// Non-fatal: temp directory cleanup failed but files were moved successfully
console.warn(`Warning: Could not clean up temp directory: ${cleanupError.message}`);
}
}
// Create module config (include collected config from module.yaml prompts)
@ -1066,9 +1086,8 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
config.customContent.selectedFiles
) {
// Filter out custom modules that were already installed
const customHandler = new CustomHandler();
for (const customFile of config.customContent.selectedFiles) {
const { CustomHandler } = require('../custom/handler');
const customHandler = new CustomHandler();
const customInfo = await customHandler.getCustomInfo(customFile, projectDir);
// Skip if this was installed as a module
@ -1080,7 +1099,6 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
if (remainingCustomContent.length > 0) {
spinner.start('Installing remaining custom content...');
const { CustomHandler } = require('../custom/handler');
const customHandler = new CustomHandler();
// Use the remaining files
@ -2581,18 +2599,7 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
installedModules,
);
// Handle both old return format (array) and new format (object)
let validCustomModules = [];
let keptModulesWithoutSources = [];
if (Array.isArray(customModuleResult)) {
// Old format - just an array
validCustomModules = customModuleResult;
} else if (customModuleResult && typeof customModuleResult === 'object') {
// New format - object with two arrays
validCustomModules = customModuleResult.validCustomModules || [];
keptModulesWithoutSources = customModuleResult.keptModulesWithoutSources || [];
}
const { validCustomModules, keptModulesWithoutSources } = customModuleResult;
const customModulesFromManifest = validCustomModules.map((m) => ({
...m,
@ -3371,7 +3378,10 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
// If no missing sources, return immediately
if (customModulesWithMissingSources.length === 0) {
return validCustomModules;
return {
validCustomModules,
keptModulesWithoutSources: [],
};
}
// Stop any spinner for interactive prompts

4
tools/cli/installers/lib/modules/manager.js
View File

@ -391,8 +391,8 @@ class ModuleManager {
if (config.code === moduleName) {
return modulePath;
}
} catch {
// Skip if can't read config
} catch (error) {
throw new Error(`Failed to parse module.yaml at ${configPath}: ${error.message}`);
}
}
}

24
tools/cli/lib/ui.js
View File

@ -24,6 +24,7 @@ const path = require('node:path');
const os = require('node:os');
const fs = require('fs-extra');
const { CLIUtils } = require('./cli-utils');
const { CustomHandler } = require('../installers/lib/custom/handler');
/**
* UI utilities for the installer
@ -150,7 +151,6 @@ class UI {
const { CustomModuleCache } = require('../installers/lib/core/custom-module-cache');
const cache = new CustomModuleCache(bmadDir);
const { CustomHandler } = require('../installers/lib/custom/handler');
const customHandler = new CustomHandler();
const customFiles = await customHandler.findCustomContent(customContentConfig.customPath);
@ -218,7 +218,6 @@ class UI {
customContentConfig.selectedFiles = selectedCustomContent.map((mod) => mod.replace('__CUSTOM_CONTENT__', ''));
// Convert custom content to module IDs for installation
const customContentModuleIds = [];
const { CustomHandler } = require('../installers/lib/custom/handler');
const customHandler = new CustomHandler();
for (const customFile of customContentConfig.selectedFiles) {
// Get the module info to extract the ID
@ -637,8 +636,8 @@ class UI {
moduleData = yaml.load(yamlContent);
foundPath = configPath;
break;
} catch {
// Continue to next path
} catch (error) {
throw new Error(`Failed to parse config at ${configPath}: ${error.message}`);
}
}
}
@ -654,20 +653,11 @@ class UI {
cached: true,
});
} else {
// Debug: show what paths we tried to check
console.log(chalk.dim(`DEBUG: No module config found for ${cachedModule.id}`));
console.log(
chalk.dim(
`DEBUG: Tried paths:`,
possibleConfigPaths.map((p) => p.replace(cachedModule.cachePath, '.')),
),
);
console.log(chalk.dim(`DEBUG: cachedModule:`, JSON.stringify(cachedModule, null, 2)));
// Module config not found - skip silently (non-critical)
}
}
} else if (customContentConfig.customPath) {
// Existing installation - show from directory
const { CustomHandler } = require('../installers/lib/custom/handler');
const customHandler = new CustomHandler();
const customFiles = await customHandler.findCustomContent(customContentConfig.customPath);
@ -882,7 +872,6 @@ class UI {
expandedPath = this.expandUserPath(directory.trim());
// Check if directory has custom content
const { CustomHandler } = require('../installers/lib/custom/handler');
const customHandler = new CustomHandler();
const customFiles = await customHandler.findCustomContent(expandedPath);
@ -1277,7 +1266,6 @@ class UI {
const resolvedPath = CLIUtils.expandPath(customPath);
// Find custom content
const { CustomHandler } = require('../installers/lib/custom/handler');
const customHandler = new CustomHandler();
const customFiles = await customHandler.findCustomContent(resolvedPath);
@ -1302,12 +1290,10 @@ class UI {
// Display found items
console.log(chalk.cyan(`\nFound ${customFiles.length} custom content file(s):`));
const { CustomHandler: CustomHandler2 } = require('../installers/lib/custom/handler');
const customHandler2 = new CustomHandler2();
const customContentItems = [];
for (const customFile of customFiles) {
const customInfo = await customHandler2.getCustomInfo(customFile);
const customInfo = await customHandler.getCustomInfo(customFile);
if (customInfo) {
customContentItems.push({
name: `${chalk.cyan('✓')} ${customInfo.name} ${chalk.gray(`(${customInfo.relativePath})`)}`,