chore: code quality cleanup (Snow Leopard pass)#20
Merged
aaditagrawal merged 5 commits intomainfrom Mar 17, 2026
Merged
Conversation
github-actions
bot
added
the
vouch:trusted
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces logging infrastructure across server modules, refines type constraints in schema handling, extracts formatRelativeTime to a shared utility, simplifies test mocks, removes unused error handlers and type imports, standardizes array sorting patterns, and removes web persistence layer orchestration. Changes span documentation, server orchestration, adapter logic, web utilities, and desktop entry points. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
the
size:L
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 33: Update the phrase "Long term maintainability is a core priority." in
AGENTS.md to the hyphenated form "Long-term maintainability is a core priority."
to use the correct compound-adjective style and maintain consistency in
documentation.
In `@apps/web/src/lib/utils.ts`:
- Around line 34-42: The formatRelativeTime function can produce NaN when given
a malformed ISO, so validate the parsed timestamp before math: compute const t =
new Date(iso).getTime() and check Number.isFinite(t) (or !Number.isNaN(t)); if
the timestamp is invalid return a safe fallback like "just now" or "unknown"
instead of proceeding. Update formatRelativeTime to use t for diff calculations
and guard minutes/hours/days on finite values so you never return "NaNd ago".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c742e566-7fd1-4d91-873e-4c5418da9c42
📒 Files selected for processing (20)
.github/ISSUE_TEMPLATE/bug_report.ymlAGENTS.mdapps/server/src/ampServerManager.tsapps/server/src/codexAppServerManager.tsapps/server/src/geminiCliServerManager.test.tsapps/server/src/kiloServerManager.tsapps/server/src/opencodeServerManager.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/projector.tsapps/server/src/persistence/NodeSqliteClient.tsapps/server/src/provider/Layers/ClaudeCodeAdapter.tsapps/server/src/provider/Layers/CopilotAdapter.tsapps/server/src/workspaceEntries.tsapps/server/src/wsServer.test.tsapps/web/src/appSettings.tsapps/web/src/components/CommandPalette.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/lib/utils.tsapps/web/src/store.test.ts
💤 Files with no reviewable changes (3)
- apps/server/src/workspaceEntries.ts
- apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
- apps/web/src/appSettings.ts
…licate utils - Fix all 27 oxlint warnings: unused imports/declarations, Array#sort→toSorted, no-map-spread, preserve-caught-error cause, no-useless-spread, unsafe optional chaining - Replace console.log/warn with createLogger() in codexAppServerManager and ClaudeCodeAdapter - Remove all as any casts: use Schema.Top constraint in projector, Effect.die() for test stubs, SQLInputValue[] for sqlite params - Extract duplicate formatRelativeTime into shared lib/utils.ts - Remove dead code: unused functions, interfaces, and imports across server and web
- Guard formatRelativeTime against malformed ISO input (NaN protection) - Fix "Long term" → "Long-term" hyphenation in AGENTS.md
- Guard formatRelativeTime against malformed ISO input (NaN protection) - Fix "Long term" → "Long-term" hyphenation in AGENTS.md - Remove unused ProviderApprovalDecision imports in kilo/opencode types - Remove useless try/catch wrappers in kilo/opencode serverLifecycle - Replace .sort() with .toSorted() in desktop log listing - Suppress array-index-key lint for static log line rendering
aaditagrawal
force-pushed
the
code-quality
branch
from
e6339ec to
022fbd8
Compare
github-actions
bot
added
size:XXL
github-actions
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/server/src/geminiCliServerManager.test.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFormatting gate is currently failing CI.
oxfmt --checkis failing for this PR branch, so this cannot merge cleanly until formatting is applied.As per coding guidelines
**/*.{ts,tsx,js,jsx}: All ofbun fmt,bun lint, andbun typecheckmust pass before considering tasks completed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/geminiCliServerManager.test.ts` at line 1, CI is failing oxfmt --check for the test file that starts with the import line `import { describe, expect, it, vi, beforeEach } from "vitest";`; run your formatter and linters (oxfmt --write or bun fmt), then run bun lint and bun typecheck locally, stage and commit the formatted changes so the import line and file conform to the repository format rules and the formatting gate passes.apps/server/src/kiloServerManager.ts (1)
767-1015:⚠️ Potential issue | 🟠 MajorMove session startup/resume and turn lifecycle brokerage back to the codex manager boundary.
This file now directly brokers startup/resume and turn lifecycle transitions, which diverges from the documented server orchestration boundary and can cause lifecycle drift across providers.
As per coding guidelines: Session startup/resume and turn lifecycle are brokered in
apps/server/src/codexAppServerManager.ts.Also applies to: 1183-1313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/kiloServerManager.ts` around lines 767 - 1015, The startSession and sendTurn methods are currently performing session startup/resume and turn lifecycle brokerage (emitting runtime events and mutating session state) but per guidelines that orchestration belongs in codexAppServerManager; revert these responsibilities in startSession and sendTurn so they only perform provider-level ops and delegate lifecycle brokerage to the codex manager boundary. Specifically, in startSession (function startSession and KiloSessionContext initialization) remove or stop emitting lifecycle events (emitRuntimeEvent calls with types "session.started" and "thread.started") and avoid driving session state transitions beyond returning the created initial session and starting the provider stream (context.streamTask = this.startStream(context)); in sendTurn (function sendTurn) do not emit turn/session lifecycle events ("turn.started", "session.state.changed", error/turn completion events) or unilaterally flip context.session.status/activeTurnId beyond what is necessary to call client.session.promptAsync; instead keep provider-level bookkeeping minimal (set context.activeTurnId while awaiting provider prompt, capture provider errors into context.lastError) and return the provider turn result so the codexAppServerManager can broker lifecycle changes. Locate uses of emitRuntimeEvent, context.session mutations, and the streamTask/startStream code paths to adjust behavior.apps/server/src/opencodeServerManager.ts (3)
1392-1416:⚠️ Potential issue | 🔴 CriticalComplete and clear the in-flight turn when the event stream fails.
This catch path marks the session errored, but it leaves
activeTurnId,pendingPermissions,pendingQuestions, andpartStreamByIdintact, and it never emits a terminalturn.completedorsession.state.changed. If the subscription drops mid-turn, consumers never see a closing event and stale per-turn state bleeds into later traffic.🤖 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 1392 - 1416, The catch block currently sets session to error but leaves in-flight turn state (context.activeTurnId, context.pendingPermissions, context.pendingQuestions, context.partStreamById) and doesn't emit terminal events; update the catch in opencodeServerManager to: finalize/clear any in-flight turn by setting context.activeTurnId = undefined and clearing context.pendingPermissions, context.pendingQuestions, and context.partStreamById; emit a terminal turn completion via this.emitRuntimeEvent for "turn.completed" (include threadId, turnId if present, provider, eventId, createdAt and a payload indicating error/aborted) and emit a session state change "session.state.changed" reflecting status "error" (use the same event structure pattern as the existing runtime.error emission); ensure these updates happen before returning so consumers receive a proper closing event when the stream fails.
928-953:⚠️ Potential issue | 🔴 CriticalReject overlapping turns before overwriting session state.
This manager only tracks one
activeTurnId, onelastError, and one set of pending request maps, butsendTurn()accepts a new turn unconditionally. A second call will overwrite the first turn's bookkeeping and misattribute subsequent stream events.Possible guard
async sendTurn(input: ProviderSendTurnInput): Promise<ProviderTurnStartResult> { const openCodeInput = input as OpenCodeSendTurnInput; const context = this.requireSession(input.threadId); + if (context.activeTurnId || context.session.status === "running") { + throw new Error( + `OpenCode session ${input.threadId} already has a turn in progress (${context.activeTurnId ?? "unknown"})`, + ); + } const turnId = createTurnId();🤖 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 928 - 953, sendTurn currently unconditionally overwrites session bookkeeping (context.activeTurnId, context.lastError, pending request maps) which allows overlapping turns to clobber each other; before generating a new turnId or mutating context, check the existing session state (e.g., context.activeTurnId and/or context.session.status === "running") and reject the incoming turn (throw or return a failed ProviderTurnStartResult) if a turn is already active, so you do not proceed to createTurnId or update context; update the logic in sendTurn (referencing sendTurn, context.activeTurnId, context.lastError, and context.session) to perform this guard early and only mutate session state after the guard passes.
1247-1277:⚠️ Potential issue | 🟠 MajorThe shared server cache ignores per-session endpoint and auth options.
After the first call populates
this.serverorthis.serverPromise, later sessions bypass their ownserverUrl,hostname,port,username,password, andbinaryPath. Starting two threads against different OpenCode endpoints will silently route both through the first server configuration.🤖 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 1247 - 1277, The ensureServer method currently caches a single this.server / this.serverPromise causing subsequent calls with different OpenCodeProviderOptions (serverUrl, hostname, port, username, password, binaryPath) to reuse the first server; change the caching so it is keyed by the computed server identity instead of global: compute a deterministic key from options (e.g. baseUrl plus auth/hash from buildAuthHeader or username/password) then use a Map<string, Promise<SharedServerState>> and Map<string, SharedServerState> (or a single Map for promise/state) instead of this.server/this.serverPromise; update ensureServer to look up/insert by that key, keep probeServer, buildAuthHeader and SharedServerState logic the same, and ensure concurrent calls for the same key reuse the same promise while different keys create separate server entries.
🤖 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/kiloServerManager.ts`:
- Around line 1373-1879: emitRuntimeEvent currently emits events locally
(this.emit("event", ...)) which bypasses the centralized coordinator; change
emitRuntimeEvent to forward all ProviderRuntimeEvent objects to the
ProviderManager coordinator instead. Inject or use the existing providerManager
instance on KiloServerManager and replace the body of emitRuntimeEvent to call
the providerManager coordination API (e.g.,
providerManager.coordinateRuntimeEvent(event) or
providerManager.dispatchRuntimeEvent(event)) so all runtime events (emitted from
handleSessionStatusEvent, handleSessionErrorEvent, handlePermissionAskedEvent,
handlePermissionRepliedEvent, handleQuestionAskedEvent,
handleQuestionRepliedEvent, handleQuestionRejectedEvent,
handleMessagePartUpdatedEvent, handleMessagePartDeltaEvent,
handleTodoUpdatedEvent) are routed and logged via providerManager rather than
this.emit; ensure providerManager is available on the class (constructor
injection or field) and update any unit tests/mocks accordingly.
In `@apps/server/src/opencodeServerManager.ts`:
- Around line 1174-1175: The public method rollbackThread currently always
throws (rollbackThread), which leaves callers with a runtime-failing API
surface; either implement rollbackThread or remove/disable its exposure at the
API boundary. Fix by removing or guarding the exposed rollback capability:
update the provider/capability descriptor (where OpenCode features are listed)
to mark rollback as unsupported, and change any surface that advertises/exports
rollback for OpenCode to check that capability before calling rollbackThread; if
you must keep the stub, replace the always-throwing implementation with a clear
UnsupportedOperationError and ensure callers check the provider's
supportsRollback flag before invoking rollbackThread.
- Around line 1892-1906: The emitted runtime event always sets payload.status to
"completed" for lifecycleType === "item.completed", which hides tool errors;
update the payload building in emitRuntimeEvent so payload.status reflects
actual error states (e.g., set status to "error" when the original tool
lifecycle indicates an error even if toToolLifecycleEventType(...) maps it to
"item.completed"). Locate the branch that sets status (the payload block in
emitRuntimeEvent) and change the condition to derive status from the true tool
lifecycle (use lifecycleType or the original toolLifecycle/error flag used by
toToolLifecycleEventType()), and also include any error details in the payload
when status === "error" so downstream consumers can distinguish failures from
successes.
- Around line 505-508: readResumeSessionId only extracts sessionId and the
resume path swallows session.get failures (session.get(...).catch(() =>
undefined)), which causes workspace to be lost and transient errors to silently
create a new session; update the resume flow so the saved workspace is restored
along with sessionId and transient auth/transport errors are not converted into
a successful "no session" case. Specifically, change readResumeSessionId (or
replace it) to extract both sessionId and workspace from the resumeCursor using
asRecord/asString (e.g., return { sessionId, workspace }), then in the resume
logic remove the .catch(() => undefined) on session.get so errors are surfaced
(either rethrow or return an explicit failure to the caller) and only call
session.create() when you have a deliberate, confirmed "no existing session"
result, not on any get error; ensure any error paths log or propagate the
original error instead of falling back to session.create().
---
Outside diff comments:
In `@apps/server/src/geminiCliServerManager.test.ts`:
- Line 1: CI is failing oxfmt --check for the test file that starts with the
import line `import { describe, expect, it, vi, beforeEach } from "vitest";`;
run your formatter and linters (oxfmt --write or bun fmt), then run bun lint and
bun typecheck locally, stage and commit the formatted changes so the import line
and file conform to the repository format rules and the formatting gate passes.
In `@apps/server/src/kiloServerManager.ts`:
- Around line 767-1015: The startSession and sendTurn methods are currently
performing session startup/resume and turn lifecycle brokerage (emitting runtime
events and mutating session state) but per guidelines that orchestration belongs
in codexAppServerManager; revert these responsibilities in startSession and
sendTurn so they only perform provider-level ops and delegate lifecycle
brokerage to the codex manager boundary. Specifically, in startSession (function
startSession and KiloSessionContext initialization) remove or stop emitting
lifecycle events (emitRuntimeEvent calls with types "session.started" and
"thread.started") and avoid driving session state transitions beyond returning
the created initial session and starting the provider stream (context.streamTask
= this.startStream(context)); in sendTurn (function sendTurn) do not emit
turn/session lifecycle events ("turn.started", "session.state.changed",
error/turn completion events) or unilaterally flip
context.session.status/activeTurnId beyond what is necessary to call
client.session.promptAsync; instead keep provider-level bookkeeping minimal (set
context.activeTurnId while awaiting provider prompt, capture provider errors
into context.lastError) and return the provider turn result so the
codexAppServerManager can broker lifecycle changes. Locate uses of
emitRuntimeEvent, context.session mutations, and the streamTask/startStream code
paths to adjust behavior.
In `@apps/server/src/opencodeServerManager.ts`:
- Around line 1392-1416: The catch block currently sets session to error but
leaves in-flight turn state (context.activeTurnId, context.pendingPermissions,
context.pendingQuestions, context.partStreamById) and doesn't emit terminal
events; update the catch in opencodeServerManager to: finalize/clear any
in-flight turn by setting context.activeTurnId = undefined and clearing
context.pendingPermissions, context.pendingQuestions, and
context.partStreamById; emit a terminal turn completion via
this.emitRuntimeEvent for "turn.completed" (include threadId, turnId if present,
provider, eventId, createdAt and a payload indicating error/aborted) and emit a
session state change "session.state.changed" reflecting status "error" (use the
same event structure pattern as the existing runtime.error emission); ensure
these updates happen before returning so consumers receive a proper closing
event when the stream fails.
- Around line 928-953: sendTurn currently unconditionally overwrites session
bookkeeping (context.activeTurnId, context.lastError, pending request maps)
which allows overlapping turns to clobber each other; before generating a new
turnId or mutating context, check the existing session state (e.g.,
context.activeTurnId and/or context.session.status === "running") and reject the
incoming turn (throw or return a failed ProviderTurnStartResult) if a turn is
already active, so you do not proceed to createTurnId or update context; update
the logic in sendTurn (referencing sendTurn, context.activeTurnId,
context.lastError, and context.session) to perform this guard early and only
mutate session state after the guard passes.
- Around line 1247-1277: The ensureServer method currently caches a single
this.server / this.serverPromise causing subsequent calls with different
OpenCodeProviderOptions (serverUrl, hostname, port, username, password,
binaryPath) to reuse the first server; change the caching so it is keyed by the
computed server identity instead of global: compute a deterministic key from
options (e.g. baseUrl plus auth/hash from buildAuthHeader or username/password)
then use a Map<string, Promise<SharedServerState>> and Map<string,
SharedServerState> (or a single Map for promise/state) instead of
this.server/this.serverPromise; update ensureServer to look up/insert by that
key, keep probeServer, buildAuthHeader and SharedServerState logic the same, and
ensure concurrent calls for the same key reuse the same promise while different
keys create separate server entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65cc5bb5-e4d1-45f9-b0a5-0b80f68cac92
📒 Files selected for processing (25)
AGENTS.mdapps/desktop/src/main.tsapps/server/src/ampServerManager.tsapps/server/src/codexAppServerManager.tsapps/server/src/geminiCliServerManager.test.tsapps/server/src/kilo/serverLifecycle.tsapps/server/src/kilo/types.tsapps/server/src/kiloServerManager.tsapps/server/src/opencode/serverLifecycle.tsapps/server/src/opencode/types.tsapps/server/src/opencodeServerManager.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.tsapps/server/src/orchestration/projector.tsapps/server/src/persistence/NodeSqliteClient.tsapps/server/src/provider/Layers/ClaudeCodeAdapter.tsapps/server/src/provider/Layers/CopilotAdapter.tsapps/server/src/workspaceEntries.tsapps/server/src/wsServer.test.tsapps/web/src/appSettings.tsapps/web/src/components/CommandPalette.tsxapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/lib/utils.tsapps/web/src/routes/_chat.settings.tsxapps/web/src/store.test.ts
💤 Files with no reviewable changes (5)
- apps/server/src/orchestration/Layers/ProviderCommandReactor.ts
- apps/server/src/opencode/types.ts
- apps/server/src/kilo/types.ts
- apps/server/src/workspaceEntries.ts
- apps/web/src/appSettings.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/server/src/persistence/NodeSqliteClient.ts
- apps/web/src/store.test.ts
- apps/server/src/provider/Layers/CopilotAdapter.ts
- apps/server/src/codexAppServerManager.ts
- apps/web/src/components/chat/ProviderModelPicker.tsx
- apps/web/src/components/CommandPalette.tsx
- apps/web/src/components/Sidebar.tsx
apps/server/src/kiloServerManager.ts
Outdated
Comment on lines
1373
to
1879
| private handleEvent(context: KiloSessionContext, event: KiloEvent): void { | ||
| switch (event.type) { | ||
| case "session.status": | ||
| this.handleSessionStatusEvent(context, event); | ||
| return; | ||
| case "session.error": | ||
| this.handleSessionErrorEvent(context, event); | ||
| return; | ||
| case "permission.asked": | ||
| this.handlePermissionAskedEvent(context, event); | ||
| return; | ||
| case "permission.replied": | ||
| this.handlePermissionRepliedEvent(context, event); | ||
| return; | ||
| case "question.asked": | ||
| this.handleQuestionAskedEvent(context, event); | ||
| return; | ||
| case "question.replied": | ||
| this.handleQuestionRepliedEvent(context, event); | ||
| return; | ||
| case "question.rejected": | ||
| this.handleQuestionRejectedEvent(context, event); | ||
| return; | ||
| case "message.part.updated": | ||
| this.handleMessagePartUpdatedEvent(context, event); | ||
| return; | ||
| case "message.part.delta": | ||
| this.handleMessagePartDeltaEvent(context, event); | ||
| return; | ||
| case "todo.updated": | ||
| this.handleTodoUpdatedEvent(context, event); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| private handleSessionStatusEvent(context: KiloSessionContext, event: EventSessionStatus): void { | ||
| const { sessionID: sessionId, status } = event.properties; | ||
| if (sessionId !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| const statusType = status.type; | ||
|
|
||
| if (statusType === "busy") { | ||
| context.session = { | ||
| ...context.session, | ||
| status: "running", | ||
| updatedAt: nowIso(), | ||
| }; | ||
| this.emitRuntimeEvent({ | ||
| type: "session.state.changed", | ||
| eventId: eventId("kilo-status-busy"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| payload: { | ||
| state: "running", | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: statusType, | ||
| payload: event, | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (statusType === "retry") { | ||
| this.emitRuntimeEvent({ | ||
| type: "session.exited", | ||
| eventId: eventId("kilo-session-exited-error"), | ||
| type: "session.state.changed", | ||
| eventId: eventId("kilo-status-retry"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| payload: { | ||
| reason: message, | ||
| exitKind: "error", | ||
| recoverable: false, | ||
| state: "waiting", | ||
| reason: "retry", | ||
| detail: event, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: statusType, | ||
| payload: event, | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (statusType === "idle") { | ||
| const completedAt = nowIso(); | ||
| const turnId = context.activeTurnId; | ||
| const lastError = context.lastError; | ||
| context.activeTurnId = undefined; | ||
| context.lastError = undefined; | ||
| context.session = { | ||
| ...stripTransientSessionFields(context.session), | ||
| status: lastError ? "error" : "ready", | ||
| updatedAt: completedAt, | ||
| ...(lastError ? { lastError } : {}), | ||
| }; | ||
|
|
||
| this.emitRuntimeEvent({ | ||
| type: "session.state.changed", | ||
| eventId: eventId("kilo-status-idle"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: completedAt, | ||
| ...(turnId ? { turnId } : {}), | ||
| payload: { | ||
| state: lastError ? "error" : "ready", | ||
| ...(lastError ? { reason: lastError } : {}), | ||
| detail: event, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: statusType, | ||
| payload: event, | ||
| }, | ||
| }); | ||
|
|
||
| if (turnId) { | ||
| this.emitRuntimeEvent({ | ||
| type: "turn.completed", | ||
| eventId: eventId("kilo-turn-completed"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: completedAt, | ||
| turnId, | ||
| payload: { | ||
| state: lastError ? "failed" : "completed", | ||
| ...(lastError ? { errorMessage: lastError } : {}), | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: statusType, | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private handleSessionErrorEvent(context: KiloSessionContext, event: EventSessionError): void { | ||
| const { sessionID: sessionId, error } = event.properties; | ||
| if (sessionId && sessionId !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| const errorMessage = sessionErrorMessage(error) ?? "Kilo session error"; | ||
| context.lastError = errorMessage; | ||
| context.session = { | ||
| ...stripTransientSessionFields(context.session), | ||
| status: "error", | ||
| updatedAt: nowIso(), | ||
| lastError: errorMessage, | ||
| }; | ||
| this.emitRuntimeEvent({ | ||
| type: "runtime.error", | ||
| eventId: eventId("kilo-session-error"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| payload: { | ||
| message: errorMessage, | ||
| class: "provider_error", | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: "session.error", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| private handlePermissionAskedEvent( | ||
| context: KiloSessionContext, | ||
| event: EventPermissionAsked, | ||
| ): void { | ||
| const { id: requestIdValue, sessionID: sessionId, permission } = event.properties; | ||
| if (sessionId !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| const requestType = toOpencodeRequestType(permission); | ||
| const requestId = ApprovalRequestId.makeUnsafe(requestIdValue); | ||
| context.pendingPermissions.set(requestId, { requestId, requestType }); | ||
| this.emitRuntimeEvent({ | ||
| type: "request.opened", | ||
| eventId: eventId("kilo-request-opened"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| requestId: RuntimeRequestId.makeUnsafe(requestId), | ||
| payload: { | ||
| requestType, | ||
| detail: permission, | ||
| args: event.properties, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.permission", | ||
| messageType: "permission.asked", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| private handlePermissionRepliedEvent( | ||
| context: KiloSessionContext, | ||
| event: EventPermissionReplied, | ||
| ): void { | ||
| const { requestID: requestIdValue, sessionID: sessionId, reply } = event.properties; | ||
| if (sessionId !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| const pending = context.pendingPermissions.get(requestIdValue); | ||
| context.pendingPermissions.delete(requestIdValue); | ||
| this.emitRuntimeEvent({ | ||
| type: "request.resolved", | ||
| eventId: eventId("kilo-request-resolved"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| requestId: RuntimeRequestId.makeUnsafe(requestIdValue), | ||
| payload: { | ||
| requestType: pending?.requestType ?? "unknown", | ||
| decision: reply, | ||
| resolution: event.properties, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.permission", | ||
| messageType: "permission.replied", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| private handleQuestionAskedEvent(context: KiloSessionContext, event: EventQuestionAsked): void { | ||
| const { | ||
| id: requestIdValue, | ||
| sessionID: sessionId, | ||
| questions: askedQuestions, | ||
| } = event.properties; | ||
| if (sessionId !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| const questions = askedQuestions.map((question: QuestionInfo, index) => ({ | ||
| answerIndex: index, | ||
| id: `${requestIdValue}:${index}`, | ||
| header: question.header, | ||
| question: question.question, | ||
| options: question.options.map((option) => ({ | ||
| label: option.label, | ||
| description: option.description, | ||
| })), | ||
| })); | ||
| const runtimeQuestions = questions.map((question) => ({ | ||
| id: question.id, | ||
| header: question.header, | ||
| question: question.question, | ||
| options: question.options, | ||
| })); | ||
|
|
||
| const requestId = ApprovalRequestId.makeUnsafe(requestIdValue); | ||
| context.pendingQuestions.set(requestId, { | ||
| requestId, | ||
| questionIds: questions.map((question) => question.id), | ||
| questions, | ||
| }); | ||
| this.emitRuntimeEvent({ | ||
| type: "user-input.requested", | ||
| eventId: eventId("kilo-user-input-requested"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| requestId: RuntimeRequestId.makeUnsafe(requestId), | ||
| payload: { | ||
| questions: runtimeQuestions, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.question", | ||
| messageType: "question.asked", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| private handleQuestionRepliedEvent( | ||
| context: KiloSessionContext, | ||
| event: EventQuestionReplied, | ||
| ): void { | ||
| const { | ||
| requestID: requestIdValue, | ||
| sessionID: sessionId, | ||
| answers: answerArrays, | ||
| } = event.properties; | ||
| if (sessionId !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| const pending = context.pendingQuestions.get(requestIdValue); | ||
| context.pendingQuestions.delete(requestIdValue); | ||
| 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)]; | ||
| }), | ||
| ); | ||
| this.emitRuntimeEvent({ | ||
| type: "user-input.resolved", | ||
| eventId: eventId("kilo-user-input-resolved"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| requestId: RuntimeRequestId.makeUnsafe(requestIdValue), | ||
| payload: { | ||
| answers, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.question", | ||
| messageType: "question.replied", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| private handleQuestionRejectedEvent( | ||
| context: KiloSessionContext, | ||
| event: EventQuestionRejected, | ||
| ): void { | ||
| const { requestID: requestIdValue, sessionID: sessionId } = event.properties; | ||
| if (sessionId !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| context.pendingQuestions.delete(requestIdValue); | ||
| this.emitRuntimeEvent({ | ||
| type: "user-input.resolved", | ||
| eventId: eventId("kilo-user-input-rejected"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| requestId: RuntimeRequestId.makeUnsafe(requestIdValue), | ||
| payload: { | ||
| answers: {}, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.question", | ||
| messageType: "question.rejected", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| private handleMessagePartUpdatedEvent( | ||
| context: KiloSessionContext, | ||
| event: EventMessagePartUpdated, | ||
| ): void { | ||
| const { part } = event.properties; | ||
| if (part.sessionID !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| if (part.type === "text") { | ||
| context.partStreamById.set(part.id, { kind: "text", streamKind: "assistant_text" }); | ||
| return; | ||
| } | ||
| if (part.type === "reasoning") { | ||
| context.partStreamById.set(part.id, { kind: "reasoning", streamKind: "reasoning_text" }); | ||
| return; | ||
| } | ||
|
|
||
| if (part.type === "tool") { | ||
| this.handleToolPartUpdatedEvent(context, event, part); | ||
| } | ||
| } | ||
|
|
||
| private handleToolPartUpdatedEvent( | ||
| context: KiloSessionContext, | ||
| event: EventMessagePartUpdated, | ||
| part: KiloToolPart, | ||
| ): void { | ||
| const previous = context.partStreamById.get(part.id); | ||
| const title = toolStateTitle(part.state); | ||
| const detail = toolStateDetail(part.state); | ||
| const lifecycleType = toToolLifecycleEventType(previous, part.state.status); | ||
|
|
||
| context.partStreamById.set(part.id, { kind: "tool" }); | ||
| this.emitRuntimeEvent({ | ||
| type: lifecycleType, | ||
| eventId: eventId(`kilo-tool-${lifecycleType.replace(".", "-")}`), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| itemId: RuntimeItemId.makeUnsafe(part.id), | ||
| payload: { | ||
| itemType: toToolItemType(part.tool), | ||
| ...(lifecycleType !== "item.updated" | ||
| ? { | ||
| status: lifecycleType === "item.completed" ? "completed" : "inProgress", | ||
| } | ||
| : {}), | ||
| title: toToolTitle(part.tool), | ||
| ...(detail ? { detail } : {}), | ||
| data: { | ||
| item: part, | ||
| }, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: "message.part.updated", | ||
| payload: event, | ||
| }, | ||
| }); | ||
|
|
||
| if ((part.state.status === "completed" || part.state.status === "error") && title) { | ||
| this.emitRuntimeEvent({ | ||
| type: "tool.summary", | ||
| eventId: eventId("kilo-tool-summary"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| itemId: RuntimeItemId.makeUnsafe(part.id), | ||
| payload: { | ||
| summary: `${part.tool}: ${title}`, | ||
| precedingToolUseIds: [part.id], | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: "message.part.updated", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| private handleMessagePartDeltaEvent( | ||
| context: KiloSessionContext, | ||
| event: EventMessagePartDelta, | ||
| ): void { | ||
| const { sessionID, partID: partId, delta } = event.properties; | ||
| if (sessionID !== context.providerSessionId) { | ||
| return; | ||
| } | ||
| if (!context.activeTurnId || delta.length === 0) { | ||
| return; | ||
| } | ||
| const partState = context.partStreamById.get(partId); | ||
| if (partState?.kind === "tool") { | ||
| return; | ||
| } | ||
| this.emitRuntimeEvent({ | ||
| type: "content.delta", | ||
| eventId: eventId("kilo-content-delta"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| turnId: context.activeTurnId, | ||
| itemId: RuntimeItemId.makeUnsafe(partId), | ||
| payload: { | ||
| streamKind: partState?.streamKind ?? "assistant_text", | ||
| delta, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: "message.part.delta", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| private handleTodoUpdatedEvent(context: KiloSessionContext, event: EventTodoUpdated): void { | ||
| const { sessionID, todos } = event.properties; | ||
| if (sessionID !== context.providerSessionId || !context.activeTurnId) { | ||
| return; | ||
| } | ||
| const plan = todos.map((todo) => ({ | ||
| step: todo.content, | ||
| status: toPlanStepStatus(todo.status), | ||
| })); | ||
| this.emitRuntimeEvent({ | ||
| type: "turn.plan.updated", | ||
| eventId: eventId("kilo-plan-updated"), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| turnId: context.activeTurnId, | ||
| payload: { | ||
| plan, | ||
| }, | ||
| raw: { | ||
| source: "kilo.server.event", | ||
| messageType: "todo.updated", | ||
| payload: event, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| emitRuntimeEvent(event: KiloProviderRuntimeEvent): void { | ||
| this.emit("event", event as unknown as ProviderRuntimeEvent); | ||
| private emitRuntimeEvent(event: ProviderRuntimeEvent): void { | ||
| this.emit("event", event); | ||
| } |
There was a problem hiding this comment.
Route provider dispatch and thread event logging through providerManager coordination.
Local event routing/dispatch in this manager bypasses the designated coordinator and increases cross-provider inconsistency risk.
As per coding guidelines: Provider dispatch and thread event logging are coordinated in apps/server/src/providerManager.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/kiloServerManager.ts` around lines 1373 - 1879,
emitRuntimeEvent currently emits events locally (this.emit("event", ...)) which
bypasses the centralized coordinator; change emitRuntimeEvent to forward all
ProviderRuntimeEvent objects to the ProviderManager coordinator instead. Inject
or use the existing providerManager instance on KiloServerManager and replace
the body of emitRuntimeEvent to call the providerManager coordination API (e.g.,
providerManager.coordinateRuntimeEvent(event) or
providerManager.dispatchRuntimeEvent(event)) so all runtime events (emitted from
handleSessionStatusEvent, handleSessionErrorEvent, handlePermissionAskedEvent,
handlePermissionRepliedEvent, handleQuestionAskedEvent,
handleQuestionRepliedEvent, handleQuestionRejectedEvent,
handleMessagePartUpdatedEvent, handleMessagePartDeltaEvent,
handleTodoUpdatedEvent) are routed and logged via providerManager rather than
this.emit; ensure providerManager is available on the class (constructor
injection or field) and update any unit tests/mocks accordingly.
Comment on lines
+505
to
+508
| function readResumeSessionId(resumeCursor: unknown): string | undefined { | ||
| const record = asRecord(resumeCursor); | ||
| return asString(record?.sessionId); | ||
| } |
There was a problem hiding this comment.
Resume drops the saved workspace and silently forks the thread on lookup failures.
resumeCursor persists workspace, but only sessionId is restored here. On top of that, session.get(...).catch(() => undefined) converts any auth/transport failure into a fresh session.create(), so a transient resume failure can quietly start a brand-new OpenCode session instead of surfacing the error.
Also applies to: 829-846
🤖 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 505 - 508,
readResumeSessionId only extracts sessionId and the resume path swallows
session.get failures (session.get(...).catch(() => undefined)), which causes
workspace to be lost and transient errors to silently create a new session;
update the resume flow so the saved workspace is restored along with sessionId
and transient auth/transport errors are not converted into a successful "no
session" case. Specifically, change readResumeSessionId (or replace it) to
extract both sessionId and workspace from the resumeCursor using
asRecord/asString (e.g., return { sessionId, workspace }), then in the resume
logic remove the .catch(() => undefined) on session.get so errors are surfaced
(either rethrow or return an explicit failure to the caller) and only call
session.create() when you have a deliberate, confirmed "no existing session"
result, not on any get error; ensure any error paths log or propagate the
original error instead of falling back to session.create().
Comment on lines
+1174
to
+1175
| async rollbackThread(threadId: ThreadId): Promise<ProviderThreadSnapshot> { | ||
| throw new Error(`OpenCode rollback is not implemented for thread '${threadId}'`); |
There was a problem hiding this comment.
Don't leave a public rollback path as a runtime stub.
rollbackThread() now always throws, so any caller that still exposes rollback for OpenCode will fail at runtime. If rollback is intentionally unsupported, that capability needs to be blocked at the API boundary instead of preserved as a method that always explodes.
🤖 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 1174 - 1175, The
public method rollbackThread currently always throws (rollbackThread), which
leaves callers with a runtime-failing API surface; either implement
rollbackThread or remove/disable its exposure at the API boundary. Fix by
removing or guarding the exposed rollback capability: update the
provider/capability descriptor (where OpenCode features are listed) to mark
rollback as unsupported, and change any surface that advertises/exports rollback
for OpenCode to check that capability before calling rollbackThread; if you must
keep the stub, replace the always-throwing implementation with a clear
UnsupportedOperationError and ensure callers check the provider's
supportsRollback flag before invoking rollbackThread.
Comment on lines
+1892
to
+1906
| this.emitRuntimeEvent({ | ||
| type: lifecycleType, | ||
| eventId: eventId(`opencode-tool-${lifecycleType.replace(".", "-")}`), | ||
| provider: PROVIDER, | ||
| threadId: context.threadId, | ||
| createdAt: nowIso(), | ||
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | ||
| itemId: RuntimeItemId.makeUnsafe(part.id), | ||
| payload: { | ||
| itemType: toToolItemType(part.tool), | ||
| ...(lifecycleType !== "item.updated" | ||
| ? { | ||
| status: lifecycleType === "item.completed" ? "completed" : "inProgress", | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
Tool errors are emitted as successful completions.
toToolLifecycleEventType() maps both completed and error to "item.completed", but this payload always reports status: "completed" for that branch. Downstream consumers cannot distinguish a failed tool run from a successful one.
Minimal fix
payload: {
itemType: toToolItemType(part.tool),
...(lifecycleType !== "item.updated"
? {
- status: lifecycleType === "item.completed" ? "completed" : "inProgress",
+ status:
+ lifecycleType === "item.completed"
+ ? part.state.status === "error"
+ ? "failed"
+ : "completed"
+ : "inProgress",
}
: {}),📝 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
| this.emitRuntimeEvent({ | |
| type: lifecycleType, | |
| eventId: eventId(`opencode-tool-${lifecycleType.replace(".", "-")}`), | |
| provider: PROVIDER, | |
| threadId: context.threadId, | |
| createdAt: nowIso(), | |
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | |
| itemId: RuntimeItemId.makeUnsafe(part.id), | |
| payload: { | |
| itemType: toToolItemType(part.tool), | |
| ...(lifecycleType !== "item.updated" | |
| ? { | |
| status: lifecycleType === "item.completed" ? "completed" : "inProgress", | |
| } | |
| : {}), | |
| this.emitRuntimeEvent({ | |
| type: lifecycleType, | |
| eventId: eventId(`opencode-tool-${lifecycleType.replace(".", "-")}`), | |
| provider: PROVIDER, | |
| threadId: context.threadId, | |
| createdAt: nowIso(), | |
| ...(context.activeTurnId ? { turnId: context.activeTurnId } : {}), | |
| itemId: RuntimeItemId.makeUnsafe(part.id), | |
| payload: { | |
| itemType: toToolItemType(part.tool), | |
| ...(lifecycleType !== "item.updated" | |
| ? { | |
| status: | |
| lifecycleType === "item.completed" | |
| ? part.state.status === "error" | |
| ? "failed" | |
| : "completed" | |
| : "inProgress", | |
| } | |
| : {}), |
🤖 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 1892 - 1906, The
emitted runtime event always sets payload.status to "completed" for
lifecycleType === "item.completed", which hides tool errors; update the payload
building in emitRuntimeEvent so payload.status reflects actual error states
(e.g., set status to "error" when the original tool lifecycle indicates an error
even if toToolLifecycleEventType(...) maps it to "item.completed"). Locate the
branch that sets status (the payload block in emitRuntimeEvent) and change the
condition to derive status from the true tool lifecycle (use lifecycleType or
the original toolLifecycle/error flag used by toToolLifecycleEventType()), and
also include any error details in the payload when status === "error" so
downstream consumers can distinguish failures from successes.
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
A focused code quality sweep — no behavior changes, just cleaning up warnings, dead code, and type safety issues.
Array#sort()→toSorted(),no-map-spread(useObject.assign),preserve-caught-error(add{ cause }),no-useless-spread,no-unsafe-optional-chainingconsole.log/warnwithcreateLogger()incodexAppServerManagerandClaudeCodeAdapteras anycasts: properSchema.Topconstraint in projector,Effect.die()for test stubs,SQLInputValue[]for sqlite paramsformatRelativeTimefromCommandPalette+Sidebarinto sharedlib/utils.tsTest plan
bun lint— 0 warnings, 0 errorsbun typecheck— 0 type errors across all 7 packagesbun run test— 547 tests pass, 0 failuresSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores