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 ):