feat(agents): add Hermes ACP adapter#942
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 2m 40s —— View job PR Review: Add Hermes ACP Adapter
Overall this is a clean, well-structured addition that follows established patterns for host-local ACP adapters. Tests are thorough and coverage spans the full stack. A few items worth addressing before merge: Issues1. Dual env var names for command override ( process.env.HERMES_ACP_COMMAND?.trim() ||
process.env.BROWSEROS_HERMES_ACP_COMMAND?.trim() ||Two env var names for the same thing, with no docs explaining which is canonical. The PR description only mentions 2. Reasoning selector shown with only one option for Hermes ( The model select is conditionally hidden when {harnessModels.length > 0 ? (<div>...model select...</div>) : null}But the reasoning select is always rendered regardless. For Hermes, 3. Install command suggests editable dev install ( installCommand: "pip install -e '.[acp]'",The Minor ObservationsHermes and OpenClaw share the same Both map to
if (value === 'openclaw' || value === 'claude' || value === 'codex' || value === 'hermes') {This needs to be updated manually when new adapters are added. The DB schema enum is TypeScript-level only — no migration file needed. The SQLite column is What's Well Done
|
✅ Tests passed — 1110/1114
|
Greptile SummaryThis PR registers Hermes as a host-local ACP adapter across the full stack: DB enum, server catalog, ACPX prep/command resolver, health probe, and agent UI with an install-prompt gate. The integration follows the same patterns established by Claude Code and Codex adapters.
Confidence Score: 3/5Mostly safe but the health probe for Hermes is insufficient, creating a silent path to runtime failures for users with base Hermes installed without the ACP extra. One P1 finding (health probe doesn't confirm ACP availability, bypasses the install-prompt gate) pulls the score below 4. The rest of the integration is clean, well-tested, and consistent with existing adapter patterns. packages/browseros-agent/apps/server/src/lib/agents/adapter-health.ts — probe command should verify ACP availability, not just base CLI presence. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as NewAgentDialog (UI)
participant AH as AdapterHealthChecker
participant RT as AcpxRuntime
participant HK as resolveHermesAcpCommand
participant HP as prepareHermesContext
participant CLI as hermes acp (host CLI)
UI->>AH: getHealth('hermes')
AH->>CLI: hermes --version
CLI-->>AH: exit 0 / error
AH-->>UI: AdapterHealth { healthy, reason }
alt unhealthy
UI->>UI: show HermesInstallPrompt (blocks create)
else healthy
UI->>RT: send({ agent, message })
RT->>HK: resolveHermesAcpCommand()
HK-->>RT: HERMES_ACP_COMMAND env || 'hermes acp'
RT->>HP: prepareHermesContext(input)
HP-->>RT: PreparedAcpxAgentContext { commandEnv: { AGENT_HOME } }
RT->>CLI: env AGENT_HOME=... hermes acp
CLI-->>RT: ACP stream events
end
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/apps/server/src/lib/agents/adapter-health.ts:119-122
**Health probe insufficient for ACP availability**
`hermes --version` confirms the base Hermes CLI is installed, but does **not** verify the `[acp]` extra is present. The Hermes docs make clear that `hermes acp` only exists after `pip install -e '.[acp]'` is run separately. A user who has Hermes installed without the ACP extra will pass this probe (healthy = true, no install prompt shown), then hit a spawn failure at runtime when BrowserOS tries to execute `hermes acp`. Consider using `hermes acp --version` or a similar ACP-specific check as the probe command instead.
### Issue 2 of 2
packages/browseros-agent/apps/server/src/lib/agents/acpx-runtime.ts:761-767
`BROWSEROS_HERMES_ACP_COMMAND` is an undocumented second env-var fallback that has no test coverage and isn't mentioned in the PR description or any configuration docs. Per the team's policy on removing dead code, this silent fallback should be dropped. If there's a reason to keep a `BROWSEROS_`-namespaced override, it should be the *only* env var and should be tested.
```suggestion
function resolveHermesAcpCommand(): string {
return process.env.HERMES_ACP_COMMAND?.trim() || 'hermes acp'
}
```
Reviews (1): Last reviewed commit: "fix(agents): align hermes override and p..." | Re-trigger Greptile |
|
Addressed review feedback in 409e0ee:
|
Summary
hermes acpwithHERMES_ACP_COMMANDoverride support.Verification
bun run --filter @browseros/server test:agentbun --env-file=apps/server/.env.development test apps/server/tests/lib/agents/agent-catalog.test.ts apps/server/tests/lib/agents/acpx-agent-adapter.test.ts apps/server/tests/lib/agents/acpx-runtime.test.ts apps/server/tests/lib/agents/adapter-health.test.ts apps/server/tests/api/routes/agents.test.tsbun run --filter @browseros/server typecheckbun run --filter @browseros/agent typecheckbun run --filter @browseros/agent testbunx biome check apps/server/src/lib/agents apps/server/tests/lib/agents apps/server/tests/api/routes apps/agent/entrypoints/app/agents apps/agent/entrypoints/app/agent-command apps/agent/entrypoints/sidepanel/index(exits 0; existing warnings remain in active-turn/route-test files)Hermes ACP docs referenced: https://hermes-agent.nousresearch.com/docs/user-guide/features/acp