feat(skills): add TOML-based skill customization system#75
Open
feat(skills): add TOML-based skill customization system#75
Conversation
Add customize.toml with stock customization fields (inject before/after, additional_resources) to all 4 builder skills. Include resolve-customization.py script in each skill's scripts/ directory. Add customization resolve and inject points to all SKILL.md files.
WalkthroughAdds per-skill customization: defaults Changes
Sequence Diagram(s)sequenceDiagram
participant Skill
participant Resolver as resolve-customization.py
participant FS as Project FS
participant Workflow
Skill->>Resolver: run with skill name + --key inject --key additional_resources
Resolver->>FS: read defaults `customize.toml`
Resolver->>FS: read _bmad/customizations/{skill}.toml (team)
Resolver->>FS: read _bmad/customizations/{skill}.user.toml (user)
Resolver-->>Skill: emit merged JSON (inject, additional_resources)
Skill->>Skill: if inject.before non-empty -> prepend & follow
Skill->>Skill: record additional_resources for later reference
Skill->>Workflow: run main workflow
Workflow-->>Skill: workflow complete
Skill->>Resolver: run with --key inject.after
Resolver-->>Skill: emit merged JSON (inject.after)
Skill->>Skill: if inject.after non-empty -> append & follow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Augment PR SummarySummary: Adds a TOML-based customization system for the BMad Builder skills so teams and individuals can inject additional context/resources without editing stock skill files. Changes:
Technical Notes: Resolver requires Python 3.11+ ( 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| def merge_menu(base: list[dict], override: list[dict]) -> list[dict]: | ||
| """Merge-by-code: matching codes replace; new codes append.""" | ||
| result_by_code: dict[str, dict] = {item["code"]: dict(item) for item in base} |
There was a problem hiding this comment.
merge_menu assumes every item in both base and override has a "code" key; a malformed customization TOML (or unexpected data) would raise KeyError and abort resolution. Consider validating items (or making _is_menu_array stricter) so bad input fails with a clear message instead of a crash.
Severity: medium
Other Locations
skills/bmad-bmb-setup/scripts/resolve-customization.py:71skills/bmad-module-builder/scripts/resolve-customization.py:71skills/bmad-workflow-builder/scripts/resolve-customization.py:71
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| result_by_code: dict[str, dict] = {item["code"]: dict(item) for item in base} | ||
| for item in override: | ||
| result_by_code[item["code"]] = dict(item) | ||
| return list(result_by_code.values()) |
There was a problem hiding this comment.
merge_menu returns list(result_by_code.values()), so the resulting order depends on dict insertion behavior and may not preserve the original menu ordering (and won’t reflect intended reordering when an override replaces an existing code). If menu order is user-visible, this can produce surprising ordering changes after customization merges.
Severity: low
Other Locations
skills/bmad-bmb-setup/scripts/resolve-customization.py:74skills/bmad-module-builder/scripts/resolve-customization.py:74skills/bmad-workflow-builder/scripts/resolve-customization.py:74
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| try: | ||
| with open(path, "rb") as f: | ||
| return tomllib.load(f) | ||
| except Exception as exc: |
There was a problem hiding this comment.
load_toml logs a warning and returns {} on parse errors, so an invalid customization file silently falls back to defaults and still exits successfully. Consider whether parse failures should be treated as hard errors (non-zero exit) so callers can reliably detect misconfiguration.
Severity: medium
Other Locations
skills/bmad-bmb-setup/scripts/resolve-customization.py:50skills/bmad-module-builder/scripts/resolve-customization.py:50skills/bmad-workflow-builder/scripts/resolve-customization.py:50
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
skills/bmad-bmb-setup/scripts/resolve-customization.py (1)
50-52: Narrow exception handling in TOML loadingCatching all
Exceptionmakes debugging harder and may mask non-I/O defects. Prefer targeted exception types for parse and file errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/bmad-bmb-setup/scripts/resolve-customization.py` around lines 50 - 52, Replace the broad "except Exception as exc" handler around the TOML load with targeted exceptions: catch the TOML parse error (e.g., toml.TomlDecodeError or the parser's specific exception class) and file I/O errors (FileNotFoundError and OSError) so only parse/file issues are handled; keep the same warning print to sys.stderr and the return {} for those cases, and let other exceptions propagate. Reference the existing except block that uses variables path and exc and the print(..., file=sys.stderr) call when making the change.skills/bmad-workflow-builder/scripts/resolve-customization.py (1)
50-52: Narrow the parse exception handling.Catching
Exceptionhere hides unrelated runtime faults and makes debugging harder. Limit this to TOML parse and file I/O errors.Diff suggestion
- except Exception as exc: + except (tomllib.TOMLDecodeError, OSError) as exc: print(f"warning: failed to parse {path}: {exc}", file=sys.stderr) return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/bmad-workflow-builder/scripts/resolve-customization.py` around lines 50 - 52, Replace the broad "except Exception as exc" handler with a narrow set of exceptions that reflect TOML parse and I/O errors (e.g. tomllib.TOMLDecodeError or toml.TomlDecodeError depending on which parser you use, plus FileNotFoundError and OSError), so the code only swallows parse/file errors; keep the same print/log line and return {} but preserve the original exception variable in the message (print(f"warning: failed to parse {path}: {exc}", file=sys.stderr)). Ensure any required parser-specific exception is imported or referenced correctly where the current parse call (the block handling "path"/parsing) occurs.skills/bmad-agent-builder/scripts/resolve-customization.py (1)
50-52: Narrow exception handling inload_toml.The broad
Exceptioncatch at line 50 can hide unexpected bugs and reduces debuggability. Catch only the expected exceptions from TOML parsing and file I/O:Proposed fix
- except Exception as exc: + except (tomllib.TOMLDecodeError, OSError) as exc: print(f"warning: failed to parse {path}: {exc}", file=sys.stderr) return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/bmad-agent-builder/scripts/resolve-customization.py` around lines 50 - 52, Narrow the broad exception in load_toml: replace the generic "except Exception as exc" that prints a warning for path with explicit catches for the expected errors (e.g., FileNotFoundError and OSError for file I/O, and the TOML parse error type used by your TOML library such as tomllib.TOMLDecodeError or toml.TomlDecodeError). Handle the parse error and I/O errors separately (log the same warning message and return {}), and let other unexpected exceptions propagate so they aren't silently swallowed; ensure the code references load_toml and the path variable so the correct block is updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/bmad-agent-builder/scripts/resolve-customization.py`:
- Around line 59-74: The helper _is_menu_array currently only checks the first
element for being a dict with a "code" key, which allows malformed later entries
to reach merge_menu and cause KeyError; update _is_menu_array to ensure every
element is a dict and contains the "code" key (e.g., all(isinstance(i, dict) and
"code" in i for i in value)), and harden merge_menu to not assume item["code"]
blindly by either skipping entries that lack a "code" or raising a clear
ValueError with context (referring to the merge_menu and _is_menu_array
functions so you modify both checks accordingly) so malformed menu items cannot
crash at runtime.
In `@skills/bmad-agent-builder/SKILL.md`:
- Line 13: The current behavior reads files from additional_resources without
validation; fix the loader that "read[s] each listed file" by normalizing and
constraining paths before opening: reject absolute paths and any path containing
"..", resolve each path against the repository root (or an explicit allowlist of
subdirectories) and ensure the resolved path starts with that root/allowlist
prefix; only then read the file, otherwise skip and log/throw a clear error.
Ensure the path-check logic is applied wherever additional_resources is
processed so no traversal or out-of-repo file reads occur.
- Line 9: Update the run commands that invoke resolve-customization.py to use an
explicit Python 3.11+ interpreter (e.g., python3 or python3.11) instead of plain
python; specifically change the command that runs "python
./scripts/resolve-customization.py bmad-agent-builder --key inject --key
additional_resources" so resolve-customization.py is executed with a Python
3.11+ interpreter to ensure tomllib is available.
In `@skills/bmad-bmb-setup/scripts/resolve-customization.py`:
- Around line 129-131: Validate and sanitize the skill_name argument before
using it to build file paths: in the argparse option that creates skill_name and
wherever skill_name is used to compose filenames (the occurrences referenced
around the "skill_name" add_argument and the later path usage), reject or
normalize values containing path separators or traversal patterns and enforce an
allowlist pattern (e.g. only alphanumerics, dashes, underscores) or use
os.path.basename/Path(name).name to strip directories; additionally, when
resolving the final Path for files under the base directory (e.g. the
_bmad/customizations target), resolve it and assert the resolved path is inside
the base directory (using Path.resolve() and Path.is_relative_to() or
os.path.commonpath) before opening/reading the TOML to prevent directory
traversal.
- Around line 69-74: merge_menu currently assumes every dict has a "code" key
and will raise if an entry is malformed; update merge_menu to safely handle
missing or non-string codes by using item.get("code") and skipping entries
without a valid code (or optionally logging them), ensuring you still build
result_by_code from valid base and override items and that override items
replace by code when present; reference the merge_menu function, the
result_by_code dict, and the loops over base and override when making the
change.
In `@skills/bmad-bmb-setup/SKILL.md`:
- Line 9: The documented resolver commands in the SKILL.md files call the
resolver with `python` but resolve-customization.py requires Python 3.11+ (uses
tomllib), so update the command invocations (e.g., the line running `python
./scripts/resolve-customization.py bmad-bmb-setup --key inject --key
additional_resources`) to use `python3` instead; apply the same change in the
other SKILL.md files referenced (skills/bmad-workflow-builder/SKILL.md,
skills/bmad-agent-builder/SKILL.md, skills/bmad-module-builder/SKILL.md) so
every occurrence of `python ./scripts/resolve-customization.py` becomes `python3
./scripts/resolve-customization.py`.
In `@skills/bmad-module-builder/scripts/resolve-customization.py`:
- Around line 59-66: The _is_menu_array helper currently only checks the first
list item which lets a later malformed entry raise KeyError; update
_is_menu_array to validate every element (use all(...) or an explicit loop)
ensuring the value is a non-empty list and each item is a dict that contains the
"code" key, and adjust any callers (the code that merges/iterates menus around
the existing checks) to rely on this stricter predicate so you never assume
later items have "code".
- Around line 43-53: The load_toml function currently swallows all parse errors
and returns an empty dict; change load_toml(path: Path, *, strict: bool = False)
to keep lenient behavior by default but when strict is True re-raise the
exception (or raise a RuntimeError) instead of returning {} so critical files
fail fast; then update the call that loads the shipped defaults (the place that
calls load_toml for the defaults file around where defaults are built/loaded) to
call load_toml(..., strict=True) while leaving team/user override loads using
the default lenient behavior.
In `@skills/bmad-module-builder/SKILL.md`:
- Around line 9-10: The run instructions in SKILL.md call the script with
"python ./scripts/resolve-customization.py" which may invoke a Python 2 or
system-default interpreter; update those invocations (the lines that run
resolve-customization.py and the duplicated lines at 46-47) to explicitly use
"python3" so the script is executed with Python 3, keeping the same flags (--key
inject --key additional_resources) and wording otherwise.
In `@skills/bmad-workflow-builder/scripts/resolve-customization.py`:
- Around line 59-66: The _is_menu_array predicate is too permissive because it
only inspects value[0], so later malformed entries without a "code" key can
cause KeyError in merge_menu; change _is_menu_array to ensure value is a
non-empty list and that every element is a dict containing the "code" key (e.g.,
use an all(...) check over the list) and similarly update any analogous
predicate around lines 69-74 to validate every item before treating it as a
[[menu]] array.
- Around line 173-176: The json.dump calls that write either result or merged
need a custom serializer for TOML temporal types; update the two json.dump
invocations in resolve-customization.py to pass a default handler that converts
datetime.date, datetime.time, and datetime.datetime to ISO strings (using
.isoformat()) and falls back to str() for any unexpected types, and ensure
datetime is imported so the handler can use isinstance checks against
datetime.date, datetime.time, and datetime.datetime; apply this change where
json.dump(result, ...) and json.dump(merged, ...) are called.
In `@skills/bmad-workflow-builder/SKILL.md`:
- Around line 9-10: Update the command in SKILL.md that runs the resolver script
so it invokes Python 3 explicitly (change occurrences of `python
./scripts/resolve-customization.py bmad-workflow-builder --key inject --key
additional_resources` to use `python3`) to ensure tomllib and Python 3.11+
requirements are met; update any other instances in SKILL.md that call `python`
for the same script to `python3`.
---
Nitpick comments:
In `@skills/bmad-agent-builder/scripts/resolve-customization.py`:
- Around line 50-52: Narrow the broad exception in load_toml: replace the
generic "except Exception as exc" that prints a warning for path with explicit
catches for the expected errors (e.g., FileNotFoundError and OSError for file
I/O, and the TOML parse error type used by your TOML library such as
tomllib.TOMLDecodeError or toml.TomlDecodeError). Handle the parse error and I/O
errors separately (log the same warning message and return {}), and let other
unexpected exceptions propagate so they aren't silently swallowed; ensure the
code references load_toml and the path variable so the correct block is updated.
In `@skills/bmad-bmb-setup/scripts/resolve-customization.py`:
- Around line 50-52: Replace the broad "except Exception as exc" handler around
the TOML load with targeted exceptions: catch the TOML parse error (e.g.,
toml.TomlDecodeError or the parser's specific exception class) and file I/O
errors (FileNotFoundError and OSError) so only parse/file issues are handled;
keep the same warning print to sys.stderr and the return {} for those cases, and
let other exceptions propagate. Reference the existing except block that uses
variables path and exc and the print(..., file=sys.stderr) call when making the
change.
In `@skills/bmad-workflow-builder/scripts/resolve-customization.py`:
- Around line 50-52: Replace the broad "except Exception as exc" handler with a
narrow set of exceptions that reflect TOML parse and I/O errors (e.g.
tomllib.TOMLDecodeError or toml.TomlDecodeError depending on which parser you
use, plus FileNotFoundError and OSError), so the code only swallows parse/file
errors; keep the same print/log line and return {} but preserve the original
exception variable in the message (print(f"warning: failed to parse {path}:
{exc}", file=sys.stderr)). Ensure any required parser-specific exception is
imported or referenced correctly where the current parse call (the block
handling "path"/parsing) occurs.
🪄 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: ec928a38-d70d-42f2-ab6e-5198e251ae9f
📒 Files selected for processing (12)
skills/bmad-agent-builder/SKILL.mdskills/bmad-agent-builder/customize.tomlskills/bmad-agent-builder/scripts/resolve-customization.pyskills/bmad-bmb-setup/SKILL.mdskills/bmad-bmb-setup/customize.tomlskills/bmad-bmb-setup/scripts/resolve-customization.pyskills/bmad-module-builder/SKILL.mdskills/bmad-module-builder/customize.tomlskills/bmad-module-builder/scripts/resolve-customization.pyskills/bmad-workflow-builder/SKILL.mdskills/bmad-workflow-builder/customize.tomlskills/bmad-workflow-builder/scripts/resolve-customization.py
Comment on lines
+129
to
+131
| "skill_name", | ||
| help="Skill identifier (e.g. bmad-agent-pm, bmad-product-brief)", | ||
| ) |
There was a problem hiding this comment.
Sanitize skill_name before composing filesystem paths
skill_name is interpolated directly into filenames. A crafted value like ../../foo can escape _bmad/customizations and read unintended TOML files.
Suggested fix
@@
import argparse
import json
-import os
+import os
+import re
@@
def main() -> None:
@@
args = parser.parse_args()
+ if not re.fullmatch(r"[A-Za-z0-9][A-Za-z0-9._-]*", args.skill_name):
+ print(f"error: invalid skill name: {args.skill_name!r}", file=sys.stderr)
+ sys.exit(2)
@@
customizations_dir = project_root / "_bmad" / "customizations"Also applies to: 159-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/bmad-bmb-setup/scripts/resolve-customization.py` around lines 129 -
131, Validate and sanitize the skill_name argument before using it to build file
paths: in the argparse option that creates skill_name and wherever skill_name is
used to compose filenames (the occurrences referenced around the "skill_name"
add_argument and the later path usage), reject or normalize values containing
path separators or traversal patterns and enforce an allowlist pattern (e.g.
only alphanumerics, dashes, underscores) or use os.path.basename/Path(name).name
to strip directories; additionally, when resolving the final Path for files
under the base directory (e.g. the _bmad/customizations target), resolve it and
assert the resolved path is inside the base directory (using Path.resolve() and
Path.is_relative_to() or os.path.commonpath) before opening/reading the TOML to
prevent directory traversal.
Comment on lines
+173
to
+176
| json.dump(result, sys.stdout, indent=2, ensure_ascii=False) | ||
| else: | ||
| json.dump(merged, sys.stdout, indent=2, ensure_ascii=False) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import json, datetime
try:
json.dumps({"when": datetime.date.today()})
except TypeError as e:
print("TypeError:", e)
PY
rg -n 'json\.dump\(' skills/bmad-workflow-builder/scripts/resolve-customization.pyRepository: bmad-code-org/bmad-builder
Length of output: 269
🏁 Script executed:
cat -n skills/bmad-workflow-builder/scripts/resolve-customization.py | head -50Repository: bmad-code-org/bmad-builder
Length of output: 1976
🏁 Script executed:
cat -n skills/bmad-workflow-builder/scripts/resolve-customization.py | sed -n '160,185p'Repository: bmad-code-org/bmad-builder
Length of output: 877
🏁 Script executed:
rg -n 'tomllib|import json' skills/bmad-workflow-builder/scripts/resolve-customization.py | head -10Repository: bmad-code-org/bmad-builder
Length of output: 140
🏁 Script executed:
rg -n 'json\.' skills/bmad-workflow-builder/scripts/resolve-customization.pyRepository: bmad-code-org/bmad-builder
Length of output: 213
🏁 Script executed:
cat -n skills/bmad-workflow-builder/scripts/resolve-customization.py | sed -n '100,145p'Repository: bmad-code-org/bmad-builder
Length of output: 1999
🏁 Script executed:
cat -n skills/bmad-workflow-builder/scripts/resolve-customization.py | sed -n '70,100p'Repository: bmad-code-org/bmad-builder
Length of output: 1427
Add JSON serializer for temporal types from TOML.
tomllib.load() can return date/time/datetime objects, which json.dump() cannot serialize. Add a custom default handler to prevent TypeError when temporal values appear in TOML.
Diff suggestion
+from datetime import date, datetime, time
@@
+def _json_default(value: Any) -> Any:
+ if isinstance(value, (date, datetime, time)):
+ return value.isoformat()
+ raise TypeError(f"Object of type {type(value).__name__} is not JSON serializable")
@@
- json.dump(result, sys.stdout, indent=2, ensure_ascii=False)
+ json.dump(result, sys.stdout, indent=2, ensure_ascii=False, default=_json_default)
else:
- json.dump(merged, sys.stdout, indent=2, ensure_ascii=False)
+ json.dump(merged, sys.stdout, indent=2, ensure_ascii=False, default=_json_default)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| json.dump(result, sys.stdout, indent=2, ensure_ascii=False) | |
| else: | |
| json.dump(merged, sys.stdout, indent=2, ensure_ascii=False) | |
| from datetime import date, datetime, time | |
| def _json_default(value: Any) -> Any: | |
| if isinstance(value, (date, datetime, time)): | |
| return value.isoformat() | |
| raise TypeError(f"Object of type {type(value).__name__} is not JSON serializable") | |
| json.dump(result, sys.stdout, indent=2, ensure_ascii=False, default=_json_default) | |
| else: | |
| json.dump(merged, sys.stdout, indent=2, ensure_ascii=False, default=_json_default) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/bmad-workflow-builder/scripts/resolve-customization.py` around lines
173 - 176, The json.dump calls that write either result or merged need a custom
serializer for TOML temporal types; update the two json.dump invocations in
resolve-customization.py to pass a default handler that converts datetime.date,
datetime.time, and datetime.datetime to ISO strings (using .isoformat()) and
falls back to str() for any unexpected types, and ensure datetime is imported so
the handler can use isinstance checks against datetime.date, datetime.time, and
datetime.datetime; apply this change where json.dump(result, ...) and
json.dump(merged, ...) are called.
Change except Exception to except (tomllib.TOMLDecodeError, OSError) for clearer failure signaling in user-facing tooling.
- Drop ./ prefix from script paths (use scripts/ not ./scripts/) - Use python3 instead of python for explicitness - Add Available Scripts listing to all SKILL.md files
- Fix merge_menu KeyError crash when menu items missing 'code' key - Fix _is_menu_array to check ALL elements, not just first - Remove unused import os from resolve-customization.py
- Add type: ignore[arg-type] to merge_menu call (Pylance narrowing limitation) - Reword inject.before in workflows: "prepend to active instructions and follow it" - Reword inject.after in workflows: "append to active instructions and follow it" - Make additional_resources lazy: note list but don't eagerly load
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
skills/bmad-module-builder/scripts/resolve-customization.py (1)
42-52:⚠️ Potential issue | 🟠 MajorFail fast for unreadable defaults TOML instead of silently degrading output.
At Line 42–Line 52, parse/I-O errors are always downgraded to warnings and
{}. At Line 154, this same lenient path is used for the shipped defaults file. Ifskills/bmad-module-builder/customize.tomlis broken, resolution should fail hard to surface a packaging defect.Suggested patch
-def load_toml(path: Path) -> dict[str, Any]: - """Return parsed TOML or empty dict if the file doesn't exist.""" +def load_toml(path: Path, *, strict: bool = False) -> dict[str, Any]: + """Return parsed TOML; optionally fail hard on parse/I-O errors.""" if not path.is_file(): return {} try: with open(path, "rb") as f: return tomllib.load(f) except (tomllib.TOMLDecodeError, OSError) as exc: + if strict: + raise RuntimeError(f"failed to load required TOML: {path}") from exc print(f"warning: failed to parse {path}: {exc}", file=sys.stderr) return {} @@ - defaults = load_toml(defaults_path) + defaults = load_toml(defaults_path, strict=True)Also applies to: 154-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/bmad-module-builder/scripts/resolve-customization.py` around lines 42 - 52, The current load_toml function swallows TOML parse/I-O errors and returns {}; change it to accept an optional parameter (e.g., fail_on_error: bool = False) and, when fail_on_error is True, re-raise the tomllib.TOMLDecodeError and OSError instead of printing a warning and returning {}; then update the call site that loads the shipped defaults (the place that currently calls load_toml for the packaged defaults) to call load_toml(fail_on_error=True) so a broken customize TOML surfaces as a hard failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/bmad-agent-builder/scripts/resolve-customization.py`:
- Around line 130-145: Validate and sanitize the CLI skill name
(args.skill_name) before using it to build override paths: ensure it contains
only allowed characters (e.g., alphanumerics, hyphen, underscore, dot), disallow
path separators or traversal tokens like '/' or '..', and reject/exit on invalid
values; then construct team/user override paths by joining the sanitized name to
a fixed base directory (derived from script_dir/skill_dir or a constant like
"_bmad/customizations") rather than interpolating the raw input. Update the code
around parser.add_argument("skill_name") and where args.skill_name is used to
select TOML files so it validates the value and only uses the sanitized version
to form override file paths.
- Around line 14-17: Update the usage examples in the top-of-file help text in
resolve-customization.py to call python3 instead of python (i.e., change "python
./scripts/resolve-customization.py" to "python3
./scripts/resolve-customization.py" in all three example lines) so the
documented invocation matches the file's Python 3.11+ requirement and the
SKILL.md guidance.
- Around line 30-39: The current find_project_root returns the first .git
ancestor which can be an inner repo/submodule and prevents finding a higher
_bmad directory; change the search so we prefer any _bmad directory above the
start and only fall back to a .git if no _bmad exists above it. Update
find_project_root to walk ancestors, track the first .git encountered (e.g.,
first_git variable) but continue walking to see if any (current /
"_bmad").is_dir() exists higher; if you find _bmad return it, otherwise return
the recorded first_git if present, else None.
In `@skills/bmad-module-builder/scripts/resolve-customization.py`:
- Around line 129-132: The CLI accepts a skill_name via
parser.add_argument("skill_name") but the code later unconditionally loads
defaults from the script's skill directory and separately selects team/user
files by the CLI skill_name, which can silently mix layers when they differ;
after parsing, validate that the provided skill_name matches the skill
directory/name used for defaults (or the expected skill identifier from the
defaults metadata) and fail fast with a clear error if they differ, and then use
the validated skill_name for both the defaults-loading logic and the team/user
file selection to ensure the same skill is used consistently (update the code
paths that load defaults and that select team/user overrides so they reference
the single validated skill_name).
- Around line 58-75: The _is_menu_array and merge_menu functions need stronger
validation of the menu item's "code" to avoid TypeError on non-hashable or
malformed values: update _is_menu_array to only return True when every item is a
dict that has a "code" key whose value is of an expected primitive type (e.g.,
str or int) rather than an arbitrary object/list; in merge_menu, before using
item["code"] as a dict key, validate the code's type (and/or attempt to hash it)
and if it's not a valid primitive/hashable value, emit the existing warning and
skip that item (do not attempt to insert into result_by_code), and optionally
wrap the dict-key assignment in a try/except TypeError to guard against
unexpected cases. Ensure you reference _is_menu_array and merge_menu when making
these checks.
---
Duplicate comments:
In `@skills/bmad-module-builder/scripts/resolve-customization.py`:
- Around line 42-52: The current load_toml function swallows TOML parse/I-O
errors and returns {}; change it to accept an optional parameter (e.g.,
fail_on_error: bool = False) and, when fail_on_error is True, re-raise the
tomllib.TOMLDecodeError and OSError instead of printing a warning and returning
{}; then update the call site that loads the shipped defaults (the place that
currently calls load_toml for the packaged defaults) to call
load_toml(fail_on_error=True) so a broken customize TOML surfaces as a hard
failure.
🪄 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: acf6deff-ab26-4afb-904d-985d1dcdf6a8
📒 Files selected for processing (8)
skills/bmad-agent-builder/SKILL.mdskills/bmad-agent-builder/scripts/resolve-customization.pyskills/bmad-bmb-setup/SKILL.mdskills/bmad-bmb-setup/scripts/resolve-customization.pyskills/bmad-module-builder/SKILL.mdskills/bmad-module-builder/scripts/resolve-customization.pyskills/bmad-workflow-builder/SKILL.mdskills/bmad-workflow-builder/scripts/resolve-customization.py
✅ Files skipped from review due to trivial changes (4)
- skills/bmad-agent-builder/SKILL.md
- skills/bmad-workflow-builder/SKILL.md
- skills/bmad-workflow-builder/scripts/resolve-customization.py
- skills/bmad-bmb-setup/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/bmad-module-builder/SKILL.md
- skills/bmad-bmb-setup/scripts/resolve-customization.py
Comment on lines
+14
to
+17
| Usage: | ||
| python ./scripts/resolve-customization.py {skill-name} | ||
| python ./scripts/resolve-customization.py {skill-name} --key persona | ||
| python ./scripts/resolve-customization.py {skill-name} --key persona.displayName --key inject |
There was a problem hiding this comment.
Use python3 in the usage examples.
The file header requires Python 3.11+, and skills/bmad-agent-builder/SKILL.md:8-18 already documents python3. Keeping python here invites copy/paste failures on hosts where python is not that interpreter.
📝 Proposed fix
- python ./scripts/resolve-customization.py {skill-name}
- python ./scripts/resolve-customization.py {skill-name} --key persona
- python ./scripts/resolve-customization.py {skill-name} --key persona.displayName --key inject
+ python3 ./scripts/resolve-customization.py {skill-name}
+ python3 ./scripts/resolve-customization.py {skill-name} --key persona
+ python3 ./scripts/resolve-customization.py {skill-name} --key persona.displayName --key inject📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Usage: | |
| python ./scripts/resolve-customization.py {skill-name} | |
| python ./scripts/resolve-customization.py {skill-name} --key persona | |
| python ./scripts/resolve-customization.py {skill-name} --key persona.displayName --key inject | |
| Usage: | |
| python3 ./scripts/resolve-customization.py {skill-name} | |
| python3 ./scripts/resolve-customization.py {skill-name} --key persona | |
| python3 ./scripts/resolve-customization.py {skill-name} --key persona.displayName --key inject |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/bmad-agent-builder/scripts/resolve-customization.py` around lines 14 -
17, Update the usage examples in the top-of-file help text in
resolve-customization.py to call python3 instead of python (i.e., change "python
./scripts/resolve-customization.py" to "python3
./scripts/resolve-customization.py" in all three example lines) so the
documented invocation matches the file's Python 3.11+ requirement and the
SKILL.md guidance.
Comment on lines
+30
to
+39
| def find_project_root(start: Path) -> Path | None: | ||
| """Walk up from *start* looking for a directory containing ``_bmad/`` or ``.git``.""" | ||
| current = start.resolve() | ||
| while True: | ||
| if (current / "_bmad").is_dir() or (current / ".git").exists(): | ||
| return current | ||
| parent = current.parent | ||
| if parent == current: | ||
| return None | ||
| current = parent |
There was a problem hiding this comment.
Prefer a higher _bmad/ over the first .git ancestor.
If the resolver is run inside a nested repo or submodule, Line 34 returns that inner .git directory and stops before checking higher ancestors. The real {project-root}/_bmad/customizations folder is then missed, so team/user overrides never load.
🔎 Proposed fix
def find_project_root(start: Path) -> Path | None:
- """Walk up from *start* looking for a directory containing ``_bmad/`` or ``.git``."""
+ """Walk up from *start*, preferring ``_bmad/`` and falling back to the nearest ``.git``."""
current = start.resolve()
+ git_fallback: Path | None = None
while True:
- if (current / "_bmad").is_dir() or (current / ".git").exists():
+ if (current / "_bmad").is_dir():
return current
+ if git_fallback is None and (current / ".git").exists():
+ git_fallback = current
parent = current.parent
if parent == current:
- return None
+ return git_fallback
current = parent🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/bmad-agent-builder/scripts/resolve-customization.py` around lines 30 -
39, The current find_project_root returns the first .git ancestor which can be
an inner repo/submodule and prevents finding a higher _bmad directory; change
the search so we prefer any _bmad directory above the start and only fall back
to a .git if no _bmad exists above it. Update find_project_root to walk
ancestors, track the first .git encountered (e.g., first_git variable) but
continue walking to see if any (current / "_bmad").is_dir() exists higher; if
you find _bmad return it, otherwise return the recorded first_git if present,
else None.
Comment on lines
+130
to
+145
| "skill_name", | ||
| help="Skill identifier (e.g. bmad-agent-pm, bmad-product-brief)", | ||
| ) | ||
| parser.add_argument( | ||
| "--key", | ||
| action="append", | ||
| dest="keys", | ||
| metavar="FIELD", | ||
| help="Dotted field path to resolve (repeatable). Omit for full dump.", | ||
| ) | ||
| args = parser.parse_args() | ||
|
|
||
| # Locate the skill's own customize.toml (one level up from scripts/) | ||
| script_dir = Path(__file__).resolve().parent | ||
| skill_dir = script_dir.parent | ||
| defaults_path = skill_dir / "customize.toml" |
There was a problem hiding this comment.
Validate skill_name before using it in override paths.
defaults_path always comes from this script's parent skill directory, but Lines 160-161 use the raw CLI argument to choose the team/user TOML files. A typo in SKILL.md or a name containing path separators can silently mix another skill's overrides—or escape _bmad/customizations entirely—with this skill's defaults.
🛡️ Proposed fix
script_dir = Path(__file__).resolve().parent
skill_dir = script_dir.parent
+ expected_skill_name = skill_dir.name
+ if args.skill_name != expected_skill_name:
+ parser.error(
+ f"this script resolves '{expected_skill_name}', not '{args.skill_name}'"
+ )
defaults_path = skill_dir / "customize.toml"Also applies to: 159-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/bmad-agent-builder/scripts/resolve-customization.py` around lines 130
- 145, Validate and sanitize the CLI skill name (args.skill_name) before using
it to build override paths: ensure it contains only allowed characters (e.g.,
alphanumerics, hyphen, underscore, dot), disallow path separators or traversal
tokens like '/' or '..', and reject/exit on invalid values; then construct
team/user override paths by joining the sanitized name to a fixed base directory
(derived from script_dir/skill_dir or a constant like "_bmad/customizations")
rather than interpolating the raw input. Update the code around
parser.add_argument("skill_name") and where args.skill_name is used to select
TOML files so it validates the value and only uses the sanitized version to form
override file paths.
Comment on lines
+58
to
+75
| def _is_menu_array(value: Any) -> bool: | ||
| """True when *value* is a non-empty list where ALL items are dicts with a ``code`` key.""" | ||
| return ( | ||
| isinstance(value, list) | ||
| and len(value) > 0 | ||
| and all(isinstance(item, dict) and "code" in item for item in value) | ||
| ) | ||
|
|
||
|
|
||
| def merge_menu(base: list[dict], override: list[dict]) -> list[dict]: | ||
| """Merge-by-code: matching codes replace; new codes append.""" | ||
| result_by_code: dict[str, dict] = {item["code"]: dict(item) for item in base if "code" in item} | ||
| for item in override: | ||
| if "code" not in item: | ||
| print(f"warning: menu item missing 'code' key, skipping: {item}", file=sys.stderr) | ||
| continue | ||
| result_by_code[item["code"]] = dict(item) | ||
| return list(result_by_code.values()) |
There was a problem hiding this comment.
Validate menu.code type to prevent merge-time crashes on malformed input.
_is_menu_array (Line 58–Line 64) only checks key presence, and merge_menu (Line 67–Line 75) uses item["code"] as dict key. A non-hashable code (e.g., list/object from malformed TOML) can still raise TypeError during merge.
Suggested hardening patch
def _is_menu_array(value: Any) -> bool:
- """True when *value* is a non-empty list where ALL items are dicts with a ``code`` key."""
+ """True when *value* is a non-empty list of dicts with string ``code`` keys."""
return (
isinstance(value, list)
and len(value) > 0
- and all(isinstance(item, dict) and "code" in item for item in value)
+ and all(isinstance(item, dict) and isinstance(item.get("code"), str) for item in value)
)
def merge_menu(base: list[dict], override: list[dict]) -> list[dict]:
"""Merge-by-code: matching codes replace; new codes append."""
- result_by_code: dict[str, dict] = {item["code"]: dict(item) for item in base if "code" in item}
+ result_by_code: dict[str, dict] = {}
+ for item in base:
+ code = item.get("code")
+ if isinstance(code, str):
+ result_by_code[code] = dict(item)
+ else:
+ print(f"warning: base menu item missing/invalid 'code', skipping: {item}", file=sys.stderr)
for item in override:
- if "code" not in item:
- print(f"warning: menu item missing 'code' key, skipping: {item}", file=sys.stderr)
+ code = item.get("code")
+ if not isinstance(code, str):
+ print(f"warning: menu item missing/invalid 'code', skipping: {item}", file=sys.stderr)
continue
- result_by_code[item["code"]] = dict(item)
+ result_by_code[code] = dict(item)
return list(result_by_code.values())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/bmad-module-builder/scripts/resolve-customization.py` around lines 58
- 75, The _is_menu_array and merge_menu functions need stronger validation of
the menu item's "code" to avoid TypeError on non-hashable or malformed values:
update _is_menu_array to only return True when every item is a dict that has a
"code" key whose value is of an expected primitive type (e.g., str or int)
rather than an arbitrary object/list; in merge_menu, before using item["code"]
as a dict key, validate the code's type (and/or attempt to hash it) and if it's
not a valid primitive/hashable value, emit the existing warning and skip that
item (do not attempt to insert into result_by_code), and optionally wrap the
dict-key assignment in a try/except TypeError to guard against unexpected cases.
Ensure you reference _is_menu_array and merge_menu when making these checks.
Comment on lines
+129
to
+132
| parser.add_argument( | ||
| "skill_name", | ||
| help="Skill identifier (e.g. bmad-agent-pm, bmad-product-brief)", | ||
| ) |
There was a problem hiding this comment.
Guard against mismatched skill_name to avoid cross-skill layer mixing.
Defaults are always loaded from this skill directory (Line 145–Line 146), but team/user files are selected by CLI skill_name (Line 160–Line 161). A wrong argument silently mixes unrelated layers.
Suggested patch
args = parser.parse_args()
@@
script_dir = Path(__file__).resolve().parent
skill_dir = script_dir.parent
+ expected_skill = skill_dir.name
+ if args.skill_name != expected_skill:
+ parser.error(
+ f"skill_name '{args.skill_name}' does not match this resolver's skill '{expected_skill}'"
+ )
defaults_path = skill_dir / "customize.toml"Also applies to: 145-146, 160-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/bmad-module-builder/scripts/resolve-customization.py` around lines 129
- 132, The CLI accepts a skill_name via parser.add_argument("skill_name") but
the code later unconditionally loads defaults from the script's skill directory
and separately selects team/user files by the CLI skill_name, which can silently
mix layers when they differ; after parsing, validate that the provided
skill_name matches the skill directory/name used for defaults (or the expected
skill identifier from the defaults metadata) and fail fast with a clear error if
they differ, and then use the validated skill_name for both the defaults-loading
logic and the team/user file selection to ensure the same skill is used
consistently (update the code paths that load defaults and that select team/user
overrides so they reference the single validated skill_name).
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
1 participant
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.
Summary
customize.tomlwith stock customization fields (injectbefore/after,additional_resources) to all 4 builder skillsresolve-customization.pyscript in each skill'sscripts/directory for three-layer TOML merge (user > team > defaults)Part of the cross-module skill customization initiative. See bmad-code-org/BMAD-METHOD#2262 for the core implementation.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation