fix: complete setup skill with legacy cleanup, result templates, and path fixes#36
Merged
bmadcode merged 4 commits intobmad-code-org:mainfrom Mar 25, 2026
Merged
Conversation
- New cleanup-legacy.py script removes redundant installer directories
from _bmad/ after config migration, with safety verification that
skills exist at .claude/skills/ before removal
- Fix merge-config.py to apply result templates from module.yaml so
values like "skills" are stored as "{project-root}/skills"
- Update SKILL.md with fresh-vs-legacy detection, directory creation
step, and cleanup step
- Add comprehensive tests for both changes (81 total, all passing)
- Update module-configuration.md docs with cleanup section
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove "Use a structured question tool if available" from Collect Configuration — causes inconsistent behavior across LLM sessions - Bundle communication_language and document_output_language into a single question since the answer is always the same Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
All references to assets/ and scripts/ paths now use ./ prefix to
distinguish them from {project-root} paths per BMad path conventions.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughAdds migration and cleanup for legacy per-module Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
Completes the bmad-builder-setup skill’s end-to-end setup/migration flow by correctly distinguishing fresh installs from legacy layouts, applying module.yaml result templates to stored answers, and adding a post-merge cleanup step to remove redundant installer directories under _bmad/.
Changes:
- Update
SKILL.mdto refine install vs. legacy detection guidance, add output-directory creation + legacy directory cleanup steps, and fix relative path references. - Add
apply_result_templates()tomerge-config.pyso stored module config values respect each variable’sresulttemplate. - Introduce
cleanup-legacy.py(with unit tests) to safely remove redundant legacy_bmad/directory trees after migration, plus docs describing the cleanup behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/skills/bmad-builder-setup/SKILL.md | Updates setup skill instructions for fresh vs legacy detection, templating, output dir creation, and directory cleanup. |
| src/skills/bmad-builder-setup/scripts/merge-config.py | Applies module.yaml result templates when writing module values into _bmad/config.yaml. |
| src/skills/bmad-builder-setup/scripts/cleanup-legacy.py | New script to verify installed skills and remove redundant legacy directories under _bmad/. |
| src/skills/bmad-builder-setup/scripts/tests/test-merge-config.py | Adds coverage for result templating and updates expectations for templated module paths. |
| src/skills/bmad-builder-setup/scripts/tests/test-cleanup-legacy.py | New unit test suite covering discovery, verification, and deletion logic for legacy cleanup. |
| docs/explanation/module-configuration.md | Documents the new legacy directory cleanup step and its safety guarantees. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+194
to
+213
| def apply_result_templates( | ||
| module_yaml: dict, module_answers: dict, verbose: bool = False | ||
| ) -> dict: | ||
| """Apply result templates from module.yaml to transform raw answer values. | ||
|
|
||
| For each answer, if the corresponding variable definition in module.yaml has | ||
| a 'result' field, replaces {value} in that template with the answer. Skips | ||
| the template if the answer already contains '{project-root}' to prevent | ||
| double-prefixing. | ||
| """ | ||
| transformed = {} | ||
| for key, value in module_answers.items(): | ||
| var_def = module_yaml.get(key) | ||
| if ( | ||
| isinstance(var_def, dict) | ||
| and "result" in var_def | ||
| and "{project-root}" not in str(value) | ||
| ): | ||
| template = var_def["result"] | ||
| transformed[key] = template.replace("{value}", str(value)) |
There was a problem hiding this comment.
apply_result_templates() will blindly apply the result template to any value that doesn’t already contain "{project-root}". If a user provides an absolute path (e.g. "/tmp/out") or a home-relative path ("~/out"), this will be transformed into something like "{project-root}//tmp/out", which is almost certainly unintended. Consider detecting absolute/home paths (and possibly Windows drive/UNC paths) and skipping templating in those cases, or otherwise validating/normalizing before applying the template.
Suggested change
| def apply_result_templates( | |
| module_yaml: dict, module_answers: dict, verbose: bool = False | |
| ) -> dict: | |
| """Apply result templates from module.yaml to transform raw answer values. | |
| For each answer, if the corresponding variable definition in module.yaml has | |
| a 'result' field, replaces {value} in that template with the answer. Skips | |
| the template if the answer already contains '{project-root}' to prevent | |
| double-prefixing. | |
| """ | |
| transformed = {} | |
| for key, value in module_answers.items(): | |
| var_def = module_yaml.get(key) | |
| if ( | |
| isinstance(var_def, dict) | |
| and "result" in var_def | |
| and "{project-root}" not in str(value) | |
| ): | |
| template = var_def["result"] | |
| transformed[key] = template.replace("{value}", str(value)) | |
| def _looks_like_rooted_path(value: str) -> bool: | |
| """Heuristically determine whether a value looks like an absolute or home/Windows path. | |
| This is intentionally conservative: if it looks like a rooted path, we avoid | |
| prefixing it with {project-root} in result templates. | |
| """ | |
| s = str(value) | |
| if not s: | |
| return False | |
| # POSIX absolute path | |
| if s.startswith("/"): | |
| return True | |
| # Home-relative path | |
| if s.startswith("~"): | |
| return True | |
| # Windows UNC path | |
| if s.startswith("\\\\"): | |
| return True | |
| # Windows drive letter path, e.g. C:\ or D:/... | |
| if len(s) >= 3 and s[0].isalpha() and s[1] == ":" and s[2] in ("/", "\\"): | |
| return True | |
| return False | |
| def apply_result_templates( | |
| module_yaml: dict, module_answers: dict, verbose: bool = False | |
| ) -> dict: | |
| """Apply result templates from module.yaml to transform raw answer values. | |
| For each answer, if the corresponding variable definition in module.yaml has | |
| a 'result' field, replaces {value} in that template with the answer. | |
| When the template contains '{project-root}', the transformation is skipped if | |
| the answer already contains '{project-root}' or if the answer looks like an | |
| absolute/home/Windows path, to avoid unintended double-prefixing such as | |
| '{project-root}//tmp/out'. | |
| """ | |
| transformed = {} | |
| for key, value in module_answers.items(): | |
| var_def = module_yaml.get(key) | |
| if isinstance(var_def, dict) and "result" in var_def: | |
| template = var_def["result"] | |
| value_str = str(value) | |
| if "{project-root}" in template: | |
| # Skip templating if the value already contains {project-root} or | |
| # looks like an absolute/home/Windows path. | |
| if "{project-root}" in value_str or _looks_like_rooted_path(value_str): | |
| transformed[key] = value | |
| continue | |
| transformed[key] = template.replace("{value}", value_str) |
Comment on lines
+203
to
+207
| bmad_dir = args.bmad_dir | ||
| module_code = args.module_code | ||
|
|
||
| # Build the list of directories to remove | ||
| dirs_to_remove = [module_code, "core"] + args.also_remove |
There was a problem hiding this comment.
cleanup-legacy.py uses user-controlled directory names (module_code and --also-remove entries) directly in Path(bmad_dir) / dirname. Without validating that these are simple directory basenames (no path separators, no '.'/'..'), a malicious or accidental value like ".." could cause the script to delete outside of _bmad/. Please validate/sanitize these inputs before building dirs_to_remove and refuse unsafe names.
|
|
||
| ## Create Output Directories | ||
|
|
||
| After writing config, create any output directories that were configured. Resolve `{project-root}` to the actual project root and create each path-type value from `config.yaml` that does not yet exist — this includes `output_folder` and any module variable whose value starts with `{project-root}/`. Use `mkdir -p` or equivalent to create the full path. |
There was a problem hiding this comment.
This section says to "Resolve {project-root} to the actual project root" when creating directories, but earlier the skill notes {project-root} is a literal token that should not be substituted in config values. Consider clarifying here that substitution is only for filesystem operations (mkdir), while the config files must continue to store the literal token.
Suggested change
| After writing config, create any output directories that were configured. Resolve `{project-root}` to the actual project root and create each path-type value from `config.yaml` that does not yet exist — this includes `output_folder` and any module variable whose value starts with `{project-root}/`. Use `mkdir -p` or equivalent to create the full path. | |
| After writing config, create any output directories that were configured. For filesystem operations only (such as creating directories), resolve the `{project-root}` token to the actual project root and create each path-type value from `config.yaml` that does not yet exist — this includes `output_folder` and any module variable whose value starts with `{project-root}/`. The paths stored in the config files must continue to use the literal `{project-root}` token; only the directories on disk should use the resolved paths. Use `mkdir -p` or equivalent to create the full path. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/skills/bmad-builder-setup/scripts/tests/test-cleanup-legacy.py (1)
206-218: Consider prefixing unused unpacked variables with underscores.Static analysis flags several unused variables from tuple unpacking (e.g.,
count,not_found). While the tests work correctly, prefixing unused variables with_follows Python convention and silences linter warnings.Example fix for line 212
- removed, not_found, count = cleanup_directories( + removed, _not_found, _count = cleanup_directories( bmad_dir, ["bmb", "core", "_config"] )Similar changes apply to lines 236, 265, 295, and 414.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/bmad-builder-setup/scripts/tests/test-cleanup-legacy.py` around lines 206 - 218, In the test functions (e.g., test_removes_module_core_and_config) rename unused tuple-unpacked variables to start with an underscore to satisfy linters: change count and not_found to _count and _not_found (and similarly prefix any other unused unpacked vars in the other test functions referenced at lines 236, 265, 295, 414) so the tuple unpacking still returns the same values but unused names follow Python convention and silence static analysis warnings.src/skills/bmad-builder-setup/scripts/cleanup-legacy.py (1)
207-215: Consider using unpacking syntax for list construction.The current concatenation works correctly, but unpacking is more idiomatic in modern Python.
Suggested change
- dirs_to_remove = [module_code, "core"] + args.also_remove + dirs_to_remove = [module_code, "core", *args.also_remove]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/bmad-builder-setup/scripts/cleanup-legacy.py` around lines 207 - 215, Replace the manual concatenation and dedup loop for dirs_to_remove with a more idiomatic unpacking + order-preserving dedupe: create dirs_to_remove by combining module_code, the literal "core", and the elements of args.also_remove using unpacking, then deduplicate while preserving order (e.g., via dict.fromkeys or an OrderedDict) and assign back to dirs_to_remove; remove the seen/unique_dirs loop and retain the same final semantics. Reference symbols: dirs_to_remove, module_code, "core", args.also_remove, seen, unique_dirs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/skills/bmad-builder-setup/scripts/cleanup-legacy.py`:
- Around line 207-215: Replace the manual concatenation and dedup loop for
dirs_to_remove with a more idiomatic unpacking + order-preserving dedupe: create
dirs_to_remove by combining module_code, the literal "core", and the elements of
args.also_remove using unpacking, then deduplicate while preserving order (e.g.,
via dict.fromkeys or an OrderedDict) and assign back to dirs_to_remove; remove
the seen/unique_dirs loop and retain the same final semantics. Reference
symbols: dirs_to_remove, module_code, "core", args.also_remove, seen,
unique_dirs.
In `@src/skills/bmad-builder-setup/scripts/tests/test-cleanup-legacy.py`:
- Around line 206-218: In the test functions (e.g.,
test_removes_module_core_and_config) rename unused tuple-unpacked variables to
start with an underscore to satisfy linters: change count and not_found to
_count and _not_found (and similarly prefix any other unused unpacked vars in
the other test functions referenced at lines 236, 265, 295, 414) so the tuple
unpacking still returns the same values but unused names follow Python
convention and silence static analysis warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20819e93-59cc-4503-b5ca-3d08e795285f
📒 Files selected for processing (6)
docs/explanation/module-configuration.mdsrc/skills/bmad-builder-setup/SKILL.mdsrc/skills/bmad-builder-setup/scripts/cleanup-legacy.pysrc/skills/bmad-builder-setup/scripts/merge-config.pysrc/skills/bmad-builder-setup/scripts/tests/test-cleanup-legacy.pysrc/skills/bmad-builder-setup/scripts/tests/test-merge-config.py
Address review feedback: the Overview says {project-root} is a literal
token that should never be substituted, but Create Output Directories
said to "resolve" it. Clarify that substitution applies only to
filesystem operations (mkdir), not to stored config values.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/skills/bmad-builder-setup/SKILL.md`:
- Around line 33-37: Wrap long Markdown lines in the SKILL.md sections that
include the "Default priority" and "Core config" paragraphs so they satisfy the
repo's markdown-lint rules; break the long sentences/lists into shorter lines
(e.g., ~80 characters) while preserving content and punctuation, and ensure the
paragraphs that reference keys like user_name, communication_language,
document_output_language, output_folder and the rule about legacy configs are
wrapped consistently; also apply the same hard-wrapping to the other noted long
lines in the file (around the later paragraphs) so all MD paragraphs and list
items pass linting.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40f9b0f4-e877-43ed-83cb-b6653db1cd04
📒 Files selected for processing (1)
src/skills/bmad-builder-setup/SKILL.md
Comment on lines
+33
to
+37
| Ask the user for values. Show defaults in brackets. Present all values together so the user can respond once with only the values they want to change (e.g. "change language to Swahili, rest are fine"). Never tell the user to "press enter" or "leave blank" — in a chat interface they must type something to respond. | ||
|
|
||
| **Default priority** (highest wins): existing new config values > legacy config values > `assets/module.yaml` defaults. When legacy configs exist, read them and use matching values as defaults instead of `module.yaml` defaults. Only keys that match the current schema are carried forward — changed or removed keys are ignored. | ||
| **Default priority** (highest wins): existing new config values > legacy config values > `./assets/module.yaml` defaults. When legacy configs exist, read them and use matching values as defaults instead of `module.yaml` defaults. Only keys that match the current schema are carried forward — changed or removed keys are ignored. | ||
|
|
||
| **Core config** (only if no core keys exist yet): `user_name` (default: BMad), `communication_language` (default: English), `document_output_language` (default: English), `output_folder` (default: `{project-root}/_bmad-output`). Of these, `user_name` and `communication_language` are written exclusively to `config.user.yaml`. The rest go to `config.yaml` at root and are shared across all modules. | ||
| **Core config** (only if no core keys exist yet): `user_name` (default: BMad), `communication_language` and `document_output_language` (default: English — ask as a single language question, both keys get the same answer), `output_folder` (default: `{project-root}/_bmad-output`). Of these, `user_name` and `communication_language` are written exclusively to `config.user.yaml`. The rest go to `config.yaml` at root and are shared across all modules. |
There was a problem hiding this comment.
Please wrap long Markdown lines to avoid lint failures.
Several updated lines are very long and may fail markdown lint in CI. Please hard-wrap these paragraphs/lists into shorter lines.
Suggested wrap style
-Ask the user for values. Show defaults in brackets. Present all values together so the user can respond once with only the values they want to change (e.g. "change language to Swahili, rest are fine"). Never tell the user to "press enter" or "leave blank" — in a chat interface they must type something to respond.
+Ask the user for values. Show defaults in brackets.
+Present all values together so the user can respond once with only the values
+they want to change (e.g. "change language to Swahili, rest are fine").
+Never tell the user to "press enter" or "leave blank" — in a chat interface
+they must type something to respond.As per coding guidelines: "**/*.md: Markdown files must pass markdown linting as part of the test suite".
Also applies to: 56-57, 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/skills/bmad-builder-setup/SKILL.md` around lines 33 - 37, Wrap long
Markdown lines in the SKILL.md sections that include the "Default priority" and
"Core config" paragraphs so they satisfy the repo's markdown-lint rules; break
the long sentences/lists into shorter lines (e.g., ~80 characters) while
preserving content and punctuation, and ensure the paragraphs that reference
keys like user_name, communication_language, document_output_language,
output_folder and the rule about legacy configs are wrapped consistently; also
apply the same hard-wrapping to the other noted long lines in the file (around
the later paragraphs) so all MD paragraphs and list items pass linting.
bmadcode
approved these changes
Mar 23, 2026
This was referenced Mar 28, 2026
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the fresh-install-misidentified-as-legacy bug and completes
missing functionality in the bmad-builder-setup skill: result template
application, output directory creation, and legacy directory cleanup.
Fixes #35
Why
Fresh installs via the BMAD installer create per-module config files
that the setup skill incorrectly classifies as "legacy." Additionally,
the skill stored raw user answers without applying module.yaml's
resulttemplate, never created configured output directories, andleft hundreds of redundant skill files in
_bmad/subdirectories.How
on whether
config.yamlalready has a module section; add CreateOutput Directories and Cleanup Legacy Directories sections; bundle
language questions; remove inconsistent "structured question tool"
hint; fix bare skill-internal paths to use
./prefixapply_result_templates()soskillsbecomes
{project-root}/skillsper module.yaml'sresultfield.claude/skills/then removes
_bmad/{module-code}/,_bmad/core/,_bmad/_config/Cleanup section
Testing
Tested on a fresh BMAD install (
npx github:bmad-code-org/BMAD-METHOD).Ran
/bmad-builder-setupand confirmed: config.yaml has templatedvalues, config.user.yaml has user-only keys, output directories created,
legacy directories removed. 81 unit tests passing (26 new for
cleanup-legacy, 55 updated for merge-config).
Summary by CodeRabbit
New Features
Documentation