mirror of
https://github.com/github/spec-kit.git
synced 2026-07-06 22:32:13 +08:00
* feat: add Hermes Agent integration * feat: add Hermes Agent integration * feat: add Hermes Agent integration * feat: add Hermes Agent integration (with review fixes) - Full SkillsIntegration subclass with dual install strategy (project-local .hermes/skills/ + global ~/.hermes/skills/) - CLI fix: integration_uninstall now calls integration.teardown() instead of manifest.uninstall() directly, allowing custom cleanup - Fix Copilot review issues: - Docstring now reflects both -Q (quiet) and -q (query) flags - Empty command guard prevents passing empty skill names - Add catalog entry for hermes in integrations/catalog.json Co-authored-by: Zhaoxiaoguang001 <3357983213@qq.com> * feat: write Hermes skills directly to global ~/.hermes/skills/ Hermes loads skills from the global ~/.hermes/skills/ directory, not from project-local paths. The old dual-install strategy copied SKILL.md files to both locations — project-local (for manifest tracking) and global (for Hermes discovery). This change removes the project-local copies entirely: - setup() writes directly to ~/.hermes/skills/speckit-*/SKILL.md - An empty .hermes/skills/ marker directory is created in the project so extension commands (e.g. git) can detect Hermes as an active integration via register_commands_for_all_agents() - teardown() cleans both the global speckit-* dirs and the local marker - import yaml moved to local import inside setup() Tests updated: Hermes-specific tests now assert global skill location, and shared SkillsIntegrationTests that assumed project-local files are overridden with Hermes-appropriate assertions. Co-authored-by: Zhaoxiaoguang001 <3357983213@qq.com> * fix: address Copilot review feedback on Hermes integration Addresses all 6 review comments from copilot-pull-request-reviewer: 1. Hard-fail on missing integration key → fall back to manifest.uninstall() with a warning instead of raising an error. Allows users to always remove stale integration files even when the integration class is missing from the registry. 2. HOME isolation in tests → every test that calls setup() or CliRunner now monkeypatches Path.home() to a temp directory, keeping the test suite hermetic and non-destructive. 3. HermesIntegration.teardown() now delegates to manifest.uninstall() for project-local tracked files (scripts, manifest), merging results with global cleanup. 4. Global skills cleanup gated behind force=True to avoid destroying speckit-* skills shared across multiple Spec Kit projects when running 'specify integration uninstall hermes' without --force. 5. Line 160 isolation (CLI test test_complete_file_inventory_sh). 6. Line 258 isolation (Path.home assertion in test_ai_hermes_without_ai_skills_auto_promotes). * 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) * fix: add first-class global/home-based agent dir support in CommandRegistrar Resolves Copilot HIGH concern (discussion_r3312194525): HermesIntegration.registrar_config.dir was '.hermes/skills' (project- relative), but skills live in ~/.hermes/skills/ (global). Extensions and presets registering commands for the 'hermes' agent via CommandRegistrar would write to the project-local marker directory instead of the real global skills directory, making those commands invisible to Hermes. Fix consists of three parts: 1. CommandRegistrar._resolve_agent_dir now supports '~/'-prefixed and absolute paths in agent_config['dir']. Relative paths still resolve against project_root as before — zero change for existing agents (Claude, Codex, Gemini, etc.). 2. HermesIntegration.registrar_config.dir changed from '.hermes/skills' to '~/.hermes/skills', so extensions/presets write directly to the global directory Hermes searches at runtime. 3. Two inline project_root / agent_config['dir'] calls in the extension update backup/restore paths (src/specify_cli/__init__.py) now delegate to _resolve_agent_dir, giving them the same global-dir support plus the legacy_dir fallback they were missing (improvement for all agents). Test side-effect: test_update_failure_rolls_back_registry_hooks_and_commands was constructing verification paths with project_dir / '~/.hermes/skills' (literal tilde) — fixed to use _resolve_agent_dir and monkeypatch Path.home() so Hermes' global dir doesn't leak into the real filesystem. * 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. * fix: address Copilot round 4 — home-relative dir resolution + project-local detection 1. _resolve_agent_dir(): expand ~/... via Path.home() + slice instead of expanduser(), so tests that monkeypatch Path.home() properly isolate the home directory (Copilot r3312731595, r3312731729) 2. Add detect_dir field to registrar_config: Hermes declares detect_dir='.hermes/skills' (project-local marker). CommandRegistrar checks detect_dir before resolving the output dir, preventing global dirs like ~/.hermes/skills from causing false detection in every project (Copilot r3312731682) 3. test_update_failure_rolls_back: no additional changes needed — the _resolve_agent_dir fix makes the existing Path.home() monkeypatch effective, so ~/.hermes/skills is not found in the fake home and Hermes is properly skipped. Tests: 2236 passed (2009 integration + 195 extension + 32 hermes) --------- Co-authored-by: Zhaoxiaoguang001 <3357983213@qq.com> Co-authored-by: majordave <majordave@users.noreply.github.com>
330 lines
13 KiB
Python
330 lines
13 KiB
Python
"""Tests for HermesIntegration.
|
|
|
|
Hermes is special among SkillsIntegration subclasses: it writes skills
|
|
to ``~/.hermes/skills/`` (global) rather than the project-local
|
|
``.hermes/skills/`` directory. A project-local marker (empty directory)
|
|
is created so extension commands (e.g. git) can detect Hermes.
|
|
|
|
All tests that touch ``~/.hermes/`` use ``monkeypatch`` to isolate
|
|
``Path.home()`` to a temp directory so the test suite is hermetic and
|
|
non-destructive to a developer's real Hermes installation.
|
|
"""
|
|
|
|
from pathlib import Path
|
|
|
|
from specify_cli.integrations import get_integration
|
|
from specify_cli.integrations.manifest import IntegrationManifest
|
|
|
|
from .test_integration_base_skills import SkillsIntegrationTests
|
|
|
|
|
|
def _fake_home(tmp_path: Path) -> Path:
|
|
"""Create and return an isolated home directory under *tmp_path*."""
|
|
home = tmp_path / "home"
|
|
home.mkdir(exist_ok=True)
|
|
return home
|
|
|
|
|
|
class TestHermesIntegration(SkillsIntegrationTests):
|
|
KEY = "hermes"
|
|
FOLDER = ".hermes/"
|
|
COMMANDS_SUBDIR = "skills"
|
|
REGISTRAR_DIR = "~/.hermes/skills"
|
|
CONTEXT_FILE = "AGENTS.md"
|
|
|
|
# -- Hermes-specific setup: skills go to ~/.hermes/skills/ -------------
|
|
|
|
def test_setup_writes_to_global_skills_dir(self, tmp_path, monkeypatch):
|
|
"""Skills are written to ~/.hermes/skills/, not project-local."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
i = get_integration(self.KEY)
|
|
m = IntegrationManifest(self.KEY, tmp_path)
|
|
created = i.setup(tmp_path, m)
|
|
skill_files = [f for f in created if "scripts" not in f.parts]
|
|
|
|
assert len(skill_files) > 0, "No skill files were created"
|
|
for f in skill_files:
|
|
# Every skill file should be under ~/.hermes/skills/speckit-*/
|
|
expected_prefix = str(home / ".hermes" / "skills")
|
|
assert str(f).startswith(expected_prefix), (
|
|
f"{f} is not under ~/.hermes/skills/"
|
|
)
|
|
|
|
def test_local_marker_dir_created(self, tmp_path, monkeypatch):
|
|
"""Project-local .hermes/skills/ should exist but be empty."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
i = get_integration(self.KEY)
|
|
m = IntegrationManifest(self.KEY, tmp_path)
|
|
i.setup(tmp_path, m)
|
|
marker = tmp_path / ".hermes" / "skills"
|
|
assert marker.is_dir(), "Marker directory was not created"
|
|
# Should be empty (no SKILL.md files)
|
|
children = list(marker.iterdir())
|
|
assert children == [], f"Marker directory should be empty, got: {children}"
|
|
|
|
# -- Override shared tests that assume project-local skills ------------
|
|
|
|
def test_setup_writes_to_correct_directory(self, tmp_path, monkeypatch):
|
|
"""Override: Hermes writes to global, not project-local."""
|
|
self.test_setup_writes_to_global_skills_dir(tmp_path, monkeypatch)
|
|
|
|
def test_plan_references_correct_context_file(self, tmp_path, monkeypatch):
|
|
"""Plan skill goes to global dir, but we check it still references AGENTS.md."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
i = get_integration(self.KEY)
|
|
if not i.context_file:
|
|
return
|
|
m = IntegrationManifest(self.KEY, tmp_path)
|
|
i.setup(tmp_path, m)
|
|
# Find the plan skill in global ~/.hermes/skills/
|
|
plan_file = home / ".hermes" / "skills" / "speckit-plan" / "SKILL.md"
|
|
assert plan_file.exists(), f"Plan skill {plan_file} not created globally"
|
|
content = plan_file.read_text(encoding="utf-8")
|
|
assert i.context_file in content, (
|
|
f"Plan skill should reference {i.context_file!r} but it was not found"
|
|
)
|
|
assert "__CONTEXT_FILE__" not in content, (
|
|
"Plan skill has unprocessed __CONTEXT_FILE__ placeholder"
|
|
)
|
|
|
|
def test_all_files_tracked_in_manifest(self, tmp_path, monkeypatch):
|
|
"""Override: Hermes does not track skills in the project manifest
|
|
since they live globally. Only project-local files (scripts,
|
|
templates, context) are tracked."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
i = get_integration(self.KEY)
|
|
m = IntegrationManifest(self.KEY, tmp_path)
|
|
created = i.setup(tmp_path, m)
|
|
for f in created:
|
|
# Global files (in ~/.hermes/) are not tracked in manifest
|
|
if str(f).startswith(str(home)):
|
|
continue
|
|
rel = f.resolve().relative_to(tmp_path.resolve()).as_posix()
|
|
assert rel in m.files, f"{rel} not tracked in manifest"
|
|
|
|
def test_install_uninstall_roundtrip(self, tmp_path, monkeypatch):
|
|
"""Override: Hermes uninstall removes global skills + local marker."""
|
|
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)
|
|
assert len(created) > 0
|
|
m.save()
|
|
# All SKILL.md files should exist globally
|
|
for f in created:
|
|
if "SKILL.md" in str(f):
|
|
assert f.exists(), f"{f} does not exist"
|
|
# 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 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)
|
|
|
|
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")
|
|
removed, skipped = i.uninstall(tmp_path, m)
|
|
assert not modified_file.exists(), (
|
|
"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):
|
|
"""Pre-existing non-speckit global skills should survive Hermes uninstall."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
i = get_integration(self.KEY)
|
|
# Create a foreign skill in the global dir first
|
|
global_skills_dir = i._hermes_home_skills_dir()
|
|
foreign_dir = global_skills_dir / "other-tool"
|
|
foreign_dir.mkdir(parents=True, exist_ok=True)
|
|
(foreign_dir / "SKILL.md").write_text("# Foreign skill\n")
|
|
|
|
m = IntegrationManifest(self.KEY, tmp_path)
|
|
i.setup(tmp_path, m)
|
|
|
|
# Run teardown to verify foreign skill survives uninstall
|
|
i.teardown(tmp_path, m)
|
|
|
|
assert (foreign_dir / "SKILL.md").exists(), (
|
|
"Foreign skill was removed by teardown"
|
|
)
|
|
|
|
def test_complete_file_inventory_sh(self, tmp_path, monkeypatch):
|
|
"""Override: Hermes init produces no local SKILL.md files,
|
|
only the empty .hermes/skills/ marker."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
from typer.testing import CliRunner
|
|
from specify_cli import app
|
|
|
|
project = tmp_path / f"inventory-sh-{self.KEY}"
|
|
project.mkdir()
|
|
old_cwd = Path.cwd()
|
|
import os
|
|
try:
|
|
os.chdir(project)
|
|
result = CliRunner().invoke(app, [
|
|
"init", "--here", "--integration", self.KEY,
|
|
"--script", "sh", "--no-git", "--ignore-agent-tools",
|
|
], catch_exceptions=False)
|
|
finally:
|
|
os.chdir(old_cwd)
|
|
assert result.exit_code == 0, f"init failed: {result.output}"
|
|
actual = sorted(
|
|
p.relative_to(project).as_posix()
|
|
for p in project.rglob("*") if p.is_file()
|
|
)
|
|
# Ensure no .hermes/skills/speckit-*/SKILL.md in project dir
|
|
hermes_skill_files = [f for f in actual if f.startswith(".hermes/skills/speckit-")]
|
|
assert hermes_skill_files == [], (
|
|
f"Expected no local SKILL.md files, found: {hermes_skill_files}"
|
|
)
|
|
# Ensure the marker exists (empty dir won't appear in file listing)
|
|
assert (project / ".hermes" / "skills").is_dir()
|
|
|
|
def test_complete_file_inventory_ps(self, tmp_path, monkeypatch):
|
|
"""Override: Same as sh variant but for PowerShell script type."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
from typer.testing import CliRunner
|
|
from specify_cli import app
|
|
|
|
project = tmp_path / f"inventory-ps-{self.KEY}"
|
|
project.mkdir()
|
|
old_cwd = Path.cwd()
|
|
import os
|
|
try:
|
|
os.chdir(project)
|
|
result = CliRunner().invoke(app, [
|
|
"init", "--here", "--integration", self.KEY,
|
|
"--script", "ps", "--no-git", "--ignore-agent-tools",
|
|
], catch_exceptions=False)
|
|
finally:
|
|
os.chdir(old_cwd)
|
|
assert result.exit_code == 0, f"init failed: {result.output}"
|
|
actual = sorted(
|
|
p.relative_to(project).as_posix()
|
|
for p in project.rglob("*") if p.is_file()
|
|
)
|
|
hermes_skill_files = [f for f in actual if f.startswith(".hermes/skills/speckit-")]
|
|
assert hermes_skill_files == [], (
|
|
f"Expected no local SKILL.md files, found: {hermes_skill_files}"
|
|
)
|
|
assert (project / ".hermes" / "skills").is_dir()
|
|
|
|
def test_install_uninstall_cleanup(self, tmp_path, monkeypatch):
|
|
"""Verify global skills are cleaned and local marker is removed."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
i = get_integration(self.KEY)
|
|
m = IntegrationManifest(self.KEY, tmp_path)
|
|
created = i.setup(tmp_path, m)
|
|
|
|
# Verify global skills exist
|
|
global_skills = [
|
|
f for f in created
|
|
if "SKILL.md" in str(f)
|
|
and str(f).startswith(str(home / ".hermes"))
|
|
]
|
|
assert len(global_skills) > 0
|
|
for f in global_skills:
|
|
assert f.exists()
|
|
|
|
# Verify local marker exists
|
|
assert (tmp_path / ".hermes" / "skills").is_dir()
|
|
|
|
# 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:
|
|
assert not f.exists(), f"{f} should have been removed"
|
|
|
|
# Local marker removed
|
|
assert not (tmp_path / ".hermes" / "skills").exists(), (
|
|
"Local marker should be removed on teardown"
|
|
)
|
|
|
|
|
|
class TestHermesAutoPromote:
|
|
"""--ai hermes auto-promotes to integration path."""
|
|
|
|
def test_ai_hermes_without_ai_skills_auto_promotes(self, tmp_path, monkeypatch):
|
|
"""--ai hermes should work the same as --integration hermes,
|
|
creating global skills and a local marker."""
|
|
home = _fake_home(tmp_path)
|
|
monkeypatch.setattr(Path, "home", lambda: home)
|
|
|
|
from typer.testing import CliRunner
|
|
from specify_cli import app
|
|
|
|
runner = CliRunner()
|
|
target = tmp_path / "test-proj"
|
|
result = runner.invoke(app, [
|
|
"init", str(target),
|
|
"--ai", "hermes",
|
|
"--no-git",
|
|
"--ignore-agent-tools",
|
|
"--script", "sh",
|
|
])
|
|
|
|
assert result.exit_code == 0, f"init --ai hermes failed: {result.output}"
|
|
# Skills should be in global ~/.hermes/skills/
|
|
assert (home / ".hermes" / "skills" / "speckit-plan" / "SKILL.md").exists()
|
|
# Local marker should exist
|
|
assert (target / ".hermes" / "skills").is_dir()
|
|
# No SKILL.md files in project-local dir
|
|
local_skills = list((target / ".hermes" / "skills").iterdir())
|
|
assert local_skills == [], f"Local skills dir should be empty, got: {local_skills}"
|