Remove legacy workflow.xml/yaml references and complete workflow.md migration#1567
Remove legacy workflow.xml/yaml references and complete workflow.md migration#1567dickymoore wants to merge 77 commits intobmad-code-org:mainfrom
Conversation
📝 WalkthroughWalkthroughMigrates workflow/task definitions from XML/YAML to Markdown-with-frontmatter, updates workflow references and agent configs, adds many new MD workflow/task and step files, refactors CLI/installer discovery and manifest generation to parse MD frontmatter, adds tests and helpers for the new discovery rules, and removes legacy XML/YAML workflow templates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tools/cli/installers/lib/modules/manager.js (1)
1096-1102:⚠️ Potential issue | 🟠 MajorRegex doesn't match hardcoded
bmaddespite the comment claiming it does.Lines 1096–1097 state the regex handles both
_bmadand hardcodedbmad, but the regex(?:_bmad)on line 1098 only matches the literal_bmad. A path like{project-root}/bmad/bmm/workflows/...(second example in the comment) would fail to match, causing the workflow to be silently skipped.Proposed fix — make the underscore optional
- const sourceMatch = sourceWorkflowPath.match(/\{project-root\}\/(?:_bmad)\/([^/]+)\/workflows\/(.+)/); + const sourceMatch = sourceWorkflowPath.match(/\{project-root\}\/(?:_?bmad)\/([^/]+)\/workflows\/(.+)/);src/bmm/workflows/generate-project-context/steps/step-02-generate.md (1)
270-270:⚠️ Potential issue | 🟡 MinorStale XML reference:
advanced-elicitation.xmlnot updated to match the migration.Line 32 was correctly updated to reference
workflow.md, but line 270 still says "Execute advanced-elicitation.xml with current category rules." This should be updated to reflect the.mdformat for consistency.Note: This stale reference won't be caught by the new guard tests (Test 6) since they check for
advanced-elicitation/workflow.xmlspecifically, notadvanced-elicitation.xml.Proposed fix
-- Execute advanced-elicitation.xml with current category rules +- Execute advanced-elicitation workflow with current category rulestools/cli/installers/lib/ide/_base-ide.js (1)
290-299:⚠️ Potential issue | 🔴 CriticalBug:
findWorkflowYamlFilesno longer exists — core workflow discovery will crash.Line 292 still calls
this.findWorkflowYamlFiles(coreWorkflowsPath), but the method was renamed tofindWorkflowFileson Line 331. This will throwTypeError: this.findWorkflowYamlFiles is not a functionat runtime when discovering core workflows. Line 307 was correctly updated for module workflows.const coreWorkflows = await this.findWorkflowFiles(coreWorkflowsPath);tools/cli/installers/lib/ide/shared/workflow-command-generator.js (1)
221-237:⚠️ Potential issue | 🔴 CriticalBug:
transformWorkflowPathnever handles/src/core/paths and returnsundefinedfor them.The
else ifon line 228 is nested inside theif (workflowPath.includes('/src/bmm/'))block, making it unreachable for core-only paths. When a path contains/src/core/but not/src/bmm/, the outerifisfalse, the function falls through with noreturn, and returnsundefined. Thereturn transformedon line 235 is also only reachable inside the outerif.Additionally, line 228's
else ifis dead code: ifincludes('/src/bmm/')is true, the regex on line 225 will always match, so theelsebranch is never entered.🐛 Proposed fix — flatten the nesting
transformWorkflowPath(workflowPath) { let transformed = workflowPath; if (workflowPath.includes('/src/bmm/')) { const match = workflowPath.match(/\/src\/bmm\/(.+)/); if (match) { transformed = `{project-root}/${this.bmadFolderName}/bmm/${match[1]}`; } - } else if (workflowPath.includes('/src/core/')) { - const match = workflowPath.match(/\/src\/core\/(.+)/); - if (match) { - transformed = `{project-root}/${this.bmadFolderName}/core/${match[1]}`; + } else if (workflowPath.includes('/src/core/')) { + const match = workflowPath.match(/\/src\/core\/(.+)/); + if (match) { + transformed = `{project-root}/${this.bmadFolderName}/core/${match[1]}`; } - } + } - return transformed; - } + return transformed; }tools/cli/installers/lib/ide/_config-driven.js (1)
157-177:⚠️ Potential issue | 🟠 MajorWorkflow template name is doubled:
${templateType}-workflow-workflow.md.Line 163 builds
workflowTemplateType = \${templateType}-workflow`and then line 167 passes it toloadTemplate(workflowTemplateType, 'workflow', ...). InsideloadTemplate(line 196), the template file is resolved as`${templateType}-${artifactType}.md`→ e.g.claude-workflow-workflow.md`.The primary template lookup will always miss, silently falling through to the
default-workflow.mdfallback. This means per-IDE workflow template customization is broken.Either pass just
templateTypetoloadTemplate, or don't append-workflowon line 163:🐛 Proposed fix (option A — align with agent pattern)
if (artifact.type === 'workflow-command') { - // Default to 'default' template type, but allow override via config - const workflowTemplateType = config.md_workflow_template || `${templateType}-workflow`; - - // Fall back to default template if the requested one doesn't exist - const finalTemplateType = 'default-workflow'; - const template = await this.loadTemplate(workflowTemplateType, 'workflow', config, finalTemplateType); + const template = await this.loadTemplate(templateType, 'workflow', config, 'default-workflow');
🤖 Fix all issues with AI agents
In
`@src/bmm/workflows/2-plan-workflows/create-prd/steps-e/step-e-01-discovery.md`:
- Around line 7-9: The prdPurpose path references are inconsistent across
workflow step files (e.g., prdPurpose in step-e-01-discovery.md,
step-e-01b-legacy-conversion.md, step-e-02-review.md, step-e-03-edit.md,
step-v-01-discovery.md); update each file to use a single standardized path (use
the installed-context recommended path
`{project-root}/_bmad/bmm/workflows/2-plan-workflows/create-prd/data/prd-purpose.md`)
by replacing the existing prdPurpose value in those markdown front-matter blocks
so all steps reference the same prd-purpose.md location (search for the
`prdPurpose` key in those filenames and change its value accordingly).
In `@src/bmm/workflows/4-implementation/create-story/workflow.md`:
- Around line 134-189: Remove the duplicated auto-discover block that repeats
the sprint-status parsing, backlog-story discovery, epic-status validation, and
GOTO logic (the second copy starting with <action>Load the FULL file:
{{sprint_status}}</action> and ending with <action>GOTO step 2a</action>); keep
only the original guarded copy inside the <check if="no user input provided">
flow so the agent does not re-run the auto-discover steps unconditionally.
Ensure the remaining block still contains the parsing, the "Find the FIRST
story" logic, the epic-first-story check (epic-{{epic_num}} handling), and the
GOTO step 2a call.
In `@src/bmm/workflows/excalidraw-diagrams/create-dataflow/instructions.md`:
- Around line 123-125: The <invoke-task> in step n="9" (goal="Validate Content")
is missing the explicit task file path; update the <invoke-task> element in
src/bmm/workflows/excalidraw-diagrams/create-dataflow/instructions.md (the step
with n="9" and goal="Validate Content") to include the task path consistent with
sibling workflows (e.g., same pattern used in create-diagram and
create-flowchart) so it reads "Validate against {{validation}} using
{_bmad}/core/tasks/validate-workflow.md".
In `@src/bmm/workflows/testarch/trace/instructions.md`:
- Around line 930-934: Remove the stray fenced-code-marker that opens an
unintended code block near the "Enhance P2 Coverage" section: delete the lone
"```" that appears after the closed four-backtick block (the one noted around
the "Enhance P2 Coverage: Add E2E validation for session timeout" text) so the
rest of instructions.md (Validation Checklist, Notes, Troubleshooting, Related
Workflows) renders as normal Markdown; ensure the original four-backtick block
opened earlier remains unchanged and that there are no other unmatched fence
markers.
In `@test/test-installation-components.js`:
- Around line 238-274: The scan never excludes the intended file because
excludedFile is never checked; update the walk function (used in the for loop
that iterates searchRoots) to skip the exclusion before reading files by
comparing the current fullPath to excludedFile (use path.resolve or
path.normalize on both sides to avoid path-sep differences) — if they match,
continue without reading/inspecting the file; keep the existing checks for
allowedExtensions and forbiddenRef and push matches into offenders as before.
- Around line 278-314: The test currently uses content.includes(forbiddenRef)
which false-positives on names like "validate-workflow.xml"; update the check in
the walk function to perform a filename-aware match: either replace the simple
substring test with a regex that matches whole-filename occurrences (e.g.
/\bworkflow\.xml\b/.test(content)) or add an exclusion check (e.g. skip when
content.includes('validate-workflow.xml')) before pushing to offenders; refer to
the variables/function names walk, forbiddenRef, offenders, allowedExtensions
and apply the change where content.includes(forbiddenRef) is used so only true
references to "workflow.xml" are flagged.
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 778-789: The CSV column-count check is incorrect so existing
agents are never preserved: inside the loop that parses lines using split('","')
(variables lines, parts, name, module, existingEntries), change the guard from
parts.length >= 11 to parts.length >= 10 so 10-column rows are accepted; keep
the existing extraction of name = parts[0].replace(/^"/, '') and module =
parts[8] and continue to set existingEntries.set(`${module}:${name}`, line).
In `@tools/cli/installers/lib/ide/_base-ide.js`:
- Around line 347-358: The frontmatter regex in the workflow.md reader is
LF-only and will fail on CRLF files; update the regex used in the workflow.md
branch (the block that reads workflow.md in the method containing the else-if
where entry.isFile() && entry.name === 'workflow.md') to accept CRLF like the
one used in scanDirectoryWithStandalone (use a pattern with \r? before newlines)
so frontmatter is matched on both LF and CRLF line endings; ensure you use the
same normalized regex as in scanDirectoryWithStandalone to keep behavior
consistent.
In `@tools/cli/installers/lib/modules/manager.js`:
- Around line 765-772: The first replaceAll call on mdContent
(mdContent.replaceAll('_bmad', '_bmad')) is a no-op and should be removed or
corrected; update the code around mdContent in manager.js so that either (A) you
remove the redundant mdContent.replaceAll('_bmad', '_bmad') line, or (B) if you
intended to normalize a different placeholder (for example '{_bmad}' or 'bmad'),
change the search string to that placeholder (e.g.,
mdContent.replaceAll('{_bmad}', '_bmad')) before the subsequent replacement that
uses this.bmadFolderName; ensure you keep the following replacement and the call
to this.stripWebBundleFromFrontmatter(mdContent) and the final write to
targetFile.
🧹 Nitpick comments (5)
tools/cli/installers/lib/modules/manager.js (2)
1000-1044:processAgentFilesis a no-op with its body entirely commented out — dead code.The method body is fully commented out, yet it's still called at lines 551 and 608.
findAgentMdFiles(lines 1025–1044) is also only referenced inside the commented-out block, making it unreachable. Consider removing the commented-out code and the unused helper, or at minimum adding a TODO explaining why it's retained and when it will be restored.
419-419: Replace deprecated--productionflag with--omit=dev.The
--productionflag is deprecated in npm v7+. Update lines 419 and 444:Suggested change
execSync('npm install --omit=dev --no-audit --no-fund --prefer-offline --no-progress', {test/test-installation-components.js (1)
189-316: Consider extracting the duplicatedwalkfunction into a shared helper.The recursive directory walk logic is copy-pasted three times across Tests 6, 7, and 8. Extracting it into a reusable helper would reduce duplication and make it easier to apply fixes (like the missing exclusion check) consistently.
Suggested helper extraction
+/** + * Recursively scan directories for files containing a forbidden reference string. + * `@param` {string[]} searchRoots - Directories to scan + * `@param` {Set<string>} allowedExtensions - File extensions to check + * `@param` {string} forbiddenRef - String to search for in file contents + * `@param` {string} projectRoot - Project root for relative path output + * `@param` {Set<string>} [excludedFiles] - Full paths to skip + * `@returns` {Promise<string[]>} Relative paths of offending files + */ +async function findForbiddenRefs(searchRoots, allowedExtensions, forbiddenRef, projectRoot, excludedFiles = new Set()) { + const offenders = []; + const walk = async (dir) => { + const entries = await fs.readdir(dir, { withFileTypes: true }); + for (const entry of entries) { + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory()) { + await walk(fullPath); + continue; + } + if (!allowedExtensions.has(path.extname(entry.name))) continue; + if (excludedFiles.has(fullPath)) continue; + const content = await fs.readFile(fullPath, 'utf8'); + if (content.includes(forbiddenRef)) { + offenders.push(path.relative(projectRoot, fullPath)); + } + } + }; + for (const root of searchRoots) { + if (await fs.pathExists(root)) { + await walk(root); + } + } + return offenders; +}tools/cli/installers/lib/ide/shared/workflow-command-generator.js (1)
115-148: Redundant template path resolution and no-opreplaceAll.Two issues in
generateCommandContent:
Line 117:
templatePathis re-derived fromthis.templatePathbut resolves to the exact same file (workflow-commander.md). This is dead code left from when multiple templates existed.Lines 147–148: The second
.replaceAll('_bmad', '_bmad')is a no-op — it replaces_bmadwith itself. If the intent was to prevent the first replacement from touching certain occurrences, it doesn't achieve that becausereplaceAllon line 147 already replaced all occurrences.Proposed cleanup
async generateCommandContent(workflow, bmadDir) { - // Determine template based on workflow file type - const templatePath = path.join(path.dirname(this.templatePath), 'workflow-commander.md'); - // Load the appropriate template - const template = await fs.readFile(templatePath, 'utf8'); + const template = await fs.readFile(this.templatePath, 'utf8'); // ... // Replace template variables return template .replaceAll('{{name}}', workflow.name) .replaceAll('{{module}}', workflow.module) .replaceAll('{{description}}', workflow.description) .replaceAll('{{workflow_path}}', workflowPath) - .replaceAll('_bmad', this.bmadFolderName) - .replaceAll('_bmad', '_bmad'); + .replaceAll('_bmad', this.bmadFolderName); }tools/cli/installers/lib/core/manifest-generator.js (1)
316-323:cleanForCSVis redefined on every loop iteration.The helper is a pure function with no closure over per-iteration state. Defining it once outside the
forloop (or as a module-level/static helper) avoids unnecessary re-creation.
src/bmm/workflows/2-plan-workflows/create-prd/steps-e/step-e-01-discovery.md
Outdated
Show resolved
Hide resolved
src/bmm/workflows/excalidraw-diagrams/create-dataflow/instructions.md
Outdated
Show resolved
Hide resolved
bmadcode
left a comment
bmadcode
left a comment
There was a problem hiding this comment.
I would love to merge this and be done with it, but the stright conversion of the yaml to md will cause issues without also converting to step files or including even more instructions from the workflow.xml into each instruction file. best to instead finish the conversion of each file to the step flow.
Also the pr adds back deleted excalidraw content.
Would love the hlep with this though if you would like to run the bmb and help convert some of these workflows that remain.
The goal is to do this right after we finish beta on sunday.
|
@bmadcode thanks for the comment, and for the invitation to help. I'd love to help out if I can. I'll continue working through this, and will make the PR as draft until it's ready for another review. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Process update (clean):
If preferred, this can be split for easier review due size, I can break follow-on cleanup into smaller PRs and keep this PR strictly migration-critical. |
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (36)
src/bmm/workflows/3-solutioning/create-architecture/steps/step-04-decisions.md (10)
14-14:⚠️ Potential issue | 🟠 MajorPlaceholder brace style is inconsistent and likely breaks templating.
{communication_language}uses single braces while all other placeholders use{{...}}, so this value may never resolve. Standardize the placeholder format.Proposed fix
-- ✅ YOU MUST ALWAYS SPEAK OUTPUT In your Agent communication style with the config `{communication_language}` +- ✅ YOU MUST ALWAYS SPEAK OUTPUT In your Agent communication style with the config `{{communication_language}}`
57-76:⚠️ Potential issue | 🟠 MajorMismatched placeholder names will drop user preferences.
You reference
{{user_technical_preferences_from_step_3}}and later{{user_technology_preferences}}. If only one exists, the other renders empty and breaks “Already Decided.” Unify to a single variable.
29-31:⚠️ Potential issue | 🟠 Major“C (Continue)” is defined three different ways.
Line 31 says C proceeds to the next decision category, while Line 261 says it moves to implementation patterns and Lines 281–286 load the next step. This ambiguity will cause premature step transitions.
Also applies to: 258-262, 281-286
47-47:⚠️ Potential issue | 🟠 MajorYou ban recommendations, then give recommendations.
Line 47 says “Collaborative decision making, not recommendations,” but Intermediate/Beginner modes explicitly recommend options. Either allow recommendations or remove the “not recommendations” rule.
Also applies to: 138-159
18-18:⚠️ Potential issue | 🟠 Major“Show your analysis” conflicts with non-disclosure policies.
Requiring analysis disclosure forces chain-of-thought exposure, which is unsafe and typically disallowed. Replace with “brief rationale” or “summary.”
Proposed fix
-- 🎯 Show your analysis before taking any action +- 🎯 Provide a brief rationale before taking any action
5-5:⚠️ Potential issue | 🟠 Major“Never generate content without user input” contradicts “This step will generate content.”
Line 27 implies auto-generation independent of user input, directly violating Line 5. Clarify that content is generated only after the user has provided decisions.
Also applies to: 27-27
78-81:⚠️ Potential issue | 🟠 MajorNo enforcement that critical decisions are completed before Continue.
You declare critical decisions must be made before implementation, but the Continue path lacks any guard. Add a check to prevent advancing if any critical decision is still unresolved.
Also applies to: 281-286
163-170:⚠️ Potential issue | 🟡 MinorWeb search instructions assume every tech has “LTS.”
Many technologies have no LTS track; this will produce irrelevant searches and confusion. Make LTS optional or conditional on the technology.
20-20:⚠️ Potential issue | 🟠 MajorMenu cadence is inconsistent.
Line 20 says present A/P/C after each major decision category, while Lines 248–261 present it after all content generation. Pick one cadence to avoid duplicate or missing menus.
Also applies to: 248-261
21-21:⚠️ Potential issue | 🟠 MajorSaving semantics are unclear and conflicting.
Line 21 says “ONLY save when user chooses C,” but C is offered after each category, while Lines 199–201 and 281–289 imply a single final append. Specify whether saves are incremental or only at the end.
Also applies to: 199-201, 281-289
src/bmm/workflows/2-plan-workflows/create-prd/steps-e/step-e-01-discovery.md (2)
88-95:⚠️ Potential issue | 🟡 MinorShell command in a workflow step assumes Unix environment and relies on
lswith glob.Lines 92-95 embed a bash command (
ls -t ... 2>/dev/null | head -1) for auto-detecting validation reports. This is executed conceptually by an AI agent, not a real shell — but the instruction to usels -twith glob patterns and pipe toheadpresumes a Unix filesystem. If the agent interprets this literally in a non-Unix context (e.g., Windows, or an API-only environment), it will fail silently. The2>/dev/nullsuppression means the agent gets no feedback on failure.This is pre-existing and tangential to the migration, but it's a fragility worth noting.
110-143:⚠️ Potential issue | 🟡 MinorSteps 4 and 5 are redundant — both ask about validation reports.
Step 4 (lines 88-124) auto-detects a validation report in the PRD folder and asks the user whether to use it. Step 5 (lines 126-143) then asks again "Do you have a validation report to guide edits?" — even if the user already said yes or no in step 4. If the user chose "Use validation report" in step 4, step 5 re-asks the same question. If no report was found in step 4 and the flow falls through silently (line 124), step 5 asks again.
This creates a confusing double-prompt experience. Step 5 only makes sense if step 4 found nothing, but the control flow doesn't skip step 5 in the "found and accepted" case.
src/bmm/workflows/2-plan-workflows/create-ux-design/steps/step-11-component-strategy.md (1)
196-200:⚠️ Potential issue | 🟠 MajorNo validation of "enhanced component insights" returned from advanced elicitation.
Lines 196-200 describe the flow: invoke advanced-elicitation workflow.md, "process the enhanced component insights that come back," then ask the user to accept or reject. But there's no specification for:
- What format should "enhanced component insights" take?
- How does the agent know the returned content is actually "enhanced insights" vs. an error message or malformed output?
- What if advanced-elicitation returns empty content or
null?- Is there a schema or structure that the returned insights must conform to?
Without validation, the agent might present garbage data to the user as "enhanced insights," leading them to accept changes that break the component strategy. This is especially risky in step 11, where component decisions affect downstream implementation.
Add validation logic for returned insights
Before presenting the insights to the user, validate the structure:
#### If 'A' (Advanced Elicitation): - Read fully and follow: {project-root}/_bmad/core/workflows/advanced-elicitation/workflow.md with the current component strategy content - Process the enhanced component insights that come back - **Validate insights structure**: Confirm returned content includes component specifications, rationale, and accessibility considerations - **If validation fails**: Inform user that advanced elicitation didn't produce valid insights and return to A/P/C menu without applying changes - Ask user: "Accept these improvements to the component strategy? (y/n)"src/bmm/workflows/3-solutioning/create-architecture/steps/step-06-structure.md (1)
328-332:⚠️ Potential issue | 🟠 MajorArchitecture decisions require deeper validation than generic workflow outputs.
Lines 328-332 invoke advanced-elicitation and process "enhanced organizational insights" for project structure, then ask the user to accept or reject changes. But project structure decisions are foundational—they affect every subsequent implementation story, file organization, and deployment strategy.
Unlike UX insights (which can be adjusted later), bad architecture insights create technical debt that compounds over time. The validation here needs to be stricter:
- Verify that returned insights include complete directory structure (not partial)
- Check that boundary definitions (API, component, service, data) are all addressed
- Confirm that requirements-to-structure mappings are present
- Validate that integration points are defined
Currently, there's no validation beyond "did advanced-elicitation return something?" This is insufficient for architecture decisions.
tools/cli/installers/lib/modules/manager.js (3)
604-609:⚠️ Potential issue | 🔴 Critical
optionsis not defined inupdate()— will throwReferenceErrorat runtime.Line 607 references
options.installer, but the method signature on line 584 isasync update(moduleName, bmadDir, force = false)— there is nooptionsparameter. This will crash whenever a non-force update triggers theelsebranch.Proposed fix
- async update(moduleName, bmadDir, force = false) { + async update(moduleName, bmadDir, force = false, options = {}) {
999-1017:⚠️ Potential issue | 🟠 Major
processAgentFilesis called but entirely commented out — activation injection silently skipped.Line 551 calls
await this.processAgentFiles(targetPath, moduleName), but the method body (lines 999–1017) is entirely commented out, making it a no-op. If this is intentional (activation is handled elsewhere now), remove both the call and the dead code. If it's a regression, restore the logic.
1095-1109:⚠️ Potential issue | 🟡 MinorVendoring regex only matches
bmador_bmad— won't work with custom folder names.The regex
(?:_?bmad)on lines 1097 and 1109 matches exactlybmador_bmad. If the user configured a differentbmadFolderName(e.g.,.bmad-custom), these patterns silently fail to match, and vendored workflows are skipped with a warning. Consider building the regex dynamically fromthis.bmadFolderName:const folderPattern = this.bmadFolderName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const sourceMatch = sourceWorkflowPath.match( new RegExp(`\\{project-root\\}/${folderPattern}/([^/]+)/workflows/(.+)`) );src/bmm/workflows/2-plan-workflows/create-prd/steps-v/step-v-01-discovery.md (1)
1-10:⚠️ Potential issue | 🟠 MajorAdd
validationReportPathto frontmatter; it's referenced in the body but undefined.Lines 133 and 174 use
{validationReportPath}, yet the frontmatter (lines 1–10) omits this variable. All subsequent validation steps (step-v-02 through step-v-13) define it; step-v-01 must too. Without it, the agent cannot initialize the validation report. Add to frontmatter:validationReportPath: '{validation_report_path}'src/bmm/workflows/2-plan-workflows/create-prd/steps-c/step-06-innovation.md (1)
19-19:⚠️ Potential issue | 🟡 MinorStep total count mismatch with sibling step file.
Line 19 states "Step 6 of 11", but
step-05-domain.md(in the samesteps-cdirectory) states "Step 5 of 13" at its Line 17, and its menu text references "Step 6 of 13" when pointing to this file. One of these totals is wrong. Pre-existing issue, but this migration pass is a good opportunity to reconcile.src/bmm/workflows/2-plan-workflows/create-prd/steps-c/step-09-functional.md (1)
1-12:⚠️ Potential issue | 🟠 MajorSecurity concern: Markdown files can contain executable code.
The migration from XML/YAML to Markdown introduces a security risk. Markdown files can contain code blocks, HTML, and potentially JavaScript (if rendered in a web context). If agents parse and execute these MD files without sanitization, malicious workflow.md files could inject executable code. The PR provides no discussion of security boundaries or sanitization strategy.
Establish security boundaries for workflow.md parsing:
- Whitelist allowed Markdown features: Restrict to headers, lists, code blocks, emphasis — no raw HTML or JavaScript
- Sanitize frontmatter: Validate YAML frontmatter doesn't contain executable directives (e.g.,
!!python/objecttags)- Path traversal protection: Ensure path interpolation can't escape repository root (
{project-root}/../../../etc/passwd)- Content Security Policy: If workflows are rendered in web UI, enforce strict CSP
Document the security model for workflow.md files and implement parser-level sanitization.
src/bmm/workflows/2-plan-workflows/create-prd/steps-c/step-04-journeys.md (1)
158-161:⚠️ Potential issue | 🟠 MajorMenu loop has no escape hatch—infinite loop risk.
The menu handling logic (lines 158-161) can redisplay the menu after executing advanced elicitation or party mode, but there's no maximum attempt counter, timeout, or explicit exit condition beyond selecting 'C'. If a workflow returns invalid state or the user gets stuck in refinement loops, there's no safety valve.
src/bmm/workflows/document-project/instructions.md (8)
13-16:⚠️ Potential issue | 🟠 MajorMissing guard when workflow-status is unavailable.
You invoke
{project-root}/_bmad/bmm/workflows/workflow-statuswithout a fallback if that workflow is missing or fails. The rest of Step 1 assumes status variables exist and will break silently. Add an explicit failure path (e.g., setstandalone_mode=true,status_file_found=false, and continue) when the invoke fails.As per coding guidelines: "Review with extreme skepticism — assume problems exist. Find at least 10 issues to fix or improve."
18-23:⚠️ Potential issue | 🟠 MajorAmbiguous condition uses implicit boolean.
status_exists == falseis referenced without defining howstatus_existsis computed or whether it can be null/undefined. If the status workflow returns nothing, this check may not fire, leading to undefined flow. Add an explicit default assignment before the checks.As per coding guidelines: "Be skeptical of everything. Look for what's missing, not just what's wrong."
30-36:⚠️ Potential issue | 🟠 MajorGreenfield prompt exits without cleaning up state.
If the user answers “n” to continue, you exit without updating or clearing any status markers. That can leave a workflow-status file in an inconsistent “in-progress” state. Add a status update (e.g., mark aborted) before exit when status tracking is enabled.
As per coding guidelines: "Look for what's missing, not just what's wrong."
39-52:⚠️ Potential issue | 🟡 MinorWarning handling is destructive without preserving reason.
When
warning != ''and the user chooses "n", you exit without storing the warning or suggestion in state. That loses valuable context for audit/troubleshooting. Persistwarning/suggestionto the status file before exit.As per coding guidelines: "Be skeptical of everything."
60-66:⚠️ Potential issue | 🟠 Major“Check for state file” doesn’t specify existence failure path.
The step only describes what happens if the state file exists. It never explicitly defines the else path (file missing), yet later steps rely on
resume_mode. Defineresume_mode = falsewhen the file doesn’t exist to avoid undefined branching.As per coding guidelines: "Look for what's missing, not just what's wrong."
121-126:⚠️ Potential issue | 🟠 MajorConflicting resume logic can double-trigger.
The “state file age >= 24 hours” check runs after user selection blocks; if the user already chose resume (1), this later check can still force a fresh scan, contradicting user input. Gate the age check before the user prompt or short-circuit if a selection was made.
As per coding guidelines: "Be skeptical of everything."
148-161:⚠️ Potential issue | 🟠 MajorNo validation that sub-workflow actually completes.
Steps assume the sub-workflow completes and then continue to Step 4. If the sub-workflow fails or is aborted, Step 4 still runs and may incorrectly mark status complete. Add an explicit success check (e.g.,
subworkflow_success == true) before continuing.As per coding guidelines: "Look for what's missing, not just what's wrong."
192-217:⚠️ Potential issue | 🟠 MajorStatus output block has mismatched templating scope.
You open
{{#ifstatus_file_found}}and close with{{/if}}inside<output>but the indentation and placement suggest mixed templating/agent markers; there’s no explicit guarantee the engine supports this inside<output>. This risks rendering the template literally. Confirm supported template scope or move conditional logic outside<output>.Based on learnings: In BMAD-METHOD workflow files under src/bmm/workflows/, XML-style tags like , , etc. are processing markers for AI agents executing workflows and should not be treated as HTML/Markdown for GitHub rendering. These tags may contain Markdown that will be output by the agent, not rendered by GitHub's sanitizer. When reviewing, ensure these markers are preserved and that any Markdown inside them is allowed for agent output; do not modify or strip content inside these processing tags to maintain correct agent behavior.
src/bmm/workflows/2-plan-workflows/create-prd/steps-c/step-03-success.md (2)
175-181:⚠️ Potential issue | 🟠 MajorMenu flow uses advanced/party workflows without specifying return contract.
The A/P paths call other workflows and then “redisplay menu” after asking for acceptance. There’s no explicit requirement that those workflows return enhanced criteria in a known variable. Define the expected output variable names so content replacement is deterministic.
As per coding guidelines: "Look for what's missing, not just what's wrong."
178-180:⚠️ Potential issue | 🟠 MajorPotential infinite loop when user rejects changes.
If the user answers “no” to accepting improvements, you “keep original content then redisplay menu,” but nothing changes; the user can get stuck in a loop with the same content. Provide a path to exit or refine without re-running external workflows.
As per coding guidelines: "Be skeptical of everything."
src/bmm/workflows/3-solutioning/create-epics-and-stories/steps/step-01-validate-prerequisites.md (4)
6-21:⚠️ Potential issue | 🟠 MajorDuplicate
epicsTemplatekey in frontmatter.
epicsTemplateis declared twice (Lines 13 and 20). That is ambiguous in YAML frontmatter and may clobber earlier values. Remove one and keep a single authoritative definition.As per coding guidelines: "Be skeptical of everything."
77-94:⚠️ Potential issue | 🟠 MajorDocument search patterns rely on globbing with no mention of precedence resolution.
You list multiple patterns but don’t specify how to choose when multiple matches exist. This can result in inconsistent document selection. Add deterministic selection rules (e.g., prefer most recent, exact match, or prompt user).
As per coding guidelines: "Look for what's missing, not just what's wrong."
98-100:⚠️ Potential issue | 🟡 MinorInstruction typo causes mis-execution.
“read then entire document” is ambiguous and could be interpreted incorrectly. It should be “read the entire document.” This is a functional instruction to the agent, not a style nit.
As per coding guidelines: "Be skeptical of everything."
232-234:⚠️ Potential issue | 🟡 MinorMenu handling references an anchor that may not exist.
“Redisplay Menu Options” assumes a GitHub-generated anchor, but this file’s headers include “### 10. Present MENU OPTIONS” and may produce a different anchor. If the anchor is wrong, navigation breaks. Ensure the anchor matches GitHub rules.
Based on learnings: When documenting cross-reference patterns in Markdown, ensure header anchors follow GitHub's anchor generation rules: lowercase, spaces to hyphens, remove emojis/special characters, and strip leading/trailing hyphens. Example: '### 🚨 R-001: Title' becomes '#r-001-title'. Use anchors that reflect these rules in documentation across Markdown files.
test/test-installation-components.js (1)
186-198:⚠️ Potential issue | 🟡 MinorTest 3 is a permanent placeholder that always passes — this silently inflates the pass count.
Line 192 asserts
trueunconditionally. This test has never validated anything. If path resolution testing is planned, track it as a TODO. If not, remove it so the pass count reflects reality.
🤖 Fix all issues with AI agents
In
`@src/bmm/workflows/2-plan-workflows/create-prd/steps-v/step-v-01-discovery.md`:
- Line 9: The prdPurpose path points to a non-existent _bmad location; update
the prdPurpose entry in step-v-01-discovery.md to reference the actual file
location src/bmm/workflows/2-plan-workflows/create-prd/data/prd-purpose.md (or a
correct relative path from this markdown file) so the link resolves to the
existing prd-purpose.md; update the prdPurpose string value accordingly.
In
`@src/bmm/workflows/2-plan-workflows/create-ux-design/steps/step-04-emotional-response.md`:
- Line 33: The markdown references use a non-existent path pattern
"{project-root}/_bmad/core/workflows/..."; update this step
(step-04-emotional-response.md) and all other workflow files using the same
pattern to point to the actual repo location "src/core/workflows/..." or add a
repository-level README explaining how "{project-root}" is resolved at runtime.
Specifically, in the current file replace
"{project-root}/_bmad/core/workflows/advanced-elicitation/workflow.md" with
"src/core/workflows/advanced-elicitation/workflow.md" and run a repo-wide search
for the "{project-root}/_bmad/core/" pattern to either correct paths to
"src/core/" or document the resolution mechanism.
In
`@src/bmm/workflows/4-implementation/code-review/steps/step-03-execute-review.md`:
- Around line 7-57: The "Execute adversarial review" step (step n="3") lacks a
defined findings output structure, so define and document a clear findings
schema (fields: id, severity, type, summary, detail, file:line, proof,
suggested_fix, reviewer, timestamp) and a storage/transfer mechanism (e.g.,
write JSON array to a findings file like review-findings.json or emit to a
shared in-memory artifact) and update the step-03-execute-review.md text to
state: how findings are created (map actions that produce findings to populate
those schema fields), where they are persisted, and how
step-04-present-and-resolve.md should consume them (path/variable and expected
schema), plus include an example record to make the contract explicit.
- Around line 21-26: The AC validation step "Determine: IMPLEMENTED, PARTIAL, or
MISSING" in step-03-execute-review.md is underspecified; add a clear decision
algorithm that defines (a) precise criteria for IMPLEMENTED (e.g., direct code
evidence + tests or documentation references covering all AC clauses), PARTIAL
(some code or tests cover subset of clauses or only non-functional aspects), and
MISSING (no code/tests/docs addressing any clause), (b) rules for evidence types
(comments/README count as weak evidence and require corroborating code or tests
to be IMPLEMENTED), (c) guidance for indirect evidence (e.g., helper functions
or generic utilities are PARTIAL unless explicitly wired or covered by
integration tests), and (d) a severity mapping that ties PARTIAL/MISSING to
HIGH/MEDIUM/LOW severities; reference the phrase "Determine: IMPLEMENTED,
PARTIAL, or MISSING" and the action block so reviewers update that section with
these explicit thresholds and examples.
In `@src/bmm/workflows/4-implementation/correct-course/workflow.md`:
- Around line 1-6: The workflow references installed_path and expects
instructions.md and checklist.md under
{project-root}/_bmad/bmm/workflows/4-implementation/correct-course
(installed_path), but the files currently live at
src/bmm/workflows/4-implementation/correct-course; fix by either moving
instructions.md and checklist.md into the directory matching installed_path
({project-root}/_bmad/bmm/workflows/4-implementation/correct-course) or update
the installed_path value in the workflow metadata to point to
src/bmm/workflows/4-implementation/correct-course so the references to
{installed_path}/instructions.md and {installed_path}/checklist.md resolve
correctly.
In `@src/bmm/workflows/4-implementation/create-story/instructions.xml`:
- Line 313: The invoke-task line references a non-existent path
"_bmad/core/tasks/validate-workflow.md"; update the invoke-task element that
currently contains "Validate against checklist at {installed_path}/checklist.md
using _bmad/core/tasks/validate-workflow.md" to point to the correct file
"src/core/tasks/validate-workflow.md" so the task resolver can find the
validate-workflow.md resource.
In `@src/bmm/workflows/4-implementation/dev-story/steps/step-02-load-context.md`:
- Around line 11-14: The parsing step (step-02-load-context.md) currently lists
actions like "Parse sections" and "Load comprehensive context" but lacks
validation; add explicit checks when parsing each named section (Story,
Acceptance Criteria, Tasks/Subtasks, Dev Notes, Dev Agent Record, File List,
Change Log, Status) to detect missing files, missing sections, empty content, or
malformed structure, and handle them by (1) returning clear validation errors or
logging and halting the workflow when critical (e.g., missing story file or Dev
Notes), (2) providing safe defaults or warnings for non-critical empty sections
(e.g., Change Log), and (3) validating structure (expected keys/format) before
extracting developer guidance; ensure these checks are performed before the
"Load comprehensive context" and that failures propagate as explicit errors
rather than failing silently.
In
`@src/bmm/workflows/4-implementation/dev-story/steps/step-08-mark-task-complete.md`:
- Around line 43-46: Initialize the missing variables before the conditional:
set review_continuation by checking the original task list for the "[AI-Review]"
prefix (e.g., scan original_task_list or task_title and produce a boolean
assigned to review_continuation), compute resolved_count as the length/size of
resolved_review_items (resolved_count = len(resolved_review_items) or
equivalent), and set date from the system-generated timestamp (call the
workflow/system date function to produce a formatted date string and assign it
to date); then use these variables in the existing check and Change Log action
so the conditional and changelog entry succeed.
- Around line 17-29: Initialize the resolved_review_items collection before use
(e.g., set resolved_review_items = [] at workflow start or at the beginning of
this step) and then, when marking review follow-ups, append the resolved item to
that list; for matching the task description to an action item in "Senior
Developer Review (AI) → Action Items", implement a fuzzy-match routine:
normalize both strings (lowercase, trim, remove prefixes like "[AI-Review]"),
try exact substring match first, then compute token-set overlap or a simple
fuzzy score (e.g., Jaccard/token intersection) and pick the highest-scoring
action item if score >= threshold (e.g., 0.6); if multiple candidates tie pick
the best score or log ambiguity, and if no match meets the threshold, log a
warning, add the task to an unresolved_review_items list and still mark the task
checkbox in "Tasks/Subtasks → Review Follow-ups (AI)" and record the resolution
note in Dev Agent Record noting that no corresponding action item was found.
In
`@src/bmm/workflows/4-implementation/dev-story/steps/step-09-mark-review-ready.md`:
- Around line 30-36: The conditional uses an uninitialized variable
{{current_sprint_status}}; initialize it or change the check so it doesn't rely
on that undefined value. Specifically, before the existing check, read the
sprint tracking setting (e.g., set current_sprint_status from the project config
or from the contents of {sprint_status} if present) so {{current_sprint_status}}
is defined, or replace the condition with a direct existence/validity test on
{sprint_status} (e.g., "if {sprint_status} file exists AND its content !=
'no-sprint-tracking'"). Ensure the code that updates
development_status[{{story_key}}] = "review" only runs after the validated
initialization of current_sprint_status or after the direct file validity check.
In `@src/bmm/workflows/4-implementation/dev-story/workflow.md`:
- Line 4: The config references an undefined placeholder {project-root} (e.g.,
in main_config) which will break loading; update the workflow to either (a)
replace {project-root} with a defined variable name and add its initialization
in the workflow config/vars (e.g., projectRoot) or (b) resolve it at startup by
computing the repository root (e.g., using process.cwd() or a locate-repo-root
helper) and interpolating that value into main_config and the other occurrence
before loading the YAML; ensure the chosen symbol (projectRoot or the resolver
function) is declared and used consistently where {project-root} appears.
- Line 43: Define the HALT condition explicitly in the workflow: add a concise
operational definition of "HALT" (what events/criteria trigger it), the exact
behavior when HALT occurs (e.g., stop execution, persist state, notify user,
rollback), and the resume semantics (manual resume, automatic retry, or
checkpoint-based continuation). Update all references (including the "HALT"
mention in the workflow and step-08) to point to that definition and specify any
user prompts or log messages that must be emitted when HALT is reached so the
workflow engine can implement consistent handling.
In `@src/bmm/workflows/document-project/workflows/deep-dive.yaml`:
- Line 10: Fix the typo in the config_source path: update the config_source
value in deep-dive.yaml (key: config_source) from
"{project-root}/_bmad/bmb/config.yaml" to
"{project-root}/_bmad/bmm/config.yaml"; also make the identical change in
full-scan.yaml so both workflows reference the existing _bmad/bmm directory and
can load config.yaml at runtime.
In `@src/core/tasks/validate-workflow.md`:
- Around line 35-37: The "Edits (if applicable)" / Step 5 flow must add a
safe-edit protocol: before applying changes (the part that currently says "ask
for confirmation before editing" and "Only apply changes after user approval"),
create a backup/snapshot and store a reversible diff (or create a temporary
branch/working copy), produce a diff preview for the user, run validation/syntax
checks on the edited copy, and implement an undo/rollback path that restores the
backup if validation fails or the user cancels; update the workflow text (the
"Edits (if applicable)" section / Step 5) to describe these steps and the
locations where backups/diffs/rollbacks are created and verified.
- Around line 10-12: The current docs allow silent failures when loading
`{project-root}/_bmad/core/config.yaml` and treating `communication_language`,
`user_name`, and `document_output_language` as optional; update the
config-loading logic (e.g., the loadConfig / resolveVariables flow) to validate
that the file exists and parses correctly, and enforce required fields or apply
explicit defaults: if parsing fails or required keys are missing, surface a
clear error via logger and abort initialization (or throw), and where defaults
are acceptable set and document them (e.g., default communication_language="en",
document_output_language="en", and require user_name or fail); ensure the
validation is clearly referenced in the README/docs and that the validation
function name (e.g., validateConfig or ensureRequiredConfig) performs these
checks.
- Around line 23-24: Replace the vague "Infer the target file from the checklist
context or workflow inputs" with concrete inference rules: look for explicit
keys in checklist entries and workflow inputs (e.g., "file", "path", "target",
"filePath"), accept single paths or glob patterns, normalize/resolve relative
paths, check repository for existence and that the file type matches expected
(e.g., .yaml/.yml/.json), and parse ambiguous entries by preferring explicit
file fields, then path-like tokens, then first matching file from globs; if no
valid candidate found, prompt the user with a clear schema example (e.g.,
"Please provide the exact file path (relative to repo root), e.g.,
./workflows/ci.yml") and validate the user's reply by normalizing and verifying
existence before proceeding.
In `@src/utility/agent-components/handler-validate-workflow.txt`:
- Line 3: The code currently mandates loading
{project-root}/_bmad/core/tasks/validate-workflow.md with no checks; add robust
error handling around the load so missing/inaccessible/malformed files are
detected and handled gracefully: before attempting to read, verify existence and
read permissions for _bmad/core/tasks/validate-workflow.md, wrap the read/parse
operation in a try/catch, log a clear error via the existing logger (include the
path and caught error), and implement a fallback path—either fail fast with a
descriptive exception, return a default/embedded validation workflow, or mark
the agent as degraded—so callers of the loader get a deterministic result;
update the loader routine that performs the file read (the code that issues the
"MUST LOAD" instruction) to include these checks and fallbacks.
In `@src/utility/agent-components/handler-workflow.txt`:
- Line 4: The "CRITICAL: Always LOAD
{project-root}/_bmad/core/tasks/workflow.md" directive lacks fallback/error
handling; update the loader that enforces this (the code that resolves and reads
workflow.md) to first check file existence and readability for
{project-root}/_bmad/core/tasks/workflow.md, attempt a safe path resolution, and
if missing or unreadable either (a) load a bundled default workflow.json/md
fallback or (b) emit a clear error via the agent logger and fail fast with a
descriptive message; ensure the change updates the same component that
references the "CRITICAL: Always LOAD" directive so it logs the missing-file
path and underlying error before using the fallback or aborting.
In `@tools/cli/installers/lib/ide/shared/workflow-command-generator.js`:
- Line 11: Update the constructor in workflow-command-generator.js to use the
shared constant: import BMAD_FOLDER_NAME from './path-utils' and change the
constructor signature's default from the hardcoded 'bmad' to BMAD_FOLDER_NAME;
adjust any references to the parameter if necessary but keep the parameter name
(bmadFolderName) the same so behavior is unchanged while ensuring it defaults to
the canonical BMAD_FOLDER_NAME constant.
src/bmm/workflows/2-plan-workflows/create-prd/steps-v/step-v-01-discovery.md
Outdated
Show resolved
Hide resolved
src/bmm/workflows/2-plan-workflows/create-ux-design/steps/step-04-emotional-response.md
Show resolved
Hide resolved
src/bmm/workflows/4-implementation/code-review/steps/step-03-execute-review.md
Show resolved
Hide resolved
src/bmm/workflows/4-implementation/code-review/steps/step-03-execute-review.md
Show resolved
Hide resolved
tools/cli/installers/lib/ide/shared/workflow-command-generator.js
Outdated
Show resolved
Hide resolved
|
I hope this is helpful. I tried to progress this work as much as I could. There are lots of changes over many commits in response to coderabbitai and local adversarial reviews. What I fixed
Tests/validation
PR size
Thanks, Dicky |
|
@coderabbitai review |
|
This pull request is too large for Augment to review. The PR exceeds the maximum size limit of 100000 tokens (approximately 400000 characters) for automated code review. Please consider breaking this PR into smaller, more focused changes. |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/bmm/workflows/document-project/instructions.md (4)
34-36:⚠️ Potential issue | 🟡 MinorAvoid outputting empty
{{suggestion}}.When
status_exists == false,suggestionis still defaulted to empty, so the first output can be blank. Guard it or remove it to prevent confusing empty output.Suggested fix
- <output>{{suggestion}}</output> + <check if="suggestion != ''"> + <output>{{suggestion}}</output> + </check>
117-123:⚠️ Potential issue | 🟠 MajorResume path assumes cached project types always exist.
If
cached_project_typesis empty/missing, you skip loading project-types/architecture registries, but then try to load doc requirements per cached ID — which will be empty and break downstream steps. Add a fallback to full CSV loading when cached IDs are absent.Suggested fix
<action>Load cached project_type_id(s) from state file</action> + <check if="cached_project_types == ''"> + <output>No cached project types found. Falling back to full CSV load.</output> + <action>Load project-types.csv and architecture_registry.csv</action> + </check>
154-159:⚠️ Potential issue | 🟡 Minor“Start fresh” doesn’t reset state-derived fields.
After moving the old state file, you only set
resume_mode = false. Any previously loaded fields (mode, scan_level, cached_project_types, etc.) can leak into Step 4 outputs. Reset those to defaults when starting fresh.Suggested fix
<action>Move old state file to: {output_folder}/.archive/project-scan-report-{{timestamp}}.json</action> <action>Set resume_mode = false</action> + <action>Reset workflow_mode, scan_level, cached_project_types, current_step to defaults</action> <action>Continue to Step 3</action>
199-203:⚠️ Potential issue | 🟡 Minor
scan_levelis never set for full rescan.Step 4 outputs
scan_level, but in the full rescan branch it remains empty/undefined. Set an explicitscan_levelhere.Suggested fix
<action>Set workflow_mode = "full_rescan"</action> + <action>Set scan_level = "standard"</action> <action>Display: "Starting full project rescan..."</action>tools/cli/installers/lib/ide/shared/workflow-command-generator.js (2)
134-145:⚠️ Potential issue | 🟠 MajorPath transformation only handles
bmmandcoremodules — other modules (bmb,tea,cis,gds) will retain raw source paths.The path extraction logic on lines 134–145 only matches
/src/bmm/and/src/core/. According to docs (line 123 ofcommands.md), the system supports modules likebmb,tea,cis, andgds. Workflows from those modules would pass through with untransformed absolute paths, producing broken references in the generated command files.A generic extraction would be more robust:
Proposed fix
- if (workflowPath.includes('/src/bmm/')) { - // bmm is directly under src/ - const match = workflowPath.match(/\/src\/bmm\/(.+)/); - if (match) { - workflowPath = `${this.bmadFolderName}/bmm/${match[1]}`; - } - } else if (workflowPath.includes('/src/core/')) { - const match = workflowPath.match(/\/src\/core\/(.+)/); - if (match) { - workflowPath = `${this.bmadFolderName}/core/${match[1]}`; - } + // Generic extraction: /src/<module>/... → <bmadFolderName>/<module>/... + const srcMatch = workflowPath.match(/\/src\/([^/]+)\/(.+)/); + if (srcMatch) { + workflowPath = `${this.bmadFolderName}/${srcMatch[1]}/${srcMatch[2]}`; }The same pattern exists in
transformWorkflowPath(lines 235–245) andcollectWorkflowArtifacts(lines 80–85) and should be updated consistently.
74-85:⚠️ Potential issue | 🟡 Minor
_bmad/split-and-rejoin silently corrupts paths if_bmad/appears more than once.
parts.slice(1).join('/')uses/as the joiner instead of the original_bmad/separator. If the path contains multiple_bmad/segments (e.g., nested install), segments are joined with a bare/, silently mangling the path. This is an edge case but the fix is trivial — take only the last segment:Proposed fix
if (parts.length > 1) { - workflowRelPath = parts.slice(1).join('/'); + workflowRelPath = parts[parts.length - 1]; }
🤖 Fix all issues with AI agents
In `@docs/tutorials/getting-started.md`:
- Around line 81-83: Update the Phase 1 workflow list so each workflow name is
paired with its full invocation command (e.g., show the pattern like
**brainstorming** (bmad-bmm-brainstorming) — Guided ideation); specifically add
full command forms for `brainstorming`, `research`, and `create-product-brief`
(or at minimum show one full example and clarify the naming pattern
`bmad-bmm-<workflow>`), ensuring the displayed syntax matches how users actually
invoke those workflows.
- Around line 165-175: Update the Quick Reference table so the bare workflow
names (e.g., "help", "prd", "create-architecture", "create-epics-and-stories",
"check-implementation-readiness", "sprint-planning", "create-story",
"dev-story", "code-review") are mapped to their actual slash command names from
commands.md (e.g., "bmad-help", "bmad-bmm-create-prd",
"bmad-bmm-create-architecture", etc.): either add a third column showing the
real slash command for each workflow or add a clear callout above the table
stating "Prefix with bmad- (or bmad-bmm- for module workflows) when using slash
commands", and ensure the examples for "help" include the existing parenthetical
"(bmad-help on most platforms)" for consistency.
In `@src/bmm/workflows/4-implementation/create-story/workflow.md`:
- Line 62: The GOTO targets "<action>GOTO step 2a</action>" do not match any
defined step; update either the jump targets or the step label so they match:
locate the three occurrences of the GOTO action (the lines containing
"<action>GOTO step 2a</action>") and either change them to "<action>GOTO step
2</action>" or rename the step declaration "<step n=\"2\" ...>" to "<step
n=\"2a\" ...>" so the GOTO references resolve correctly; ensure all three GOTOs
and the single step declaration use the identical label ("2" or "2a") so the
workflow is unambiguous.
- Around line 176-180: Define and set previous_story_num before using it in the
file-loading action: inside the existing conditional that checks "story_num >
1", add an explicit assignment computing previous_story_num = story_num - 1
(ensuring it's an integer) and then use {{previous_story_num}} in the Load
previous story file path; also ensure the branch skips the load when story_num
<= 1 to avoid undefined references.
In `@src/bmm/workflows/document-project/instructions.md`:
- Around line 235-239: When handling the branch where index.md does not exist
(the initial_scan path), set scan_level (e.g., scan_level = "initial") before
delegating to the full-scan workflow and before checking/setting
subworkflow_success; specifically add scan_level = "initial" alongside the
existing assignments of workflow_mode and before the action that reads
{installed_path}/workflows/full-scan-instructions.md so downstream logic that
expects scan_level won't be undefined.
- Around line 23-32: The current load of
`{output_folder}/bmm-workflow-status.yaml` silently falls back to standalone
mode on missing/unreadable/malformed files; update the load logic that sets
status_exists, status_file_found, standalone_mode, and status_file_path to catch
file-read and parse exceptions and emit a clear warning containing the failure
reason (e.g., file not found, permission error, YAML parse error) before
continuing; ensure the warning references the attempted path
(`{output_folder}/bmm-workflow-status.yaml`) and include any parser error
message so downstream uses of field_type, warning, suggestion, next_workflow,
and next_agent are traceable.
- Around line 281-287: The template block that renders the "Next Steps" uses
{{next_workflow}} and {{next_agent}} directly and can display empty placeholders
if those fields are missing; update the conditional rendering inside the check
(where status_file_found == true AND status_update_success == true) to fall back
to safe text like "unknown" or "not specified" (or hide the line) when
next_workflow or next_agent are empty/undefined, e.g., add conditional checks or
default values for next_workflow and next_agent in the same template so the
output never shows blank placeholders.
- Around line 84-90: The archive step assumes creating {output_folder}/.archive/
and moving project-scan-report-{{timestamp}}.json always succeed; add explicit
error handling around the "Create archive directory: {output_folder}/.archive/"
and "Archive old state file to:
{output_folder}/.archive/project-scan-report-{{timestamp}}.json" actions so
failures (mkdir or move) are detected, logged, and handled: if directory
creation or file move fails, log a clear error, do not discard the original
state file, set resume_mode = true (or abort the workflow) and surface the
failure to the user instead of continuing to Step 3. Ensure the implementation
checks return values/exceptions from the filesystem calls and uses the existing
variables resume_mode and {output_folder} to implement the fallback/abort
behavior.
- Around line 113-118: The workflow assigns workflow_mode directly from the
persisted state variable mode (when setting resume_mode=true and
subworkflow_success=false) without validation; update the loading logic that
sets workflow_mode so it validates mode against the canonical list of allowed
modes (e.g., ["create","update","review"] or the project's defined enum) and
only assigns it if it matches, otherwise set a safe default (e.g., "create") or
raise a clear error/flag to abort resumption; ensure the validation happens
right after "Load findings summaries from state file"/"Load cached
project_type_id(s) from state file" and refer to the workflow_mode and mode
variables to locate the change.
- Around line 79-83: The state-age computation currently trusts the state file's
last_updated value; add a parse-and-validate step when reading
project-scan-report.json: verify last_updated exists and is a valid ISO/epoch
timestamp, and if missing/invalid treat it as stale by setting age > 24 hours
(or a sentinel like null/Infinity) so the existing "Calculate age of state file"
/ "24‑hour logic" branch treats it as expired; update the extraction routine
that reads last_updated (the place implementing "Read state file and extract:
timestamps, ... last_updated") to perform this validation and fallback rather
than using the raw value.
In `@src/core/tasks/validate-workflow.md`:
- Around line 49-57: The Safe-edit protocol under "Edits (if applicable)" must
specify a deterministic backup location and cleanup behavior: update that
section to define a default backup strategy (e.g., configurable temp directory
with timestamped filenames and an optional adjacent .bak fallback) and require
recording the full backup path in the task output, and update "Finalize" (step
6) to state that backups/diff artifacts are deleted on successful apply (or
retained if user requests retention) with an explicit retention flag and a
documented rollback path; reference the "Create backup snapshot", "Record
backup/diff locations in task output", and the "Finalize" step so the markdown
clearly prescribes where backups go, how they are named, and when they are
cleaned up.
- Line 34: Update the validation sentence in validate-workflow.md that currently
lists acceptable extensions to explicitly include .md; locate the line
containing "Validate candidate existence and expected file type (`.yaml`,
`.yml`, `.json`, or checklist-defined extension)" and add `.md` to the
parenthetical list so it reads something like "`.yaml`, `.yml`, `.json`, `.md`,
or checklist-defined extension" to ensure Markdown workflows are recognized.
In `@tools/cli/installers/lib/ide/shared/workflow-command-generator.js`:
- Around line 213-222: The generated launcher instructions include an invalid
hardcoded fallback "{project-root}/src/core/tasks/workflow.md" for installed
projects; update the generator in workflow-command-generator.js (look for the
code using this.bmadFolderName, the "workflow-config" parameter and the block
that builds the two loader paths) to stop emitting the /src/... fallback for
distribution: either remove the fallback entirely for installed builds, or make
it conditional by checking whether the source fallback actually exists
(fs.accessSync or fs.existsSync) or by detecting dev vs installed mode (env flag
or presence of a src folder) and only include the fallback when present; also
update the warning/error messages so they report the resolved attempts (primary
and, if used, the valid fallback) and avoid halting on a non-existent /src path.
🧹 Nitpick comments (10)
src/bmm/workflows/document-project/instructions.md (1)
13-21: Initialize all later-used fields upfront to avoid undefined state.Several fields are referenced later without defaults (e.g.,
status_file_path,field_type,workflow_mode,scan_level,subworkflow_success,status_update_success,cached_project_types). This risks empty outputs and wrong branching when prior state leaks in. Initialize them here.Suggested fix
<action>Initialize status defaults: - Set status_exists = false - Set status_file_found = false - Set standalone_mode = true - Set warning = "" - Set suggestion = "" - Set next_workflow = "" - Set next_agent = "" + - Set status_file_path = "" + - Set field_type = "" + - Set workflow_mode = "" + - Set scan_level = "" + - Set subworkflow_success = false + - Set status_update_success = false + - Set cached_project_types = "" </action>tools/cli/installers/lib/ide/shared/workflow-command-generator.js (1)
121-123: Redundant template path calculation —this.templatePathalready holds this exact value.Line 123 reconstructs the template path by joining
path.dirname(this.templatePath)with'workflow-commander.md', which produces exactlythis.templatePath(set on line 12). This is likely a leftover from when there were multiple templates selected conditionally.Proposed fix
async generateCommandContent(workflow, bmadDir) { - // Determine template based on workflow file type - const templatePath = path.join(path.dirname(this.templatePath), 'workflow-commander.md'); - // Load the appropriate template - const template = await fs.readFile(templatePath, 'utf8'); + const template = await fs.readFile(this.templatePath, 'utf8');docs/tutorials/getting-started.md (1)
68-68: Good bridging pattern — extend it beyond justhelp.Line 68 shows the right approach:
Run the help workflow (bmad-help on most platforms). This parenthetical mapping should be applied to at least the first occurrence of each workflow category (agent load, module workflow, core workflow) so users learn the naming convention early.tools/cli/installers/lib/modules/manager.js (4)
815-817: Redundantrequire('yaml')shadows the module-level import.
yamlis already required at line 3. This innerconst yaml = require('yaml')shadows it unnecessarily, adding confusion about which binding is in scope throughout the method.Proposed fix
try { - const yaml = require('yaml'); - const parsed = yaml.parse(frontmatterMatch[1]); + const parsed = yaml.parse(frontmatterMatch[1]);
1169-1172:replacestrips the workflow filename suffix — verify behaviour when the path has no matching suffix.
sourceWorkflowSubPath.replace(workflowSuffixPattern, '')silently returns the original string when the pattern doesn't match (e.g., if the path already ends at a directory or uses an unexpected filename). The subsequentfs.pathExistscheck on line 1175 will catch it, so this is safe, but be aware that an unexpected path shape will produce a confusing "Source workflow not found" warning rather than a clear "unexpected path format" message.
1189-1194: Workflow definition lookup in vendored directory assumes a flat listing.
fs.readdir(actualDestWorkflowPath)returns entries for the immediate directory only. If the workflow definition file is nested in a subdirectory (unlikely but possible given recursive copy on line 1187),findwon't locate it. This appears safe given the current file layout, but worth noting if directory structures evolve.
1214-1217:replaceAllwith a global regex is redundant but functional.
String.prototype.replaceAllrequires the regex to have thegflag (which it does), butreplacewith a global regex already replaces all occurrences. Either works — just a minor readability note;replacewould be the more conventional choice with a/gregex.tools/cli/installers/lib/ide/_config-driven.js (1)
169-174: Misleading variable namefinalTemplateType— it's only the fallback, not the resolved type.
finalTemplateTypeis always'default-workflow'regardless of whatworkflowTemplateTyperesolved to. The name suggests it's the type that will ultimately be used, but it's only the fallback parameter passed toloadTemplateWithMetadata. Consider renaming tofallbackTemplateTypefor clarity.Proposed fix
- const finalTemplateType = 'default-workflow'; - const { template, extension } = await this.loadTemplateWithMetadata(workflowTemplateType, 'workflow', config, finalTemplateType); + const fallbackType = 'default-workflow'; + const { template, extension } = await this.loadTemplateWithMetadata(workflowTemplateType, 'workflow', config, fallbackType);test/test-installation-components.js (2)
434-463: Temp directories leak when tests throw before cleanup.Tests 11, 12, 13, 18 (and others) create temp directories with
fs.mkdtempand clean up withfs.remove(tmpRoot)at the end. If any assertion or intermediate operation throws, thecatchblock callsassert(false, ...)but never removestmpRoot. Over repeated test runs or CI, this accumulates orphaned dirs in the system temp folder.Consider a
finallyblock or a cleanup helper:Example pattern
+ const tmpRoots = []; + const trackTmp = async (prefix) => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + tmpRoots.push(dir); + return dir; + }; // ... at the end of runTests(), before summary: + for (const dir of tmpRoots) { + await fs.remove(dir).catch(() => {}); + }Also applies to: 472-498, 507-532, 719-744
282-310: Test 9 appears before Tests 6, 7, 8 in the source — numbering is out of order.Tests are numbered 1, 2, 3, 4, 5, 9, 6, 7, 8, 10, 11, ... This likely happened during iterative development. It won't affect execution but makes the output confusing to read and cross-reference.
tools/cli/installers/lib/ide/shared/workflow-command-generator.js
Outdated
Show resolved
Hide resolved
|
Regarding the issue of this being a very large PR, here is how it could be split into multiple PRs which gives reviewable chunks, each green and mergeable, without breaking runtime behaviour. If maintainers can give me guidance on whether that would be helpful or not, I'll act accordingly. Here's the plan:
PRs:1. PR A: CLI/runtime migration foundation
2. PR B: Core workflow/task format clean-up
3. PR C: BMM Phase 1–3 step-file migration
4. PR D: BMM Phase 4 implementation workflows
5. PR E: Docs and onboarding alignment
|
…-clean # Conflicts: # tools/cli/installers/lib/core/manifest-generator.js
…-clean # Conflicts: # tools/cli/installers/lib/modules/manager.js
…-clean # Conflicts: # src/bmm/workflows/4-implementation/correct-course/workflow.yaml # src/bmm/workflows/4-implementation/retrospective/workflow.yaml # src/bmm/workflows/4-implementation/sprint-status/workflow.yaml
…-clean # Conflicts: # src/bmm/workflows/4-implementation/code-review/workflow.yaml # src/bmm/workflows/4-implementation/correct-course/workflow.yaml # src/bmm/workflows/4-implementation/create-story/checklist.md # src/bmm/workflows/4-implementation/create-story/workflow.yaml # src/bmm/workflows/4-implementation/dev-story/instructions.xml # src/bmm/workflows/4-implementation/dev-story/workflow.yaml # src/bmm/workflows/4-implementation/retrospective/workflow.yaml # src/bmm/workflows/4-implementation/sprint-planning/workflow.yaml # src/bmm/workflows/4-implementation/sprint-status/workflow.yaml # src/bmm/workflows/document-project/instructions.md # src/bmm/workflows/document-project/workflow.yaml # src/bmm/workflows/document-project/workflows/deep-dive.yaml # src/bmm/workflows/document-project/workflows/full-scan.yaml # src/bmm/workflows/qa/automate/workflow.yaml
…-clean # Conflicts: # tools/cli/installers/lib/ide/templates/combined/opencode-workflow-yaml.md # tools/cli/installers/lib/ide/templates/combined/opencode-workflow.md
|
Hey @dickymoore sorry for the late reply, have been a bit under the weather the past few days, but doing better now will have time to look at the changes and suggestion. Will respond more in the morning after I can read through the proposal and better answer the questions. Appreciate this! |
…-clean # Conflicts: # src/bmm/workflows/document-project/instructions.md
Sorry to hear you've been under the weather. I've been knocked over by a virus for the last few days too. Winter is hitting. Apologies this PR is so huge. No worries I need to try something else; if it's implemented the wrong way or isn't the direction you wanted to go in - I'd be happy to try a different tack. Will follow your guidance. |
I think I am finally almost on the other side of it - good luck for you also getting through it I hope MUCH quicker :) I think you are on discord - let me ping you there and we can discuss this one a bit easier. |
|
closing this as discussed, will align on plan in discord @dickymoore |
2 participants
What
Migrated workflows to the Markdown-based
workflow.mdformat and removed legacy XML/YAML workflow artifacts and references across the repo. This includes CLI/template/docs/test updates to keep tooling aligned with the single workflow definition format.Why
To align with the v6 step-file workflow architecture and eliminate format drift that caused stale references and inconsistent behavior in tooling and workflows. This consolidation reduces maintenance and avoids mismatched file expectations.
How
workflow.mdand removedworkflow.yaml/workflow.xmlartifacts.workflow.mdas the canonical workflow definition format.Testing
Ran
npm test(test:schemas, test:install, validate:schemas, lint, lint:md, format:check).Summary
Migrated workflows to the Markdown-based format and cleaned up legacy XML/YAML references across the repo, including CLI/template/docs/test updates that align with the new workflow.md structure.
Detailed Changes
workflow.mdand eliminatedworkflow.yamlreferences (including the QA automate workflow).Scope/Overlap Check
CHANGELOG.md,docs/**,src/bmm/agents/**,src/bmm/module-help.csv,src/bmm/workflows/**,src/core/tasks/**,src/core/workflows/**,src/core/resources/excalidraw/**,src/utility/agent-components/**,test/test-installation-components.js,tools/cli/installers/**.Potential Conflicts
tools/cli/installers/lib/ide/**and workflow manifest generation intools/cli/installers/lib/core/**.upstream/mainbefore merge to pick up recent CLI/IDE template updates.Migration Note
workflow.mdis supported; legacyworkflow.xml/workflow.yamlfiles should be converted to Markdown-based workflows.Compatibility Notes
bmm-workflow-status.yaml/workflow-status) are preserved; status tracking behavior not changed.Commit Breakdown
Notes / Breaking Changes
workflow.xmlandworkflow.yamlartifacts are no longer supported; workflows should useworkflow.md.Note for maintainers:
Tracking
Refs #1585