fix(skill): clean up bmad-create-product-brief validation findings#1982
fix(skill): clean up bmad-create-product-brief validation findings#1982
Conversation
Remove name/description from step frontmatter (STEP-06), inline
intra-skill path variables (PATH-04), prefix bare external path
with {project-root} (PATH-03), and normalize template placeholders
to double-curly convention (REF-01).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
🤖 Augment PR SummarySummary: Cleans up the
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| inputDocuments: [] | ||
| date: { system-date } | ||
| author: { user } | ||
| date: {{system-date}} |
There was a problem hiding this comment.
Most workflows/templates in this repo use {{date}} as the system-generated datetime variable; {{system-date}} may not be defined (and also diverges from the {{date}} used in the output filename patterns). If it’s not supported by the templating/installer, the product brief frontmatter date could remain unrendered.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
📝 WalkthroughWalkthroughThis PR modifies the product brief workflow by replacing dynamic placeholder references with explicit file paths and updating template metadata syntax. Frontmatter fields are removed from several step files, and template variables shift from token-based syntax to template-style notation (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md (2)
84-108:⚠️ Potential issue | 🟡 MinorMissing error handling for file discovery and copy operations.
Lines 84-108 describe file discovery and template copying but provide no error-handling instructions. What if:
- Discovery finds no files but the workflow expects input?
- The template copy fails due to permissions or disk space?
{outputFile}path doesn't exist yet?The executing agent has no guidance on how to handle these failures, leading to inconsistent behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md` around lines 84 - 108, The discovery and template-copy steps lack error handling: ensure functions that perform file discovery validate results (e.g., when matching Brainstorming/Research/docs/project-context patterns) and if none are found, prompt the user/raise a clear recoverable error before proceeding; when the user confirms files, only then add them to the frontmatter inputDocuments array and load them fully, otherwise abort the workflow. Before copying ../product-brief.template.md to {outputFile}, ensure the destination directory exists (create it if missing), catch and handle filesystem errors (permissions, disk full, missing template) with descriptive messages and a retry/abort path, and ensure the copy operation only updates the outputFile and inputDocuments on successful completion. Also log and surface any file-level errors during sharded-folder loading (index.md missing, partial reads) so the agent can ask the user to provide or confirm missing pieces.
137-146:⚠️ Potential issue | 🔴 CriticalAuto-proceed without user confirmation contradicts universal rule.
Lines 137-146 describe auto-proceeding to the next step ("without delay, read fully and follow: ./step-02-vision.md"), but the MANDATORY EXECUTION RULES at line 47 state: "🚫 FORBIDDEN to proceed without user confirmation through menu." These instructions conflict.
Is this step an exception to the menu confirmation rule? If so, call that out explicitly in the Step-Specific Rules. If not, this is a critical behavioral inconsistency that will confuse the executing agent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md` around lines 137 - 146, There is a conflict between the auto-proceed language in step-01-init.md (the "EXECUTION RULES" and "CRITICAL STEP COMPLETION NOTE" instructing to "proceed directly to next step" and to "read fully and follow: ./step-02-vision.md") and the global MANDATORY EXECUTION RULES that forbid proceeding without user confirmation through the menu; fix by either (A) removing the auto-proceed wording and requiring explicit menu confirmation before moving to step-02-vision.md, or (B) making an explicit, clearly labeled exception in the Step-Specific Rules that states this step is allowed to auto-proceed despite the global rule and under exactly what conditions (e.g., "only when setup completion is achieved and frontmatter updated"), then update the "EXECUTION RULES" and "CRITICAL STEP COMPLETION NOTE" to match that choice so the language is consistent with the global "FORBIDDEN to proceed without user confirmation through menu" rule.src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-02-vision.md (1)
1-8:⚠️ Potential issue | 🟠 MajorInconsistent variable inlining policy creates confusion.
The frontmatter retains
{advancedElicitationTask}and{partyModeWorkflow}as variables (lines 6-7), but{nextStepFile}is inlined throughout. No documented rationale explains why task/workflow references remain variables while step progression is hardcoded. This inconsistency will confuse future maintainers.Is the rule "external references stay variables, internal step flow is hardcoded"? If so, document it. If not, explain why cherry-picking which variables to inline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-02-vision.md` around lines 1 - 8, The frontmatter inconsistent variable inlining (keep {advancedElicitationTask} and {partyModeWorkflow} as variables but inlining {nextStepFile}) should be resolved and documented: choose the policy (e.g., "external task/workflow references remain variables; internal step progression is hardcoded") and apply it consistently in step-02-vision.md by either 1) converting {advancedElicitationTask} and {partyModeWorkflow} to inlined file/skill strings like nextStepFile, or 2) replacing nextStepFile in the content with a variable and keeping the two existing variables, and add a short explanatory comment in the frontmatter describing the rule; reference the symbols advancedElicitationTask, partyModeWorkflow, and nextStepFile when making the change.
🧹 Nitpick comments (2)
src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md (1)
108-108: Relative path../is fragile to directory structure changes.The path
../product-brief.template.mdassumes the step file is exactly one directory level below the template. If thesteps/directory is moved, renamed, or nested deeper (e.g.,steps/init/step-01-init.md), this path breaks. The old{productBriefTemplate}variable provided indirection that protected against restructuring.Document the directory structure assumption explicitly:
<!-- ASSUMPTION: This file is at <skill-root>/steps/step-01-init.md Template is at <skill-root>/product-brief.template.md If directory structure changes, update relative path at line 108 -->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md` at line 108, Replace the fragile hard-coded relative path "../product-brief.template.md" by restoring/use the indirection previously provided by the {productBriefTemplate} variable (or, if you choose not to reintroduce the variable, add the explicit ASSUMPTION comment block above the line to document that this step file lives at <skill-root>/steps/step-01-init.md and the template at <skill-root>/product-brief.template.md); update the line that currently says "Copy the template from `../product-brief.template.md` to `{outputFile}`" to reference {productBriefTemplate} (or keep the existing text but add the suggested ASSUMPTION comment so future moves won’t silently break).src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-02-vision.md (1)
158-158: Hardcoded step path creates maintenance burden and brittleness.Replacing
{nextStepFile}with./step-03-users.mdmakes workflow reordering expensive. If a new step is inserted between 02 and 03, you must manually update this file, plus the CRITICAL NOTE at line 170, plus any other references. The old variable-based approach allowed centralized control.Consider whether the maintenance cost of hardcoded paths (across 7 step files per PR description) justifies the minor benefit of removing one level of indirection. If you must inline, at least add a comment:
# IMPORTANT: If step order changes, update hardcoded references at lines 158, 170🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-02-vision.md` at line 158, Replace the hardcoded next-step path "./step-03-users.md" with the variable placeholder {nextStepFile} in the step text so the workflow relies on the centralized next-step value; update the clause that reads "then read fully and follow: ./step-03-users.md" to "then read fully and follow: {nextStepFile}" and ensure the surrounding references to {outputFile} and stepsCompleted: [1, 2] remain intact; also update the nearby CRITICAL NOTE block that mentions hardcoded paths (the "CRITICAL NOTE" section) to either remove outdated line-specific guidance or to call out that any hardcoded references must be kept in sync if reordering steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/bmm/workflows/1-analysis/bmad-create-product-brief/product-brief.template.md`:
- Around line 4-5: Update the PR description and changelog to state this change
is a bug fix (not just a "cleanup"): explain that the template engine uses the
regex /\{\{(\w+)\}\}/g and therefore the old single-brace tokens like "{
system-date }" were never matched, and that you converted them to the correct
double-brace tokens ("{{system-date}}", "{{user_name}}") so the fields now
render; mention the affected template (product-brief.template.md) and include an
explicit note that this enables those fields to work for the first time.
- Around line 4-5: The template uses the wrong date placeholder: replace the
incorrect {{system-date}} token with the workflow-provided {{date}} token in
product-brief.template.md so the template engine can interpolate the value;
leave {{user_name}} unchanged.
In
`@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md`:
- Line 84: The change left the `{project-root}` variable in place while other
path variables like `{productBriefTemplate}` were inlined (per PATH-04); either
fully inline `{project-root}/docs/**` to the equivalent literal relative path
used elsewhere (for example the same number of ../ segments used to reach repo
root plus /docs/**) or remove the `{project-root}` prefix and use the
already-inlined form `docs/**` to match the PR's "inline path variables as
literal relative paths" rule; update the entry that currently reads
`{project-root}/docs/**` to one consistent approach and ensure it aligns with
the other inlined paths in the file.
---
Outside diff comments:
In
`@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md`:
- Around line 84-108: The discovery and template-copy steps lack error handling:
ensure functions that perform file discovery validate results (e.g., when
matching Brainstorming/Research/docs/project-context patterns) and if none are
found, prompt the user/raise a clear recoverable error before proceeding; when
the user confirms files, only then add them to the frontmatter inputDocuments
array and load them fully, otherwise abort the workflow. Before copying
../product-brief.template.md to {outputFile}, ensure the destination directory
exists (create it if missing), catch and handle filesystem errors (permissions,
disk full, missing template) with descriptive messages and a retry/abort path,
and ensure the copy operation only updates the outputFile and inputDocuments on
successful completion. Also log and surface any file-level errors during
sharded-folder loading (index.md missing, partial reads) so the agent can ask
the user to provide or confirm missing pieces.
- Around line 137-146: There is a conflict between the auto-proceed language in
step-01-init.md (the "EXECUTION RULES" and "CRITICAL STEP COMPLETION NOTE"
instructing to "proceed directly to next step" and to "read fully and follow:
./step-02-vision.md") and the global MANDATORY EXECUTION RULES that forbid
proceeding without user confirmation through the menu; fix by either (A)
removing the auto-proceed wording and requiring explicit menu confirmation
before moving to step-02-vision.md, or (B) making an explicit, clearly labeled
exception in the Step-Specific Rules that states this step is allowed to
auto-proceed despite the global rule and under exactly what conditions (e.g.,
"only when setup completion is achieved and frontmatter updated"), then update
the "EXECUTION RULES" and "CRITICAL STEP COMPLETION NOTE" to match that choice
so the language is consistent with the global "FORBIDDEN to proceed without user
confirmation through menu" rule.
In
`@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-02-vision.md`:
- Around line 1-8: The frontmatter inconsistent variable inlining (keep
{advancedElicitationTask} and {partyModeWorkflow} as variables but inlining
{nextStepFile}) should be resolved and documented: choose the policy (e.g.,
"external task/workflow references remain variables; internal step progression
is hardcoded") and apply it consistently in step-02-vision.md by either 1)
converting {advancedElicitationTask} and {partyModeWorkflow} to inlined
file/skill strings like nextStepFile, or 2) replacing nextStepFile in the
content with a variable and keeping the two existing variables, and add a short
explanatory comment in the frontmatter describing the rule; reference the
symbols advancedElicitationTask, partyModeWorkflow, and nextStepFile when making
the change.
---
Nitpick comments:
In
`@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md`:
- Line 108: Replace the fragile hard-coded relative path
"../product-brief.template.md" by restoring/use the indirection previously
provided by the {productBriefTemplate} variable (or, if you choose not to
reintroduce the variable, add the explicit ASSUMPTION comment block above the
line to document that this step file lives at <skill-root>/steps/step-01-init.md
and the template at <skill-root>/product-brief.template.md); update the line
that currently says "Copy the template from `../product-brief.template.md` to
`{outputFile}`" to reference {productBriefTemplate} (or keep the existing text
but add the suggested ASSUMPTION comment so future moves won’t silently break).
In
`@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-02-vision.md`:
- Line 158: Replace the hardcoded next-step path "./step-03-users.md" with the
variable placeholder {nextStepFile} in the step text so the workflow relies on
the centralized next-step value; update the clause that reads "then read fully
and follow: ./step-03-users.md" to "then read fully and follow: {nextStepFile}"
and ensure the surrounding references to {outputFile} and stepsCompleted: [1, 2]
remain intact; also update the nearby CRITICAL NOTE block that mentions
hardcoded paths (the "CRITICAL NOTE" section) to either remove outdated
line-specific guidance or to call out that any hardcoded references must be kept
in sync if reordering steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1700c1f9-5c71-4e00-813d-3e8f547d71c2
📒 Files selected for processing (8)
src/bmm/workflows/1-analysis/bmad-create-product-brief/product-brief.template.mdsrc/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.mdsrc/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01b-continue.mdsrc/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-02-vision.mdsrc/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-03-users.mdsrc/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-04-metrics.mdsrc/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-05-scope.mdsrc/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-06-complete.md
💤 Files with no reviewable changes (2)
- src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01b-continue.md
- src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-06-complete.md
| date: {{system-date}} | ||
| author: {{user_name}} |
There was a problem hiding this comment.
Document that the old syntax was broken, not just "cleaned up."
The PR description calls this "cleanup," but the old single-brace syntax { system-date } would never have been processed by the template engine (which uses /\{\{(\w+)\}\}/g). This is actually a bug fix that enables these fields to work for the first time. The PR description understates the impact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/1-analysis/bmad-create-product-brief/product-brief.template.md`
around lines 4 - 5, Update the PR description and changelog to state this change
is a bug fix (not just a "cleanup"): explain that the template engine uses the
regex /\{\{(\w+)\}\}/g and therefore the old single-brace tokens like "{
system-date }" were never matched, and that you converted them to the correct
double-brace tokens ("{{system-date}}", "{{user_name}}") so the fields now
render; mention the affected template (product-brief.template.md) and include an
explicit note that this enables those fields to work for the first time.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for where template variables are defined and what names are used
rg -n --type=js -C3 '\b(user_name|userName|user)\b.*=' tools/cli/Repository: bmad-code-org/BMAD-METHOD
Length of output: 2961
🏁 Script executed:
#!/bin/bash
# Check the product-brief.template.md file
cat -n src/bmm/workflows/1-analysis/bmad-create-product-brief/product-brief.template.md | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 348
🏁 Script executed:
#!/bin/bash
# Search for template engine usage and how variables are provided to templates
rg -n "system-date|system_date" --type=js tools/cli/ | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look for template processing/rendering code
rg -n "template.*date|system.*date" --type=js -i tools/cli/ | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for template rendering/processing code
rg -n "{{.*}}" --type=md | head -5Repository: bmad-code-org/BMAD-METHOD
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for where template files are processed
rg -n "template\.md|renderTemplate|processTemplate" --type=js -i | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 1162
🏁 Script executed:
#!/bin/bash
# Look for date variable assignment across the codebase
rg -n "system.*date|\.date\s*=" --type=js -i tools/cli/ -A 2 | head -30Repository: bmad-code-org/BMAD-METHOD
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for project_name variable to understand variable pattern
rg -n "project_name|projectName" --type=js tools/cli/ -C 2 | head -30Repository: bmad-code-org/BMAD-METHOD
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Read the template-engine.js file to understand how variables are processed
cat -n tools/cli/lib/agent/template-engine.jsRepository: bmad-code-org/BMAD-METHOD
Length of output: 5745
🏁 Script executed:
#!/bin/bash
# Search for where product-brief.template.md is referenced or processed
rg -n "product-brief" --type=js -C 3Repository: bmad-code-org/BMAD-METHOD
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for where date variables are set/created
rg -n "\"date\"|'date'|\.date\s*=" --type=js tools/cli/installers -A 2 | head -40Repository: bmad-code-org/BMAD-METHOD
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for workflow processing or where template files are loaded
rg -n "template\.md|\.template\.md" --type=js | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 357
🏁 Script executed:
#!/bin/bash
# Search for where system-date might be set - check all file types
rg -n "system.date|systemDate" --type=js --type=ts -i | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Look at the workflow directory structure to understand how it's organized
find src/bmm/workflows -type f -name "*.js" -o -name "*.ts" -o -name "*.json" | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for any reference to "date" in workflow config or similar
rg -n "date" src/bmm/workflows -i --type=json --type=yaml --type=md | head -30Repository: bmad-code-org/BMAD-METHOD
Length of output: 5223
🏁 Script executed:
#!/bin/bash
# Look at the tech-spec-template.md to see the exact old syntax
cat -n src/bmm/workflows/bmad-quick-flow/bmad-quick-spec/tech-spec-template.md | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 458
🏁 Script executed:
#!/bin/bash
# Search for documentation about available template variables
rg -n "date.*system|system.*date|available.*variable|variable.*available" src/bmm/workflows -i -B 2 -A 2 | head -50Repository: bmad-code-org/BMAD-METHOD
Length of output: 5318
🏁 Script executed:
#!/bin/bash
# Check if there are other product-brief templates or references
find src/bmm/workflows -name "*product*brief*" -o -name "*product*" | grep -i briefRepository: bmad-code-org/BMAD-METHOD
Length of output: 204
Variable name mismatch: {{system-date}} should be {{date}}
The new template changed {date} to {{system-date}}, but all workflow documentation consistently defines the available variable as date, not system-date. The template engine will fail to replace {{system-date}} and leave a literal placeholder in the rendered output.
Correct to {{date}} to match the variable provided by the workflow system.
The variable {{user_name}} is correct and properly defined in the configuration system.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/1-analysis/bmad-create-product-brief/product-brief.template.md`
around lines 4 - 5, The template uses the wrong date placeholder: replace the
incorrect {{system-date}} token with the workflow-provided {{date}} token in
product-brief.template.md so the template engine can interpolate the value;
leave {{user_name}} unchanged.
| - {output_folder}/** | ||
| - {product_knowledge}/** | ||
| - docs/** | ||
| - {project-root}/docs/** |
There was a problem hiding this comment.
Inconsistent path inlining: why keep {project-root} variable here?
Line 84 changes docs/** to {project-root}/docs/**, adding a variable prefix instead of inlining. But line 108 fully inlines {productBriefTemplate} to ../product-brief.template.md. The PR goal (PATH-04) is to "inline path variables as literal relative paths," yet {project-root} is retained as a variable. Either inline it to ../../../../../../ (brittle) or explain why this path gets special variable treatment while others don't.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bmm/workflows/1-analysis/bmad-create-product-brief/steps/step-01-init.md`
at line 84, The change left the `{project-root}` variable in place while other
path variables like `{productBriefTemplate}` were inlined (per PATH-04); either
fully inline `{project-root}/docs/**` to the equivalent literal relative path
used elsewhere (for example the same number of ../ segments used to reach repo
root plus /docs/**) or remove the `{project-root}` prefix and use the
already-inlined form `docs/**` to match the PR's "inline path variables as
literal relative paths" rule; update the entry that currently reads
`{project-root}/docs/**` to one consistent approach and ensure it aligns with
the other inlined paths in the file.
1 participant
Summary
name/descriptionfrom all 7 step file frontmatters (STEP-06)nextStepFileandproductBriefTemplatepath variables as literal relative paths (PATH-04)docs/**with{project-root}(PATH-03){{double-curly}}convention (REF-01)Test plan
🤖 Generated with Claude Code