fix: address code review — fix interactive flow crash, simplify resolveCustomContentPaths
- Fix promptCustomContentSource() to use resolveCustomContentPaths() instead of reading module.yaml from root (would crash on parent dirs since validation was loosened but consumer was not updated) - Simplify resolveCustomContentPaths() to only check file existence, not parse YAML (eliminates double read — callers already handle parsing and validation) - Add edge case tests: module.yaml without code field, malformed YAML in subdirs, mixed direct + parent path resolution Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
7530f3b58a
commit
177353c77a
|
|
@ -67,6 +67,10 @@ async function createParentDirFixture() {
|
||||||
await fs.ensureDir(path.join(parentDir, 'not-a-module'));
|
await fs.ensureDir(path.join(parentDir, 'not-a-module'));
|
||||||
await fs.writeFile(path.join(parentDir, 'not-a-module', 'readme.md'), '# Not a module\n');
|
await fs.writeFile(path.join(parentDir, 'not-a-module', 'readme.md'), '# Not a module\n');
|
||||||
|
|
||||||
|
// Module with missing code field (invalid)
|
||||||
|
await fs.ensureDir(path.join(parentDir, 'bad-module'));
|
||||||
|
await fs.writeFile(path.join(parentDir, 'bad-module', 'module.yaml'), 'name: Bad Module\n');
|
||||||
|
|
||||||
return parentDir;
|
return parentDir;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -154,22 +158,26 @@ async function runTests() {
|
||||||
`Expected ${directModuleDir}, got ${directPaths[0]}`,
|
`Expected ${directModuleDir}, got ${directPaths[0]}`,
|
||||||
);
|
);
|
||||||
|
|
||||||
// Parent dir expands to individual module subdirs
|
// Parent dir expands to individual module subdirs (any dir with module.yaml)
|
||||||
const parentPaths = ui.resolveCustomContentPaths(parentDir);
|
const parentPaths = ui.resolveCustomContentPaths(parentDir);
|
||||||
assert(Array.isArray(parentPaths), 'resolveCustomContentPaths returns an array for parent dir');
|
assert(Array.isArray(parentPaths), 'resolveCustomContentPaths returns an array for parent dir');
|
||||||
assert(
|
assert(
|
||||||
parentPaths.length === 2,
|
parentPaths.length === 3,
|
||||||
'Parent dir resolves to 2 module paths (skips not-a-module)',
|
'Parent dir resolves to 3 module paths (skips not-a-module which has no module.yaml)',
|
||||||
`Expected 2, got ${parentPaths.length}: ${JSON.stringify(parentPaths)}`,
|
`Expected 3, got ${parentPaths.length}: ${JSON.stringify(parentPaths)}`,
|
||||||
);
|
);
|
||||||
|
|
||||||
const resolvedNames = parentPaths.map((p) => path.basename(p)).sort();
|
const resolvedNames = parentPaths.map((p) => path.basename(p)).sort();
|
||||||
assert(
|
assert(
|
||||||
resolvedNames[0] === 'module-a' && resolvedNames[1] === 'module-b',
|
resolvedNames.includes('module-a') && resolvedNames.includes('module-b'),
|
||||||
'Parent dir resolves to module-a and module-b',
|
'Parent dir includes module-a and module-b',
|
||||||
`Got: ${JSON.stringify(resolvedNames)}`,
|
`Got: ${JSON.stringify(resolvedNames)}`,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// bad-module has module.yaml but no code field — resolveCustomContentPaths includes it
|
||||||
|
// (callers are responsible for filtering invalid modules)
|
||||||
|
assert(resolvedNames.includes('bad-module'), 'Parent dir includes bad-module (has module.yaml, callers filter by code)');
|
||||||
|
|
||||||
// Empty dir returns empty array
|
// Empty dir returns empty array
|
||||||
const emptyPaths = ui.resolveCustomContentPaths(emptyDir);
|
const emptyPaths = ui.resolveCustomContentPaths(emptyDir);
|
||||||
assert(Array.isArray(emptyPaths), 'resolveCustomContentPaths returns an array for empty dir');
|
assert(Array.isArray(emptyPaths), 'resolveCustomContentPaths returns an array for empty dir');
|
||||||
|
|
@ -185,6 +193,55 @@ async function runTests() {
|
||||||
if (emptyDir) await fs.remove(emptyDir).catch(() => {});
|
if (emptyDir) await fs.remove(emptyDir).catch(() => {});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ============================================================
|
||||||
|
// Test Suite 3: Edge cases
|
||||||
|
// ============================================================
|
||||||
|
console.log(`\n${colors.yellow}Test Suite 3: Edge cases${colors.reset}\n`);
|
||||||
|
|
||||||
|
let parentWithBadModule;
|
||||||
|
let directModule2;
|
||||||
|
|
||||||
|
try {
|
||||||
|
// Parent dir where only subdir has module.yaml without code field
|
||||||
|
parentWithBadModule = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-bad-'));
|
||||||
|
await fs.ensureDir(path.join(parentWithBadModule, 'no-code-module'));
|
||||||
|
await fs.writeFile(path.join(parentWithBadModule, 'no-code-module', 'module.yaml'), 'name: No Code Module\n');
|
||||||
|
|
||||||
|
// resolveCustomContentPaths includes it (callers filter)
|
||||||
|
const badPaths = ui.resolveCustomContentPaths(parentWithBadModule);
|
||||||
|
assert(badPaths.length === 1, 'Dir with only code-less module.yaml still resolves (callers filter)');
|
||||||
|
|
||||||
|
// validateCustomContentPathSync accepts it (has module.yaml in subdir)
|
||||||
|
const badValidation = ui.validateCustomContentPathSync(parentWithBadModule);
|
||||||
|
assert(
|
||||||
|
badValidation === undefined,
|
||||||
|
'Parent dir with code-less module.yaml passes validation (callers handle filtering)',
|
||||||
|
`Got error: ${badValidation}`,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Subdir with malformed YAML
|
||||||
|
await fs.ensureDir(path.join(parentWithBadModule, 'malformed'));
|
||||||
|
await fs.writeFile(path.join(parentWithBadModule, 'malformed', 'module.yaml'), '{{invalid yaml');
|
||||||
|
const malformedPaths = ui.resolveCustomContentPaths(parentWithBadModule);
|
||||||
|
assert(malformedPaths.length === 2, 'Subdirs with malformed YAML are still resolved (callers handle parse errors)');
|
||||||
|
|
||||||
|
// Direct module alongside parent dir (simulates comma-separated usage)
|
||||||
|
directModule2 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-direct2-'));
|
||||||
|
await fs.writeFile(path.join(directModule2, 'module.yaml'), 'code: direct-2\nname: Direct Two\n');
|
||||||
|
|
||||||
|
const directPaths2 = ui.resolveCustomContentPaths(directModule2);
|
||||||
|
const parentPaths2 = ui.resolveCustomContentPaths(parentWithBadModule);
|
||||||
|
const combined = [...directPaths2, ...parentPaths2];
|
||||||
|
assert(
|
||||||
|
combined.length === 3,
|
||||||
|
'Mixed direct + parent paths combine correctly (1 direct + 2 from parent)',
|
||||||
|
`Expected 3, got ${combined.length}`,
|
||||||
|
);
|
||||||
|
} finally {
|
||||||
|
if (parentWithBadModule) await fs.remove(parentWithBadModule).catch(() => {});
|
||||||
|
if (directModule2) await fs.remove(directModule2).catch(() => {});
|
||||||
|
}
|
||||||
|
|
||||||
// ============================================================
|
// ============================================================
|
||||||
// Summary
|
// Summary
|
||||||
// ============================================================
|
// ============================================================
|
||||||
|
|
|
||||||
|
|
@ -1500,17 +1500,7 @@ class UI {
|
||||||
if (entry.isDirectory()) {
|
if (entry.isDirectory()) {
|
||||||
const subModuleYaml = path.join(inputPath, entry.name, 'module.yaml');
|
const subModuleYaml = path.join(inputPath, entry.name, 'module.yaml');
|
||||||
if (fs.pathExistsSync(subModuleYaml)) {
|
if (fs.pathExistsSync(subModuleYaml)) {
|
||||||
// Validate the module.yaml has a code field
|
resolved.push(path.join(inputPath, entry.name));
|
||||||
try {
|
|
||||||
const yaml = require('yaml');
|
|
||||||
const content = fs.readFileSync(subModuleYaml, 'utf8');
|
|
||||||
const moduleData = yaml.parse(content);
|
|
||||||
if (moduleData && moduleData.code) {
|
|
||||||
resolved.push(path.join(inputPath, entry.name));
|
|
||||||
}
|
|
||||||
} catch {
|
|
||||||
// Skip invalid module.yaml files
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -1624,20 +1614,45 @@ class UI {
|
||||||
isValid = true;
|
isValid = true;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Read module.yaml to get module info
|
// Resolve path to module directories (supports parent dirs with subdirectories)
|
||||||
const yaml = require('yaml');
|
const resolvedPaths = this.resolveCustomContentPaths(sourcePath);
|
||||||
const moduleYamlPath = path.join(sourcePath, 'module.yaml');
|
|
||||||
const moduleContent = await fs.readFile(moduleYamlPath, 'utf8');
|
|
||||||
const moduleData = yaml.parse(moduleContent);
|
|
||||||
|
|
||||||
// Add to sources
|
if (resolvedPaths.length === 0) {
|
||||||
customContentConfig.sources.push({
|
await prompts.log.warn(`No module.yaml found in ${sourcePath} (checked root and immediate subdirectories)`);
|
||||||
path: sourcePath,
|
continue;
|
||||||
id: moduleData.code,
|
}
|
||||||
name: moduleData.name || moduleData.code,
|
|
||||||
});
|
|
||||||
|
|
||||||
await prompts.log.success(`Confirmed local custom module: ${moduleData.name || moduleData.code}`);
|
if (resolvedPaths.length > 1) {
|
||||||
|
await prompts.log.info(`Found ${resolvedPaths.length} modules in ${sourcePath}`);
|
||||||
|
}
|
||||||
|
|
||||||
|
for (const modulePath of resolvedPaths) {
|
||||||
|
// Read module.yaml to get module info
|
||||||
|
const yaml = require('yaml');
|
||||||
|
const moduleYamlPath = path.join(modulePath, 'module.yaml');
|
||||||
|
let moduleData;
|
||||||
|
try {
|
||||||
|
const moduleContent = await fs.readFile(moduleYamlPath, 'utf8');
|
||||||
|
moduleData = yaml.parse(moduleContent);
|
||||||
|
} catch (error) {
|
||||||
|
await prompts.log.warn(`Skipping ${modulePath} - failed to read module.yaml: ${error.message}`);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!moduleData || !moduleData.code) {
|
||||||
|
await prompts.log.warn(`Skipping ${modulePath} - module.yaml missing 'code' field`);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Add to sources
|
||||||
|
customContentConfig.sources.push({
|
||||||
|
path: modulePath,
|
||||||
|
id: moduleData.code,
|
||||||
|
name: moduleData.name || moduleData.code,
|
||||||
|
});
|
||||||
|
|
||||||
|
await prompts.log.success(`Confirmed local custom module: ${moduleData.name || moduleData.code}`);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Ask if user wants to add these to the installation
|
// Ask if user wants to add these to the installation
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue