Repository navigation
Conversation
Native command registration wrote extension skills over any existing SKILL.md at the same path, even when the file was a user's own skill with no extension marker. The supplemental route already skipped such files, but it ran after native registration had already overwritten them. The overwritten file then carried the extension's marker, so extension remove deleted the user's skill as well. Registering a /SKILL.md output now checks whether the existing file was created by this extension (native, supplemental, or preset marker, or a dev-cache symlink) and skips it with a warning when it was not. A skipped skill is never added to the registry, so remove never targets it either. Adds a regression test: install with a pre-existing user skill leaves the skill and its notes file untouched, and remove leaves them in place. Fixes github#5380
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The ownership guards can incorrectly accept another extension’s symlink and read non-regular targets, risking unintended skill replacement, blocked installation, or memory exhaustion.
6 open issues
What changed in this PR
Adds collision protection for user-authored skills to Spec Kit’s shared command registrar, targeting the install/remove data-loss issue in #5380.
Changes:
- Checks ownership before replacing native
SKILL.mdcommand and alias files, warning when registration is skipped. - Adds an install/remove regression test verifying that an existing user skill and its notes survive.
Validation used isolated checks; project suites and the before/after regression comparison were not run.
AI-generated review by GitHub Copilot (model unknown; automated; requester identity not provided).
| File | Description |
|---|---|
tests/test_extension_skills.py |
Tests user-skill preservation and exclusion from registration across installation and removal. |
src/specify_cli/agents.py |
Adds ownership detection and collision guards for native command and alias skills. |
Review effort: Max (auto)
Comment on lines
+638
to
+639
| if target is not None and ".specify-dev" in target.parts: | ||
| return True |
Comment on lines
+640
to
+643
| try: | ||
| raw = path.read_text(encoding="utf-8") | ||
| except (OSError, UnicodeDecodeError): | ||
| return False |
Comment on lines
+645
to
+647
| source = ( | ||
| fm.get("metadata", {}).get("source", "") if isinstance(fm, dict) else "" | ||
| ) |
Comment on lines
+934
to
+937
| # Never overwrite a pre-existing user skill: the supplemental | ||
| # skills route already skips unowned directories, and native | ||
| # registration must do the same (#5380). The command is left | ||
| # unregistered so a later removal cannot delete it either. |
Comment on lines
+1032
to
+1035
| if ( | ||
| agent_config["extension"] == "/SKILL.md" | ||
| and (alias_file.exists() or alias_file.is_symlink()) | ||
| and not self._skill_file_owned_by_source(alias_file, source_id) |
Collaborator
|
Please address Copilot feedback |
… routes The github#5380 ownership guard in CommandRegistrar treated any SKILL.md not created by the registering source as a user file and skipped it. That broke preset registration: the preset override of speckit.specify lands on a file init created (metadata.source templates/commands/specify.md), so the preset was never recorded and two integration tests failed. Judge ownership tooling-wide instead: a file is ours when its metadata.source matches a route the tooling itself writes (extension:, preset:, native <source_id>:<file>, or templates/commands/*.md). User-authored files (no recognized source) are still skipped, so the github#5380 protection is unchanged.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Overly permissive ownership checks can misidentify unrelated skills and symlinks as managed, causing user-owned content to be overwritten or deleted.
6 open issues
1 resolved since last review
Review effort: Balanced (auto)
Comment on lines
+666
to
+667
| and tail | ||
| and ("/" in tail or tail.endswith(".md")) |
Collaborator
|
Please address Copilot feedback |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What broke
extension addoverwrote a user's own pre-existing skill when an extension command landed on the same path, andextension removethen deleted the user's skill. The supplemental route already skipped such files, but native command registration ran first and overwrote them, stamping the extension's marker on the file.What changed
CommandRegistrar.register_commandsnow checks whether an existingSKILL.mdwas created by the extension being installed (native, supplemental, or preset marker, or a dev-cache symlink) and skips it with a warning when it was not. A skipped skill is never registered, so remove never targets it.How verified
New regression test in
tests/test_extension_skills.py: install with a pre-existing user skill leaves the skill and its notes file untouched, and remove leaves them in place. The test fails on main and passes with the fix. Targeted suites pass: extension skills, registration, extensions, command-add, presets, bundles (1699 passed, 1 skipped).ruff checkclean.Fixes #5380