Skip to content

fix(extensions): parse SKILL.md on the --- delimiter line during removal#3634

Merged
mnriem merged 2 commits into
github:mainfrom
Quratulain-bilal:probe/script-parity-2607
Jul 23, 2026
Merged

fix(extensions): parse SKILL.md on the --- delimiter line during removal#3634
mnriem merged 2 commits into
github:mainfrom
Quratulain-bilal:probe/script-parity-2607

Conversation

@Quratulain-bilal

Copy link
Copy Markdown
Contributor

ExtensionManager._unregister_extension_skills verifies an installed skill before deleting it by reading metadata.source back from its SKILL.md. It located the closing frontmatter delimiter 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 is then truncated mid-value, metadata.source parses 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 generated SKILL.md files.

Fix

Parse on the --- delimiter line by reusing the line-anchored CommandRegistrar.parse_frontmatter (the #3590 parser) in both removal paths:

  • the fast, registry-driven path, and
  • the fallback directory-scan path.

Repro

raw = '''---
name: speckit-myext-report
description: Separate sections with --- markers
metadata:
  source: extension:myext
---
Body here.
'''
# old: raw.split("---", 2)[1] -> "\nname: ...\ndescription: Separate sections with "
#      -> metadata.source == ""  -> dir treated as unrelated, orphaned

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. Full tests/test_extension_skills.py, test_extensions.py, and test_extension_registration.py suites pass (467 tests).

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/specify_cli/extensions/__init__.py

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@Quratulain-bilal

Copy link
Copy Markdown
Contributor Author

Good catch — addressed in 2b44ad2. You're right that skills_project keeps ai_skills enabled, so remove() resolved the skills dir and only the fast branch ran; a substring-split regression in the fallback would have stayed green.

Added 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. Confirmed it protects that independently duplicated parser: reverting only the fallback site back to the old split("---", 2) fails the new test (skill dir orphaned) while the fast-path test still passes.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@mnriem
mnriem self-requested a review July 23, 2026 17:40
@mnriem
mnriem merged commit 58b3cad into github:main Jul 23, 2026
14 checks passed
@mnriem

mnriem commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants