diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 39235bf198..a90df4e871 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1330,19 +1330,20 @@ def _unregister_extension_skills( if not skill_md.is_file(): continue try: - import yaml as _yaml + from ..agents import CommandRegistrar as _Registrar raw = skill_md.read_text(encoding="utf-8") - source = "" - if raw.startswith("---"): - parts = raw.split("---", 2) - if len(parts) >= 3: - fm = _yaml.safe_load(parts[1]) or {} - source = ( - fm.get("metadata", {}).get("source", "") - if isinstance(fm, dict) - else "" - ) + # Parse on the ``---`` delimiter *line*, not any ``---`` + # substring: a description containing ``---`` would trip a + # raw ``split("---", 2)`` and hide metadata.source, so this + # extension's own skill would look unrelated and be left + # orphaned. Mirrors the #3590 parse_frontmatter fix. + fm, _ = _Registrar.parse_frontmatter(raw) + source = ( + fm.get("metadata", {}).get("source", "") + if isinstance(fm, dict) + else "" + ) if source != f"extension:{extension_id}": continue except (OSError, UnicodeDecodeError, Exception): @@ -1386,19 +1387,20 @@ def _unregister_extension_skills( if not skill_md.is_file(): continue try: - import yaml as _yaml + from ..agents import CommandRegistrar as _Registrar raw = skill_md.read_text(encoding="utf-8") - source = "" - if raw.startswith("---"): - parts = raw.split("---", 2) - if len(parts) >= 3: - fm = _yaml.safe_load(parts[1]) or {} - source = ( - fm.get("metadata", {}).get("source", "") - if isinstance(fm, dict) - else "" - ) + # Parse on the ``---`` delimiter *line*, not any ``---`` + # substring: a description containing ``---`` would trip + # a raw ``split("---", 2)`` and hide metadata.source, so + # this extension's own skill would look unrelated and be + # left orphaned. Mirrors the #3590 parse_frontmatter fix. + fm, _ = _Registrar.parse_frontmatter(raw) + source = ( + fm.get("metadata", {}).get("source", "") + if isinstance(fm, dict) + else "" + ) # Only remove skills explicitly created by this extension if source != f"extension:{extension_id}": continue diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 799629b076..bef7601377 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -163,6 +163,58 @@ def _create_unicode_extension_dir(temp_dir: Path, ext_id: str = "uni-ext") -> Pa return ext_dir +def _create_dashed_description_extension_dir( + temp_dir: Path, ext_id: str = "dash-ext" +) -> Path: + """Create an extension whose command description contains a ``---`` run. + + A ``---`` inside the description survives into the generated SKILL.md + frontmatter and exercises the delimiter-line parsing used when reading + metadata.source back during removal (regression guard for the + split("---", 2) substring bug, mirroring #3590). + """ + ext_dir = temp_dir / ext_id + ext_dir.mkdir() + description = "Separate sections with --- markers" + + manifest_data = { + "schema_version": "1.0", + "extension": { + "id": ext_id, + "name": "Dashed Extension", + "version": "1.0.0", + "description": description, + }, + "requires": {"speckit_version": ">=0.1.0"}, + "provides": { + "commands": [ + { + "name": f"speckit.{ext_id}.hello", + "file": "commands/hello.md", + "description": description, + }, + ] + }, + } + + with open(ext_dir / "extension.yml", "w", encoding="utf-8") as f: + yaml.safe_dump(manifest_data, f, allow_unicode=True) + + commands_dir = ext_dir / "commands" + commands_dir.mkdir() + (commands_dir / "hello.md").write_text( + "---\n" + f'description: "{description}"\n' + "---\n" + "\n" + "# Hello\n" + "\n" + "Body.\n", + encoding="utf-8", + ) + return ext_dir + + def _can_create_symlink(temp_dir: Path) -> bool: """Return True when the current platform/user can create file symlinks.""" target = temp_dir / "symlink-target.txt" @@ -1658,6 +1710,65 @@ def test_skills_removed_on_extension_remove(self, skills_project, extension_dir) assert not (skills_dir / "speckit-test-ext-hello").exists() assert not (skills_dir / "speckit-test-ext-world").exists() + def test_skills_removed_when_description_contains_dashes( + self, skills_project, temp_dir + ): + """A ``---`` in the command description must not orphan the skill dir. + + The removal safety check reads metadata.source back from the generated + SKILL.md. A raw ``split("---", 2)`` stopped at the ``---`` embedded in + the description, so metadata.source parsed empty, the skill looked + unrelated, and its directory was left behind. Regression guard for the + delimiter-line fix (mirrors #3590). + """ + project_dir, skills_dir = skills_project + ext_dir = _create_dashed_description_extension_dir(temp_dir) + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + ext_dir, "0.1.0", register_commands=False + ) + + skill_dir = skills_dir / "speckit-dash-ext-hello" + skill_md = skill_dir / "SKILL.md" + assert skill_md.exists() + # The dashed description must have survived into the frontmatter. + assert "--- markers" in skill_md.read_text(encoding="utf-8") + + result = manager.remove(manifest.id, keep_config=False) + assert result is True + + # The extension's own skill must be recognised and removed, not orphaned. + assert not skill_dir.exists() + + def test_skills_removed_with_dashes_via_fallback_scan( + self, skills_project, temp_dir + ): + """Same ``---`` guard, but exercised through the fallback scan branch. + + The fast path resolves the skills dir from init-options; the fallback + branch scans every candidate agent dir when that resolution returns + None, and it re-reads metadata.source with an independently duplicated + parser. Deleting init-options.json after install forces removal down + the fallback path so a substring-split regression there is caught too. + """ + project_dir, skills_dir = skills_project + ext_dir = _create_dashed_description_extension_dir(temp_dir) + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory( + ext_dir, "0.1.0", register_commands=False + ) + + skill_dir = skills_dir / "speckit-dash-ext-hello" + assert (skill_dir / "SKILL.md").exists() + + # Drop init-options so _get_skills_dir() returns None and removal takes + # the fallback directory-scan branch instead of the fast path. + (project_dir / ".specify" / "init-options.json").unlink() + + result = manager.remove(manifest.id, keep_config=False) + assert result is True + assert not skill_dir.exists() + def test_other_skills_preserved_on_remove(self, skills_project, extension_dir): """Non-extension skills should not be affected by extension removal.""" project_dir, skills_dir = skills_project