Skip to content

Commit a677787

Browse files
committed
fix: address twenty-fourth round of Copilot PR review feedback
- Use yaml.safe_load for frontmatter parsing in resolve_content instead of CommandRegistrar.parse_frontmatter which uses naive find('---',3); strip strategy key from final frontmatter to prevent leaking internal composition directives into rendered agent command files - Filter _reconcile_skills to specific commands: use _FilteredManifest wrapper so only the commands being reconciled get their skills updated, preventing accidental overwrites of other commands' skills that may be owned by higher-priority presets
1 parent 0d2dd42 commit a677787

File tree

1 file changed

+71
-33
lines changed

1 file changed

+71
-33
lines changed

src/specify_cli/presets.py

Lines changed: 71 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -841,6 +841,27 @@ def _register_for_non_skill_agents(
841841
commands, source_id, source_dir, self.project_root
842842
)
843843

844+
class _FilteredManifest:
845+
"""Wrapper that exposes only selected command templates from a manifest.
846+
847+
Used by _reconcile_skills to avoid overwriting skills for commands
848+
that aren't being reconciled.
849+
"""
850+
851+
def __init__(self, manifest: "PresetManifest", cmd_names: set):
852+
self._manifest = manifest
853+
self._cmd_names = cmd_names
854+
855+
def __getattr__(self, name: str):
856+
return getattr(self._manifest, name)
857+
858+
@property
859+
def templates(self) -> List[Dict[str, Any]]:
860+
return [
861+
t for t in self._manifest.templates
862+
if t.get("name") in self._cmd_names
863+
]
864+
844865
def _reconcile_skills(self, command_names: List[str]) -> None:
845866
"""Re-register skills for commands whose winning layer changed.
846867
@@ -857,14 +878,16 @@ def _reconcile_skills(self, command_names: List[str]) -> None:
857878
resolver = PresetResolver(self.project_root)
858879
skills_dir = self._get_skills_dir()
859880

881+
# Group command names by winning preset to batch _register_skills calls
882+
# while only registering skills for the specific commands being reconciled.
883+
preset_cmds: Dict[str, List[str]] = {}
884+
860885
for cmd_name in command_names:
861886
layers = resolver.collect_all_layers(cmd_name, "command")
862887
if not layers:
863888
continue
864889

865890
# Ensure skill directory exists so _register_skills can write to it.
866-
# After _unregister_skills removes a skill dir, this re-creates it
867-
# so the next winning preset's skill content can be registered.
868891
if skills_dir:
869892
skill_name, _ = self._skill_names_for_command(cmd_name)
870893
skill_subdir = skills_dir / skill_name
@@ -876,15 +899,25 @@ def _reconcile_skills(self, command_names: List[str]) -> None:
876899
for pack_id, _meta in PresetRegistry(self.presets_dir).list_by_priority():
877900
pack_dir = self.presets_dir / pack_id
878901
if top_path.is_relative_to(pack_dir):
879-
manifest_path = pack_dir / "preset.yml"
880-
if manifest_path.exists():
881-
try:
882-
manifest = PresetManifest(manifest_path)
883-
except PresetValidationError:
884-
break
885-
self._register_skills(manifest, pack_dir)
902+
preset_cmds.setdefault(pack_id, []).append(cmd_name)
886903
break
887904

905+
# Register skills only for the specific commands being reconciled,
906+
# not all commands in each winning preset's manifest.
907+
for pack_id, cmds in preset_cmds.items():
908+
pack_dir = self.presets_dir / pack_id
909+
manifest_path = pack_dir / "preset.yml"
910+
if not manifest_path.exists():
911+
continue
912+
try:
913+
manifest = PresetManifest(manifest_path)
914+
except PresetValidationError:
915+
continue
916+
# Filter manifest to only the commands being reconciled
917+
cmds_set = set(cmds)
918+
filtered_manifest = self._FilteredManifest(manifest, cmds_set)
919+
self._register_skills(filtered_manifest, pack_dir)
920+
888921
def _get_skills_dir(self) -> Optional[Path]:
889922
"""Return the active skills directory for preset skill overrides.
890923
@@ -2755,32 +2788,37 @@ def _split_frontmatter(text: str) -> tuple:
27552788
content = layer_content.replace(placeholder, content)
27562789

27572790
# Reattach the highest-priority frontmatter for commands,
2758-
# inheriting scripts/agent_scripts from the base if missing.
2791+
# inheriting scripts/agent_scripts from the base if missing
2792+
# and stripping the strategy key (internal-only, not for agent output).
27592793
if is_command and top_frontmatter_text:
2760-
if base_frontmatter_text and base_frontmatter_text != top_frontmatter_text:
2761-
# Parse both frontmatters to check for missing keys
2794+
def _parse_fm_yaml(fm_block: str) -> dict:
2795+
"""Parse YAML from a frontmatter block (with --- fences)."""
2796+
lines = fm_block.splitlines()
2797+
# Strip opening/closing --- fences
2798+
yaml_lines = [line for line in lines if line.strip() != "---"]
27622799
try:
2763-
from .agents import CommandRegistrar
2764-
_, _ = CommandRegistrar.parse_frontmatter("placeholder") # ensure import
2765-
base_fm, _ = CommandRegistrar.parse_frontmatter(
2766-
base_frontmatter_text + "\nbody"
2767-
)
2768-
top_fm, _ = CommandRegistrar.parse_frontmatter(
2769-
top_frontmatter_text + "\nbody"
2770-
)
2771-
inherited = False
2772-
for key in ("scripts", "agent_scripts"):
2773-
if key not in top_fm and key in base_fm:
2774-
top_fm[key] = base_fm[key]
2775-
inherited = True
2776-
if inherited:
2777-
top_frontmatter_text = (
2778-
"---\n"
2779-
+ yaml.safe_dump(top_fm, sort_keys=False).strip()
2780-
+ "\n---"
2781-
)
2782-
except Exception:
2783-
pass # best-effort; fall back to top frontmatter as-is
2800+
return yaml.safe_load("\n".join(yaml_lines)) or {}
2801+
except yaml.YAMLError:
2802+
return {}
2803+
2804+
top_fm = _parse_fm_yaml(top_frontmatter_text)
2805+
2806+
# Inherit scripts/agent_scripts from base frontmatter if missing
2807+
if base_frontmatter_text and base_frontmatter_text != top_frontmatter_text:
2808+
base_fm = _parse_fm_yaml(base_frontmatter_text)
2809+
for key in ("scripts", "agent_scripts"):
2810+
if key not in top_fm and key in base_fm:
2811+
top_fm[key] = base_fm[key]
2812+
2813+
# Strip strategy key — it's an internal composition directive,
2814+
# not meant for rendered agent command files
2815+
top_fm.pop("strategy", None)
2816+
2817+
top_frontmatter_text = (
2818+
"---\n"
2819+
+ yaml.safe_dump(top_fm, sort_keys=False).strip()
2820+
+ "\n---"
2821+
)
27842822
content = top_frontmatter_text + "\n\n" + content
27852823

27862824
return content

0 commit comments

Comments
 (0)