feat(skills): add Hermes skill host#122
Merged
Merged
Conversation
Hermes reuses the Claude Code skill template and is integrated into all skill lifecycle commands (install, list, preflight, remove). Its home directory defaults to ~/.hermes and can be overridden via the HERMES_HOME environment variable. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbitRelease Notes
WalkthroughThis pull request adds support for Hermes as a new bundled-skill host to the Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/application/commands/skills/list.cli.test.ts (1)
256-297: Consider table-driving the host-specific startup-synchronized list tests.This Hermes case repeats the same arrange/act/assert structure already used for other hosts, which makes future host additions noisier.
♻️ Suggested refactor sketch
+async function expectStartupSynchronizedBundledList( + resolver: (env: Record<string, string | undefined>) => string, + hostLabel: string, +) { + const sandbox = await createCliSandbox(); + const hostHomeDirectory = resolver(sandbox.env); + try { + await mkdir(hostHomeDirectory, { recursive: true }); + await sandbox.run(["skills", "install", "oo"], { version: "9.9.9" }); + const result = await sandbox.run(["skills", "list"], { version: "9.9.9" }); + expect(result.exitCode).toBe(0); + expect(result.stderr).toBe(""); + expect(result.stdout).toBe( + [ + "✓ Found 3 oo-managed skills.", + "", + "oo", + ` Host: ${hostLabel}`, + " Source: bundled", + " Version: 9.9.9", + "", + "oo-find-skills", + ` Host: ${hostLabel}`, + " Source: bundled", + " Version: 9.9.9", + "", + "oo-create-skill", + ` Host: ${hostLabel}`, + " Source: bundled", + " Version: 9.9.9", + "", + ].join("\n"), + ); + } + finally { + await sandbox.cleanup(); + } +}-test("lists startup-synchronized Hermes bundled installs when Codex is not installed", async () => { - ... -}); +test("lists startup-synchronized Hermes bundled installs when Codex is not installed", async () => { + await expectStartupSynchronizedBundledList(resolveHermesHomeDirectory, "Hermes"); +});As per coding guidelines: "Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/list.cli.test.ts` around lines 256 - 297, This Hermes-specific test duplicates arrange/act/assert logic used for other hosts; refactor by table-driving host-specific startup-synchronized list tests: extract the repeated setup into a local factory (e.g., a helper that calls createCliSandbox(), creates the home dir via resolveHermesHomeDirectory or a generic resolve<Home> function, runs sandbox.run for install and list, and calls sandbox.cleanup), then replace the individual Hermes test with a parameterized test (using test.each or iterating over an array of host descriptors) that supplies host name, resolve function, and expected stdout strings; ensure you reuse sandbox.run, createCliSandbox, and sandbox.cleanup from the original code so each row performs the same arrange/act/assert with host-specific inputs.src/application/commands/skills/list.ts (1)
29-37: Unify host ordering and host-label mapping into one source of truth.Hermes had to be added in two separate host-key structures (
managedSkillHostOrderandreadManagedSkillHostLabel), which is easy to drift over time.♻️ Suggested refactor
-const managedSkillHostOrder = { - codex: 0, - claude: 1, - hermes: 2, - codebuddy: 3, - workbuddy: 4, - openclaw: 5, - qoderwork: 6, -} as const satisfies Record<BundledSkillAgentName, number>; +const managedSkillHostConfig = { + codex: { order: 0, labelKey: "skills.list.host.codex" }, + claude: { order: 1, labelKey: "skills.list.host.claude" }, + hermes: { order: 2, labelKey: "skills.list.host.hermes" }, + codebuddy: { order: 3, labelKey: "skills.list.host.codebuddy" }, + workbuddy: { order: 4, labelKey: "skills.list.host.workbuddy" }, + openclaw: { order: 5, labelKey: "skills.list.host.openclaw" }, + qoderwork: { order: 6, labelKey: "skills.list.host.qoderwork" }, +} as const satisfies Record<BundledSkillAgentName, { order: number; labelKey: string }>;-function readManagedSkillHostLabel( - hostName: BundledSkillAgentName, - context: Pick<CliExecutionContext, "translator">, -): string { - switch (hostName) { - case "claude": - return context.translator.t("skills.list.host.claude"); - case "codebuddy": - return context.translator.t("skills.list.host.codebuddy"); - case "codex": - return context.translator.t("skills.list.host.codex"); - case "hermes": - return context.translator.t("skills.list.host.hermes"); - case "openclaw": - return context.translator.t("skills.list.host.openclaw"); - case "qoderwork": - return context.translator.t("skills.list.host.qoderwork"); - case "workbuddy": - return context.translator.t("skills.list.host.workbuddy"); - default: - return hostName satisfies never; - } -} +function readManagedSkillHostLabel( + hostName: BundledSkillAgentName, + context: Pick<CliExecutionContext, "translator">, +): string { + return context.translator.t(managedSkillHostConfig[hostName].labelKey); +}-const hostOrderDifference - = managedSkillHostOrder[left.hostName] - managedSkillHostOrder[right.hostName]; +const hostOrderDifference + = managedSkillHostConfig[left.hostName].order + - managedSkillHostConfig[right.hostName].order;As per coding guidelines: "Consolidate multiple switch/map structures that share the same keys into a single configuration object (data-driven over parallel mappings)."
Also applies to: 320-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/list.ts` around lines 29 - 37, The two parallel maps (managedSkillHostOrder and readManagedSkillHostLabel) should be consolidated into one data-driven configuration: create a single const (e.g., managedSkillHosts) typed as Record<BundledSkillAgentName, { order: number; label: string }>, move each host's numeric order and human label into that object (including hermes), then change all usages that read managedSkillHostOrder[...] or call readManagedSkillHostLabel(...) to reference managedSkillHosts[host].order or .label; also update the code regions around the other occurrence referenced in the comment to use the unified object and remove the duplicate map/function so there’s one source of truth.src/application/commands/skills/index.test.ts (1)
824-906: Extract repeated multi-host skill-directory lists into a local helper in each test.Hermes support required updates across several parallel host arrays; centralizing the list in one local variable per test would reduce future misses.
As per coding guidelines: "Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup."
Also applies to: 1333-1446
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/index.test.ts` around lines 824 - 906, The test "uninstalls a published skill from every existing supported host" repeats the same list of per-host skill directory variables (codexSkillDirectoryPath, claudeSkillDirectoryPath, hermesSkillDirectoryPath, codeBuddySkillDirectoryPath, workBuddySkillDirectoryPath, openClawSkillDirectoryPath, qoderWorkSkillDirectoryPath) multiple times; extract these into a single local array (e.g. const skillDirectoryPaths = [...]) at the top of the test and replace every repeated for-loop and expectation that iterates those paths with iterations over skillDirectoryPaths, and similarly reuse that array when creating directories, writing files, asserting stdout/stderr, and cleaning up; use the existing resolver helpers (resolveCodexHomeDirectory, resolveClaudeHomeDirectory, resolveHermesHomeDirectory, etc.) to populate the array and apply the same refactor for the similar block referenced at lines 1333-1446.src/application/commands/skills/check.test.ts (1)
114-146: Reduce repeated preflight success test scaffolding with a shared helper.This new Hermes case follows the same setup/run/assert/cleanup pattern used by other agent tests. Consider extracting a local helper (or a small table-driven loop) to reduce copy-paste and future drift.
As per coding guidelines,
**/*.test.{ts,tsx}: “Extract repeated setup (mocks, stubs, setup objects) into local factory functions in test files; avoid copy-pasting test setup”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/check.test.ts` around lines 114 - 146, Extract the repeated preflight test scaffolding into a local helper (e.g. runPreflightAgentCheck) that takes the agent name and expected stdout path; move the sandbox creation (createCliSandbox), environment/path resolution (resolveHermesHomeDirectory, resolveStorePaths, resolveLocalSkillCanonicalRootDirectoryPath), mkdir, sandbox.run(["skills","preflight","--agent", agent]) assertions (exitCode, stderr, stdout) and sandbox.cleanup into that helper so the Hermes test just calls runPreflightAgentCheck("hermes", canonicalRootDirectoryPath) (or use a small table-driven loop to call the helper for multiple agents). Ensure the helper returns/throws so tests still fail on errors and keep unique references to sandbox.run and sandbox.cleanup as shown.src/application/commands/skills/embedded-assets.ts (1)
141-149: Deduplicate Claude/Hermes registry definitions to prevent drift.Hermes intentionally reuses Claude-compatible assets, but each block currently duplicates the same structure. Reuse shared definitions per skill to keep one source of truth.
♻️ Proposed refactor pattern
+const ooClaudeCompatibleDefinition = { + files: [ + { + relativePath: "SKILL.md", + sourcePath: ooClaudeSkillPath, + }, + ...ooClaudeCompatibleReferenceFiles, + ], +} as const satisfies BundledSkillDefinition; + +const ooFindSkillsClaudeCompatibleDefinition = { + files: [ + { + relativePath: "SKILL.md", + sourcePath: ooFindSkillsClaudeSkillPath, + }, + { + relativePath: "references/oo-cli-contract.md", + sourcePath: ooFindSkillsClaudeCliContractPath, + }, + ], +} as const satisfies BundledSkillDefinition; + +const ooCreateSkillClaudeCompatibleDefinition = { + files: [ + { + relativePath: "SKILL.md", + sourcePath: ooCreateSkillClaudeSkillPath, + }, + ], +} as const satisfies BundledSkillDefinition; const bundledSkillRegistry = { "oo": { - claude: { ... }, - hermes: { ... }, + claude: ooClaudeCompatibleDefinition, + hermes: ooClaudeCompatibleDefinition, }, "oo-find-skills": { - claude: { ... }, - hermes: { ... }, + claude: ooFindSkillsClaudeCompatibleDefinition, + hermes: ooFindSkillsClaudeCompatibleDefinition, }, "oo-create-skill": { - claude: { ... }, - hermes: { ... }, + claude: ooCreateSkillClaudeCompatibleDefinition, + hermes: ooCreateSkillClaudeCompatibleDefinition, }, };As per coding guidelines: “Use factory functions when multiple definitions share the same shape with only one or two varying fields.”
Also applies to: 216-227, 298-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/embedded-assets.ts` around lines 141 - 149, The hermes registry block is duplicating the Claude-compatible asset structure; create a shared factory (e.g., makeClaudeCompatibleRegistry or buildRegistryFiles) that returns the files array used by both the "claude" and "hermes" entries so each skill (referenced by ooClaudeSkillPath and ooClaudeCompatibleReferenceFiles) reuses one source of truth; replace the inlined hermes: { files: [...] } and the other duplicated blocks (around the other occurrences) with calls to that factory, passing only the varying fields (like relativePath/sourcePath) so the shape is centralized and duplication is removed.src/application/commands/skills/embedded-assets.test.ts (1)
35-43: Assert Hermes parity against Claude directly to avoid duplicated expectations.Since Hermes is intentionally Claude-compatible here, these three Hermes expected arrays can be derived from Claude assertions instead of repeating literals.
♻️ Proposed refactor
- expect(getBundledSkillFiles("oo", "hermes").map(file => file.relativePath)).toEqual([ - "SKILL.md", - "references/auth-and-billing.md", - "references/search-and-selection.md", - "references/package-execution.md", - "references/connector-execution.md", - "references/file-transfer.md", - "references/task-lifecycle.md", - ]); + expect(getBundledSkillFiles("oo", "hermes").map(file => file.relativePath)).toEqual( + getBundledSkillFiles("oo", "claude").map(file => file.relativePath), + ); - expect( - getBundledSkillFiles("oo-find-skills", "hermes").map( - file => file.relativePath, - ), - ).toEqual([ - "SKILL.md", - "references/oo-cli-contract.md", - ]); + expect( + getBundledSkillFiles("oo-find-skills", "hermes").map( + file => file.relativePath, + ), + ).toEqual( + getBundledSkillFiles("oo-find-skills", "claude").map( + file => file.relativePath, + ), + ); - expect( - getBundledSkillFiles("oo-create-skill", "hermes").map( - file => file.relativePath, - ), - ).toEqual([ - "SKILL.md", - ]); + expect( + getBundledSkillFiles("oo-create-skill", "hermes").map( + file => file.relativePath, + ), + ).toEqual( + getBundledSkillFiles("oo-create-skill", "claude").map( + file => file.relativePath, + ), + );Based on learnings: “When extracting a shared utility from production code, also replace any test helpers or inline expressions that duplicate the same logic in test files.”
Also applies to: 97-104, 152-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/application/commands/skills/embedded-assets.test.ts` around lines 35 - 43, Replace the duplicated Hermes literal expectations with a direct parity assertion against Claude: compute the expected array once by calling getBundledSkillFiles("oo", "claude").map(f => f.relativePath) and then assert that getBundledSkillFiles("oo", "hermes").map(f => f.relativePath) equals that array; apply the same refactor pattern to the other duplicated Hermes assertions referenced (the blocks around lines 97-104 and 152-158) so all Hermes expectations derive from Claude rather than repeating literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/application/commands/skills/check.test.ts`:
- Around line 114-146: Extract the repeated preflight test scaffolding into a
local helper (e.g. runPreflightAgentCheck) that takes the agent name and
expected stdout path; move the sandbox creation (createCliSandbox),
environment/path resolution (resolveHermesHomeDirectory, resolveStorePaths,
resolveLocalSkillCanonicalRootDirectoryPath), mkdir,
sandbox.run(["skills","preflight","--agent", agent]) assertions (exitCode,
stderr, stdout) and sandbox.cleanup into that helper so the Hermes test just
calls runPreflightAgentCheck("hermes", canonicalRootDirectoryPath) (or use a
small table-driven loop to call the helper for multiple agents). Ensure the
helper returns/throws so tests still fail on errors and keep unique references
to sandbox.run and sandbox.cleanup as shown.
In `@src/application/commands/skills/embedded-assets.test.ts`:
- Around line 35-43: Replace the duplicated Hermes literal expectations with a
direct parity assertion against Claude: compute the expected array once by
calling getBundledSkillFiles("oo", "claude").map(f => f.relativePath) and then
assert that getBundledSkillFiles("oo", "hermes").map(f => f.relativePath) equals
that array; apply the same refactor pattern to the other duplicated Hermes
assertions referenced (the blocks around lines 97-104 and 152-158) so all Hermes
expectations derive from Claude rather than repeating literals.
In `@src/application/commands/skills/embedded-assets.ts`:
- Around line 141-149: The hermes registry block is duplicating the
Claude-compatible asset structure; create a shared factory (e.g.,
makeClaudeCompatibleRegistry or buildRegistryFiles) that returns the files array
used by both the "claude" and "hermes" entries so each skill (referenced by
ooClaudeSkillPath and ooClaudeCompatibleReferenceFiles) reuses one source of
truth; replace the inlined hermes: { files: [...] } and the other duplicated
blocks (around the other occurrences) with calls to that factory, passing only
the varying fields (like relativePath/sourcePath) so the shape is centralized
and duplication is removed.
In `@src/application/commands/skills/index.test.ts`:
- Around line 824-906: The test "uninstalls a published skill from every
existing supported host" repeats the same list of per-host skill directory
variables (codexSkillDirectoryPath, claudeSkillDirectoryPath,
hermesSkillDirectoryPath, codeBuddySkillDirectoryPath,
workBuddySkillDirectoryPath, openClawSkillDirectoryPath,
qoderWorkSkillDirectoryPath) multiple times; extract these into a single local
array (e.g. const skillDirectoryPaths = [...]) at the top of the test and
replace every repeated for-loop and expectation that iterates those paths with
iterations over skillDirectoryPaths, and similarly reuse that array when
creating directories, writing files, asserting stdout/stderr, and cleaning up;
use the existing resolver helpers (resolveCodexHomeDirectory,
resolveClaudeHomeDirectory, resolveHermesHomeDirectory, etc.) to populate the
array and apply the same refactor for the similar block referenced at lines
1333-1446.
In `@src/application/commands/skills/list.cli.test.ts`:
- Around line 256-297: This Hermes-specific test duplicates arrange/act/assert
logic used for other hosts; refactor by table-driving host-specific
startup-synchronized list tests: extract the repeated setup into a local factory
(e.g., a helper that calls createCliSandbox(), creates the home dir via
resolveHermesHomeDirectory or a generic resolve<Home> function, runs sandbox.run
for install and list, and calls sandbox.cleanup), then replace the individual
Hermes test with a parameterized test (using test.each or iterating over an
array of host descriptors) that supplies host name, resolve function, and
expected stdout strings; ensure you reuse sandbox.run, createCliSandbox, and
sandbox.cleanup from the original code so each row performs the same
arrange/act/assert with host-specific inputs.
In `@src/application/commands/skills/list.ts`:
- Around line 29-37: The two parallel maps (managedSkillHostOrder and
readManagedSkillHostLabel) should be consolidated into one data-driven
configuration: create a single const (e.g., managedSkillHosts) typed as
Record<BundledSkillAgentName, { order: number; label: string }>, move each
host's numeric order and human label into that object (including hermes), then
change all usages that read managedSkillHostOrder[...] or call
readManagedSkillHostLabel(...) to reference managedSkillHosts[host].order or
.label; also update the code regions around the other occurrence referenced in
the comment to use the unified object and remove the duplicate map/function so
there’s one source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8135b31-5674-4644-8ff3-410013361032
📒 Files selected for processing (14)
README-ZH_CN.mdREADME.mddocs/commands.mddocs/commands.zh-CN.mdsrc/application/commands/skills/bundled-skill-observation.test.tssrc/application/commands/skills/bundled-skill-paths.tssrc/application/commands/skills/check.test.tssrc/application/commands/skills/embedded-assets.test.tssrc/application/commands/skills/embedded-assets.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/list.cli.test.tssrc/application/commands/skills/list.tssrc/application/commands/skills/managed-skill-host-errors.tssrc/i18n/catalog.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hermes reuses the Claude Code skill template and is integrated into all skill lifecycle commands (install, list, preflight, remove). Its home directory defaults to ~/.hermes and can be overridden via the HERMES_HOME environment variable.