From ec5471af61c3f9fe94cdb6122849ac43ca55ef5a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 20 Mar 2026 21:03:34 +0000 Subject: [PATCH] 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 --- .../core_pack/agents/amp/bootstrap.py | 16 ++++++++++++---- .../core_pack/agents/codex/bootstrap.py | 16 ++++++++++++---- .../core_pack/agents/copilot/bootstrap.py | 16 ++++++++++++---- .../core_pack/agents/tabnine/bootstrap.py | 16 ++++++++++++---- tests/test_agent_pack.py | 14 +++++++++----- 5 files changed, 57 insertions(+), 21 deletions(-) diff --git a/src/specify_cli/core_pack/agents/amp/bootstrap.py b/src/specify_cli/core_pack/agents/amp/bootstrap.py index 51b676bf..e5e52021 100644 --- a/src/specify_cli/core_pack/agents/amp/bootstrap.py +++ b/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() diff --git a/src/specify_cli/core_pack/agents/codex/bootstrap.py b/src/specify_cli/core_pack/agents/codex/bootstrap.py index 8f9a60a9..82afbc64 100644 --- a/src/specify_cli/core_pack/agents/codex/bootstrap.py +++ b/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() diff --git a/src/specify_cli/core_pack/agents/copilot/bootstrap.py b/src/specify_cli/core_pack/agents/copilot/bootstrap.py index 44a23e1f..052473d5 100644 --- a/src/specify_cli/core_pack/agents/copilot/bootstrap.py +++ b/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() diff --git a/src/specify_cli/core_pack/agents/tabnine/bootstrap.py b/src/specify_cli/core_pack/agents/tabnine/bootstrap.py index f04411f3..810a75c3 100644 --- a/src/specify_cli/core_pack/agents/tabnine/bootstrap.py +++ b/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() diff --git a/tests/test_agent_pack.py b/tests/test_agent_pack.py index 77b5a74d..ae42f052 100644 --- a/tests/test_agent_pack.py +++ b/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 # ===================================================================