Skip to content

Commit cf50f49

Browse files
alexeyvalexeyv
authored
fix: address code review issues from alpha.14 to alpha.15 (#1068)
* 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
1 parent 55cb468 commit cf50f49

File tree

4 files changed

+66
-58
lines changed

4 files changed

+66
-58
lines changed

‎tools/cli/installers/lib/core/custom-module-cache.js‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,19 @@ class CustomModuleCache {
5151
}
5252

5353
/**
54-
* Calculate hash of a file or directory
54+
* Stream a file into the hash to avoid loading entire file into memory
55+
*/
56+
async hashFileStream(filePath, hash) {
57+
return new Promise((resolve, reject) => {
58+
const stream = require('node:fs').createReadStream(filePath);
59+
stream.on('data', (chunk) => hash.update(chunk));
60+
stream.on('end', resolve);
61+
stream.on('error', reject);
62+
});
63+
}
64+
65+
/**
66+
* Calculate hash of a file or directory using streaming to minimize memory usage
5567
*/
5668
async calculateHash(sourcePath) {
5769
const hash = crypto.createHash('sha256');
@@ -76,14 +88,14 @@ class CustomModuleCache {
7688
files.sort(); // Ensure consistent order
7789

7890
for (const file of files) {
79-
const content = await fs.readFile(file);
8091
const relativePath = path.relative(sourcePath, file);
81-
hash.update(relativePath + '|' + content.toString('base64'));
92+
// Hash the path first, then stream file contents
93+
hash.update(relativePath + '|');
94+
await this.hashFileStream(file, hash);
8295
}
8396
} else {
84-
// For single files
85-
const content = await fs.readFile(sourcePath);
86-
hash.update(content);
97+
// For single files, stream directly into hash
98+
await this.hashFileStream(sourcePath, hash);
8799
}
88100

89101
return hash.digest('hex');

‎tools/cli/installers/lib/core/installer.js‎

Lines changed: 41 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ const { CLIUtils } = require('../../../lib/cli-utils');
3939
const { ManifestGenerator } = require('./manifest-generator');
4040
const { IdeConfigManager } = require('./ide-config-manager');
4141
const { replaceAgentSidecarFolders } = require('./post-install-sidecar-replacement');
42+
const { CustomHandler } = require('../custom/handler');
4243

4344
class Installer {
4445
constructor() {
@@ -407,7 +408,10 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
407408
* @param {string[]} config.ides - IDEs to configure
408409
* @param {boolean} config.skipIde - Skip IDE configuration
409410
*/
410-
async install(config) {
411+
async install(originalConfig) {
412+
// Clone config to avoid mutating the caller's object
413+
const config = { ...originalConfig };
414+
411415
// Display BMAD logo
412416
CLIUtils.displayLogo();
413417

@@ -440,7 +444,6 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
440444

441445
// Handle selectedFiles (from existing install path or manual directory input)
442446
if (config.customContent && config.customContent.selected && config.customContent.selectedFiles) {
443-
const { CustomHandler } = require('../custom/handler');
444447
const customHandler = new CustomHandler();
445448
for (const customFile of config.customContent.selectedFiles) {
446449
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:
837840
// Regular custom content from user input (non-cached)
838841
if (finalCustomContent && finalCustomContent.selected && finalCustomContent.selectedFiles) {
839842
// Add custom modules to the installation list
843+
const customHandler = new CustomHandler();
840844
for (const customFile of finalCustomContent.selectedFiles) {
841-
const { CustomHandler } = require('../custom/handler');
842-
const customHandler = new CustomHandler();
843845
const customInfo = await customHandler.getCustomInfo(customFile, projectDir);
844846
if (customInfo && customInfo.id) {
845847
allModules.push(customInfo.id);
@@ -929,7 +931,6 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
929931

930932
// Finally check regular custom content
931933
if (!isCustomModule && finalCustomContent && finalCustomContent.selected && finalCustomContent.selectedFiles) {
932-
const { CustomHandler } = require('../custom/handler');
933934
const customHandler = new CustomHandler();
934935
for (const customFile of finalCustomContent.selectedFiles) {
935936
const info = await customHandler.getCustomInfo(customFile, projectDir);
@@ -943,7 +944,6 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
943944

944945
if (isCustomModule && customInfo) {
945946
// Install custom module using CustomHandler but as a proper module
946-
const { CustomHandler } = require('../custom/handler');
947947
const customHandler = new CustomHandler();
948948

949949
// 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:
972972
if (await fs.pathExists(customDir)) {
973973
// Move contents to module directory
974974
const items = await fs.readdir(customDir);
975-
for (const item of items) {
976-
const srcPath = path.join(customDir, item);
977-
const destPath = path.join(moduleTargetPath, item);
975+
const movedItems = [];
976+
try {
977+
for (const item of items) {
978+
const srcPath = path.join(customDir, item);
979+
const destPath = path.join(moduleTargetPath, item);
978980

979-
// If destination exists, remove it first (or we could merge)
980-
if (await fs.pathExists(destPath)) {
981-
await fs.remove(destPath);
982-
}
981+
// If destination exists, remove it first (or we could merge)
982+
if (await fs.pathExists(destPath)) {
983+
await fs.remove(destPath);
984+
}
983985

984-
await fs.move(srcPath, destPath);
986+
await fs.move(srcPath, destPath);
987+
movedItems.push({ src: srcPath, dest: destPath });
988+
}
989+
} catch (moveError) {
990+
// Rollback: restore any successfully moved items
991+
for (const moved of movedItems) {
992+
try {
993+
await fs.move(moved.dest, moved.src);
994+
} catch {
995+
// Best-effort rollback - log if it fails
996+
console.error(`Failed to rollback ${moved.dest} during cleanup`);
997+
}
998+
}
999+
throw new Error(`Failed to move custom module files: ${moveError.message}`);
9851000
}
9861001
}
987-
await fs.remove(tempCustomPath);
1002+
try {
1003+
await fs.remove(tempCustomPath);
1004+
} catch (cleanupError) {
1005+
// Non-fatal: temp directory cleanup failed but files were moved successfully
1006+
console.warn(`Warning: Could not clean up temp directory: ${cleanupError.message}`);
1007+
}
9881008
}
9891009

9901010
// 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:
10661086
config.customContent.selectedFiles
10671087
) {
10681088
// Filter out custom modules that were already installed
1089+
const customHandler = new CustomHandler();
10691090
for (const customFile of config.customContent.selectedFiles) {
1070-
const { CustomHandler } = require('../custom/handler');
1071-
const customHandler = new CustomHandler();
10721091
const customInfo = await customHandler.getCustomInfo(customFile, projectDir);
10731092

10741093
// 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:
10801099

10811100
if (remainingCustomContent.length > 0) {
10821101
spinner.start('Installing remaining custom content...');
1083-
const { CustomHandler } = require('../custom/handler');
10841102
const customHandler = new CustomHandler();
10851103

10861104
// Use the remaining files
@@ -2581,18 +2599,7 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
25812599
installedModules,
25822600
);
25832601

2584-
// Handle both old return format (array) and new format (object)
2585-
let validCustomModules = [];
2586-
let keptModulesWithoutSources = [];
2587-
2588-
if (Array.isArray(customModuleResult)) {
2589-
// Old format - just an array
2590-
validCustomModules = customModuleResult;
2591-
} else if (customModuleResult && typeof customModuleResult === 'object') {
2592-
// New format - object with two arrays
2593-
validCustomModules = customModuleResult.validCustomModules || [];
2594-
keptModulesWithoutSources = customModuleResult.keptModulesWithoutSources || [];
2595-
}
2602+
const { validCustomModules, keptModulesWithoutSources } = customModuleResult;
25962603

25972604
const customModulesFromManifest = validCustomModules.map((m) => ({
25982605
...m,
@@ -3371,7 +3378,10 @@ If AgentVibes party mode is enabled, immediately trigger TTS with agent's voice:
33713378

33723379
// If no missing sources, return immediately
33733380
if (customModulesWithMissingSources.length === 0) {
3374-
return validCustomModules;
3381+
return {
3382+
validCustomModules,
3383+
keptModulesWithoutSources: [],
3384+
};
33753385
}
33763386

33773387
// Stop any spinner for interactive prompts

‎tools/cli/installers/lib/modules/manager.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -391,8 +391,8 @@ class ModuleManager {
391391
if (config.code === moduleName) {
392392
return modulePath;
393393
}
394-
} catch {
395-
// Skip if can't read config
394+
} catch (error) {
395+
throw new Error(`Failed to parse module.yaml at ${configPath}: ${error.message}`);
396396
}
397397
}
398398
}

‎tools/cli/lib/ui.js‎

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ const path = require('node:path');
2424
const os = require('node:os');
2525
const fs = require('fs-extra');
2626
const { CLIUtils } = require('./cli-utils');
27+
const { CustomHandler } = require('../installers/lib/custom/handler');
2728

2829
/**
2930
* UI utilities for the installer
@@ -150,7 +151,6 @@ class UI {
150151
const { CustomModuleCache } = require('../installers/lib/core/custom-module-cache');
151152
const cache = new CustomModuleCache(bmadDir);
152153

153-
const { CustomHandler } = require('../installers/lib/custom/handler');
154154
const customHandler = new CustomHandler();
155155
const customFiles = await customHandler.findCustomContent(customContentConfig.customPath);
156156

@@ -218,7 +218,6 @@ class UI {
218218
customContentConfig.selectedFiles = selectedCustomContent.map((mod) => mod.replace('__CUSTOM_CONTENT__', ''));
219219
// Convert custom content to module IDs for installation
220220
const customContentModuleIds = [];
221-
const { CustomHandler } = require('../installers/lib/custom/handler');
222221
const customHandler = new CustomHandler();
223222
for (const customFile of customContentConfig.selectedFiles) {
224223
// Get the module info to extract the ID
@@ -637,8 +636,8 @@ class UI {
637636
moduleData = yaml.load(yamlContent);
638637
foundPath = configPath;
639638
break;
640-
} catch {
641-
// Continue to next path
639+
} catch (error) {
640+
throw new Error(`Failed to parse config at ${configPath}: ${error.message}`);
642641
}
643642
}
644643
}
@@ -654,20 +653,11 @@ class UI {
654653
cached: true,
655654
});
656655
} else {
657-
// Debug: show what paths we tried to check
658-
console.log(chalk.dim(`DEBUG: No module config found for ${cachedModule.id}`));
659-
console.log(
660-
chalk.dim(
661-
`DEBUG: Tried paths:`,
662-
possibleConfigPaths.map((p) => p.replace(cachedModule.cachePath, '.')),
663-
),
664-
);
665-
console.log(chalk.dim(`DEBUG: cachedModule:`, JSON.stringify(cachedModule, null, 2)));
656+
// Module config not found - skip silently (non-critical)
666657
}
667658
}
668659
} else if (customContentConfig.customPath) {
669660
// Existing installation - show from directory
670-
const { CustomHandler } = require('../installers/lib/custom/handler');
671661
const customHandler = new CustomHandler();
672662
const customFiles = await customHandler.findCustomContent(customContentConfig.customPath);
673663

@@ -882,7 +872,6 @@ class UI {
882872
expandedPath = this.expandUserPath(directory.trim());
883873

884874
// Check if directory has custom content
885-
const { CustomHandler } = require('../installers/lib/custom/handler');
886875
const customHandler = new CustomHandler();
887876
const customFiles = await customHandler.findCustomContent(expandedPath);
888877

@@ -1277,7 +1266,6 @@ class UI {
12771266
const resolvedPath = CLIUtils.expandPath(customPath);
12781267

12791268
// Find custom content
1280-
const { CustomHandler } = require('../installers/lib/custom/handler');
12811269
const customHandler = new CustomHandler();
12821270
const customFiles = await customHandler.findCustomContent(resolvedPath);
12831271

@@ -1302,12 +1290,10 @@ class UI {
13021290

13031291
// Display found items
13041292
console.log(chalk.cyan(`\nFound ${customFiles.length} custom content file(s):`));
1305-
const { CustomHandler: CustomHandler2 } = require('../installers/lib/custom/handler');
1306-
const customHandler2 = new CustomHandler2();
13071293
const customContentItems = [];
13081294

13091295
for (const customFile of customFiles) {
1310-
const customInfo = await customHandler2.getCustomInfo(customFile);
1296+
const customInfo = await customHandler.getCustomInfo(customFile);
13111297
if (customInfo) {
13121298
customContentItems.push({
13131299
name: `${chalk.cyan('✓')} ${customInfo.name} ${chalk.gray(`(${customInfo.relativePath})`)}`,

0 commit comments

Comments
 (0)