Skip to content

feat: add Hermes Agent integration (with review fixes)#2651

Open
majordave wants to merge 11 commits into
github:mainfrom
majordave:feat/hermes-integration
Open

feat: add Hermes Agent integration (with review fixes)#2651
majordave wants to merge 11 commits into
github:mainfrom
majordave:feat/hermes-integration

Conversation

@majordave
Copy link
Copy Markdown

@majordave majordave commented May 20, 2026

Description

Adds a new built-in integration for Hermes Agent (by Nous Research) to Spec Kit's integration system, enabling specify init --integration hermes / --ai hermes to scaffold Spec Kit commands as Hermes skills under ~/.hermes/skills/ with an AGENTS.md context file.

Based on the original work by @Zhaoxiaoguang001 in PR #2547, with the following fixes and improvements:

Changes from original PR #2547

  • src/specify_cli/integrations/hermes/__init__.py — Full Hermes integration:
    • Uses SkillsIntegration (skills-based, same as Claude/Codex)
    • Skills are written directly to ~/.hermes/skills/speckit-<name>/SKILL.md — no project-local copies (Hermes discovers skills globally). An empty .hermes/skills/ marker is created in the project so extension commands (e.g. git) can detect Hermes as active.
    • setup() now includes manifest.project_root safety checks matching the standard pattern
    • teardown() always removes global speckit-* skills (matching standard integration behaviour where all created files are cleaned up), plus the project-local marker and manifest
    • import yaml moved to module scope (was inside the per-template loop)
    • Removal tracking reports paths only after successful deletion (no false positives on failure)
  • src/specify_cli/__init__.py — CLI integration_uninstall now calls integration.teardown() instead of calling manifest.uninstall() directly, allowing custom teardown logic in integrations to run properly. Falls back to manifest.uninstall() when the integration class is missing from the registry.
  • integrations/catalog.json — Added hermes catalog entry
  • tests/integrations/test_integration_hermes.py — Uses shared SkillsIntegrationTests (32 tests, all passing). Every test that touches ~/.hermes/ monkeypatches Path.home() to a temp directory for hermetic, non-destructive execution.

Why global skills only?

Unlike Claude Code (.claude/skills/) or Codex (.agents/skills/) which load skills from a project-local directory, Hermes loads skills exclusively from ~/.hermes/skills/ (user home directory). Skills are written directly to the global directory — a project-local .hermes/skills/ directory is created empty as a detection marker for Spec Kit extension commands.

All global speckit-* skills are cleaned up on specify integration uninstall (standard behaviour, matching every other built-in integration).

Co-authored-by: Zhaoxiaoguang001 3357983213@qq.com

@majordave majordave requested a review from mnriem as a code owner May 20, 2026 19:25
majordave and others added 2 commits May 20, 2026 13:32
- 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>
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>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new built-in Hermes Agent integration to Spec Kit’s integration system, enabling specify init --integration hermes / --ai hermes to install Spec Kit command skills under Hermes’ expected skills directory and wire up project context via AGENTS.md.

Changes:

  • Introduces HermesIntegration (a SkillsIntegration subclass) and registers it as a built-in integration.
  • Updates specify integration uninstall to call integration.teardown() (enabling integrations to perform custom uninstall logic).
  • Adds Hermes to the integration catalog and adds a Hermes integration test suite based on shared SkillsIntegrationTests.
Show a summary per file
File Description
src/specify_cli/integrations/hermes/__init__.py New Hermes integration implementation (global skills install + custom teardown + CLI dispatch args).
src/specify_cli/integrations/__init__.py Registers Hermes as a built-in integration.
src/specify_cli/__init__.py Switches uninstall path to use integration.teardown() and adds an error when integration is missing from registry.
integrations/catalog.json Adds hermes entry to the integration catalog.
tests/integrations/test_integration_hermes.py Adds Hermes-specific integration tests, overriding shared assumptions about project-local skills.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 5/5 changed files
  • Comments generated: 6

Comment thread src/specify_cli/__init__.py Outdated
Comment thread tests/integrations/test_integration_hermes.py Outdated
Comment thread src/specify_cli/integrations/hermes/__init__.py
Comment thread src/specify_cli/integrations/hermes/__init__.py Outdated
Comment thread tests/integrations/test_integration_hermes.py
Comment thread tests/integrations/test_integration_hermes.py Outdated
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).
@majordave majordave force-pushed the feat/hermes-integration branch from 2c4604e to 70f5732 Compare May 22, 2026 17:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 5/5 changed files
  • Comments generated: 6

Hermes discovers them globally. A project-local marker directory
(``.hermes/skills/`` empty) is created so extension commands (e.g.
git) can detect Hermes as an active integration. Uninstall removes
both the marker and global skills.
Comment on lines +90 to +103
templates = self.list_command_templates()
if not templates:
return []

script_type = opts.get("script_type", "sh")
arg_placeholder = (
self.registrar_config.get("args", "$ARGUMENTS")
if self.registrar_config
else "$ARGUMENTS"
)

global_skills_dir = self._hermes_home_skills_dir()
global_skills_dir.mkdir(parents=True, exist_ok=True)

