Skip to content

Commit b938619

Browse files
committed
fix: address remaining 3 Copilot review observations (round 3)
- teardown docstring: clarify marker removal is conditional (if empty) - test_pre_existing_skills_not_removed: now actually calls teardown() to verify foreign skills survive uninstall (was only running setup) - integration_switch Phase 1: replaced old_manifest.uninstall() + remove_context_section() with current_integration.teardown(), matching the pattern already used in integration_uninstall. This ensures custom teardown logic (e.g. Hermes global skills cleanup) runs during switches.
1 parent 3da5e0b commit b938619

3 files changed

Lines changed: 11 additions & 6 deletions

File tree

src/specify_cli/__init__.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2093,8 +2093,9 @@ def integration_switch(
20932093
f"run [cyan]specify integration uninstall {installed_key}[/cyan], then retry."
20942094
)
20952095
raise typer.Exit(1)
2096-
removed, skipped = old_manifest.uninstall(project_root, force=force)
2097-
current_integration.remove_context_section(project_root)
2096+
removed, skipped = current_integration.teardown(
2097+
project_root, old_manifest, force=force,
2098+
)
20982099
if removed:
20992100
console.print(f" Removed {len(removed)} file(s)")
21002101
if skipped:

src/specify_cli/integrations/hermes/__init__.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,7 @@ def teardown(
201201
"""Uninstall integration files including global Hermes skills.
202202
203203
Removes the managed context section from AGENTS.md, removes the
204-
project-local marker directory, delegates to
204+
project-local marker directory (if empty), delegates to
205205
``manifest.uninstall()`` for project-local tracked files, and
206206
removes all ``speckit-*`` skills under ``~/.hermes/skills/``.
207207

tests/integrations/test_integration_hermes.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,7 @@ def test_modified_global_skill_removed_on_teardown(self, tmp_path, monkeypatch):
176176
)
177177

178178
def test_pre_existing_skills_not_removed(self, tmp_path, monkeypatch):
179-
"""Override: pre-existing non-speckit skills in the global dir
180-
should survive Hermes uninstall."""
179+
"""Pre-existing non-speckit global skills should survive Hermes uninstall."""
181180
home = _fake_home(tmp_path)
182181
monkeypatch.setattr(Path, "home", lambda: home)
183182

@@ -191,7 +190,12 @@ def test_pre_existing_skills_not_removed(self, tmp_path, monkeypatch):
191190
m = IntegrationManifest(self.KEY, tmp_path)
192191
i.setup(tmp_path, m)
193192

194-
assert (foreign_dir / "SKILL.md").exists(), "Foreign skill was removed"
193+
# Run teardown to verify foreign skill survives uninstall
194+
i.teardown(tmp_path, m)
195+
196+
assert (foreign_dir / "SKILL.md").exists(), (
197+
"Foreign skill was removed by teardown"
198+
)
195199

196200
def test_complete_file_inventory_sh(self, tmp_path, monkeypatch):
197201
"""Override: Hermes init produces no local SKILL.md files,

0 commit comments

Comments
 (0)