feat: bring OpenCode adapter to feature parity with other adapters#16
Merged
aaditagrawal merged 5 commits intomainfrom Mar 16, 2026
Merged
feat: bring OpenCode adapter to feature parity with other adapters#16aaditagrawal merged 5 commits intomainfrom
aaditagrawal merged 5 commits intomainfrom
Conversation
- Emit session.configured after session creation and session.exited on stop/stream death for lifecycle parity with Claude Code and Copilot - Forward session.diff as turn.diff.updated with unified diff conversion - Handle 6 new SSE event types: session.compacted, session.updated, vcs.branch.updated, file.edited, command.executed, message.part.removed - Enrich existing events: todo priority prefixes, isRetryable on API errors, permission title/pattern metadata, question multiple/custom fields - Implement rollbackThread via session.revert API with message ID tracking - Extract opencode/ module files from opencodeServerManager.ts - Fix flaky OpenCodeAdapter stream forwarding test (race condition) - Improve runtime error display with error class labels
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds complete provider subsystems for OpenCode and Kilo: new types, utilities, centralized SSE event dispatchers, error classification/formatting, server lifecycle orchestration (probe/spawn/create client/ensure), refactored server managers with rollback and lifecycle events, and minor test consumption fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as Provider Process / SDK
participant Lifecycle as ServerLifecycle
participant Handler as EventHandler
participant Context as SessionContext
participant Emitter as RuntimeEmitter
participant Client as OpencodeClient
Note over Lifecycle,Provider: startup / probe / create client
Lifecycle->>Provider: probe / spawn or connect
Lifecycle->>Client: createClient()
Provider->>Handler: SSE event (session.*, message.part.*, permission.*, error, ...)
Handler->>Handler: validate providerSessionId
Handler->>Context: update session/turn/parts/permissions/questions/lastError
Handler->>Emitter: emit runtime event (eventId, provider, threadId, timestamps, payload)
Handler->>Client: call RPC as needed (prompt, reply, abort)
Client-->>Handler: RPC response
Handler->>Context: record retry hints / messageIds (for rollback)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
github-actions
bot
added
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/opencodeServerManager.ts (1)
1-639:⚠️ Potential issue | 🟡 MinorPipeline failure: Run
bun run fmtto fix formatting issues.The CI pipeline reports formatting issues detected by oxfmt. This applies to multiple files in the
opencode/module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencodeServerManager.ts` around lines 1 - 639, Formatting is failing CI due to oxfmt style issues across the opencode module; run the repository formatter and commit the changes. Fix by running the project's formatter command (e.g., "bun run fmt" as suggested by CI) or invoke oxfmt directly for the opencode files, then review and commit the reformatted files (ensure changes touch code in OpenCodeServerManager and related files under opencode/), and push the commit so CI can pass.
🧹 Nitpick comments (6)
apps/server/src/opencode/utils.ts (3)
312-314:readJsonDatais a pass-through function — clarify intent or remove.This function simply returns the promise without transformation. If this is a placeholder for future SDK response normalization, consider adding a comment. Otherwise, it adds indirection without value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/utils.ts` around lines 312 - 314, The function readJsonData currently just returns its input Promise and adds no behavior; either remove readJsonData and update all callers to use the original promise directly, or keep it but document its intent and purpose (e.g., add a JSDoc comment stating it is a deliberate placeholder for future SDK response normalization/validation) and, if intended, implement minimal normalization (e.g., await the promise and validate/type-cast the result). Locate the function by name readJsonData and apply one of these two fixes consistently across the codebase.
200-210: Consider adding exhaustiveness check fortoPermissionReply.The switch statement covers all current
ProviderApprovalDecisionvalues, but adding adefaultcase with an exhaustiveness assertion would catch future additions at compile time.♻️ Proposed refactor
export function toPermissionReply(decision: ProviderApprovalDecision): "once" | "always" | "reject" { switch (decision) { case "acceptForSession": return "always"; case "accept": return "once"; case "decline": case "cancel": return "reject"; + default: { + const _exhaustive: never = decision; + return "reject"; + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/utils.ts` around lines 200 - 210, The toPermissionReply function's switch over ProviderApprovalDecision should be made exhaustive: add a default branch that takes a parameter typed as never (or call an existing assertNever helper) and throws a clear error so the compiler will flag any new ProviderApprovalDecision variants; update the switch in toPermissionReply to include that default/exhaustiveness assertion (referencing the function name toPermissionReply and the type ProviderApprovalDecision) to ensure future additions are caught at compile time.
356-367: Non-standard unified diff header format.The stats line
@@ +${d.additions},-${d.deletions} @@doesn't follow standard unified diff format (@@ -start,count +start,count @@). The docstring acknowledges this is an approximation, but consumers expecting parseable unified diff may fail.If this is intentional for display purposes only, consider renaming to
fileDiffsToSummaryor adding a clearer docstring indicating it's not parseable as standard unified diff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/utils.ts` around lines 356 - 367, The function fileDiffsToUnifiedDiff emits a non-standard "stats" line (`@@ +${d.additions},-${d.deletions} @@`) which breaks unified-diff consumers; rename the function to fileDiffsToSummary (or otherwise change its name) and update its implementation and docstring to clearly indicate it produces a human-readable summary rather than a parseable unified diff, then update all call sites that reference fileDiffsToUnifiedDiff to the new name; alternatively, if you need a real unified diff header, replace the stats line with a proper unified-diff header format such as `@@ -0,${d.deletions} +0,${d.additions} @@` and ensure callers expect a parseable diff (adjust tests/docs accordingly).apps/server/src/opencodeServerManager.ts (1)
471-471: MutatingmessageIds.lengthis unconventional but valid.While
context.messageIds.length = targetIndex + 1works to truncate the array, usingspliceis more conventional and explicit:♻️ Alternative using splice
- context.messageIds.length = targetIndex + 1; + context.messageIds.splice(targetIndex + 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencodeServerManager.ts` at line 471, The code truncates context.messageIds by assigning to its length (context.messageIds.length = targetIndex + 1); replace that with an explicit splice call to make intent clear and avoid surprising length mutation—e.g., call context.messageIds.splice(targetIndex + 1) to remove all elements after targetIndex. Update the occurrence in the function/method where context.messageIds and targetIndex are used in opencodeServerManager (search for context.messageIds.length = targetIndex + 1) so other logic relying on messageIds remains unchanged.apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts (1)
276-292:detailduplicatesmessage— consider whether additional context is needed.Both
messageanddetailin the payload are set totruncateDetail(message), which seems redundant. Ifdetailis intended to provide additional context beyond the summary message, consider including more information (e.g., error class, retryability hints). Otherwise, the duplication is harmless but unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts` around lines 276 - 292, The payload currently sets both message and detail to truncateDetail(message) which duplicates info; update the payload in ProviderRuntimeIngestion (see runtimePayloadRecord, runtimeErrorClassLabel, truncateDetail) so message remains truncateDetail(message) but detail contains additional context (for example the error class label and/or extra fields from runtimePayloadRecord(event) such as the raw error info or retryability hints), passed through truncateDetail as well; ensure turnId and maybeSequence usage remains unchanged.apps/server/src/opencode/serverLifecycle.ts (1)
19-26:probeServerswallows all errors — consider logging for debugging.The
.catch(() => undefined)silently swallows network errors, which is appropriate for probing, but may complicate debugging connectivity issues. Consider adding debug-level logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/serverLifecycle.ts` around lines 19 - 26, The probeServer function currently swallows all errors via .catch(() => undefined); update the catch to log the caught error at debug level (using an existing logger if available or console.debug) while preserving the undefined return so behavior is unchanged; reference probeServer, baseUrl, authHeader and SERVER_PROBE_TIMEOUT_MS when adding the log so the message includes the URL and any auth context to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/opencode/eventHandlers.ts`:
- Line 814: The itemId generation using
RuntimeItemId.makeUnsafe(`cmd:${command}:${Date.now()}`) can collide; replace
the Date.now() suffix with a UUID or the project's eventId helper to guarantee
uniqueness (e.g., use randomUUID() from node:crypto or the existing eventId
helper), update the import to include randomUUID if chosen, and construct the
string as `cmd:${command}:${<uuid or eventId>}` so RuntimeItemId.makeUnsafe
produces a collision-resistant id.
In `@apps/server/src/opencode/serverLifecycle.ts`:
- Around line 56-63: The catch block in ensureServer is awaiting serverPromise
again which will re-throw the rejection; change the error handling so you do not
re-await the rejected promise. In ensureServer (which calls spawnOrConnect and
stores serverPromise), either re-throw the caught error (remove the second
await) or return a failure result (e.g., { state: undefined, serverPromise:
undefined }) after clearing serverPromise; pick one behavior and implement it
consistently so the function does not call await serverPromise inside the catch.
---
Outside diff comments:
In `@apps/server/src/opencodeServerManager.ts`:
- Around line 1-639: Formatting is failing CI due to oxfmt style issues across
the opencode module; run the repository formatter and commit the changes. Fix by
running the project's formatter command (e.g., "bun run fmt" as suggested by CI)
or invoke oxfmt directly for the opencode files, then review and commit the
reformatted files (ensure changes touch code in OpenCodeServerManager and
related files under opencode/), and push the commit so CI can pass.
---
Nitpick comments:
In `@apps/server/src/opencode/serverLifecycle.ts`:
- Around line 19-26: The probeServer function currently swallows all errors via
.catch(() => undefined); update the catch to log the caught error at debug level
(using an existing logger if available or console.debug) while preserving the
undefined return so behavior is unchanged; reference probeServer, baseUrl,
authHeader and SERVER_PROBE_TIMEOUT_MS when adding the log so the message
includes the URL and any auth context to aid debugging.
In `@apps/server/src/opencode/utils.ts`:
- Around line 312-314: The function readJsonData currently just returns its
input Promise and adds no behavior; either remove readJsonData and update all
callers to use the original promise directly, or keep it but document its intent
and purpose (e.g., add a JSDoc comment stating it is a deliberate placeholder
for future SDK response normalization/validation) and, if intended, implement
minimal normalization (e.g., await the promise and validate/type-cast the
result). Locate the function by name readJsonData and apply one of these two
fixes consistently across the codebase.
- Around line 200-210: The toPermissionReply function's switch over
ProviderApprovalDecision should be made exhaustive: add a default branch that
takes a parameter typed as never (or call an existing assertNever helper) and
throws a clear error so the compiler will flag any new ProviderApprovalDecision
variants; update the switch in toPermissionReply to include that
default/exhaustiveness assertion (referencing the function name
toPermissionReply and the type ProviderApprovalDecision) to ensure future
additions are caught at compile time.
- Around line 356-367: The function fileDiffsToUnifiedDiff emits a non-standard
"stats" line (`@@ +${d.additions},-${d.deletions} @@`) which breaks unified-diff
consumers; rename the function to fileDiffsToSummary (or otherwise change its
name) and update its implementation and docstring to clearly indicate it
produces a human-readable summary rather than a parseable unified diff, then
update all call sites that reference fileDiffsToUnifiedDiff to the new name;
alternatively, if you need a real unified diff header, replace the stats line
with a proper unified-diff header format such as `@@ -0,${d.deletions}
+0,${d.additions} @@` and ensure callers expect a parseable diff (adjust
tests/docs accordingly).
In `@apps/server/src/opencodeServerManager.ts`:
- Line 471: The code truncates context.messageIds by assigning to its length
(context.messageIds.length = targetIndex + 1); replace that with an explicit
splice call to make intent clear and avoid surprising length mutation—e.g., call
context.messageIds.splice(targetIndex + 1) to remove all elements after
targetIndex. Update the occurrence in the function/method where
context.messageIds and targetIndex are used in opencodeServerManager (search for
context.messageIds.length = targetIndex + 1) so other logic relying on
messageIds remains unchanged.
In `@apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts`:
- Around line 276-292: The payload currently sets both message and detail to
truncateDetail(message) which duplicates info; update the payload in
ProviderRuntimeIngestion (see runtimePayloadRecord, runtimeErrorClassLabel,
truncateDetail) so message remains truncateDetail(message) but detail contains
additional context (for example the error class label and/or extra fields from
runtimePayloadRecord(event) such as the raw error info or retryability hints),
passed through truncateDetail as well; ensure turnId and maybeSequence usage
remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22ddc973-de7d-4a1d-ba87-f6f434699e52
📒 Files selected for processing (9)
apps/server/src/opencode/errors.tsapps/server/src/opencode/eventHandlers.tsapps/server/src/opencode/index.tsapps/server/src/opencode/serverLifecycle.tsapps/server/src/opencode/types.tsapps/server/src/opencode/utils.tsapps/server/src/opencodeServerManager.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.ts
…eServer catch - Replace Date.now() with randomUUID() in command.executed itemId to prevent millisecond-level collisions - Fix ensureServer catch block that re-awaited the rejected promise instead of propagating the error for retry
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/server/src/opencode/eventHandlers.ts (1)
810-810:⚠️ Potential issue | 🟡 MinorUse collision-resistant IDs for command execution items.
Date.now()can generate duplicateitemIds under concurrent/rapid command events.🐛 Suggested fix
+import { randomUUID } from "node:crypto"; import { ApprovalRequestId, RuntimeItemId, RuntimeRequestId } from "@t3tools/contracts"; @@ - const itemId = RuntimeItemId.makeUnsafe(`cmd:${command}:${Date.now()}`); + const itemId = RuntimeItemId.makeUnsafe(`cmd:${command}:${randomUUID()}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/eventHandlers.ts` at line 810, The itemId generation using RuntimeItemId.makeUnsafe with Date.now() is vulnerable to collisions under concurrent events; update the construction of item IDs in the handler where "itemId" is created (RuntimeItemId.makeUnsafe(`cmd:${command}:${Date.now()}`)) to use a collision-resistant identifier such as crypto.randomUUID() or a nanoid-style random suffix instead of Date.now(), e.g., combine the command with a UUID or secure random string to ensure uniqueness across rapid/concurrent command executions.apps/server/src/opencode/serverLifecycle.ts (1)
63-65:⚠️ Potential issue | 🟠 MajorDon’t re-await the rejected
serverPromiseinensureServer.The catch path re-awaits the same rejected promise, so it just throws again and never returns the fallback object.
🐛 Suggested fix
const serverPromise = spawnOrConnect(options); - try { - const state = await serverPromise; - return { state, serverPromise }; - } catch { - return { state: await serverPromise, serverPromise: undefined }; - } + const state = await serverPromise; + return { state, serverPromise };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/serverLifecycle.ts` around lines 63 - 65, The catch block in ensureServer currently re-awaits the rejected serverPromise which will throw again; change the catch to not await the rejected promise and instead return a safe fallback (e.g., { state: undefined, serverPromise: undefined }) or use serverPromise.catch(() => undefined) to convert rejection to a resolved value before awaiting; update the catch to return { state: undefined, serverPromise: undefined } (or { state: await serverPromise.catch(() => undefined), serverPromise: undefined }) and ensure references to ensureServer and serverPromise are used to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/opencode/utils.ts`:
- Around line 167-171: Replace the manual copy-and-sort pattern with
Array.prototype.toSorted to satisfy the unicorn lint rule; specifically, change
the sorting in utils.ts that uses [...providers].sort((a,b)=>{...}) to
providers.toSorted((a,b)=>{...}) while keeping the same comparator logic that
uses a.name || a.id and localeCompare, and ensure the variable `providers` is an
array in the same scope so `toSorted` can be called directly.
In `@apps/server/src/opencodeServerManager.ts`:
- Around line 456-466: In rollbackThread, validate the numTurns parameter before
computing targetIndex: ensure numTurns is a positive integer (use
Number.isInteger and numTurns > 0) and that it does not exceed ids.length; if
invalid, throw a clear Error (e.g., "Invalid numTurns: must be a positive
integer and <= tracked turns"). Then compute targetIndex using the
validated/clamped numTurns (e.g., Math.min(numTurns, ids.length)) so
targetMessageId from context.messageIds cannot be out of bounds before calling
session.revert/readJsonData; reference the rollbackThread method,
context.messageIds, and session.revert when making the checks and error.
---
Duplicate comments:
In `@apps/server/src/opencode/eventHandlers.ts`:
- Line 810: The itemId generation using RuntimeItemId.makeUnsafe with Date.now()
is vulnerable to collisions under concurrent events; update the construction of
item IDs in the handler where "itemId" is created
(RuntimeItemId.makeUnsafe(`cmd:${command}:${Date.now()}`)) to use a
collision-resistant identifier such as crypto.randomUUID() or a nanoid-style
random suffix instead of Date.now(), e.g., combine the command with a UUID or
secure random string to ensure uniqueness across rapid/concurrent command
executions.
In `@apps/server/src/opencode/serverLifecycle.ts`:
- Around line 63-65: The catch block in ensureServer currently re-awaits the
rejected serverPromise which will throw again; change the catch to not await the
rejected promise and instead return a safe fallback (e.g., { state: undefined,
serverPromise: undefined }) or use serverPromise.catch(() => undefined) to
convert rejection to a resolved value before awaiting; update the catch to
return { state: undefined, serverPromise: undefined } (or { state: await
serverPromise.catch(() => undefined), serverPromise: undefined }) and ensure
references to ensureServer and serverPromise are used to locate and modify the
code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c130015-418d-4c7e-924b-84193aa0eea6
📒 Files selected for processing (4)
apps/server/src/opencode/eventHandlers.tsapps/server/src/opencode/serverLifecycle.tsapps/server/src/opencode/utils.tsapps/server/src/opencodeServerManager.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/server/src/opencode/serverLifecycle.ts (1)
73-78: Consider probing user-providedserverUrlfor early failure detection.When
serverUrlis explicitly provided, the function returns immediately without verifying the server is reachable. If the server is down or unreachable, downstream operations will fail with less context. A probe here would provide a clearer early error.This is a minor UX improvement and may have been intentionally omitted to trust user configuration.
♻️ Optional: Add health probe for user-provided serverUrl
if (options?.serverUrl) { + const healthy = await probeServer(options.serverUrl, authHeader); + if (!healthy) { + throw new Error(`OpenCode server at ${options.serverUrl} is not reachable`); + } return { baseUrl: options.serverUrl, ...(authHeader ? { authHeader } : {}), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/serverLifecycle.ts` around lines 73 - 78, The early return when options?.serverUrl is set skips any reachability check; update the logic around the options.serverUrl return to perform a lightweight health probe (e.g., GET or HEAD to a /health or root path with a short timeout) using the same authHeader if present, and only return the { baseUrl, ...(authHeader ? { authHeader } : {}) } after the probe succeeds; on failure throw or propagate a clear error that includes the serverUrl and probe error so callers get immediate, actionable feedback (refer to options.serverUrl, authHeader and the code block that builds the returned object).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/opencode/eventHandlers.ts`:
- Around line 472-480: The answers object mixes string and array values; change
the missing-answer branch in the mapping that builds answers (the map over
pending?.questions using question.answerIndex and answerArrays) to return an
empty array instead of an empty string so every question id maps to an array;
ensure the mapped value for present answers remains the filtered array
(answer.filter(...)) so the resulting answers object has a uniform Array type
for all question ids.
- Around line 752-801: Both handleVcsBranchUpdatedEvent and
handleFileEditedEvent are missing sessionID validation and may emit events into
the wrong session; at the top of each function (before emitter.emitRuntimeEvent)
extract the sessionID from event.properties (const { sessionID: sessionId } =
event.properties) and return early if sessionId is present and differs from
context.providerSessionId (if (sessionId && sessionId !==
context.providerSessionId) { return; }), matching the pattern used in
handleSessionErrorEvent to ensure only matching provider sessions are processed.
---
Nitpick comments:
In `@apps/server/src/opencode/serverLifecycle.ts`:
- Around line 73-78: The early return when options?.serverUrl is set skips any
reachability check; update the logic around the options.serverUrl return to
perform a lightweight health probe (e.g., GET or HEAD to a /health or root path
with a short timeout) using the same authHeader if present, and only return the
{ baseUrl, ...(authHeader ? { authHeader } : {}) } after the probe succeeds; on
failure throw or propagate a clear error that includes the serverUrl and probe
error so callers get immediate, actionable feedback (refer to options.serverUrl,
authHeader and the code block that builds the returned object).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41c6eac7-4118-4307-85df-fe7256c8f679
📒 Files selected for processing (2)
apps/server/src/opencode/eventHandlers.tsapps/server/src/opencode/serverLifecycle.ts
Comment on lines
+472
to
+480
| const answers = Object.fromEntries( | ||
| (pending?.questions ?? []).map((question) => { | ||
| const answer = answerArrays[question.answerIndex]; | ||
| if (!answer) { | ||
| return [question.id, ""]; | ||
| } | ||
| return [question.id, answer.filter((value) => value.length > 0)]; | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Type inconsistency: mixed string/array values in answers object.
Line 476 returns an empty string "" for missing answers, while line 478 returns a filtered array for present answers. This results in an answers object with inconsistent value types, which could cause issues for consumers expecting uniform array values.
🐛 Proposed fix for type consistency
const answers = Object.fromEntries(
(pending?.questions ?? []).map((question) => {
const answer = answerArrays[question.answerIndex];
if (!answer) {
- return [question.id, ""];
+ return [question.id, []];
}
return [question.id, answer.filter((value) => value.length > 0)];
}),
);📝 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 answers = Object.fromEntries( | |
| (pending?.questions ?? []).map((question) => { | |
| const answer = answerArrays[question.answerIndex]; | |
| if (!answer) { | |
| return [question.id, ""]; | |
| } | |
| return [question.id, answer.filter((value) => value.length > 0)]; | |
| }), | |
| ); | |
| const answers = Object.fromEntries( | |
| (pending?.questions ?? []).map((question) => { | |
| const answer = answerArrays[question.answerIndex]; | |
| if (!answer) { | |
| return [question.id, []]; | |
| } | |
| return [question.id, answer.filter((value) => value.length > 0)]; | |
| }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/opencode/eventHandlers.ts` around lines 472 - 480, The
answers object mixes string and array values; change the missing-answer branch
in the mapping that builds answers (the map over pending?.questions using
question.answerIndex and answerArrays) to return an empty array instead of an
empty string so every question id maps to an array; ensure the mapped value for
present answers remains the filtered array (answer.filter(...)) so the resulting
answers object has a uniform Array type for all question ids.
Kilo is a fork of OpenCode sharing the same SDK and SSE API. Apply the same modular extraction and feature parity improvements: - Extract monolithic kiloServerManager.ts into kilo/ module directory (types, errors, utils, eventHandlers, serverLifecycle) - Emit session.configured and session.exited lifecycle events - Forward session.diff as turn.diff.updated with unified diff conversion - Handle new SSE events: session.compacted, session.updated, vcs.branch.updated, file.edited, command.executed, message.part.removed - Enrich events: todo priority, isRetryable, permission title/metadata - Implement rollbackThread via session.revert API with message ID tracking - Fix flaky KiloAdapter stream forwarding test (same race condition fix)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/server/src/kilo/utils.ts (1)
319-321:readJsonDatais currently a pass-through — is this intentional?The function simply returns the promise without transformation. If this is a placeholder for future error handling or response unwrapping, consider adding a TODO comment. Otherwise, callers could use the promise directly.
📝 If intentional, add clarifying comment
+/** + * Unwraps SDK responses. Currently a pass-through but provides a + * consistent hook point for future error handling or response normalization. + */ export async function readJsonData<T>(promise: Promise<T>): Promise<T> { return promise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/kilo/utils.ts` around lines 319 - 321, The readJsonData function is currently a no-op that just returns the incoming Promise; update it to either be removed (and callers use the Promise directly) or make its intent explicit: replace the pass-through with an awaited try/catch that unwraps the value and adds contextual error handling/validation (or at minimum add a TODO comment explaining it’s a deliberate placeholder). Locate the function readJsonData in the file and either delete usages and the function or implement await + try/catch to enrich errors or validate the JSON before returning, or add a clarifying comment/TODO indicating why it intentionally returns the promise.apps/server/src/kilo/types.ts (2)
104-109: Consider makingKiloDiscoveredModelpropertiesreadonlyfor consistency.Other types in this file use
readonlyproperties (e.g.,KiloModel,QuestionInfo), butKiloDiscoveredModelhas mutable properties. This inconsistency could lead to accidental mutations.📝 Suggested fix
export type KiloDiscoveredModel = { - slug: string; - name: string; - variants?: ReadonlyArray<string>; - connected?: boolean; + readonly slug: string; + readonly name: string; + readonly variants?: ReadonlyArray<string>; + readonly connected?: boolean; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/kilo/types.ts` around lines 104 - 109, The KiloDiscoveredModel type currently exposes mutable properties (slug, name, variants, connected) which is inconsistent with other readonly types; update the KiloDiscoveredModel declaration so each property is readonly (e.g., readonly slug, readonly name, readonly variants?: ReadonlyArray<string>, readonly connected?: boolean) to prevent accidental mutations and align with KiloModel and QuestionInfo.
538-540: Minor type redundancy:directoryis defined in bothOpencodeClientConfigandOpencodeClientOptions.
OpencodeClientConfigalready hasdirectory?: stringat line 448. The intersection type works but creates conceptual duplication.📝 Suggested simplification
-export type OpencodeClientOptions = OpencodeClientConfig & { - directory?: string; -}; +export type OpencodeClientOptions = OpencodeClientConfig;Or if the intent is to make
directoryrequired in options but optional in config, the type should reflect that explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/kilo/types.ts` around lines 538 - 540, OpencodeClientOptions currently repeats the optional property directory which already exists on OpencodeClientConfig; remove the redundancy by deleting directory?: string from the OpencodeClientOptions declaration (or if the intention is to make directory required for options, change the type there to directory: string while keeping OpencodeClientConfig.directory?: string) so that only OpencodeClientOptions or OpencodeClientConfig conveys the intended optional/required semantics for directory (refer to OpencodeClientOptions, OpencodeClientConfig, and the directory property).apps/server/src/kilo/eventHandlers.ts (2)
803-854: Command execution emits start+complete synchronously — verify this matches expected semantics.
handleCommandExecutedEventemits bothitem.startedanditem.completedevents in immediate succession with the same timestamp. This makes sense ifcommand.executedis a past-tense notification (the command already ran), but could be confusing if consumers expect a duration between start and completion.If this is the intended behavior, consider adding a brief comment explaining why both events are emitted together.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/kilo/eventHandlers.ts` around lines 803 - 854, handleCommandExecutedEvent currently emits both "item.started" and "item.completed" back-to-back with identical timestamps; confirm this is intentional and add a concise comment above handleCommandExecutedEvent explaining that EventCommandExecuted is a past-tense notification (the command already ran) so we emit both item.started and item.completed synchronously to represent an instantaneous dynamic_tool_call (include mention of the emitted event types "item.started" and "item.completed" and the use of RuntimeItemId.makeUnsafe/randomUUID for the itemId so readers understand why timestamps match).
53-114: Consider adding a default case for exhaustiveness.The switch statement handles all known event types but lacks a default case. If the SDK introduces new event types, they'll be silently ignored. While
message.part.removedis explicitly silenced with a comment, unknown events would pass through without any indication.📝 Optional: Add exhaustiveness check
case "command.executed": handleCommandExecutedEvent(emitter, context, event); return; + default: + // Exhaustiveness check - TypeScript will error if a new event type + // is added to KiloEvent but not handled here. + event satisfies never; }Alternatively, if silent passthrough is intentional, add a comment documenting that behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/kilo/eventHandlers.ts` around lines 53 - 114, The switch in handleEvent lacks a default branch so unknown event types are silently ignored; add a default case that either logs a warning (including event.type and the event payload, e.g., via processLogger.warn or emitter.emit("warning", ...)) or enforces exhaustiveness with a TypeScript never-check (assign event.type to a never-typed variable) so new event types are surfaced during compilation/runtime; reference handleEvent and event.type when adding the default handling.apps/server/src/kilo/errors.ts (1)
3-5: Minor grammatical issue in docstrings.The article "an" should be "a" before "Kilo" (since "Kilo" starts with a consonant sound). This appears in multiple comments throughout the file (lines 4, 51, 68).
📝 Suggested fix
/** - * Maps an Kilo error name to a runtime error class used by the + * Maps a Kilo error name to a runtime error class used by the * orchestration layer to categorize errors for display. */Similar fixes needed at lines 51 and 68.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/kilo/errors.ts` around lines 3 - 5, Fix the minor grammatical errors in the file-level and inline docstrings that use "an Kilo" — change the article "an" to "a" wherever Kilo is referenced (for example in the top comment describing the mapping and the docstrings around the KiloErrorToRuntimeErrorMap / mapKiloErrorNameToRuntimeError symbols and any comments near functions or constants around lines 51 and 68); update those comment strings only, preserving punctuation and formatting otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/kilo/serverLifecycle.ts`:
- Around line 60-68: The catch block in ensureServer (where serverPromise is
assigned from spawnOrConnect) contains a misleading comment about clearing the
promise but does not actually clear it; update the code to either remove that
comment or make the function responsible for clearing the cached promise;
specifically, modify the catch in ensureServer (the block that currently
rethrows the caught error after "Clear the promise..." comment) to remove the
misleading comment and, if you prefer making this function responsible,
explicitly clear the cached.serverPromise reference before rethrowing; also
update the function-level docstring to state that KiloServerManager.ensureServer
currently handles clearing in its finally block if you choose to keep clearing
out of ensureServer.
In `@apps/server/src/kiloServerManager.ts`:
- Around line 451-471: The rollbackThread method currently computes targetIndex
even when numTurns >= ids.length which causes a no-op revert to the first
message; add validation at the start of rollbackThread to check that numTurns is
a positive integer less than ids.length and throw a clear Error (e.g., `throw
new Error(\`Cannot rollback ${numTurns} turns: only ${ids.length} turns
tracked\`)`) when the request exceeds available messages; reference and validate
the local variables `numTurns`, `ids`/`context.messageIds` and keep the existing
revert logic (targetIndex, targetMessageId, client.session.revert) unchanged
after the guard.
---
Nitpick comments:
In `@apps/server/src/kilo/errors.ts`:
- Around line 3-5: Fix the minor grammatical errors in the file-level and inline
docstrings that use "an Kilo" — change the article "an" to "a" wherever Kilo is
referenced (for example in the top comment describing the mapping and the
docstrings around the KiloErrorToRuntimeErrorMap /
mapKiloErrorNameToRuntimeError symbols and any comments near functions or
constants around lines 51 and 68); update those comment strings only, preserving
punctuation and formatting otherwise.
In `@apps/server/src/kilo/eventHandlers.ts`:
- Around line 803-854: handleCommandExecutedEvent currently emits both
"item.started" and "item.completed" back-to-back with identical timestamps;
confirm this is intentional and add a concise comment above
handleCommandExecutedEvent explaining that EventCommandExecuted is a past-tense
notification (the command already ran) so we emit both item.started and
item.completed synchronously to represent an instantaneous dynamic_tool_call
(include mention of the emitted event types "item.started" and "item.completed"
and the use of RuntimeItemId.makeUnsafe/randomUUID for the itemId so readers
understand why timestamps match).
- Around line 53-114: The switch in handleEvent lacks a default branch so
unknown event types are silently ignored; add a default case that either logs a
warning (including event.type and the event payload, e.g., via
processLogger.warn or emitter.emit("warning", ...)) or enforces exhaustiveness
with a TypeScript never-check (assign event.type to a never-typed variable) so
new event types are surfaced during compilation/runtime; reference handleEvent
and event.type when adding the default handling.
In `@apps/server/src/kilo/types.ts`:
- Around line 104-109: The KiloDiscoveredModel type currently exposes mutable
properties (slug, name, variants, connected) which is inconsistent with other
readonly types; update the KiloDiscoveredModel declaration so each property is
readonly (e.g., readonly slug, readonly name, readonly variants?:
ReadonlyArray<string>, readonly connected?: boolean) to prevent accidental
mutations and align with KiloModel and QuestionInfo.
- Around line 538-540: OpencodeClientOptions currently repeats the optional
property directory which already exists on OpencodeClientConfig; remove the
redundancy by deleting directory?: string from the OpencodeClientOptions
declaration (or if the intention is to make directory required for options,
change the type there to directory: string while keeping
OpencodeClientConfig.directory?: string) so that only OpencodeClientOptions or
OpencodeClientConfig conveys the intended optional/required semantics for
directory (refer to OpencodeClientOptions, OpencodeClientConfig, and the
directory property).
In `@apps/server/src/kilo/utils.ts`:
- Around line 319-321: The readJsonData function is currently a no-op that just
returns the incoming Promise; update it to either be removed (and callers use
the Promise directly) or make its intent explicit: replace the pass-through with
an awaited try/catch that unwraps the value and adds contextual error
handling/validation (or at minimum add a TODO comment explaining it’s a
deliberate placeholder). Locate the function readJsonData in the file and either
delete usages and the function or implement await + try/catch to enrich errors
or validate the JSON before returning, or add a clarifying comment/TODO
indicating why it intentionally returns the promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56abafd3-8337-4a42-abe2-2ae7c1a10d3d
📒 Files selected for processing (8)
apps/server/src/kilo/errors.tsapps/server/src/kilo/eventHandlers.tsapps/server/src/kilo/index.tsapps/server/src/kilo/serverLifecycle.tsapps/server/src/kilo/types.tsapps/server/src/kilo/utils.tsapps/server/src/kiloServerManager.tsapps/server/src/provider/Layers/KiloAdapter.test.ts
- Use toSorted() instead of sort() on copied arrays (unicorn lint rule) - Validate numTurns in rollbackThread (positive integer, within bounds) - Add sessionID filtering to vcs.branch.updated and file.edited handlers - Fix misleading ensureServer catch comment (caller handles cleanup)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/server/src/opencode/eventHandlers.ts (1)
472-480:⚠️ Potential issue | 🟡 MinorType inconsistency: mixed string/array values in answers object.
Line 476 returns an empty string
""for missing answers, while line 478 returns a filtered array. This creates ananswersobject with inconsistent value types (string | string[]), which could cause issues for consumers expecting uniform array values.🐛 Proposed fix for type consistency
const answers = Object.fromEntries( (pending?.questions ?? []).map((question) => { const answer = answerArrays[question.answerIndex]; if (!answer) { - return [question.id, ""]; + return [question.id, []]; } return [question.id, answer.filter((value) => value.length > 0)]; }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/eventHandlers.ts` around lines 472 - 480, The answers object mixes strings and arrays causing type inconsistency: when an answer is missing the code returns "" but otherwise returns an array; update the mapping in the answers construction to return an empty array ([]) for missing answers so every value is a string[]; adjust the branch that uses answerArrays[question.answerIndex] (and any consumers expecting string[]) if necessary to reflect the uniform string[] shape for question.id entries.
🧹 Nitpick comments (3)
apps/server/src/opencode/utils.ts (2)
310-313: Consider adding documentation for the expected URL format.The regex pattern extracts the server URL from startup output. Adding a brief comment about what the expected output looks like would aid future maintenance.
📝 Suggested documentation
+/** + * Parses the server URL from OpenCode startup output. + * Expects a line like: "opencode server listening on http://127.0.0.1:6733" + */ export function parseServerUrl(output: string): string | undefined { const match = output.match(/opencode server listening on\s+(https?:\/\/[^\s]+)(?=\r?\n)/); return match?.[1]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/utils.ts` around lines 310 - 313, Add a brief comment above parseServerUrl describing the expected startup output format and what the regex captures: show an example line like "opencode server listening on https://host:port" (http or https), note that the function returns the first captured URL (protocol + host + optional port/path) and that the regex looks for the URL followed by a newline; mention any assumptions (e.g., no surrounding punctuation) so future maintainers understand why the pattern uses /opencode server listening on\s+(https?:\/\/[^\s]+)(?=\r?\n)/.
319-321: ThereadJsonDatafunction appears to be a no-op.This function simply returns the input promise without transformation. If this is a placeholder for future SDK response unwrapping, consider adding a comment explaining its purpose.
📝 Suggested documentation
+/** + * Unwraps SDK response data. Currently a pass-through; exists to provide + * a consistent interface if the SDK response format changes. + */ export async function readJsonData<T>(promise: Promise<T>): Promise<T> { return promise; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/utils.ts` around lines 319 - 321, The readJsonData function is currently a no-op (it just returns the input promise); either document it as an intentional placeholder or implement proper unwrapping: if it’s meant as a future SDK response adapter, add a JSDoc above readJsonData explaining its purpose and intended behavior; otherwise change readJsonData to await the incoming promise and return the resolved payload (for common SDK responses, extract and return the .data field when present) and update its signature/typing accordingly so callers receive the actual JSON value rather than the raw promise.apps/server/src/opencode/serverLifecycle.ts (1)
119-135: Consider:onExitasync callback may delay rejection.The
onExithandler callsprobeServerasynchronously after cleanup. If the probe takes time (up toSERVER_PROBE_TIMEOUT_MS), the Promise resolution is delayed. This is likely intentional to support server reuse scenarios, but worth noting that the startup flow may hang briefly on abrupt exits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/opencode/serverLifecycle.ts` around lines 119 - 135, The onExit handler currently awaits probeServer which can delay rejection; change onExit so that after cleanup it immediately rejects if the process exited with a non-zero code (include the trimmed output in the Error) and only run probeServer for reuse when the exit code is zero; additionally ensure probeServer is invoked with a bounded timeout (use SERVER_PROBE_TIMEOUT_MS or wrap probeServer in a Promise.race) so the startup promise cannot hang indefinitely; refer to onExit, probeServer, cleanup, resolve, reject, and SERVER_PROBE_TIMEOUT_MS when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/server/src/opencode/eventHandlers.ts`:
- Around line 472-480: The answers object mixes strings and arrays causing type
inconsistency: when an answer is missing the code returns "" but otherwise
returns an array; update the mapping in the answers construction to return an
empty array ([]) for missing answers so every value is a string[]; adjust the
branch that uses answerArrays[question.answerIndex] (and any consumers expecting
string[]) if necessary to reflect the uniform string[] shape for question.id
entries.
---
Nitpick comments:
In `@apps/server/src/opencode/serverLifecycle.ts`:
- Around line 119-135: The onExit handler currently awaits probeServer which can
delay rejection; change onExit so that after cleanup it immediately rejects if
the process exited with a non-zero code (include the trimmed output in the
Error) and only run probeServer for reuse when the exit code is zero;
additionally ensure probeServer is invoked with a bounded timeout (use
SERVER_PROBE_TIMEOUT_MS or wrap probeServer in a Promise.race) so the startup
promise cannot hang indefinitely; refer to onExit, probeServer, cleanup,
resolve, reject, and SERVER_PROBE_TIMEOUT_MS when making the change.
In `@apps/server/src/opencode/utils.ts`:
- Around line 310-313: Add a brief comment above parseServerUrl describing the
expected startup output format and what the regex captures: show an example line
like "opencode server listening on https://host:port" (http or https), note that
the function returns the first captured URL (protocol + host + optional
port/path) and that the regex looks for the URL followed by a newline; mention
any assumptions (e.g., no surrounding punctuation) so future maintainers
understand why the pattern uses /opencode server listening
on\s+(https?:\/\/[^\s]+)(?=\r?\n)/.
- Around line 319-321: The readJsonData function is currently a no-op (it just
returns the input promise); either document it as an intentional placeholder or
implement proper unwrapping: if it’s meant as a future SDK response adapter, add
a JSDoc above readJsonData explaining its purpose and intended behavior;
otherwise change readJsonData to await the incoming promise and return the
resolved payload (for common SDK responses, extract and return the .data field
when present) and update its signature/typing accordingly so callers receive the
actual JSON value rather than the raw promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8629b5b0-c7e1-4d2e-a68a-5563db311f26
📒 Files selected for processing (8)
apps/server/src/kilo/eventHandlers.tsapps/server/src/kilo/serverLifecycle.tsapps/server/src/kilo/utils.tsapps/server/src/kiloServerManager.tsapps/server/src/opencode/eventHandlers.tsapps/server/src/opencode/serverLifecycle.tsapps/server/src/opencode/utils.tsapps/server/src/opencodeServerManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/kilo/serverLifecycle.ts
3 tasks
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.
Summary
session.configuredandsession.exitedlifecycle events for parity with Claude Code, Copilot, and Codex adapterssession.diffasturn.diff.updatedwith unified diff conversion, enabling file change tracking in the work logsession.compacted,session.updated,vcs.branch.updated,file.edited,command.executed,message.part.removedisRetryableon API errors, permission title/pattern metadatarollbackThreadvia OpenCode'ssession.revertAPI (previously threw "not implemented")opencode/module files from monolithicopencodeServerManager.tsOpenCodeAdapterstream forwarding test (emit-before-consume eliminates fiber scheduling race)ProviderRuntimeIngestionfor better error display ("Provider error", "Connection error", etc.)Test plan
bun typecheck— 7/7 packages pass cleanbun test apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.ts— 24/24 passbun test apps/server/src/provider/Layers/OpenCodeAdapter.test.ts— 3/3 pass (previously 1 flaky timeout)Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests