fix: move module directories instead of creating new ones on path change
When users modify a module's directory path during installer update/modify, the old directory is now moved to the new location instead of creating an empty directory while leaving the old one (with its documents) behind. Includes: cross-device fs.move, error handling with fallback, path normalization, parent/child path guard, and warning when both dirs exist. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
2f28ef1b80
commit
3287073530
|
|
@ -883,7 +883,7 @@ class Installer {
|
||||||
let taskResolution;
|
let taskResolution;
|
||||||
|
|
||||||
// Collect directory creation results for output after tasks() completes
|
// Collect directory creation results for output after tasks() completes
|
||||||
const dirResults = { createdDirs: [], createdWdsFolders: [] };
|
const dirResults = { createdDirs: [], movedDirs: [], createdWdsFolders: [] };
|
||||||
|
|
||||||
// Build task list conditionally
|
// Build task list conditionally
|
||||||
const installTasks = [];
|
const installTasks = [];
|
||||||
|
|
@ -1055,12 +1055,14 @@ class Installer {
|
||||||
const result = await this.moduleManager.createModuleDirectories('core', bmadDir, {
|
const result = await this.moduleManager.createModuleDirectories('core', bmadDir, {
|
||||||
installedIDEs: config.ides || [],
|
installedIDEs: config.ides || [],
|
||||||
moduleConfig: moduleConfigs.core || {},
|
moduleConfig: moduleConfigs.core || {},
|
||||||
|
existingModuleConfig: this.configCollector.existingConfig?.core || {},
|
||||||
coreConfig: moduleConfigs.core || {},
|
coreConfig: moduleConfigs.core || {},
|
||||||
logger: moduleLogger,
|
logger: moduleLogger,
|
||||||
silent: true,
|
silent: true,
|
||||||
});
|
});
|
||||||
if (result) {
|
if (result) {
|
||||||
dirResults.createdDirs.push(...result.createdDirs);
|
dirResults.createdDirs.push(...result.createdDirs);
|
||||||
|
dirResults.movedDirs.push(...(result.movedDirs || []));
|
||||||
dirResults.createdWdsFolders.push(...result.createdWdsFolders);
|
dirResults.createdWdsFolders.push(...result.createdWdsFolders);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -1072,12 +1074,14 @@ class Installer {
|
||||||
const result = await this.moduleManager.createModuleDirectories(moduleName, bmadDir, {
|
const result = await this.moduleManager.createModuleDirectories(moduleName, bmadDir, {
|
||||||
installedIDEs: config.ides || [],
|
installedIDEs: config.ides || [],
|
||||||
moduleConfig: moduleConfigs[moduleName] || {},
|
moduleConfig: moduleConfigs[moduleName] || {},
|
||||||
|
existingModuleConfig: this.configCollector.existingConfig?.[moduleName] || {},
|
||||||
coreConfig: moduleConfigs.core || {},
|
coreConfig: moduleConfigs.core || {},
|
||||||
logger: moduleLogger,
|
logger: moduleLogger,
|
||||||
silent: true,
|
silent: true,
|
||||||
});
|
});
|
||||||
if (result) {
|
if (result) {
|
||||||
dirResults.createdDirs.push(...result.createdDirs);
|
dirResults.createdDirs.push(...result.createdDirs);
|
||||||
|
dirResults.movedDirs.push(...(result.movedDirs || []));
|
||||||
dirResults.createdWdsFolders.push(...result.createdWdsFolders);
|
dirResults.createdWdsFolders.push(...result.createdWdsFolders);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -1148,6 +1152,10 @@ class Installer {
|
||||||
|
|
||||||
// Render directory creation output right after directory task
|
// Render directory creation output right after directory task
|
||||||
const color = await prompts.getColor();
|
const color = await prompts.getColor();
|
||||||
|
if (dirResults.movedDirs.length > 0) {
|
||||||
|
const lines = dirResults.movedDirs.map((d) => ` ${d}`).join('\n');
|
||||||
|
await prompts.log.message(color.cyan(`Moved directories:\n${lines}`));
|
||||||
|
}
|
||||||
if (dirResults.createdDirs.length > 0) {
|
if (dirResults.createdDirs.length > 0) {
|
||||||
const lines = dirResults.createdDirs.map((d) => ` ${d}`).join('\n');
|
const lines = dirResults.createdDirs.map((d) => ` ${d}`).join('\n');
|
||||||
await prompts.log.message(color.yellow(`Created directories:\n${lines}`));
|
await prompts.log.message(color.yellow(`Created directories:\n${lines}`));
|
||||||
|
|
|
||||||
|
|
@ -1247,17 +1247,20 @@ class ModuleManager {
|
||||||
/**
|
/**
|
||||||
* Create directories declared in module.yaml's `directories` key
|
* Create directories declared in module.yaml's `directories` key
|
||||||
* This replaces the security-risky module installer pattern with declarative config
|
* This replaces the security-risky module installer pattern with declarative config
|
||||||
|
* During updates, if a directory path changed, moves the old directory to the new path
|
||||||
* @param {string} moduleName - Name of the module
|
* @param {string} moduleName - Name of the module
|
||||||
* @param {string} bmadDir - Target bmad directory
|
* @param {string} bmadDir - Target bmad directory
|
||||||
* @param {Object} options - Installation options
|
* @param {Object} options - Installation options
|
||||||
* @param {Object} options.moduleConfig - Module configuration from config collector
|
* @param {Object} options.moduleConfig - Module configuration from config collector
|
||||||
|
* @param {Object} options.existingModuleConfig - Previous module config (for detecting path changes during updates)
|
||||||
* @param {Object} options.coreConfig - Core configuration
|
* @param {Object} options.coreConfig - Core configuration
|
||||||
* @returns {Promise<{createdDirs: string[], createdWdsFolders: string[]}>} Created directories info
|
* @returns {Promise<{createdDirs: string[], movedDirs: string[], createdWdsFolders: string[]}>} Created directories info
|
||||||
*/
|
*/
|
||||||
async createModuleDirectories(moduleName, bmadDir, options = {}) {
|
async createModuleDirectories(moduleName, bmadDir, options = {}) {
|
||||||
const moduleConfig = options.moduleConfig || {};
|
const moduleConfig = options.moduleConfig || {};
|
||||||
|
const existingModuleConfig = options.existingModuleConfig || {};
|
||||||
const projectRoot = path.dirname(bmadDir);
|
const projectRoot = path.dirname(bmadDir);
|
||||||
const emptyResult = { createdDirs: [], createdWdsFolders: [] };
|
const emptyResult = { createdDirs: [], movedDirs: [], createdWdsFolders: [] };
|
||||||
|
|
||||||
// Special handling for core module - it's in src/core not src/modules
|
// Special handling for core module - it's in src/core not src/modules
|
||||||
let sourcePath;
|
let sourcePath;
|
||||||
|
|
@ -1291,6 +1294,7 @@ class ModuleManager {
|
||||||
const directories = moduleYaml.directories;
|
const directories = moduleYaml.directories;
|
||||||
const wdsFolders = moduleYaml.wds_folders || [];
|
const wdsFolders = moduleYaml.wds_folders || [];
|
||||||
const createdDirs = [];
|
const createdDirs = [];
|
||||||
|
const movedDirs = [];
|
||||||
const createdWdsFolders = [];
|
const createdWdsFolders = [];
|
||||||
|
|
||||||
for (const dirRef of directories) {
|
for (const dirRef of directories) {
|
||||||
|
|
@ -1325,9 +1329,74 @@ class ModuleManager {
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Create directory if it doesn't exist
|
// Check if directory path changed from previous config (update/modify scenario)
|
||||||
if (!(await fs.pathExists(fullPath))) {
|
const oldDirValue = existingModuleConfig[configKey];
|
||||||
const dirName = configKey.replaceAll('_', ' ');
|
let oldFullPath = null;
|
||||||
|
let oldDirPath = null;
|
||||||
|
if (oldDirValue && typeof oldDirValue === 'string') {
|
||||||
|
// F3: Normalize both values before comparing to avoid false negatives
|
||||||
|
// from trailing slashes, separator differences, or prefix format variations
|
||||||
|
let normalizedOld = oldDirValue.replace(/^\{project-root\}\/?/, '');
|
||||||
|
normalizedOld = path.normalize(normalizedOld.replaceAll('{project-root}', ''));
|
||||||
|
const normalizedNew = path.normalize(dirPath);
|
||||||
|
|
||||||
|
if (normalizedOld !== normalizedNew) {
|
||||||
|
oldDirPath = normalizedOld;
|
||||||
|
oldFullPath = path.join(projectRoot, oldDirPath);
|
||||||
|
const normalizedOldAbsolute = path.normalize(oldFullPath);
|
||||||
|
if (!normalizedOldAbsolute.startsWith(normalizedRoot + path.sep) && normalizedOldAbsolute !== normalizedRoot) {
|
||||||
|
oldFullPath = null; // Old path escapes project root, ignore it
|
||||||
|
}
|
||||||
|
|
||||||
|
// F13: Prevent parent/child move (e.g. docs/planning → docs/planning/v2)
|
||||||
|
if (oldFullPath) {
|
||||||
|
const normalizedNewAbsolute = path.normalize(fullPath);
|
||||||
|
if (
|
||||||
|
normalizedOldAbsolute.startsWith(normalizedNewAbsolute + path.sep) ||
|
||||||
|
normalizedNewAbsolute.startsWith(normalizedOldAbsolute + path.sep)
|
||||||
|
) {
|
||||||
|
const color = await prompts.getColor();
|
||||||
|
await prompts.log.warn(
|
||||||
|
color.yellow(
|
||||||
|
`${configKey}: cannot move between parent/child paths (${oldDirPath} / ${dirPath}), creating new directory instead`,
|
||||||
|
),
|
||||||
|
);
|
||||||
|
oldFullPath = null;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
const dirName = configKey.replaceAll('_', ' ');
|
||||||
|
|
||||||
|
if (oldFullPath && (await fs.pathExists(oldFullPath)) && !(await fs.pathExists(fullPath))) {
|
||||||
|
// Path changed and old dir exists → move old to new location
|
||||||
|
// F1: Use fs.move() instead of fs.rename() for cross-device/volume support
|
||||||
|
// F2: Wrap in try/catch — fallback to creating new dir on failure
|
||||||
|
try {
|
||||||
|
await fs.ensureDir(path.dirname(fullPath));
|
||||||
|
await fs.move(oldFullPath, fullPath);
|
||||||
|
movedDirs.push(`${dirName}: ${oldDirPath} → ${dirPath}`);
|
||||||
|
} catch (moveError) {
|
||||||
|
const color = await prompts.getColor();
|
||||||
|
await prompts.log.warn(
|
||||||
|
color.yellow(
|
||||||
|
`Failed to move ${oldDirPath} → ${dirPath}: ${moveError.message}\n Creating new directory instead. Please move contents from the old directory manually.`,
|
||||||
|
),
|
||||||
|
);
|
||||||
|
await fs.ensureDir(fullPath);
|
||||||
|
createdDirs.push(`${dirName}: ${dirPath}`);
|
||||||
|
}
|
||||||
|
} else if (oldFullPath && (await fs.pathExists(oldFullPath)) && (await fs.pathExists(fullPath))) {
|
||||||
|
// F5: Both old and new directories exist — warn user about potential orphaned documents
|
||||||
|
const color = await prompts.getColor();
|
||||||
|
await prompts.log.warn(
|
||||||
|
color.yellow(
|
||||||
|
`${dirName}: path changed but both directories exist:\n Old: ${oldDirPath}\n New: ${dirPath}\n Old directory may contain orphaned documents — please review and merge manually.`,
|
||||||
|
),
|
||||||
|
);
|
||||||
|
} else if (!(await fs.pathExists(fullPath))) {
|
||||||
|
// New directory doesn't exist yet → create it
|
||||||
createdDirs.push(`${dirName}: ${dirPath}`);
|
createdDirs.push(`${dirName}: ${dirPath}`);
|
||||||
await fs.ensureDir(fullPath);
|
await fs.ensureDir(fullPath);
|
||||||
}
|
}
|
||||||
|
|
@ -1344,7 +1413,7 @@ class ModuleManager {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return { createdDirs, createdWdsFolders };
|
return { createdDirs, movedDirs, createdWdsFolders };
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue