Repository navigation
Conversation
…sion skills Removing a supplemental extension skill deleted the whole skill directory with shutil.rmtree, destroying files the user added after install (notes, extra references). Only SKILL.md is created by the extension, so removal now deletes just that file and removes the directory only if it is empty, mirroring the generic branch. Reinstall after removal also regenerates SKILL.md next to surviving user files instead of skipping the directory. A SKILL.md that is a link to a file outside the extension directory is now always left alone, including on update rollback.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved symlink-handling flaws can cause installation to write outside the skills directory and cleanup to delete an unrelated SKILL.md file.
4 open issues
What changed in this PR
Addresses #5381 by preserving user-added files when supplemental extension skills are removed or updated.
Changes:
- Deletes only extension-owned
SKILL.mdfiles and removes skill directories only when empty. - Supports regenerating skills beside preserved files during forced reinstall and update, while preserving externally linked skill files during removal.
- Adds preservation and reinstall regression tests and updates installation mocks to forward
force.
| File | Description |
|---|---|
tests/test_extension_skills.py |
Adds Bob-specific removal and reinstall regression tests. |
tests/specify_cli/extensions/test_command_update.py |
Updates the installation mock to accept and forward force. |
tests/integrations/test_integration_generic.py |
Updates generic update mocks to forward force. |
src/specify_cli/extensions/_command_update_transaction.py |
Enables forced reinstall during extension updates. |
src/specify_cli/extensions/__init__.py |
Adds selective skill cleanup, link preservation, and reinstall handling. |
Review effort: Balanced (auto)
| if not is_expected_dev_symlink and not force: | ||
| continue | ||
| elif skill_dir_preexists and not force: | ||
| elif skill_dir_preexists and not force and not reinstall: |
Comment on lines
+2252
to
+2256
| unlink(skill_file, missing_ok=True) | ||
| try: | ||
| skill_subdir.rmdir() | ||
| except OSError: | ||
| pass |
| if not is_expected_dev_symlink and not force: | ||
| continue | ||
| elif skill_dir_preexists and not force: | ||
| elif skill_dir_preexists and not force and not reinstall: |
Comment on lines
+2228
to
2231
| if skill_file.is_symlink() and not skill_file.resolve().is_relative_to( | ||
| (self.extensions_dir / extension_id).resolve() | ||
| ): | ||
| continue |
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.
Fixes #5381
What broke
Removing a supplemental extension skill deleted the whole skill directory with shutil.rmtree, so any files the user added after install (notes, extra references) were destroyed. The same removal ran during extension update.
What changed
How verified