feat: add Hermes Agent support with global skill installation#1350
feat: add Hermes Agent support with global skill installation#1350dev-v wants to merge 3 commits into
Conversation
Hermes Agent discovers skills from ~/.hermes/skills/ (user-global) rather than a project-local directory. This introduces a generic installDir field on AIToolOption to separate installation path from detection path, enabling Hermes and any future global-scope agent. Key changes: - Add installDir optional field to AIToolOption (config.ts) - Add Hermes Agent to AI_TOOLS with skillsDir='.hermes', installDir='~/.hermes/skills' - Add resolveSkillsDir() and resolveMarkerDir() shared helpers - getToolSkillStatus uses marker directory for installDir tools to prevent global skills from polluting project-level detection - init/update create project-local marker directory for installDir tools while writing skills to the global path - No command adapter — Hermes invokes skills via /skill:openspec-* - Includes OpenSpec change artifacts (proposal, design, specs, tasks) Tested: build ✅ | lint ✅ (0 errors) | test ✅ (1887/1887 passed) Pattern inspired by SpecKit's detect_dir/dir separation for Hermes Agent integration.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughHermes Agent support adds global skill installation at ChangesHermes Agent support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InitOrUpdate
participant PathResolvers
participant HermesSkills
participant ProjectMarker
User->>InitOrUpdate: initialize or update Hermes
InitOrUpdate->>PathResolvers: resolve install and marker directories
PathResolvers-->>InitOrUpdate: global install path and project marker path
InitOrUpdate->>HermesSkills: generate skills globally
InitOrUpdate->>ProjectMarker: create marker directory
ProjectMarker-->>User: Hermes configuration detected for project
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/core/available-tools.test.ts (1)
181-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest correctly validates Hermes detection; consider also asserting
installDir.The test properly creates
.hermes, verifies detection, and checksvalue,name, andskillsDir. Since this PR introduces theinstallDirfield, adding an assertion for it would guard against accidental removal of the global installation path configuration.💡 Suggested addition
expect(hermesTool?.skillsDir).toBe('.hermes'); + expect(hermesTool?.installDir).toBe('~/.hermes/skills'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/available-tools.test.ts` around lines 181 - 195, Extend the Hermes detection test around getAvailableTools to assert that the detected hermesTool also has the expected global installDir value introduced by this change. Keep the existing value, name, and skillsDir assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/supported-tools.md`:
- Around line 62-63: Update the Hermes Agent footnote to state that `.hermes` is
the project-local marker directory used for auto-detection, while
`.hermes/skills/` may be created as a subdirectory during initialization; do not
describe `.hermes/skills/` itself as the detection directory.
In `@test/core/shared/tool-detection.test.ts`:
- Around line 383-422: Make the Hermes global-install tests deterministic by
mocking or injecting the global install root used by getToolSkillStatus and
getToolVersionStatus, then create controlled SKILL.md fixtures and assert exact
skill counts instead of accepting environment-dependent values. Extend the
getToolVersionStatus tests to verify generatedByVersion and needsUpdate for
matching and differing versions, while preserving the existing marker-directory
setup. Run the focused test with pnpm exec vitest run
test/core/shared/tool-detection.test.ts.
---
Nitpick comments:
In `@test/core/available-tools.test.ts`:
- Around line 181-195: Extend the Hermes detection test around getAvailableTools
to assert that the detected hermesTool also has the expected global installDir
value introduced by this change. Keep the existing value, name, and skillsDir
assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fab203cf-e81f-4ca3-848a-cb3095548e4e
📒 Files selected for processing (18)
docs/supported-tools.mdopenspec/changes/add-hermes-agent-support/.openspec.yamlopenspec/changes/add-hermes-agent-support/README.mdopenspec/changes/add-hermes-agent-support/design.mdopenspec/changes/add-hermes-agent-support/proposal.mdopenspec/changes/add-hermes-agent-support/specs/ai-tool-paths/spec.mdopenspec/changes/add-hermes-agent-support/specs/cli-init/spec.mdopenspec/changes/add-hermes-agent-support/specs/cli-update/spec.mdopenspec/changes/add-hermes-agent-support/tasks.mdsrc/core/config.tssrc/core/init.tssrc/core/migration.tssrc/core/profile-sync-drift.tssrc/core/shared/index.tssrc/core/shared/tool-detection.tssrc/core/update.tstest/core/available-tools.test.tstest/core/shared/tool-detection.test.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks, this is close, but I think the migration/drift path still leaks global Hermes state into project-local decisions.
scanInstalledWorkflowArtifacts() now reads resolveSkillsDir() for Hermes, which is ~/.hermes/skills, without first checking the project-local marker. During one-time profile migration, a project that merely has .hermes/ detected can inherit workflows and delivery=skills from another project global Hermes install. That violates the PR core invariant that global skills must not make a project look configured. Please gate global-install tools on resolveMarkerDir() in migration/drift scanning, and add a regression with global Hermes skills present but no project marker.
Address PR Fission-AI#1350 review feedback: - migration.ts: scanInstalledWorkflowArtifacts() now checks the project-local marker directory before scanning the global skills directory for installDir tools (e.g. Hermes), preventing cross-project state leakage - migration.test.ts: add regression tests for global skills with and without project-local marker - tool-detection.test.ts: make Hermes tests deterministic by mocking os.homedir(), assert exact skillCount and version behavior - available-tools.test.ts: assert installDir field on Hermes detection - supported-tools.md: clarify .hermes (detection) vs .hermes/skills/ (marker)
|
@alfred-openspec Good catch — you're right, the migration path was violating the PR's own invariant. Fixed in a2c3e56. Root cause: Fix: Added a marker gate in if (tool.installDir) {
const markerDir = resolveMarkerDir(tool, projectPath);
if (!fs.existsSync(markerDir)) continue;
}Regression test (
Also addressed the CodeRabbit comments in the same commit:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/shared/tool-detection.test.ts (1)
376-506: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHermes global-install tests are now deterministic and well-structured.
The past review concern about non-deterministic Hermes tests has been fully addressed:
os.homedir()is mocked to a temp directory,SKILL.mdfixtures are created with controlledgeneratedByversions, exactskillCountvalues are asserted, andgeneratedByVersion/needsUpdateare verified for matching, differing, and missing version scenarios.One optional improvement: the
createGlobalSkillhelper and thebeforeEach/afterEachsetup are duplicated across thegetToolSkillStatusandgetToolVersionStatusdescribe blocks (lines 396-403 / 462-469 and 379-390 / 451-460). Extracting these into a shared helper or a parent describe block would reduce duplication if either suite's setup logic needs to change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/shared/tool-detection.test.ts` around lines 376 - 506, Optionally consolidate the duplicated Hermes test setup shared by getToolSkillStatus and getToolVersionStatus: move the fake-home beforeEach/afterEach lifecycle and createGlobalSkill helper into a common parent describe or shared helper. Preserve the existing isolated temporary home behavior and all test assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/core/shared/tool-detection.test.ts`:
- Around line 376-506: Optionally consolidate the duplicated Hermes test setup
shared by getToolSkillStatus and getToolVersionStatus: move the fake-home
beforeEach/afterEach lifecycle and createGlobalSkill helper into a common parent
describe or shared helper. Preserve the existing isolated temporary home
behavior and all test assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 339df69e-a428-4b9d-8316-de0fbda4f564
📒 Files selected for processing (5)
docs/supported-tools.mdsrc/core/migration.tstest/core/available-tools.test.tstest/core/migration.test.tstest/core/shared/tool-detection.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/supported-tools.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test/core/available-tools.test.ts
- src/core/migration.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for the follow-up commit. The migration marker gate looks like the right direction, but I still see a blocker in update cleanup: Hermes resolves skillsDir to ~/.hermes/skills, so removeUnselectedSkillDirs/removeSkillDirs can delete user-global OpenSpec skills from a project-local update.\n\nConcrete case: Project A installs Hermes with a custom profile, Project B has a core-profile Hermes marker, then running openspec update in Project B prunes non-core workflow directories out of ~/.hermes/skills and breaks Project A. The commands-only path has the same global-delete issue, and Hermes has no command adapter to replace those skills.\n\nI think global-install tools should be additive/overwrite-only during update, or cleanup should only touch project-local marker state. Please add a Hermes regression for custom-to-core update and delivery=commands so a project-local update cannot prune ~/.hermes/skills.
Global-install tools (installDir set, e.g. Hermes) must not delete skill directories from the shared global path during per-project update or init. A project-local profile or delivery change would otherwise destroy skills that other projects on the machine depend on. Hermes has no command adapter, so delivery=commands cannot substitute skills with commands — deletion leaves the user with nothing. Added installDir guards on all skill-deletion call sites: - update.ts: removeUnselectedSkillDirs (profile change) - update.ts: removeSkillDirs (delivery=commands) - init.ts: removeSkillDirs (delivery=commands) Global-install tools are now additive/overwrite-only: existing skills are overwritten, new skills are created, but no skill directories are deleted from the global path. Project-local tools are unaffected. Added regression tests: - custom→core profile switch preserves non-core skill dirs in ~/.hermes/skills/ - delivery=commands preserves skill dirs in ~/.hermes/skills/ Addresses second round of CHANGES_REQUESTED from @alfred-openspec on PR Fission-AI#1350.
|
@alfred-openspec You're right again — the cleanup path was the remaining hole. Fixed in 488cf28. Root cause: Fix: Added // update.ts — inside the per-tool update loop
// Skills delivery: skip deselected-workflow cleanup for global-install tools
if (!tool.installDir) {
removedDeselectedSkillCount += await this.removeUnselectedSkillDirs(skillsDir, desiredWorkflows);
}
// Commands-only delivery: skip all-skills cleanup for global-install tools
if (!shouldGenerateSkills && !tool.installDir) {
removedSkillCount += await this.removeSkillDirs(skillsDir);
}// init.ts — same guard on the commands-only path
if (!shouldGenerateSkills && !toolConfig?.installDir) {
removedSkillCount += await this.removeSkillDirs(skillsDir);
}Global-install tools are now additive/overwrite-only during update and init:
For project-local tools (no Design rationale: Global skills are shared across all projects on the machine. A per-project profile or delivery change must not delete skills that other projects depend on. Hermes has no command adapter, so Regression tests (
Both tests mock Artifacts updated: All 1894 tests pass. |
2 participants
Summary
Adds Hermes Agent (by Nous Research) as a supported tool, enabling
openspec init --tools hermesandopenspec updateto install OpenSpec skills where Hermes discovers them.Unlike all currently supported tools, Hermes discovers skills from a user-global directory (
~/.hermes/skills/) rather than a project-local path. OpenSpec'sskillsDirfield served double duty (detection marker + installation target), which breaks for global-scope tools. This PR introduces a genericinstallDirfield to separate the two concerns.Approach
Pattern inspired by SpecKit's Hermes integration, which uses a
detect_dir/dirseparation for the same problem. This PR adapts that pattern to OpenSpec's architecture:detect_dir(project-local)skillsDir(project-local, unchanged)dir(global)installDir(new field, global).hermes/skills/.hermes/skills/installDiris a generic mechanism — not Hermes-specific. Any future agent with global skill discovery can use it by setting the field.What Changes
AIToolOption.installDir— new optional field; when set, skills install to the expanded path (supports~) instead of<projectRoot>/<skillsDir>/skillsAI_TOOLS:skillsDir: '.hermes',installDir: '~/.hermes/skills'resolveSkillsDir()/resolveMarkerDir()— shared helpers intool-detection.ts, replacing inline path joins acrossinit,update,profile-sync-drift, andmigrationgetToolSkillStatus— forinstallDirtools, uses marker directory (not global path) to determine "configured", preventing global skills from polluting project-level detectioninit/update— create project-local marker directory (.hermes/skills/) forinstallDirtools while writing skills to the global path/skill:openspec-*, matching the Kimi/ForgeCode/Vibe patternTesting
pnpm run build✅ (TypeScript compilation)pnpm run lint✅ (0 errors)pnpm test✅ (1887/1887 passed)New test coverage:
test/core/available-tools.test.ts— Hermes detection via.hermes/directorytest/core/shared/tool-detection.test.ts—resolveSkillsDir,resolveMarkerDir, HermesgetToolSkillStatus/getToolVersionStatuswith marker-based configured checkOpenSpec Change Artifacts
This PR includes a complete OpenSpec change proposal at
openspec/changes/add-hermes-agent-support/:proposal.md— motivation and impactdesign.md— 5 design decisions with rationale and alternativesspecs/ai-tool-paths/spec.md—installDirfield, Hermes paths, helper requirementsspecs/cli-init/spec.md— global-install tool init behaviorspecs/cli-update/spec.md— global-install tool update behaviortasks.md— implementation task breakdownopenspec validate add-hermes-agent-supportpasses ✅Related
Summary by CodeRabbit
~/.hermes/skills/.installDir) to keep Hermes global skills isolated per project.