Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bmadcode merged 7 commits into from
Dec 8, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c28150f
fix: remove debug console.log statements from ui.js
alexeyv Dec 8, 2025
a2baaad
fix: add error handling and rollback for temp directory cleanup
alexeyv Dec 8, 2025
4d4290a
fix: use streaming for hash calculation to reduce memory usage
alexeyv Dec 8, 2025
6718475
refactor: hoist CustomHandler require to top of installer.js and ui.js
alexeyv Dec 8, 2025
0953809
fix: fail fast on malformed custom module YAML
alexeyv Dec 8, 2025
053840d
refactor: use consistent return type in handleMissingCustomSources
alexeyv Dec 8, 2025
3170c7a
refactor: clone config at install() entry to prevent mutation
alexeyv Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions tools/cli/installers/lib/core/custom-module-cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down
72 changes: 41 additions & 31 deletions tools/cli/installers/lib/core/installer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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);
await fs.move(srcPath, destPath);
movedItems.push({ src: srcPath, dest: 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)
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tools/cli/installers/lib/modules/manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
}
}
}
Expand Down
24 changes: 5 additions & 19 deletions tools/cli/lib/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}`);
}
}
}
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand All @@ -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})`)}`,
Expand Down