feat: add Hermes Agent support#1292
Conversation
📝 WalkthroughWalkthroughHermes Agent support adds tool registration and detection, skills-only initialization and updates, setup guidance, specification coverage, tests, and documentation for external skill directories. ChangesHermes Agent support
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
# Conflicts: # docs/supported-tools.md
alfred-openspec
left a comment
There was a problem hiding this comment.
Hermes only scans its home skills directory unless skills.external_dirs is configured, but this PR writes project .hermes/skills and only documents the manual step, so init can report success for assets Hermes ignores by default. Please add adapterless init and registry coverage and either make the generated path usable automatically or emit a clear setup warning during init and update.
…pdate Hermes only loads skills from ~/.hermes/skills unless the project .hermes/skills directory is added to skills.external_dirs in ~/.hermes/config.yaml, so init could report success for skills Hermes ignores. Add a setupNote field to AIToolOption, print it after init and update (including the up-to-date path), and cover the adapterless init path, the adapter registry, and both update paths with tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed aab39a3 addressing @alfred-openspec's review:
Verified end-to-end with the built CLI ( |
alfred-openspec
left a comment
There was a problem hiding this comment.
The remediation covers the adapterless init contract and surfaces the required Hermes external_dirs setup note during init, normal update, and the already-up-to-date path. The manual configuration limitation is now explicit, focused coverage is present, and full CI is green.
Resolve conflicts with Fission-AI#1354 (displayMissingCoreWorkflowsNote rename) and Fission-AI#1208 (kimi-code migration tests): keep both sides' tests, call the renamed note method alongside the Hermes displaySetupNotes hook. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Merged |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/core/update.test.ts (1)
216-217: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize path separators for cross-platform test reliability.
The CLI output expectation hard-codes Unix path separators (
/). If the implementation logs platform-specific paths natively (using\), this test will fail on Windows. As per coding guidelines, do not hard-code Unix path separators in CLI output expectations; instead, normalize both actual and expected strings before comparing them to guarantee cross-platform consistency.💻 Proposed fix
- const logCalls = consoleSpy.mock.calls.flat().map(String); - expect(logCalls.some((entry) => entry.includes('.kimi/skills') && entry.includes('.kimi-code/skills'))).toBe(true); + const logCalls = consoleSpy.mock.calls.flat().map(String).map(entry => entry.replace(/\\/g, '/')); + expect(logCalls.some((entry) => entry.includes('.kimi/skills') && entry.includes('.kimi-code/skills'))).toBe(true);🤖 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/update.test.ts` around lines 216 - 217, Update the log assertion around logCalls to normalize path separators in the captured CLI output before checking for the .kimi/skills and .kimi-code/skills paths, and apply the same normalization to the expected path fragments so the test passes consistently across platforms.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/core/update.test.ts`:
- Around line 216-217: Update the log assertion around logCalls to normalize
path separators in the captured CLI output before checking for the .kimi/skills
and .kimi-code/skills paths, and apply the same normalization to the expected
path fragments so the test passes consistently across platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f38dbbd6-28e2-4ad6-81ec-0d69bbbbcb11
📒 Files selected for processing (7)
docs/supported-tools.mdopenspec/specs/ai-tool-paths/spec.mdsrc/core/config.tssrc/core/init.tssrc/core/update.tstest/core/init.test.tstest/core/update.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/core/init.ts
- src/core/update.ts
- openspec/specs/ai-tool-paths/spec.md
- docs/supported-tools.md
- src/core/config.ts
alfred-openspec
left a comment
There was a problem hiding this comment.
Verified the rebase keeps the Hermes adapterless init contract, skills.external_dirs setup note across init and both update paths, and focused registry/detection coverage while preserving the renamed core-workflow note and Kimi migration tests from main. The final diff is scoped, mergeable, and fully green across Windows, macOS, and Linux.
Union resolutions with the hermes (Fission-AI#1292), zcode (Fission-AI#1209), and kimi-code (Fission-AI#1208) merges: combined tool-ID list, both detection test groups, the CodeArts assertion folded into the shared skills-only registry test, and all adapterless init tests kept. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
Adds Hermes Agent as a supported OpenSpec tool using project-local skills under
.hermes/skills/.Continues the Hermes support work from #1095 and addresses the requested review changes there.
Closes #1333
Changes
hermestoAI_TOOLSwithskillsDir: '.hermes'.hermes,HERMES.md, or.hermes.mdCONTEXT.md/openspec-*invocation, andskills.external_dirssetupValidation
./node_modules/.bin/vitest run test/core/available-tools.test.ts./node_modules/.bin/tsc --noEmit./node_modules/.bin/eslint src/(passes with existing warning insrc/core/references.ts)node build.jsSummary by CodeRabbit
HERMES.md,.hermes.md, and.hermes/) and mapping to a.hermesskills directory.skills.external_dirsin~/.hermes/config.yaml.