Fix code review issues: safe teardown for shared dirs, less brittle test assertions

- Copilot: only remove .github/agents/ (preserves workflows, templates)
- Tabnine: only remove .tabnine/agent/ (preserves other config)
- Amp/Codex: only remove respective subdirs (commands/skills)
  to avoid deleting each other's files in shared .agents/ dir
- Tests: use flexible assertions instead of hardcoded >= 25 counts

Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/spec-kit/sessions/ef8b4682-7f1a-4b04-a112-df0878236b6b

src/specify_cli/core_pack/agents/amp/bootstrap.py

@@ -18,8 +18,16 @@ class Amp(AgentBootstrap):
         commands_dir.mkdir(parents=True, exist_ok=True)
 
     def teardown(self, project_path: Path) -> None:
-        """Remove Amp agent files from the project."""
+        """Remove Amp agent files from the project.
+
+        Only removes the commands/ subdirectory — preserves other .agents/
+        content (e.g. Codex skills/) which shares the same parent directory.
+        """
         import shutil
-        agent_dir = project_path / self.AGENT_DIR
-        if agent_dir.is_dir():
-            shutil.rmtree(agent_dir)
+        commands_dir = project_path / self.AGENT_DIR / self.COMMANDS_SUBDIR
+        if commands_dir.is_dir():
+            shutil.rmtree(commands_dir)
+        # Remove .agents/ only if now empty
+        agents_dir = project_path / self.AGENT_DIR
+        if agents_dir.is_dir() and not any(agents_dir.iterdir()):
+            agents_dir.rmdir()

src/specify_cli/core_pack/agents/codex/bootstrap.py

@@ -18,8 +18,16 @@ class Codex(AgentBootstrap):
         commands_dir.mkdir(parents=True, exist_ok=True)
 
     def teardown(self, project_path: Path) -> None:
-        """Remove Codex CLI agent files from the project."""
+        """Remove Codex CLI agent files from the project.
+
+        Only removes the skills/ subdirectory — preserves other .agents/
+        content (e.g. Amp commands/) which shares the same parent directory.
+        """
         import shutil
-        agent_dir = project_path / self.AGENT_DIR
-        if agent_dir.is_dir():
-            shutil.rmtree(agent_dir)
+        skills_dir = project_path / self.AGENT_DIR / self.COMMANDS_SUBDIR
+        if skills_dir.is_dir():
+            shutil.rmtree(skills_dir)
+        # Remove .agents/ only if now empty
+        agents_dir = project_path / self.AGENT_DIR
+        if agents_dir.is_dir() and not any(agents_dir.iterdir()):
+            agents_dir.rmdir()

src/specify_cli/core_pack/agents/copilot/bootstrap.py

@@ -18,8 +18,16 @@ class Copilot(AgentBootstrap):
         commands_dir.mkdir(parents=True, exist_ok=True)
 
     def teardown(self, project_path: Path) -> None:
-        """Remove GitHub Copilot agent files from the project."""
+        """Remove GitHub Copilot agent files from the project.
+
+        Only removes the agents/ subdirectory — preserves other .github
+        content (workflows, issue templates, etc.).
+        """
         import shutil
-        agent_dir = project_path / self.AGENT_DIR
-        if agent_dir.is_dir():
-            shutil.rmtree(agent_dir)
+        agents_dir = project_path / self.AGENT_DIR / self.COMMANDS_SUBDIR
+        if agents_dir.is_dir():
+            shutil.rmtree(agents_dir)
+        # Also clean up companion .github/prompts/ if empty
+        prompts_dir = project_path / self.AGENT_DIR / "prompts"
+        if prompts_dir.is_dir() and not any(prompts_dir.iterdir()):
+            prompts_dir.rmdir()

src/specify_cli/core_pack/agents/tabnine/bootstrap.py

@@ -18,8 +18,16 @@ class Tabnine(AgentBootstrap):
         commands_dir.mkdir(parents=True, exist_ok=True)
 
     def teardown(self, project_path: Path) -> None:
-        """Remove Tabnine CLI agent files from the project."""
+        """Remove Tabnine CLI agent files from the project.
+
+        Removes the agent/ subdirectory under .tabnine/ to preserve
+        any other Tabnine configuration.
+        """
         import shutil
-        agent_dir = project_path / self.AGENT_DIR
-        if agent_dir.is_dir():
-            shutil.rmtree(agent_dir)
+        agent_subdir = project_path / self.AGENT_DIR
+        if agent_subdir.is_dir():
+            shutil.rmtree(agent_subdir)
+        # Remove .tabnine/ only if now empty
+        tabnine_dir = project_path / ".tabnine"
+        if tabnine_dir.is_dir() and not any(tabnine_dir.iterdir()):
+            tabnine_dir.rmdir()

tests/test_agent_pack.py

@@ -365,18 +365,22 @@ class TestDiscovery:
 
     def test_list_embedded_agents_nonempty(self):
         agents = list_embedded_agents()
-        assert len(agents) >= 25
+        assert len(agents) > 0
         ids = {a.id for a in agents}
-        assert "claude" in ids
-        assert "gemini" in ids
-        assert "copilot" in ids
+        # Verify core agents are present
+        for core_agent in ("claude", "gemini", "copilot"):
+            assert core_agent in ids
 
     def test_list_all_agents(self):
         all_agents = list_all_agents()
-        assert len(all_agents) >= 25
+        assert len(all_agents) > 0
         # Should be sorted by id
         ids = [a.manifest.id for a in all_agents]
         assert ids == sorted(ids)
+        # Verify core agents are present
+        id_set = set(ids)
+        for core_agent in ("claude", "gemini", "copilot"):
+            assert core_agent in id_set
 
 
 # ===================================================================