From 16959746f256c9b0f6551569dd7fdd9cb41e1f9a Mon Sep 17 00:00:00 2001 From: Saim Shafique Date: Wed, 11 Nov 2026 09:31:25 +0000 Subject: [PATCH] fix(extensions): preserve user files when removing supplemental extension 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. --- src/specify_cli/extensions/__init__.py | 56 ++++++++++--- .../extensions/_command_update_transaction.py | 8 +- .../integrations/test_integration_generic.py | 8 +- .../extensions/test_command_update.py | 4 +- tests/test_extension_skills.py | 81 +++++++++++++++++++ 5 files changed, 138 insertions(+), 19 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index c66ecaa65f..ad1918c2e3 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -1524,6 +1524,8 @@ def _register_extension_skills( extension_dir: Path, link_outputs: bool = False, force: bool = False, + *, + reinstall: bool = False, ) -> List[str]: """Generate SKILL.md files for extension commands as agent skills. @@ -1542,6 +1544,18 @@ def _register_extension_skills( ``setup()`` has just freshly regenerated core-template skill files and the skip guard would otherwise prevent extension content from being layered on top. + reinstall: If True, a pre-existing skill directory without a + SKILL.md may receive the fresh file. Set by + ``install_from_directory`` when this install removed the + same extension just now (``--force`` reinstall) or was + explicitly forced after the caller removed it + (``extension update``). A surviving directory is then one + the removal just emptied of the extension's own file + (#5381 keeps the directory when it holds user files), so + the fresh SKILL.md is written back next to them. Unlike + *force*, this never overwrites an existing SKILL.md: a + marker-mismatched file the removal deliberately preserved + stays untouched. Returns: List of skill names that were created (for registry storage). @@ -1643,11 +1657,14 @@ def _replacement(match: re.Match[str]) -> str: # overwriting it with the composed extension content is correct. 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: # Never add files to a pre-existing user directory. Without a # verifiable SKILL.md ownership marker, rollback/removal cannot # distinguish our output from unrelated user artifacts. - # Skipped when force=True (upgrade path). + # Skipped when force=True (upgrade path) or reinstall=True + # (this install follows removal of the same extension, so a + # surviving directory is one the removal just emptied of the + # extension's own SKILL.md, #5381). continue # Create skill directory; track whether we created it so we can clean @@ -2192,7 +2209,7 @@ def _unregister_extension_skills( every configured agent's skills directory is scanned instead of resolving just the currently active one. """ - from ..integrations._file_changes import changing_file, unlink + from ..integrations._file_changes import unlink generic_roots = { Path(path).parent.parent @@ -2203,12 +2220,13 @@ def _unregister_extension_skills( skill_names, extension_id, skills_dir=skills_dir ): skill_file = skill_subdir / "SKILL.md" - if ( - generic_hashes is not None - and skill_file.is_symlink() - and not skill_file.resolve().is_relative_to( - (self.extensions_dir / extension_id).resolve() - ) + # Never touch a SKILL.md that is a link to a file outside this + # extension's own directory: the user relinked it (for example + # to another extension's file) and it is not ours to remove. + # This applies regardless of generic_hashes: rollback callers + # pass None, and skipping here is the conservative choice. + if skill_file.is_symlink() and not skill_file.resolve().is_relative_to( + (self.extensions_dir / extension_id).resolve() ): continue if generic_hashes is not None and skill_file.is_relative_to( @@ -2226,8 +2244,16 @@ def _unregister_extension_skills( except OSError: pass continue - with changing_file(skill_subdir, removal=True): - shutil.rmtree(skill_subdir) + # Remove only the file this extension created. The skill + # directory may hold files the user added after install + # (notes, extra references); deleting the whole directory + # would destroy them (#5381). Mirror the generic branch: + # drop SKILL.md and remove the directory only if it is empty. + unlink(skill_file, missing_ok=True) + try: + skill_subdir.rmdir() + except OSError: + pass def _extension_owned_skill_names( self, skill_names: List[str], extension_id: str @@ -3060,8 +3086,14 @@ def rollback_generic_registration() -> None: # Auto-register extension commands as agent skills when skills mode # was used during project initialisation (feature parity). + # reinstall=True when this install just removed the same extension + # (did_remove) or was explicitly forced after the caller removed + # it (extension update): a surviving skill directory is one the + # removal just emptied of the extension's own SKILL.md (#5381), + # so the fresh file is written back next to preserved user files. registered_skills = self._register_extension_skills( - manifest, dest_dir, link_outputs=link_commands + manifest, dest_dir, link_outputs=link_commands, + reinstall=did_remove or force, ) if register_commands and generic_active and manifest.commands: expected = set(names) diff --git a/src/specify_cli/extensions/_command_update_transaction.py b/src/specify_cli/extensions/_command_update_transaction.py index 585b3cc2bf..cb4dd8d3cd 100644 --- a/src/specify_cli/extensions/_command_update_transaction.py +++ b/src/specify_cli/extensions/_command_update_transaction.py @@ -550,10 +550,16 @@ def backup_extension_skills(skill_names, *, skills_dir=None): installation_modified = True manager.remove(extension_id, keep_config=True) - # 8. Install new version + # 8. Install new version. force=True lets the reinstall + # write SKILL.md back into a skill directory that + # survived removal because it holds user files (#5381): + # remove() above already unregistered the old version, + # so force changes nothing else here — it only signals + # the reinstall to the skill registration below. _ = manager.install_from_zip( archive_path, speckit_version, + force=True, catalog_name=update.catalog_name, ) diff --git a/tests/integrations/test_integration_generic.py b/tests/integrations/test_integration_generic.py index 3ea10f7b11..c1cf1c7b7a 100644 --- a/tests/integrations/test_integration_generic.py +++ b/tests/integrations/test_integration_generic.py @@ -1407,11 +1407,11 @@ def test_generic_extension_update_uses_project_registrar( if file.is_file(): zip_file.write(file, file.relative_to(updated_source)) - def install_update(self, _zip_path, speckit_version, *, catalog_name=None): + def install_update(self, _zip_path, speckit_version, *, catalog_name=None, force=False): if fail_install: raise RuntimeError("simulated update failure") return self.install_from_directory( - updated_source, speckit_version, catalog_name=catalog_name + updated_source, speckit_version, catalog_name=catalog_name, force=force ) with ( @@ -1601,11 +1601,11 @@ def test_generic_update_rollback_restores_previous_output_directory( if file.is_file(): zip_file.write(file, file.relative_to(updated_source)) - def install_update(self, _zip_path, speckit_version, *, catalog_name=None): + def install_update(self, _zip_path, speckit_version, *, catalog_name=None, force=False): if fail_install: raise RuntimeError("simulated update failure") return self.install_from_directory( - updated_source, speckit_version, catalog_name=catalog_name + updated_source, speckit_version, catalog_name=catalog_name, force=force ) fail_install = True diff --git a/tests/specify_cli/extensions/test_command_update.py b/tests/specify_cli/extensions/test_command_update.py index c2c421439d..c3737e881d 100644 --- a/tests/specify_cli/extensions/test_command_update.py +++ b/tests/specify_cli/extensions/test_command_update.py @@ -349,10 +349,10 @@ def test_update_success_preserves_installed_at( v2_dir = self._create_extension_source(tmp_path, "2.0.0") def fake_install_from_zip( - self_obj, _zip_path, speckit_version, *, catalog_name=None + self_obj, _zip_path, speckit_version, *, catalog_name=None, force=False ): return self_obj.install_from_directory( - v2_dir, speckit_version, catalog_name=catalog_name + v2_dir, speckit_version, catalog_name=catalog_name, force=force ) with patch.object(Path, "cwd", return_value=project_dir), \ diff --git a/tests/test_extension_skills.py b/tests/test_extension_skills.py index 3f65585a30..3ef2e90f85 100644 --- a/tests/test_extension_skills.py +++ b/tests/test_extension_skills.py @@ -320,6 +320,20 @@ def skills_project(project_dir): return project_dir, skills_dir +@pytest.fixture +def bob_skills_project(project_dir): + """Create a project with Bob in its default (supplemental) skills mode. + + Bob routes extension commands through _register_extension_skills into + ``.bob/skills`` (the supplemental route from #5381); native skills + agents such as Claude render SKILL.md via CommandRegistrar instead, so + they do not exercise the supplemental removal branch. + """ + _create_init_options(project_dir, ai="bob", ai_skills=True) + skills_dir = _create_skills_dir(project_dir, ai="bob") + return project_dir, skills_dir + + @pytest.fixture def no_skills_project(project_dir): """Create a project without skills mode.""" @@ -3369,6 +3383,73 @@ 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_remove_preserves_user_files_in_supplemental_skill_dir( + self, bob_skills_project, extension_dir + ): + """Removing an extension must not delete user files in a skill dir. + + Regression guard for #5381: the supplemental (non-generic) branch + removed the whole skill directory with ``shutil.rmtree``, deleting + any file the user had added to it. Only the extension-owned + SKILL.md may go; the directory survives while it holds user files. + """ + project_dir, skills_dir = bob_skills_project + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory(extension_dir, "0.1.0") + + hello_dir = skills_dir / "speckit-test-ext-hello" + world_dir = skills_dir / "speckit-test-ext-world" + assert (hello_dir / "SKILL.md").is_file() + assert (world_dir / "SKILL.md").is_file() + + notes = hello_dir / "my-notes.md" + notes.write_text("personal notes\n", encoding="utf-8") + + result = manager.remove(manifest.id, keep_config=False) + assert result is True + + # The extension-owned SKILL.md is gone, but the user's file survives. + assert not (hello_dir / "SKILL.md").exists() + assert notes.is_file() + assert notes.read_text(encoding="utf-8") == "personal notes\n" + assert hello_dir.is_dir() + + # A skill directory with no user files is removed entirely. + assert not world_dir.exists() + + def test_reinstall_after_remove_regenerates_skill_beside_user_files( + self, bob_skills_project, extension_dir + ): + """Reinstalling after removal restores SKILL.md next to user files. + + Guard for #5381's update path: ``extension update`` removes then + reinstalls. The surviving skill directory must not make the + reinstall skip the skill — the fresh SKILL.md belongs next to the + preserved user file. + """ + project_dir, skills_dir = bob_skills_project + manager = ExtensionManager(project_dir) + manifest = manager.install_from_directory(extension_dir, "0.1.0") + + hello_dir = skills_dir / "speckit-test-ext-hello" + notes = hello_dir / "my-notes.md" + notes.write_text("personal notes\n", encoding="utf-8") + + assert manager.remove(manifest.id, keep_config=False) is True + assert not (hello_dir / "SKILL.md").exists() + assert notes.is_file() + + # Reinstall, as the update flow does after removing the old version. + manager.install_from_directory(extension_dir, "0.1.0", force=True) + + assert (hello_dir / "SKILL.md").is_file() + assert notes.is_file() + assert notes.read_text(encoding="utf-8") == "personal notes\n" + assert manager.registry.get(manifest.id)["registered_skills"] == [ + "speckit-test-ext-hello", + "speckit-test-ext-world", + ] + def test_skills_removed_when_description_contains_dashes( self, skills_project, temp_dir ):