fix: address second Copilot review round — 6 remaining observations

- Move  to module scope (was inside per-template loop)
- Add  safety checks in setup() matching standard
- Fix docstrings: global skills always removed on uninstall (standard)
- Fix removal tracking: only report after successful rmtree
- Override shared test_modified_file_survives_uninstall with Hermes-appropriate
  behaviour (global skills always removed, no hash tracking)
- Update PR description to match implementation (global-only skills + marker)

Author: majordave

src/specify_cli/integrations/hermes/__init__.py

@@ -16,6 +16,8 @@
 from shutil import rmtree
 from typing import Any
 
+import yaml
+
 from ..base import IntegrationOption, SkillsIntegration
 from ..manifest import IntegrationManifest
 
@@ -29,7 +31,8 @@ class HermesIntegration(SkillsIntegration):
     Hermes discovers them globally.  A project-local marker directory
     (``.hermes/skills/`` empty) is created so extension commands (e.g.
     git) can detect Hermes as an active integration.  Uninstall removes
-    both the marker and global skills.
+    both the marker and all global ``speckit-*`` skills, matching the
+    standard integration teardown behaviour.
     """
 
     key = "hermes"
@@ -91,6 +94,14 @@ def setup(
         if not templates:
             return []
 
+        # Safety check: verify manifest project_root matches (standard pattern)
+        project_root_resolved = project_root.resolve()
+        if manifest.project_root != project_root_resolved:
+            raise ValueError(
+                f"manifest.project_root ({manifest.project_root}) does not match "
+                f"project_root ({project_root_resolved})"
+            )
+
         script_type = opts.get("script_type", "sh")
         arg_placeholder = (
             self.registrar_config.get("args", "$ARGUMENTS")
@@ -115,8 +126,6 @@ def setup(
             if raw.startswith("---"):
                 parts = raw.split("---", 2)
                 if len(parts) >= 3:
-                    import yaml
-
                     try:
                         fm = yaml.safe_load(parts[1])
                         if isinstance(fm, dict):
@@ -189,15 +198,16 @@ def teardown(
         *,
         force: bool = False,
     ) -> tuple[list[Path], list[Path]]:
-        """Uninstall integration files and optionally clean up global skills.
+        """Uninstall integration files including global Hermes skills.
 
         Removes the managed context section from AGENTS.md, removes the
-        project-local marker directory, and delegates to
-        ``manifest.uninstall()`` for project-local tracked files.
+        project-local marker directory, delegates to
+        ``manifest.uninstall()`` for project-local tracked files, and
+        removes all ``speckit-*`` skills under ``~/.hermes/skills/``.
 
-        Global ``speckit-*`` skills under ``~/.hermes/skills/`` are only
-        removed when ``force=True`` to avoid destroying skills shared with
-        other Spec Kit projects.
+        Global skills are always removed on teardown — this matches the
+        standard integration behaviour where all files created by the
+        integration are removed on ``specify integration uninstall``.
         """
         # Remove managed context section from AGENTS.md
         self.remove_context_section(project_root)
@@ -214,17 +224,18 @@ def teardown(
             if hermes_dir.is_dir() and not any(hermes_dir.iterdir()):
                 hermes_dir.rmdir()
 
-        # Remove global Hermes skills for speckit — only when force=True
-        # to avoid destroying skills shared with other Spec Kit projects.
-        if force:
-            global_skills_dir = self._hermes_home_skills_dir()
-            if global_skills_dir.is_dir():
-                for skill_dir in sorted(global_skills_dir.iterdir()):
-                    if skill_dir.is_dir() and skill_dir.name.startswith("speckit-"):
-                        skill_file = skill_dir / "SKILL.md"
-                        if skill_file.exists():
-                            removed.append(skill_file)
-                        rmtree(skill_dir, ignore_errors=True)
+        # Remove all global Hermes skills for speckit — these are always
+        # removed on uninstall regardless of the force flag, matching the
+        # standard behaviour where all integration files are cleaned up.
+        global_skills_dir = self._hermes_home_skills_dir()
+        if global_skills_dir.is_dir():
+            for skill_dir in sorted(global_skills_dir.iterdir()):
+                if skill_dir.is_dir() and skill_dir.name.startswith("speckit-"):
+                    try:
+                        rmtree(skill_dir)
+                        removed.append(skill_dir)
+                    except OSError:
+                        skipped.append(skill_dir)
 
         return removed, skipped
 

tests/integrations/test_integration_hermes.py

@@ -124,16 +124,19 @@ def test_install_uninstall_roundtrip(self, tmp_path, monkeypatch):
         for f in created:
             if "SKILL.md" in str(f):
                 assert f.exists(), f"{f} does not exist"
-        removed, skipped = i.teardown(tmp_path, m, force=True)
+        # Global skills are removed on teardown without needing force
+        removed, skipped = i.teardown(tmp_path, m, force=False)
         for f in created:
             if "SKILL.md" in str(f):
                 assert not f.exists(), f"{f} should have been removed"
         # Local marker should be gone
         assert not (tmp_path / ".hermes" / "skills").exists()
 
     def test_modified_file_survives_uninstall(self, tmp_path, monkeypatch):
-        """Override: Hermes global skills are removed on uninstall only
-        when force=True (no hash-based preservation since they're not in manifest)."""
+        """Override: Hermes global skills are ALWAYS removed on uninstall
+        (they live outside the project root and aren't hash-tracked in the
+        manifest), so a modified global skill is still removed — matching
+        the standard behaviour where all integration files are cleaned up."""
         home = _fake_home(tmp_path)
         monkeypatch.setattr(Path, "home", lambda: home)
 
@@ -146,10 +149,30 @@ def test_modified_file_survives_uninstall(self, tmp_path, monkeypatch):
         assert len(skill_files) > 0
         modified_file = skill_files[0]
         modified_file.write_text("user modified this", encoding="utf-8")
-        # Global skills are only removed with force=True
-        removed, skipped = i.teardown(tmp_path, m, force=True)
+        removed, skipped = i.uninstall(tmp_path, m)
         assert not modified_file.exists(), (
-            "Modified global skill should be removed on teardown with force=True"
+            "Modified global skill should be removed on teardown (standard behaviour)"
+        )
+
+    def test_modified_global_skill_removed_on_teardown(self, tmp_path, monkeypatch):
+        """Override: Hermes global skills are removed on uninstall regardless
+        of the force flag, matching standard integration behaviour."""
+        home = _fake_home(tmp_path)
+        monkeypatch.setattr(Path, "home", lambda: home)
+
+        i = get_integration(self.KEY)
+        m = IntegrationManifest(self.KEY, tmp_path)
+        created = i.install(tmp_path, m)
+        m.save()
+        # Pick a global skill file
+        skill_files = [f for f in created if "SKILL.md" in str(f)]
+        assert len(skill_files) > 0
+        modified_file = skill_files[0]
+        modified_file.write_text("user modified this", encoding="utf-8")
+        # Global skills are removed on teardown regardless of force flag
+        removed, skipped = i.teardown(tmp_path, m, force=False)
+        assert not modified_file.exists(), (
+            "Modified global skill should be removed on teardown (standard behaviour)"
         )
 
     def test_pre_existing_skills_not_removed(self, tmp_path, monkeypatch):
@@ -257,8 +280,8 @@ def test_install_uninstall_cleanup(self, tmp_path, monkeypatch):
         # Verify local marker exists
         assert (tmp_path / ".hermes" / "skills").is_dir()
 
-        # Teardown with force=True to clean global skills
-        removed, skipped = i.teardown(tmp_path, m, force=True)
+        # Teardown — global skills removed without needing force=True
+        removed, skipped = i.teardown(tmp_path, m, force=False)
 
         # Global skills removed
         for f in global_skills: