Fix Hermes and OpenCode web session transports - #297
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 36 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe pull request refactors two protocol adapters (Hermes and OpenCode) to transition from event-polling mechanisms to REST+streaming APIs. Hermes now spawns a local API server and consumes OpenAI-compatible Responses streaming, while OpenCode adopts a REST endpoint pattern with SSE event streams. New test fixtures and E2E tests validate both flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HermesAdapter
participant GatewayProcess as Hermes Gateway<br/>(spawned process)
participant APIServer as API Server<br/>(:PORT)
Client->>HermesAdapter: initialize()
HermesAdapter->>GatewayProcess: spawn with API_SERVER_PORT env
HermesAdapter->>APIServer: poll /health (loop until ready)
APIServer-->>HermesAdapter: 200 OK
Client->>HermesAdapter: sendMessage(turnId, content)
HermesAdapter->>APIServer: POST /v1/responses (stream=true)
APIServer-->>HermesAdapter: SSE stream (response.created, response.output_text.delta...)
HermesAdapter->>HermesAdapter: parse event:/data: blocks
HermesAdapter-->>Client: emit chat:text-delta
APIServer-->>HermesAdapter: response.output_item.added (tool call)
HermesAdapter-->>Client: emit chat:tool-call
APIServer-->>HermesAdapter: response.completed
HermesAdapter-->>Client: emit chat:turn-completed
Client->>HermesAdapter: interrupt()
HermesAdapter->>HermesAdapter: abort active stream controller
sequenceDiagram
participant Client
participant OpenCodeAdapter
participant ServerProcess as OpenCode Server<br/>(spawned process)
participant RESTServer as REST Server<br/>(:PORT)
Client->>OpenCodeAdapter: initialize()
OpenCodeAdapter->>ServerProcess: spawn with PORT env
OpenCodeAdapter->>RESTServer: poll /global/health (loop until ready)
RESTServer-->>OpenCodeAdapter: 200 OK
OpenCodeAdapter->>RESTServer: POST /session
RESTServer-->>OpenCodeAdapter: {ses_id: '...'}
OpenCodeAdapter->>RESTServer: subscribe to /global/event (SSE)
RESTServer-->>OpenCodeAdapter: server.connected event
Client->>OpenCodeAdapter: sendMessage(turnId, content)
OpenCodeAdapter->>RESTServer: POST /session/{id}/prompt_async
RESTServer-->>OpenCodeAdapter: 204 No Content
RESTServer-->>OpenCodeAdapter: SSE session.status (active → idle)
RESTServer-->>OpenCodeAdapter: SSE message.part.updated (text delta)
OpenCodeAdapter-->>Client: emit chat:text-delta
OpenCodeAdapter-->>Client: emit chat:turn-completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes introduce significant architectural shifts across two adapters with heterogeneous patterns (Hermes streaming vs. OpenCode REST+SSE), require careful validation of new initialization flows, event parsing logic, and error handling paths, plus new test fixtures and E2E coverage that each demand separate reasoning. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the web-mode transport implementations for the Hermes and OpenCode protocol adapters to match their respective HTTP/SSE server contracts, and adds test stubs + e2e coverage to validate prompt sending and streamed event ingestion.
Changes:
- Reworks OpenCode web sessions to spawn
opencode serve, create REST sessions, send prompts viaprompt_asyncwithparts, and consume/global/eventSSE. - Updates Hermes web sessions to enable the local API server via env vars, wait for
/health, and stream turns through/v1/responsesSSE. - Adds adapter-focused e2e tests plus lightweight HTTP stubs for OpenCode serve and the Hermes API server.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
server/protocol-adapters/opencode-adapter.ts |
Replaces legacy hook/TUI driving with OpenCode’s HTTP + SSE server transport. |
server/protocol-adapters/hermes-adapter.ts |
Switches Hermes to API-server + OpenAI-compatible Responses streaming transport. |
test/opencode-adapter.e2e.test.ts |
E2E test verifying OpenCode prompt + streamed chat events through the new transport. |
test/fixtures/opencode-serve-stub.cjs |
Stub server implementing the minimal OpenCode serve REST/SSE surface needed by the adapter. |
test/fixtures/hermes-gateway-stub.cjs |
Updates the Hermes stub to stream OpenAI Responses-style SSE events. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const port = parseInt(process.argv[2], 10); | ||
| const port = parseInt(process.env.API_SERVER_PORT || process.argv[2], 10); | ||
| if (!port || isNaN(port)) { | ||
| console.error('Usage: node hermes-gateway-stub.js <port>'); |
There was a problem hiding this comment.
The usage comment says to run with API_SERVER_PORT, but the actual error message still prints the old positional-arg usage. Update the message to match the supported invocation (env var and/or argv fallback) so failures are easier to diagnose.
Suggested change
| console.error('Usage: node hermes-gateway-stub.js <port>'); | |
| console.error( | |
| 'Usage: API_SERVER_PORT=1234 node hermes-gateway-stub.js (or: node hermes-gateway-stub.js <port>)' | |
| ); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/protocol-adapters/opencode-adapter.ts (1)
519-542:⚠️ Potential issue | 🟡 Minor
session.status === 'error'leaves the current turn open.The
'active'and'error'branches return early without clearing_currentTurnIdor emittingchat:turn-completed. As a result, after a session-level error the turn is effectively stuck — subsequent SSE events (message.part.updated,tool.execute.*, etc.) will continue to be attributed to the already-failed turn, and the UI will never see a turn completion for it.handleSessionErrordoes this correctly;handleSessionStatusfor'error'should do the same.🐛 Proposed fix
if (status === 'error') { + if (this._currentTurnId) { + this.fire({ + type: 'chat:turn-completed', + turnId: this._currentTurnId, + reason: 'failed', + durationMs: 0, + toolCallCount: 0, + messageCount: 0, + }); + this._currentTurnId = null; + } this.fire({ type: 'chat:session-status', status: 'error' }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/protocol-adapters/opencode-adapter.ts` around lines 519 - 542, handleSessionStatus currently returns early on status === 'error' leaving _currentTurnId intact and not emitting a chat:turn-completed; update handleSessionStatus to mirror handleSessionError by, when status === 'error', checking if this._currentTurnId is set, firing a chat:turn-completed event (include the same fields used by handleSessionError: turnId: this._currentTurnId, reason: 'failed' or 'error' consistent with handleSessionError, durationMs, toolCallCount, messageCount), clearing this._currentTurnId, and then firing the session-status/error event so the turn is closed and subsequent events are not attributed to the stale turn.
🧹 Nitpick comments (6)
server/protocol-adapters/hermes-adapter.ts (1)
266-274: Dead code inrespondToInput.
firstAnsweris extracted but never consumed — the function is a no-op regardless of its value. Drop the unused logic or replace with an explicit unsupported signal so future readers aren't misled.🧹 Proposed cleanup
async respondToInput( _requestId: string, - answers: Record<string, string[]> + _answers: Record<string, string[]> ): Promise<void> { - const firstAnswer = Object.values(answers)[0]?.[0]; - if (!firstAnswer) return; // Hermes gateway does not currently support structured input questions // via REST; this is a no-op. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/protocol-adapters/hermes-adapter.ts` around lines 266 - 274, The respondToInput function extracts firstAnswer but never uses it, leaving misleading dead code; remove the unused extraction (firstAnswer) and either leave the function as an explicit no-op with a clarifying comment or replace it with a clear unsupported operation signal (e.g., throw or return a specific "not supported" response) so callers/readers of respondToInput know input handling is intentionally unsupported by the Hermes gateway.test/fixtures/hermes-gateway-stub.cjs (1)
44-56: GuardJSON.parseto avoid crashing the stub on malformed bodies.A malformed request body currently throws out of the
endhandler and kills the stub process, which can mask the real adapter failure under test. Cheap to wrap:🛡️ Proposed fix
req.on('end', () => { - const data = JSON.parse(body); + let data; + try { + data = JSON.parse(body); + } catch (err) { + res.writeHead(400, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: String(err) })); + return; + } res.writeHead(200, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/hermes-gateway-stub.cjs` around lines 44 - 56, Wrap the JSON.parse(body) inside a try/catch in the req.on('end') handler to prevent the stub from crashing on malformed bodies; on parse failure log the error and return an HTTP error response (e.g., 400) instead of calling emitTurn, referencing the req.on('end') callback, the local variable body, and the emitTurn(res, data.input) call so you only call emitTurn when parsing succeeds.test/opencode-adapter.e2e.test.ts (2)
17-19: Prefer a typedvi.fnoveras unknown as.The double-cast silently bypasses type checking. Vitest supports a generic form that gives you the same mock with proper typing.
♻️ Proposed fix
- const onBackendStateChanged = vi.fn() as unknown as ( - session: Session - ) => void; + const onBackendStateChanged = vi.fn<(session: Session) => void>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opencode-adapter.e2e.test.ts` around lines 17 - 19, The test creates onBackendStateChanged using a double-cast (vi.fn() as unknown as (session: Session) => void) which bypasses type checking; replace this with a properly typed mock by using Vitest’s generic form vi.fn<ReturnType, ArgTypes>()—specifically change the creation of onBackendStateChanged to use vi.fn<void, [Session]> (or vi.fn<(session: Session) => void>) so the mock is correctly typed without the unsafe as unknown as cast.
49-53: Consider an explicit timeout onvi.waitForfor CI robustness.The stub paces events up to ~100ms but this test also spawns a Node child process and waits for
/global/health— on slow CI the combined latency can flirt with vitest's defaultwaitFortimeout. Setting an explicit, generous timeout removes a future flake class without changing normal-run behavior.♻️ Proposed fix
- await vi.waitFor(() => { + await vi.waitFor(() => { expect(session.agentState).toBe('idle'); expect(events.some((e) => e.type === 'chat:text-delta')).toBe(true); expect(events.some((e) => e.type === 'chat:turn-completed')).toBe(true); - }); + }, { timeout: 5000, interval: 25 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/opencode-adapter.e2e.test.ts` around lines 49 - 53, The test uses vi.waitFor without an explicit timeout which can flake on slow CI; update the vi.waitFor call (the block asserting session.agentState and events) to include a generous explicit timeout option (e.g., 3000–10000 ms) so the waitFor for session.agentState and the chat events has more time on CI; keep the same assertions (session.agentState, events.some for 'chat:text-delta' and 'chat:turn-completed') but pass the timeout option to vi.waitFor to ensure robustness.test/fixtures/opencode-serve-stub.cjs (1)
65-76: WrapJSON.parseso malformed bodies return 400 instead of crashing the stub.If the adapter ever sends a malformed body,
JSON.parse(body)throws synchronously from the'end'callback and the entire stub process dies with an unhandled exception, producing an opaque E2E failure instead of a clear assertion against the adapter. Also note thatpayloadis untyped — if a client sendsnullor a non-object,payload.partsthrows again.♻️ Proposed fix
drain(req, (body) => { - const payload = JSON.parse(body); - if (!Array.isArray(payload.parts)) { + let payload; + try { + payload = JSON.parse(body); + } catch { + res.writeHead(400, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'invalid JSON body' })); + return; + } + if (!payload || !Array.isArray(payload.parts)) { res.writeHead(400, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ error: 'parts is required' })); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixtures/opencode-serve-stub.cjs` around lines 65 - 76, Wrap the synchronous JSON.parse inside the drain callback in a try/catch and return a 400 JSON response on parse errors; after parsing also defensively check that the parsed value is a non-null object and that payload.parts is an array (use Array.isArray(payload.parts)), returning the same 400 JSON error if those validations fail, then only proceed to writeHead(204), res.end(), and call emitTurn(); update the block around drain(req, (body) => { ... }) accordingly to avoid throwing on malformed bodies or non-object payloads.server/protocol-adapters/opencode-adapter.ts (1)
82-108:getPort+spawnis a TOCTOU race; prefer binding and reading the actual port.
getPort()reports a port that's free now, but there's a window before the spawnedopencode serveactually binds it during which another process can claim it — typically manifesting as sporadic CI failures and a crypticEADDRINUSEtraced back towaitForServer. Ifopencode servecan print its listening port to stdout (or accept--port 0and report it), parsing that output is more reliable than pre-allocating.At minimum, consider adding a dedicated error path in
waitForServerwhen the bound port logs indicateEADDRINUSE, so users can distinguish a race from a genuinely slow startup.opencode serve CLI flags print bound port stdout 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/protocol-adapters/opencode-adapter.ts` around lines 82 - 108, The current use of getPort() plus spawn creates a TOCTOU race; modify the startup flow so opencode is allowed to bind the port and the adapter reads the actual bound port from the child process output instead of preallocating one. Concretely: when building args in the constructor (see command, defaultArgs, args) allow passing '--port', '0' (or detect a {{PORT}} placeholder) so the server selects a free port, then listen to this._process.stdout for the CLI line that reports the bound port (parse and assign this._apiPort and recompute this._endpoint) before resolving readiness; as a fallback keep the existing getPort() behavior. Also update waitForServer to detect an EADDRINUSE error emitted by the child (or printed to stderr) and surface a distinct, actionable error so races are distinguishable from slow startups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/protocol-adapters/hermes-adapter.ts`:
- Around line 188-196: _reset _lastResponseId on failure paths: ensure
this._lastResponseId is cleared whenever a response fails or is aborted and
during disconnect/reconnect startup to prevent leaking an invalid
previous_response_id; specifically, clear it in the response.failed handler and
where AbortError is caught, in onDisconnect, and at the start of
connect/reconnect before calling sendMessage or building the request body so
sendMessage and the body construction (which currently uses
this._lastResponseId) never include a stale id from a prior failed/aborted turn.
- Around line 254-264: The Hermes adapter currently only fires a local event in
respondToApproval and never emits chat:approval-request because it doesn't
handle permission hook events; modify the adapter so that when processing
Hermes/Responses API hook events you detect the permission.requested hook and
emit a chat:approval-request with the permission id and metadata (matching other
adapters' behavior), and update respondToApproval to call the backend permission
endpoints (/permission/:id/allow or /permission/:id/deny) based on the decision
(mapping 'allow-always' to allow as needed) and then fire the existing local
chat:approval-response; keep the method name respondToApproval and the
SSE/responses handling code paths so tests/stub endpoints expecting
/permission/:id/(allow|deny) work.
In `@server/protocol-adapters/opencode-adapter.ts`:
- Around line 307-328: In respondToApproval: preserve the three-way mapping
required by OpenCode (map decision 'deny' → request body 'reject', 'allow' →
'once', 'allow-always' → 'always'), validate this._openCodeSessionId and abort
early (or throw) if it's null/empty instead of embedding an empty string in the
URL, perform the POST and check the fetch result (use the promise and inspect
response.ok/status rather than .catch(()=>{}) so network/HTTP errors are not
silently swallowed), and only call this.fire({ type: 'chat:approval-response',
... }) after a successful POST (or include error details in a failure event/log)
so callers don't believe the approval succeeded when it didn't.
- Around line 581-588: deltaForPart currently returns rawDelta when it's a
string but doesn't update the _partText cache, causing later events without
explicit delta to re-emit full text; to fix, compute id via textPartId(part) and
next from part.text, update this._partText.set(id, next) before returning when
typeof rawDelta === 'string' (or move the cache update to run unconditionally at
the top of deltaForPart), leaving the rest of the logic (prev lookup and
slicing) intact so subsequent calls use the correct cached value.
- Around line 346-359: Add clarifying comments to createSession and forkSession
that mirror the existing resumeSession note: state these are intentional
stubs/no-ops because the real OpenCode session is created in connect() by
createOpenCodeSession() which POSTs to /session on the OpenCode REST API;
mention that createSession returns the configured sessionId or a random id as a
placeholder and forkSession generates a random placeholder id, but neither
interacts with the OpenCode backend so callers should not expect them to create
or manage real sessions.
---
Outside diff comments:
In `@server/protocol-adapters/opencode-adapter.ts`:
- Around line 519-542: handleSessionStatus currently returns early on status ===
'error' leaving _currentTurnId intact and not emitting a chat:turn-completed;
update handleSessionStatus to mirror handleSessionError by, when status ===
'error', checking if this._currentTurnId is set, firing a chat:turn-completed
event (include the same fields used by handleSessionError: turnId:
this._currentTurnId, reason: 'failed' or 'error' consistent with
handleSessionError, durationMs, toolCallCount, messageCount), clearing
this._currentTurnId, and then firing the session-status/error event so the turn
is closed and subsequent events are not attributed to the stale turn.
---
Nitpick comments:
In `@server/protocol-adapters/hermes-adapter.ts`:
- Around line 266-274: The respondToInput function extracts firstAnswer but
never uses it, leaving misleading dead code; remove the unused extraction
(firstAnswer) and either leave the function as an explicit no-op with a
clarifying comment or replace it with a clear unsupported operation signal
(e.g., throw or return a specific "not supported" response) so callers/readers
of respondToInput know input handling is intentionally unsupported by the Hermes
gateway.
In `@server/protocol-adapters/opencode-adapter.ts`:
- Around line 82-108: The current use of getPort() plus spawn creates a TOCTOU
race; modify the startup flow so opencode is allowed to bind the port and the
adapter reads the actual bound port from the child process output instead of
preallocating one. Concretely: when building args in the constructor (see
command, defaultArgs, args) allow passing '--port', '0' (or detect a {{PORT}}
placeholder) so the server selects a free port, then listen to
this._process.stdout for the CLI line that reports the bound port (parse and
assign this._apiPort and recompute this._endpoint) before resolving readiness;
as a fallback keep the existing getPort() behavior. Also update waitForServer to
detect an EADDRINUSE error emitted by the child (or printed to stderr) and
surface a distinct, actionable error so races are distinguishable from slow
startups.
In `@test/fixtures/hermes-gateway-stub.cjs`:
- Around line 44-56: Wrap the JSON.parse(body) inside a try/catch in the
req.on('end') handler to prevent the stub from crashing on malformed bodies; on
parse failure log the error and return an HTTP error response (e.g., 400)
instead of calling emitTurn, referencing the req.on('end') callback, the local
variable body, and the emitTurn(res, data.input) call so you only call emitTurn
when parsing succeeds.
In `@test/fixtures/opencode-serve-stub.cjs`:
- Around line 65-76: Wrap the synchronous JSON.parse inside the drain callback
in a try/catch and return a 400 JSON response on parse errors; after parsing
also defensively check that the parsed value is a non-null object and that
payload.parts is an array (use Array.isArray(payload.parts)), returning the same
400 JSON error if those validations fail, then only proceed to writeHead(204),
res.end(), and call emitTurn(); update the block around drain(req, (body) => {
... }) accordingly to avoid throwing on malformed bodies or non-object payloads.
In `@test/opencode-adapter.e2e.test.ts`:
- Around line 17-19: The test creates onBackendStateChanged using a double-cast
(vi.fn() as unknown as (session: Session) => void) which bypasses type checking;
replace this with a properly typed mock by using Vitest’s generic form
vi.fn<ReturnType, ArgTypes>()—specifically change the creation of
onBackendStateChanged to use vi.fn<void, [Session]> (or vi.fn<(session: Session)
=> void>) so the mock is correctly typed without the unsafe as unknown as cast.
- Around line 49-53: The test uses vi.waitFor without an explicit timeout which
can flake on slow CI; update the vi.waitFor call (the block asserting
session.agentState and events) to include a generous explicit timeout option
(e.g., 3000–10000 ms) so the waitFor for session.agentState and the chat events
has more time on CI; keep the same assertions (session.agentState, events.some
for 'chat:text-delta' and 'chat:turn-completed') but pass the timeout option to
vi.waitFor to ensure robustness.
🪄 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: 0f63897b-b297-4552-a484-8299fecc6afe
📒 Files selected for processing (5)
server/protocol-adapters/hermes-adapter.tsserver/protocol-adapters/opencode-adapter.tstest/fixtures/hermes-gateway-stub.cjstest/fixtures/opencode-serve-stub.cjstest/opencode-adapter.e2e.test.ts
This was referenced Apr 24, 2026
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
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
Fixes web-mode agent session transports for Hermes and OpenCode:
/health, and streaming turns through/v1/responses.opencode serve, REST session creation,prompt_asyncwithparts, and/global/eventSSE updates.Verification
npm run checknpm run build:server && npx vitest run test/opencode-adapter.e2e.test.ts test/hermes-adapter.e2e.test.ts test/hermes-session-api.e2e.test.ts test/web-session-handler.test.ts test/customize-session-dialog.test.ts test/adapter-map-hook-event.test.ts test/opencode-relay.test.tsnpm run build(passes with existing Vite chunk/dynamic-import warnings)Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests