feat(providers): add Hermes Agent as community provider#1648
Closed
totomoko wants to merge 1 commit into
Closed
Conversation
Implements IAgentProvider for Nous Research Hermes Agent via CLI shell-out. - HermesProvider shells out to 'hermes chat -q --quiet --source tool' - Parses session_id from stdout first line - Maps stdout to assistant chunks, stderr to error classification - Supports model, provider, toolsets, skills, env injection, maxTurns, yolo, checkpoints, worktree flags via config - Conservative capability flags (skills, toolRestrictions, structuredOutput, envInjection wired; sessionResume, mcp, hooks, agents deferred) Files: - packages/providers/src/community/hermes/* (new) - packages/providers/src/types.ts (+HermesProviderDefaults) - packages/providers/src/registry.ts (+registerHermesProvider) - packages/providers/src/index.ts (+exports) - packages/core/src/config/config-types.ts (+re-export) Refs: coleam00#1106
📝 WalkthroughWalkthroughThis PR introduces a complete Hermes provider for the Archon framework. It defines configuration types, parsing, capabilities, spawns the hermes CLI with argument/environment construction, streams output as message chunks, classifies errors, supports session resumption, and integrates registration into the provider system. ChangesHermes Provider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/providers/src/community/hermes/config.ts (1)
31-41: ⚡ Quick winValidate that environment variable keys are non-empty.
The current implementation doesn't check whether keys are empty strings. Empty keys in the environment object could cause issues when passed to the spawned process.
♻️ Proposed fix to validate keys
if (raw.env && typeof raw.env === 'object' && !Array.isArray(raw.env)) { const env: Record<string, string> = {}; for (const [key, value] of Object.entries(raw.env as Record<string, unknown>)) { - if (typeof value === 'string') { + if (key.length > 0 && typeof value === 'string') { env[key] = value; } }🤖 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 `@packages/providers/src/community/hermes/config.ts` around lines 31 - 41, The env parsing block (checking raw.env and populating result.env) must skip entries with empty or whitespace-only keys; update the loop over Object.entries(raw.env as Record<string, unknown>) in this block to validate each key (e.g., ensure key.trim().length > 0) before accepting a string value into the local env map, and only set result.env if env has entries after that validation.packages/providers/src/community/hermes/provider.ts (1)
32-32: 💤 Low valueClarify the base directory for relative path resolution.
resolve(configPath)resolves relative paths againstprocess.cwd(), which may not be the intended base directory. Consider whether the path should be resolved relative to a specific base (e.g., workspace root or config file location) for more predictable behavior.🤖 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 `@packages/providers/src/community/hermes/provider.ts` at line 32, The resolution of configPath uses resolve(configPath) which defaults to process.cwd(), causing unpredictable base dir; update the logic around the constant named absolute (and its use of isAbsolute/configPath) to resolve relative paths against an explicit base (for example a provided workspaceRoot, repositoryRoot, or the provider's config file directory) instead of process.cwd(): detect or accept the intended base directory, then call path.resolve(base, configPath) (or adjust the caller to pass an already-resolved path) so relative paths are stable and predictable.
🤖 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 `@packages/providers/src/community/hermes/config.ts`:
- Around line 24-29: The skills parsing currently only checks typeof string for
raw.skills entries (raw.skills and result.skills) but allows empty or
whitespace-only strings; update the filter to also trim and exclude empty
strings (e.g., filter entries where typeof s === 'string' && s.trim().length >
0) so result.skills only contains non-empty skill values before assigning to
result.skills.
In `@packages/providers/src/community/hermes/provider.ts`:
- Around line 135-139: The spawn error detection races setImmediate with the
child's 'error' event causing nondeterministic misses; replace that logic in
provider.ts where spawnError and child are used with a Promise that listens for
child.once('error', err => resolve(err)) and child.once('spawn', () =>
resolve(null)) (or vice versa) and ensures the other listener is removed/cleaned
up to avoid leaks; remove the setImmediate-based fallback and rely on the
'spawn' event to deterministically detect successful spawn.
- Around line 68-72: The current assignment to toolsets treats non-array/string
allowed_tools incorrectly (String(obj) -> "[object Object]"); update the logic
around options.nodeConfig.allowed_tools to explicitly handle objects: if it's an
array use join(','), if it's a string use as-is, if it's a plain object either
convert to a deterministic CLI string via JSON.stringify(allowed_tools) or
validate and throw/log a clear error, and otherwise fall back to
config.toolsets; ensure this change is applied where toolsets is computed and
that any validation/error mentions options.nodeConfig.allowed_tools so callers
can fix malformed input.
- Line 162: Replace the non-null assertion on child.stdout to a safe runtime
check: read child.stdout into a local (e.g., const stdout = child.stdout), if
stdout is null/undefined throw or return with a clear error (or reject the
Promise) and then assign const stdoutIterator = stdout; this removes the `!`
while preserving the original guarantee from stdio and references the existing
symbols child, stdout, and stdoutIterator.
---
Nitpick comments:
In `@packages/providers/src/community/hermes/config.ts`:
- Around line 31-41: The env parsing block (checking raw.env and populating
result.env) must skip entries with empty or whitespace-only keys; update the
loop over Object.entries(raw.env as Record<string, unknown>) in this block to
validate each key (e.g., ensure key.trim().length > 0) before accepting a string
value into the local env map, and only set result.env if env has entries after
that validation.
In `@packages/providers/src/community/hermes/provider.ts`:
- Line 32: The resolution of configPath uses resolve(configPath) which defaults
to process.cwd(), causing unpredictable base dir; update the logic around the
constant named absolute (and its use of isAbsolute/configPath) to resolve
relative paths against an explicit base (for example a provided workspaceRoot,
repositoryRoot, or the provider's config file directory) instead of
process.cwd(): detect or accept the intended base directory, then call
path.resolve(base, configPath) (or adjust the caller to pass an already-resolved
path) so relative paths are stable and predictable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4af159b-5356-48cb-85c4-3294de3adb59
📒 Files selected for processing (10)
packages/core/src/config/config-types.tspackages/providers/src/community/hermes/capabilities.tspackages/providers/src/community/hermes/config.tspackages/providers/src/community/hermes/index.tspackages/providers/src/community/hermes/provider.test.tspackages/providers/src/community/hermes/provider.tspackages/providers/src/community/hermes/registration.tspackages/providers/src/index.tspackages/providers/src/registry.tspackages/providers/src/types.ts
Comment on lines
+24
to
+29
| if (Array.isArray(raw.skills)) { | ||
| const skills = (raw.skills as unknown[]).filter((s): s is string => typeof s === 'string'); | ||
| if (skills.length > 0) { | ||
| result.skills = skills; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Filter out empty strings from skills array.
The current filter only validates that array elements are strings, but doesn't exclude empty strings. Empty skills could cause unexpected behavior when passed to the Hermes CLI.
♻️ Proposed fix to filter empty strings
if (Array.isArray(raw.skills)) {
- const skills = (raw.skills as unknown[]).filter((s): s is string => typeof s === 'string');
+ const skills = (raw.skills as unknown[])
+ .filter((s): s is string => typeof s === 'string' && s.length > 0);
if (skills.length > 0) {
result.skills = skills;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (Array.isArray(raw.skills)) { | |
| const skills = (raw.skills as unknown[]).filter((s): s is string => typeof s === 'string'); | |
| if (skills.length > 0) { | |
| result.skills = skills; | |
| } | |
| } | |
| if (Array.isArray(raw.skills)) { | |
| const skills = (raw.skills as unknown[]) | |
| .filter((s): s is string => typeof s === 'string' && s.length > 0); | |
| if (skills.length > 0) { | |
| result.skills = 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 `@packages/providers/src/community/hermes/config.ts` around lines 24 - 29, The
skills parsing currently only checks typeof string for raw.skills entries
(raw.skills and result.skills) but allows empty or whitespace-only strings;
update the filter to also trim and exclude empty strings (e.g., filter entries
where typeof s === 'string' && s.trim().length > 0) so result.skills only
contains non-empty skill values before assigning to result.skills.
Comment on lines
+68
to
+72
| const toolsets = options.nodeConfig?.allowed_tools | ||
| ? (Array.isArray(options.nodeConfig.allowed_tools) | ||
| ? options.nodeConfig.allowed_tools.join(',') | ||
| : String(options.nodeConfig.allowed_tools)) | ||
| : config.toolsets; |
There was a problem hiding this comment.
Handle the object case for allowed_tools correctly.
If nodeConfig.allowed_tools is an object (not an array or string), String(allowed_tools) will produce "[object Object]", resulting in an invalid CLI argument.
🛡️ Proposed fix to handle or reject objects
const toolsets = options.nodeConfig?.allowed_tools
- ? (Array.isArray(options.nodeConfig.allowed_tools)
- ? options.nodeConfig.allowed_tools.join(',')
- : String(options.nodeConfig.allowed_tools))
+ ? (typeof options.nodeConfig.allowed_tools === 'string'
+ ? options.nodeConfig.allowed_tools
+ : Array.isArray(options.nodeConfig.allowed_tools)
+ ? options.nodeConfig.allowed_tools.join(',')
+ : undefined)
: config.toolsets;Alternatively, if objects are expected, document the expected shape and handle appropriately.
🤖 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 `@packages/providers/src/community/hermes/provider.ts` around lines 68 - 72,
The current assignment to toolsets treats non-array/string allowed_tools
incorrectly (String(obj) -> "[object Object]"); update the logic around
options.nodeConfig.allowed_tools to explicitly handle objects: if it's an array
use join(','), if it's a string use as-is, if it's a plain object either convert
to a deterministic CLI string via JSON.stringify(allowed_tools) or validate and
throw/log a clear error, and otherwise fall back to config.toolsets; ensure this
change is applied where toolsets is computed and that any validation/error
mentions options.nodeConfig.allowed_tools so callers can fix malformed input.
Comment on lines
+135
to
+139
| const spawnError = await new Promise<Error | null>((resolve) => { | ||
| child.on('error', (err) => resolve(err)); | ||
| // If no error within next tick, resolve null | ||
| setImmediate(() => resolve(null)); | ||
| }); |
There was a problem hiding this comment.
Fix race condition in spawn error detection.
The current approach races the 'error' event against setImmediate, but the timing is nondeterministic. The child process may emit 'error' after setImmediate executes, or vice versa, leading to missed spawn failures.
Node.js ChildProcess emits a 'spawn' event once the child has spawned successfully. Use this event to reliably detect successful spawn.
🐛 Proposed fix using the 'spawn' event
-// Handle spawn errors (e.g. binary not found) before streaming begins
-const spawnError = await new Promise<Error | null>((resolve) => {
- child.on('error', (err) => resolve(err));
- // If no error within next tick, resolve null
- setImmediate(() => resolve(null));
-});
+// Handle spawn errors (e.g. binary not found) before streaming begins
+const spawnError = await new Promise<Error | null>((resolve) => {
+ child.on('error', (err) => {
+ child.removeAllListeners('spawn');
+ resolve(err);
+ });
+ child.on('spawn', () => {
+ child.removeAllListeners('error');
+ resolve(null);
+ });
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const spawnError = await new Promise<Error | null>((resolve) => { | |
| child.on('error', (err) => resolve(err)); | |
| // If no error within next tick, resolve null | |
| setImmediate(() => resolve(null)); | |
| }); | |
| const spawnError = await new Promise<Error | null>((resolve) => { | |
| child.on('error', (err) => { | |
| child.removeAllListeners('spawn'); | |
| resolve(err); | |
| }); | |
| child.on('spawn', () => { | |
| child.removeAllListeners('error'); | |
| resolve(null); | |
| }); | |
| }); |
🤖 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 `@packages/providers/src/community/hermes/provider.ts` around lines 135 - 139,
The spawn error detection races setImmediate with the child's 'error' event
causing nondeterministic misses; replace that logic in provider.ts where
spawnError and child are used with a Promise that listens for
child.once('error', err => resolve(err)) and child.once('spawn', () =>
resolve(null)) (or vice versa) and ensures the other listener is removed/cleaned
up to avoid leaks; remove the setImmediate-based fallback and rely on the
'spawn' event to deterministically detect successful spawn.
| stderrBuffer += chunk.toString('utf-8'); | ||
| }); | ||
|
|
||
| const stdoutIterator = child.stdout!; |
There was a problem hiding this comment.
Resolve ESLint non-null assertion violation.
The non-null assertion violates the zero-tolerance ESLint policy. While the assertion is safe (because stdio: ['ignore', 'pipe', 'pipe'] guarantees stdout is a stream), ESLint cannot infer this. As per coding guidelines, inline disables require justification.
🔧 Proposed fixes
Option 1: Add a type guard with early return
-const stdoutIterator = child.stdout!;
-for await (const chunk of stdoutIterator) {
+if (!child.stdout) {
+ getLog().error({}, 'hermes.stdout_missing');
+ yield {
+ type: 'result',
+ isError: true,
+ errorSubtype: 'internal_error',
+ errors: ['stdout stream missing'],
+ stopReason: 'internal_error',
+ };
+ return;
+}
+for await (const chunk of child.stdout) {Option 2: Add justified eslint-disable comment
+// eslint-disable-next-line `@typescript-eslint/no-non-null-assertion` -- stdout is guaranteed to be a stream because stdio is configured as ['ignore', 'pipe', 'pipe']
const stdoutIterator = child.stdout!;🧰 Tools
🪛 ESLint
[error] 162-162: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
🤖 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 `@packages/providers/src/community/hermes/provider.ts` at line 162, Replace the
non-null assertion on child.stdout to a safe runtime check: read child.stdout
into a local (e.g., const stdout = child.stdout), if stdout is null/undefined
throw or return with a clear error (or reject the Promise) and then assign const
stdoutIterator = stdout; this removes the `!` while preserving the original
guarantee from stdio and references the existing symbols child, stdout, and
stdoutIterator.
Collaborator
|
will not be added, The underlying llm harness needs to be a coding SDK you can still use hermes to run archon through the archon skill and cli |
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
2 participants
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.
Summary
Describe this PR in 2-5 bullets:
UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
Label Snapshot
risk: low|medium|highsize: XS|S|M|L|XLcore|workflows|isolation|git|adapters|server|web|cli|paths|config|docs|dependencies|ci|tests|skills<scope>:<component>(e.g.workflows:executor,adapters:slack,core:orchestrator)Change Metadata
bug|feature|refactor|docs|security|chorecore|workflows|isolation|git|adapters|server|web|cli|paths|multiLinked Issue
Validation Evidence (required)
Commands and result summary:
Security Impact (required)
Yes/No)Yes/No)Yes/No)Yes/No)Yes, describe risk and mitigation:Compatibility / Migration
Yes/No)Yes/No)Yes/No)Human Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
Rollback Plan (required)
Risks and Mitigations
List real risks in this PR (or write
None).Summary by CodeRabbit