mirror of
https://github.com/github/spec-kit.git
synced 2026-07-07 14:39:00 +08:00
feat: add Hermes Agent integration (with review fixes) (#2651)
* 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>
This commit is contained in:
committed by
GitHub
parent
0a8f31ef18
commit
5a50b75adb
@@ -654,15 +654,28 @@ class CommandRegistrar:
|
||||
) -> Path:
|
||||
"""Return the agent command directory, falling back to legacy_dir.
|
||||
|
||||
When the canonical directory (``agent_config["dir"]``) does not
|
||||
exist but a ``legacy_dir`` is configured and present on disk,
|
||||
returns the legacy path and emits a deprecation warning advising
|
||||
the user to upgrade.
|
||||
Supports project-relative paths (e.g. ``.claude/skills/``),
|
||||
home-relative paths (e.g. ``~/.hermes/skills``), and absolute
|
||||
paths — the ``agent_config["dir"]`` value is resolved verbatim
|
||||
when absolute or starting with ``~/``, or joined with
|
||||
``project_root`` when relative.
|
||||
|
||||
When the canonical directory does not exist but a ``legacy_dir``
|
||||
is configured and present on disk, returns the legacy path and
|
||||
emits a deprecation warning advising the user to upgrade.
|
||||
|
||||
Integrations that do not declare ``legacy_dir`` get the canonical
|
||||
path unconditionally — no fallback, no warning.
|
||||
"""
|
||||
agent_dir = project_root / agent_config["dir"]
|
||||
dir_str = agent_config["dir"]
|
||||
if dir_str.startswith("~"):
|
||||
# Use Path.home() + remainder instead of expanduser() so tests
|
||||
# that monkeypatch Path.home() can properly isolate the home dir.
|
||||
# expanduser() uses OS env/user lookup and ignores monkeypatches.
|
||||
agent_dir = Path.home() / dir_str[1:].lstrip("/")
|
||||
else:
|
||||
p = Path(dir_str)
|
||||
agent_dir = p if p.is_absolute() else project_root / p
|
||||
if not agent_dir.exists():
|
||||
legacy = agent_config.get("legacy_dir")
|
||||
if legacy:
|
||||
@@ -704,6 +717,15 @@ class CommandRegistrar:
|
||||
|
||||
self._ensure_configs()
|
||||
for agent_name, agent_config in self.AGENT_CONFIGS.items():
|
||||
# Check detect_dir first (project-local marker) if configured,
|
||||
# falling back to the resolved dir for output. This prevents
|
||||
# global dirs (e.g. ~/.hermes/skills) from causing false
|
||||
# detection in every project.
|
||||
detect_dir_str = agent_config.get("detect_dir")
|
||||
if detect_dir_str:
|
||||
detect_path = project_root / detect_dir_str
|
||||
if not detect_path.exists():
|
||||
continue
|
||||
agent_dir = self._resolve_agent_dir(
|
||||
agent_name, agent_config, project_root,
|
||||
)
|
||||
@@ -755,6 +777,11 @@ class CommandRegistrar:
|
||||
for agent_name, agent_config in self.AGENT_CONFIGS.items():
|
||||
if agent_config.get("extension") == "/SKILL.md":
|
||||
continue
|
||||
detect_dir_str = agent_config.get("detect_dir")
|
||||
if detect_dir_str:
|
||||
detect_path = project_root / detect_dir_str
|
||||
if not detect_path.exists():
|
||||
continue
|
||||
agent_dir = self._resolve_agent_dir(
|
||||
agent_name, agent_config, project_root,
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user