fix(extensions): parse SKILL.md on the --- delimiter line during removal#3634
Conversation
ExtensionManager._unregister_extension_skills verified an installed skill
before deleting it by reading metadata.source back from its SKILL.md with a
raw split("---", 2). That substring split stops at the first "---" anywhere
after the opening delimiter, including one embedded in a command description
(e.g. "Separate sections with --- markers"). The frontmatter was then
truncated mid-value, metadata.source parsed empty, the skill looked
unrelated, and its directory was left orphaned on uninstall.
Parse on the "---" delimiter *line* instead, reusing CommandRegistrar.
parse_frontmatter (the line-anchored parser from github#3590) in both the fast
(registry-driven) and fallback (directory-scan) removal paths.
Add a regression test that installs an extension whose command description
contains "---", removes it, and asserts the skill directory is gone. Fails
before the fix (dir orphaned), passes after.
There was a problem hiding this comment.
Pull request overview
Fixes orphaned extension skill directories when descriptions contain ---.
Changes:
- Reuses line-anchored frontmatter parsing in both removal paths.
- Adds regression coverage for the primary removal path.
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/extensions/__init__.py |
Corrects SKILL.md ownership parsing during removal. |
tests/test_extension_skills.py |
Adds dashed-description removal regression coverage. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Medium
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback. If not applicable, please explain why
Copilot noted the new regression test only exercised the fast removal path (skills_project keeps ai_skills enabled, so remove() resolves the skills dir directly). Add test_skills_removed_with_dashes_via_fallback_scan, which deletes init-options.json after install so _get_skills_dir() returns None and removal takes the fallback directory-scan branch. That branch re-reads metadata.source with an independently duplicated parser; reverting it to the old substring split now fails this test (dir orphaned) while the fast-path test still passes.
|
Good catch — addressed in 2b44ad2. You're right that Added |
|
Thank you! |
ExtensionManager._unregister_extension_skillsverifies an installed skill before deleting it by readingmetadata.sourceback from itsSKILL.md. It located the closing frontmatter delimiter with a rawsplit("---", 2).That substring split stops at the first
---anywhere after the opening delimiter — including one embedded in a command description (e.g."Separate sections with --- markers"). The frontmatter is then truncated mid-value,metadata.sourceparses empty, the skill looks unrelated to the extension, and its directory is left orphaned on uninstall.This is the same class of bug fixed in #3590 for
CommandRegistrar.parse_frontmatter(content.find("---", 3)), which now scans on line boundaries. The two removal paths here were the remaining substring-split sites reading generatedSKILL.mdfiles.Fix
Parse on the
---delimiter line by reusing the line-anchoredCommandRegistrar.parse_frontmatter(the #3590 parser) in both removal paths:Repro
Test
Added
test_skills_removed_when_description_contains_dashes: installs an extension whose command description contains---, removes it, and asserts the skill directory is gone. It fails before the fix (assert not skill_dir.exists()→ dir orphaned) and passes after. Fulltests/test_extension_skills.py,test_extensions.py, andtest_extension_registration.pysuites pass (467 tests).