fix(skills): address PR review feedback on resolver and installer
Fixes four issues flagged by CodeRabbit on PR #2282: - SKILL.md conventions now define {skill-root} (used in the resolver command on line 16 but previously undefined alongside {project-root} and {skill-name}). - resolve_customization.py: missing or unparsable customize.yaml now exits non-zero with an error instead of silently returning {}, so the SKILL.md fallback path correctly kicks in on bad --skill paths. - resolve_customization.py: non-agent top-level keys (e.g. workflow:, config:) now deep-merge per the documented "other tables: deep merge" rule. Previously override replaced these wholesale, losing unrelated fields the user didn't touch. - installer.js _installSharedScripts: wipe _bmad/scripts/ before copy so files removed or renamed in src/scripts (e.g. the resolve-customization.js -> resolve_customization.py rename) don't linger and get tracked as installed. Also fail fast if src/scripts/ is missing.
This commit is contained in:
parent
6a5b814881
commit
d4466cc341
|
|
@ -6,6 +6,7 @@ description: Strategic business analyst and requirements expert. Use when the us
|
|||
## Conventions
|
||||
|
||||
- Bare paths (e.g. `references/guide.md`) resolve from the skill root.
|
||||
- `{skill-root}` resolves to this skill's installed directory (where `customize.yaml` lives).
|
||||
- `{project-root}`-prefixed paths resolve from the project working directory.
|
||||
- `{skill-name}` resolves to the skill directory's basename.
|
||||
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ description: Technical documentation specialist and knowledge curator. Use when
|
|||
## Conventions
|
||||
|
||||
- Bare paths (e.g. `references/guide.md`) resolve from the skill root.
|
||||
- `{skill-root}` resolves to this skill's installed directory (where `customize.yaml` lives).
|
||||
- `{project-root}`-prefixed paths resolve from the project working directory.
|
||||
- `{skill-name}` resolves to the skill directory's basename.
|
||||
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ description: Product manager for PRD creation and requirements discovery. Use wh
|
|||
## Conventions
|
||||
|
||||
- Bare paths (e.g. `references/guide.md`) resolve from the skill root.
|
||||
- `{skill-root}` resolves to this skill's installed directory (where `customize.yaml` lives).
|
||||
- `{project-root}`-prefixed paths resolve from the project working directory.
|
||||
- `{skill-name}` resolves to the skill directory's basename.
|
||||
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ description: UX designer and UI specialist. Use when the user asks to talk to Sa
|
|||
## Conventions
|
||||
|
||||
- Bare paths (e.g. `references/guide.md`) resolve from the skill root.
|
||||
- `{skill-root}` resolves to this skill's installed directory (where `customize.yaml` lives).
|
||||
- `{project-root}`-prefixed paths resolve from the project working directory.
|
||||
- `{skill-name}` resolves to the skill directory's basename.
|
||||
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ description: System architect and technical design leader. Use when the user ask
|
|||
## Conventions
|
||||
|
||||
- Bare paths (e.g. `references/guide.md`) resolve from the skill root.
|
||||
- `{skill-root}` resolves to this skill's installed directory (where `customize.yaml` lives).
|
||||
- `{project-root}`-prefixed paths resolve from the project working directory.
|
||||
- `{skill-name}` resolves to the skill directory's basename.
|
||||
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ description: Senior software engineer for story execution and code implementatio
|
|||
## Conventions
|
||||
|
||||
- Bare paths (e.g. `references/guide.md`) resolve from the skill root.
|
||||
- `{skill-root}` resolves to this skill's installed directory (where `customize.yaml` lives).
|
||||
- `{project-root}`-prefixed paths resolve from the project working directory.
|
||||
- `{skill-name}` resolves to the skill directory's basename.
|
||||
|
||||
|
|
|
|||
|
|
@ -64,15 +64,26 @@ def find_project_root(start: Path):
|
|||
current = parent
|
||||
|
||||
|
||||
def load_yaml(file_path: Path) -> dict:
|
||||
def load_yaml(file_path: Path, required: bool = False) -> dict:
|
||||
if not file_path.exists():
|
||||
if required:
|
||||
sys.stderr.write(f"error: required customization file not found: {file_path}\n")
|
||||
sys.exit(1)
|
||||
return {}
|
||||
try:
|
||||
with file_path.open("r", encoding="utf-8") as f:
|
||||
parsed = yaml.safe_load(f)
|
||||
return parsed if isinstance(parsed, dict) else {}
|
||||
if not isinstance(parsed, dict):
|
||||
if required:
|
||||
sys.stderr.write(f"error: {file_path} did not parse to a mapping\n")
|
||||
sys.exit(1)
|
||||
return {}
|
||||
return parsed
|
||||
except Exception as error:
|
||||
sys.stderr.write(f"warning: failed to parse {file_path}: {error}\n")
|
||||
level = "error" if required else "warning"
|
||||
sys.stderr.write(f"{level}: failed to parse {file_path}: {error}\n")
|
||||
if required:
|
||||
sys.exit(1)
|
||||
return {}
|
||||
|
||||
|
||||
|
|
@ -129,8 +140,10 @@ def deep_merge(base, override):
|
|||
def merge_agent_block(base: dict, override: dict) -> dict:
|
||||
"""Apply v6.1-compatible per-field merge semantics to the `agent` block,
|
||||
then deep-merge everything else normally."""
|
||||
base_agent = (base or {}).get("agent") or {}
|
||||
over_agent = (override or {}).get("agent") or {}
|
||||
base_obj = base if isinstance(base, dict) else {}
|
||||
override_obj = override if isinstance(override, dict) else {}
|
||||
base_agent = base_obj.get("agent") or {}
|
||||
over_agent = override_obj.get("agent") or {}
|
||||
|
||||
merged_agent = dict(base_agent)
|
||||
|
||||
|
|
@ -163,7 +176,12 @@ def merge_agent_block(base: dict, override: dict) -> dict:
|
|||
else:
|
||||
merged_agent[key] = over_val
|
||||
|
||||
return {**(base or {}), **(override or {}), "agent": merged_agent}
|
||||
# Deep-merge all non-agent top-level keys so tables like `workflow:` or
|
||||
# `config:` follow the documented `other tables: deep merge` rule. Then
|
||||
# overlay the specially-merged agent block.
|
||||
merged = deep_merge(base_obj, override_obj)
|
||||
merged["agent"] = merged_agent
|
||||
return merged
|
||||
|
||||
|
||||
def extract_key(data, dotted_key: str):
|
||||
|
|
@ -196,9 +214,7 @@ def main():
|
|||
skill_name = skill_dir.name
|
||||
defaults_path = skill_dir / "customize.yaml"
|
||||
|
||||
defaults = load_yaml(defaults_path)
|
||||
if not defaults:
|
||||
sys.stderr.write(f"warning: no defaults found at {defaults_path}\n")
|
||||
defaults = load_yaml(defaults_path, required=True)
|
||||
|
||||
project_root = find_project_root(Path.cwd()) or find_project_root(skill_dir)
|
||||
|
||||
|
|
|
|||
|
|
@ -568,17 +568,23 @@ class Installer {
|
|||
}
|
||||
|
||||
/**
|
||||
* Recursively copy src/scripts/* → _bmad/scripts/ so shared Python
|
||||
* scripts (e.g. resolve_customization.py) are available at install time.
|
||||
* Also seeds _bmad/custom/.gitignore on fresh installs so *.user.yaml
|
||||
* overrides stay out of version control by default.
|
||||
* Sync src/scripts/* → _bmad/scripts/ so shared Python scripts
|
||||
* (e.g. resolve_customization.py) are available at install time.
|
||||
* Wipes the destination first so files removed or renamed in source
|
||||
* (e.g. resolve-customization.js → resolve_customization.py) don't
|
||||
* linger and get recorded as installed. Also seeds _bmad/custom/.gitignore
|
||||
* on fresh installs so *.user.yaml overrides stay out of version control.
|
||||
*/
|
||||
async _installSharedScripts(paths) {
|
||||
const srcScriptsDir = path.join(paths.srcDir, 'src', 'scripts');
|
||||
if (await fs.pathExists(srcScriptsDir)) {
|
||||
if (!(await fs.pathExists(srcScriptsDir))) {
|
||||
throw new Error(`Shared scripts source directory not found: ${srcScriptsDir}`);
|
||||
}
|
||||
|
||||
await fs.remove(paths.scriptsDir);
|
||||
await fs.ensureDir(paths.scriptsDir);
|
||||
await fs.copy(srcScriptsDir, paths.scriptsDir, { overwrite: true });
|
||||
await this._trackFilesRecursive(paths.scriptsDir);
|
||||
}
|
||||
|
||||
const customGitignore = path.join(paths.customDir, '.gitignore');
|
||||
if (!(await fs.pathExists(customGitignore))) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue