fix(extensions): preserve user files when removing supplemental extension skills

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)

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

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

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), \

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