Comment on lines +113 to +125
# Parse frontmatter for description
frontmatter: dict[str, Any] = {}
if raw.startswith("---"):
parts = raw.split("---", 2)
if len(parts) >= 3:
import yaml

try:
fm = yaml.safe_load(parts[1])
if isinstance(fm, dict):
frontmatter = fm
except yaml.YAMLError:
pass
Comment on lines +225 to +227
if skill_file.exists():
removed.append(skill_file)
rmtree(skill_dir, ignore_errors=True)
Comment on lines +134 to +153
def test_modified_file_survives_uninstall(self, tmp_path, monkeypatch):
"""Override: Hermes global skills are removed on uninstall only
when force=True (no hash-based preservation since they're not in manifest)."""
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 only removed with force=True
removed, skipped = i.teardown(tmp_path, m, force=True)
assert not modified_file.exists(), (
"Modified global skill should be removed on teardown with force=True"
)
Comment on lines +80 to +88
"""Install command templates as global Hermes skills.

Writes each skill directly to
``~/.hermes/skills/speckit-<name>/SKILL.md`` where Hermes
discovers them at runtime. No project-local SKILL.md copies are
created — the global directory is the single source of truth.
A project-local marker (``.hermes/skills/`` empty) is created
so extension commands (e.g. git) can detect Hermes as an active
integration.
- 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)
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 5/5 changed files
  • Comments generated: 4

Comment on lines +46 to +51
registrar_config = {
"dir": ".hermes/skills",
"format": "markdown",
"args": "$ARGUMENTS",
"extension": "/SKILL.md",
}
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 3da5e0b — opted for option A: added first-class support for global/home-based agent dirs in CommandRegistrar._resolve_agent_dir (handles both ~ and absolute paths), and set Hermes' registrar_config.dir to ~/.hermes/skills. Extension/preset commands now resolve to the real global directory that Hermes searches at runtime.

Comment on lines +219 to +226
# Remove project-local marker directory if empty
local_skills_dir = project_root / ".hermes" / "skills"
if local_skills_dir.is_dir() and not any(local_skills_dir.iterdir()):
local_skills_dir.rmdir()
hermes_dir = project_root / ".hermes"
if hermes_dir.is_dir() and not any(hermes_dir.iterdir()):
hermes_dir.rmdir()

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in b938619 — updated teardown docstring to reflect that the marker dir is removed only when empty (the safe, conditional behavior).

Comment on lines +189 to +195
(foreign_dir / "SKILL.md").write_text("# Foreign skill\n")

m = IntegrationManifest(self.KEY, tmp_path)
i.setup(tmp_path, m)

assert (foreign_dir / "SKILL.md").exists(), "Foreign skill was removed"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in b938619 — test_pre_existing_skills_not_removed now calls i.teardown() after setup() and asserts the foreign skill still exists, properly validating the intended behavior.

Comment on lines +1947 to 1955
if not integration:
console.print(
f"[yellow]Warning:[/yellow] Integration '{key}' not found "
"in registry. Falling back to manifest-based cleanup."
)
removed, skipped = manifest.uninstall(project_root, force=force)
else:
removed, skipped = integration.teardown(project_root, manifest, force=force)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in b938619 — integration_switch Phase 1 now calls current_integration.teardown(project_root, old_manifest, force=force) instead of old_manifest.uninstall() + remove_context_section(), matching the pattern used in integration_uninstall. Custom teardown logic (Hermes global skills, etc.) now runs during switches.

majordave added 2 commits May 27, 2026 10:56
…gistrar

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.
- 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.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 3

Comment thread src/specify_cli/agents.py
Comment on lines +670 to +675
dir_str = agent_config["dir"]
if dir_str.startswith("~"):
agent_dir = Path(dir_str).expanduser()
else:
p = Path(dir_str)
agent_dir = p if p.is_absolute() else project_root / p
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in a90862b_resolve_agent_dir() now expands ~/... via Path.home() / dir_str[1:].lstrip('/') instead of expanduser(). This makes monkeypatched Path.home() effective for test isolation. expanduser() is reserved for ~user/... patterns only.

Comment on lines +185 to +188
# Create project-local marker directory so extension commands
# (e.g. git) can detect Hermes as an active integration.
# Hermes itself ignores this directory — skills live globally.
(project_root / ".hermes" / "skills").mkdir(parents=True, exist_ok=True)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in a90862b — Added detect_dir: '.hermes/skills' to Hermes' registrar_config. CommandRegistrar.register_commands_for_all_agents() and register_commands_for_non_skill_agents() now check detect_dir (project-local) before resolving the output dir. If the project-local marker doesn't exist, the agent is skipped entirely. This prevents global dirs like ~/.hermes/skills from causing false detection in unrelated projects.

Comment thread tests/test_extensions.py
# interfere — without a real skills dir, Hermes is skipped during
# command registration, keeping the test focused on Claude/Codex/etc.
fake_home = tmp_path / "home"
fake_home.mkdir()
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in a90862b — The existing monkeypatch.setattr(Path, 'home', lambda: fake_home) now works correctly because _resolve_agent_dir() uses Path.home() instead of expanduser(). Since fake_home/.hermes/skills doesn't exist, Hermes is properly skipped during command registration. No additional changes needed for this test.

Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Copilot feedback

…-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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants