fix(manifest): address PR review findings from triage
- Add missing skillClaimedDirs guard to getAgentsFromDir (F1) - Add skills to this.files[] in collectSkills (F2) - Add test for type:skill inside workflows/ dir (F5) - Warn on malformed workflow.md parse in skill dirs (F6) - Add skills count to generateManifests return value (F9) - Remove redundant \r? from regex after line normalization (F10) - Normalize path.relative to forward slashes for cross-platform (F12) - Enforce directory name as skill canonicalId, warn if manifest overrides (F13) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Alex Verkhovsky
parent
99d2afe584
commit
149c4049fd
|
|
@ -1605,7 +1605,7 @@ async function runTests() {
|
|||
// --- Skill at unusual path: core/custom-area/my-skill/ ---
|
||||
const skillDir29 = path.join(tempFixture29, 'core', 'custom-area', 'my-skill');
|
||||
await fs.ensureDir(skillDir29);
|
||||
await fs.writeFile(path.join(skillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\ncanonicalId: my-custom-skill\n');
|
||||
await fs.writeFile(path.join(skillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\n');
|
||||
await fs.writeFile(
|
||||
path.join(skillDir29, 'workflow.md'),
|
||||
'---\nname: My Custom Skill\ndescription: A skill at an unusual path\n---\n\nSkill body content\n',
|
||||
|
|
@ -1620,10 +1620,19 @@ async function runTests() {
|
|||
'---\nname: Regular Workflow\ndescription: A regular workflow not a skill\n---\n\nWorkflow body\n',
|
||||
);
|
||||
|
||||
// --- Skill inside workflows/ dir: core/workflows/wf-skill/ (exercises findWorkflows skip logic) ---
|
||||
const wfSkillDir29 = path.join(tempFixture29, 'core', 'workflows', 'wf-skill');
|
||||
await fs.ensureDir(wfSkillDir29);
|
||||
await fs.writeFile(path.join(wfSkillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\n');
|
||||
await fs.writeFile(
|
||||
path.join(wfSkillDir29, 'workflow.md'),
|
||||
'---\nname: Workflow Skill\ndescription: A skill inside workflows dir\n---\n\nSkill in workflows\n',
|
||||
);
|
||||
|
||||
// --- Skill inside tasks/ dir: core/tasks/task-skill/ ---
|
||||
const taskSkillDir29 = path.join(tempFixture29, 'core', 'tasks', 'task-skill');
|
||||
await fs.ensureDir(taskSkillDir29);
|
||||
await fs.writeFile(path.join(taskSkillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\ncanonicalId: task-skill\n');
|
||||
await fs.writeFile(path.join(taskSkillDir29, 'bmad-skill-manifest.yaml'), 'type: skill\n');
|
||||
await fs.writeFile(
|
||||
path.join(taskSkillDir29, 'workflow.md'),
|
||||
'---\nname: Task Skill\ndescription: A skill inside tasks dir\n---\n\nSkill in tasks\n',
|
||||
|
|
@ -1638,7 +1647,7 @@ async function runTests() {
|
|||
await generator29.generateManifests(tempFixture29, ['core'], [], { ides: [] });
|
||||
|
||||
// Skill at unusual path should be in skills
|
||||
const skillEntry29 = generator29.skills.find((s) => s.canonicalId === 'my-custom-skill');
|
||||
const skillEntry29 = generator29.skills.find((s) => s.canonicalId === 'my-skill');
|
||||
assert(skillEntry29 !== undefined, 'Skill at unusual path appears in skills[]');
|
||||
assert(skillEntry29 && skillEntry29.name === 'My Custom Skill', 'Skill has correct name from frontmatter');
|
||||
assert(
|
||||
|
|
@ -1665,13 +1674,16 @@ async function runTests() {
|
|||
const regularInSkills29 = generator29.skills.find((s) => s.canonicalId === 'regular-wf');
|
||||
assert(regularInSkills29 === undefined, 'Regular type:workflow does NOT appear in skills[]');
|
||||
|
||||
// Skill inside workflows/ should be in skills[], NOT in workflows[] (exercises findWorkflows skip at lines 311/322)
|
||||
const wfSkill29 = generator29.skills.find((s) => s.canonicalId === 'wf-skill');
|
||||
assert(wfSkill29 !== undefined, 'Skill in workflows/ dir appears in skills[]');
|
||||
const wfSkillInWorkflows29 = generator29.workflows.find((w) => w.name === 'Workflow Skill');
|
||||
assert(wfSkillInWorkflows29 === undefined, 'Skill in workflows/ dir does NOT appear in workflows[]');
|
||||
|
||||
// Test scanInstalledModules recognizes skill-only modules
|
||||
const skillOnlyModDir29 = path.join(tempFixture29, 'skill-only-mod');
|
||||
await fs.ensureDir(path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill'));
|
||||
await fs.writeFile(
|
||||
path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill', 'bmad-skill-manifest.yaml'),
|
||||
'type: skill\ncanonicalId: nested-skill\n',
|
||||
);
|
||||
await fs.writeFile(path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill', 'bmad-skill-manifest.yaml'), 'type: skill\n');
|
||||
await fs.writeFile(
|
||||
path.join(skillOnlyModDir29, 'deep', 'nested', 'my-skill', 'workflow.md'),
|
||||
'---\nname: Nested Skill\ndescription: desc\n---\nbody\n',
|
||||
|
|
|
|||
|
|
@ -135,6 +135,7 @@ class ManifestGenerator {
|
|||
];
|
||||
|
||||
return {
|
||||
skills: this.skills.length,
|
||||
workflows: this.workflows.length,
|
||||
agents: this.agents.length,
|
||||
tasks: this.tasks.length,
|
||||
|
|
@ -189,7 +190,7 @@ class ManifestGenerator {
|
|||
if (workflowFile === 'workflow.yaml') {
|
||||
workflow = yaml.parse(content);
|
||||
} else {
|
||||
const frontmatterMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---/);
|
||||
const frontmatterMatch = content.match(/^---\n([\s\S]*?)\n---/);
|
||||
if (!frontmatterMatch) {
|
||||
if (debug) console.log(`[DEBUG] collectSkills: skipped (no frontmatter): ${workflowPath}`);
|
||||
continue;
|
||||
|
|
@ -203,12 +204,18 @@ class ManifestGenerator {
|
|||
}
|
||||
|
||||
// Build path relative from module root
|
||||
const relativePath = path.relative(modulePath, dir);
|
||||
const relativePath = path.relative(modulePath, dir).split(path.sep).join('/');
|
||||
const installPath = relativePath
|
||||
? `${this.bmadFolderName}/${moduleName}/${relativePath}/${workflowFile}`
|
||||
: `${this.bmadFolderName}/${moduleName}/${workflowFile}`;
|
||||
|
||||
const canonicalId = this.getCanonicalId(manifest, workflowFile) || path.basename(dir);
|
||||
// Skills derive canonicalId from directory name — never from manifest
|
||||
if (manifest && manifest.__single && manifest.__single.canonicalId) {
|
||||
console.warn(
|
||||
`Warning: Skill manifest at ${dir}/bmad-skill-manifest.yaml contains canonicalId — this field is ignored for skills (directory name is the canonical ID)`,
|
||||
);
|
||||
}
|
||||
const canonicalId = path.basename(dir);
|
||||
|
||||
this.skills.push({
|
||||
name: workflow.name,
|
||||
|
|
@ -219,6 +226,14 @@ class ManifestGenerator {
|
|||
install_to_bmad: this.getInstallToBmad(manifest, workflowFile),
|
||||
});
|
||||
|
||||
// Add to files list
|
||||
this.files.push({
|
||||
type: 'skill',
|
||||
name: workflow.name,
|
||||
module: moduleName,
|
||||
path: installPath,
|
||||
});
|
||||
|
||||
this.skillClaimedDirs.add(dir);
|
||||
|
||||
if (debug) {
|
||||
|
|
@ -246,7 +261,9 @@ class ManifestGenerator {
|
|||
}
|
||||
if (hasSkillType && debug) {
|
||||
const hasWorkflow = workflowFilenames.some((f) => entries.some((e) => e.name === f));
|
||||
if (!hasWorkflow) {
|
||||
if (hasWorkflow) {
|
||||
console.log(`[DEBUG] collectSkills: dir has type:skill manifest but workflow file failed to parse: ${dir}`);
|
||||
} else {
|
||||
console.log(`[DEBUG] collectSkills: dir has type:skill manifest but no workflow.md/workflow.yaml: ${dir}`);
|
||||
}
|
||||
}
|
||||
|
|
@ -347,7 +364,7 @@ class ManifestGenerator {
|
|||
workflow = yaml.parse(content);
|
||||
} else {
|
||||
// Parse MD workflow with YAML frontmatter
|
||||
const frontmatterMatch = content.match(/^---\r?\n([\s\S]*?)\r?\n---/);
|
||||
const frontmatterMatch = content.match(/^---\n([\s\S]*?)\n---/);
|
||||
if (!frontmatterMatch) {
|
||||
if (debug) {
|
||||
console.log(`[DEBUG] Skipped (no frontmatter): ${fullPath}`);
|
||||
|
|
@ -462,6 +479,8 @@ class ManifestGenerator {
|
|||
* Only includes compiled .md files (not .agent.yaml source files)
|
||||
*/
|
||||
async getAgentsFromDir(dirPath, moduleName, relativePath = '') {
|
||||
// Skip directories claimed by collectSkills
|
||||
if (this.skillClaimedDirs && this.skillClaimedDirs.has(dirPath)) return [];
|
||||
const agents = [];
|
||||
const entries = await fs.readdir(dirPath, { withFileTypes: true });
|
||||
// Load skill manifest for this directory (if present)
|
||||
|
|
|
|||
Loading…
Reference in New Issue