feat(mobile): create sessions on local runtimes#4572
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ); | ||
|
|
||
| const openRuntimePicker = useCallback(() => { | ||
| if (viewModel.kind !== 'selecting-runtime') { |
There was a problem hiding this comment.
CRITICAL: openRuntimePicker guard makes the "Runtime" row a dead button in every state it's actually wired to
openRuntimePicker early-returns unless viewModel.kind === 'selecting-runtime'. However, it's only ever passed as onPressRuntime when viewModel.kind !== 'selecting-runtime': the ready branch (line 254 here) and the catalog-loading / catalog-error-retryable / catalog-error-non-retryable branches in local-session-config-states.tsx. The selecting-runtime state itself renders its own inline runtime list via onSelectFence and never calls onPressRuntime. As a result the guard condition can never be true when it actually fires, so tapping the always-enabled "Runtime" row silently no-ops in every reachable case — users can never reselect/change the runtime from this screen.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| runtimeId: string, | ||
| connectionId: string | ||
| ): boolean { | ||
| if (!bridge.selectionScope || !bridge.isSelectionCurrent(bridge.selectionScope)) { |
There was a problem hiding this comment.
CRITICAL: Initial runtime selection is impossible when selectionScope is null (2+ capable runtimes)
commitRuntimePickerSelection returns false (without ever calling bridge.onSelect) whenever bridge.selectionScope is falsy, without consulting isSelectionCurrent at all — even though isSelectionCurrent's type explicitly accepts RuntimePickerSelectionScope | null. The only production caller (local-session-config-screen.tsx's openRuntimePicker) sets selectionScope: viewModel.currentFence, and per local-runtime-catalog-view-model.ts, currentFence is null precisely in the primary "selecting-runtime" state (i.e. whenever the user has 2+ capable local runtimes and hasn't picked one yet — the single-runtime case auto-selects and skips this screen). In that state every tap in RuntimePickerContent calls this with a null selectionScope, so it always returns false and never fires onSelect. Users with multiple local runtimes can never make their initial runtime selection via this picker.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| sessionId: string, | ||
| deps: CompleteHappyPathDeps | ||
| ): Promise<void> { | ||
| await deps.invalidateCaches(); |
There was a problem hiding this comment.
CRITICAL: Unhandled invalidateCaches() rejection permanently strands the orchestrator in submitting
completeHappyPath awaits deps.invalidateCaches() with no try/catch, so a rejection (e.g. a transient network blip) throws out of the function before captureEvent/notificationHaptic/navigate run. This mirrors handlePromptPartial below, which deliberately wraps its invalidateCaches() call in try/catch specifically so this failure mode can't block the toast/navigate — but completeHappyPath has no equivalent protection. Downstream, use-local-session-create-orchestrator.ts's completeHappySafe only has a finally (no catch), so the rejection propagates all the way to use-local-session-create.ts's submit/retry/checkAgain, which await it without a try/catch. Net effect: a transient cache-invalidation failure on the happy path leaves "Start session" permanently disabled (isSubmitting: true) with no error shown and no recovery CTA — the user is stuck with no way to retry.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }); | ||
| } | ||
| }); | ||
| const offReconnect = connection.onReconnect(() => { |
There was a problem hiding this comment.
WARNING: Reconnect refetch closes over a stale query object after the first fence change
This effect deliberately excludes query/fence from its dependency array ([connection, queryClient]), so it effectively runs once on mount and its onReconnect handler permanently closes over the query object bound to whichever fence was active at that time (often the disabled placeholder query, since fence is frequently null on first mount). When the user later selects a runtime fence, query is re-created for the new fence via useQuery(queryOptions), but this effect never re-subscribes — so connection.onReconnect(() => void query.refetch()) keeps refetching the original (now-detached) query rather than the currently active fence's catalog query. "Refetch the catalog on kilo-remote reconnect" silently stops working correctly after the first fence is chosen.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| stored.fence.connectionId !== fence.connectionId; | ||
| if (fenceChanged) { | ||
| requestIdStoreRef.current.clearByFence(stored.fence); | ||
| orchestratorRef.current = null; |
There was a problem hiding this comment.
WARNING: Discarding the orchestrator reference doesn't cancel its in-flight work
When the fence changes while a submit/readiness-poll is in flight, this only clears the hook's own reference to the orchestrator (orchestratorRef.current = null, setActiveOrchestrator(null)); it never signals the orchestrator instance itself to stop. There is no AbortController/cancellation token anywhere in use-local-session-create-orchestrator.ts, so the abandoned orchestrator keeps running to completion against the stale deps closure (old fence/catalog/model, old router/queryClient bindings). If that stale attempt later resolves as ready or session_not_ready, completeHappySafe/handlePromptPartialSafe still fire — including navigation (router.replace), success haptics, analytics, and cache invalidation — for a runtime/agent/model selection the user has since navigated away from.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| connectionId, | ||
| issues: error instanceof Error ? error.message : 'unknown', | ||
| }); | ||
| return; |
There was a problem hiding this comment.
WARNING: Runtime-presence rejection drops the entire heartbeat, not just the runtime update
Each of the four validation failures in this block (return; at this line and at the connectionId-mismatch, runtimeId-mutation, and cross-socket-collision checks below) exits handleHeartbeat entirely, skipping the session-ownership update logic and the heartbeat_ack send that follow. lastHeartbeatAt was already bumped above, so the CLI won't be evicted as stale, but its session list goes stale and any sessions started/stopped in that same heartbeat are silently dropped until a "clean" heartbeat arrives — even though the sessions payload in the same message may have been perfectly valid. Only the runtime-registry update (this.runtimes / the runtime event) should be skipped on these rejections, not the unrelated session-handling and ack logic below.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ); | ||
| } | ||
|
|
||
| if (this.pendingCommands.size >= UserConnectionDO.MAX_PENDING_COMMANDS) { |
There was a problem hiding this comment.
WARNING: No duplicate-in-flight guard for relay-originated runtime commands, unlike the existing viewer-command path
dispatchRuntimeCommand (shared by getRuntimeCatalog and createAndRunLocalSession) only enforces the global MAX_PENDING_COMMANDS cap — it has no check for an already-pending command against the same fence. This is inconsistent with handleWebCommand above, which explicitly rejects a duplicate in-flight list_models request for the same ws+sessionId+connectionId with CATALOG_REQUEST_PENDING_ERROR. The contract even defines a COMMAND_ALREADY_PENDING error code — fully wired through routes/runtime-control.ts, local-runtime-control-router.ts, and the mobile error-classification layers — specifically for this purpose, but it's never thrown anywhere in this file. If createAndRunLocalSession's HTTP request is retried (e.g. after a client-side timeout while the first create_and_run is still in flight), the DO will dispatch a second create_and_run to the same runtime, which can create duplicate sessions/prompt-starts unless the CLI itself deduplicates by request id.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Executive SummaryThis incremental update ( Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (10 files in this incremental update)
Fix these issues in Kilo Cloud Previous Review Summary (commit 8ce53dc)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 8ce53dc)Status: 7 Issues Found | Recommendation: Address before merge Executive SummaryThe local-runtime picker/config screen has two CRITICAL dead-end interaction bugs (users with 2+ runtimes can't make an initial selection, and no one can change runtime from the config screen once one is picked), plus a happy-path cache-invalidation failure that can permanently strand the "Start session" button. Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (104 files)
Reviewed by claude-sonnet-5 · Input: 24 · Output: 6.8K · Cached: 585.5K Review guidance: REVIEW.md from base branch |
1 participant
Summary
kilo remoteruntimes even when they have no sessionsRelated
Verification
main, byte-identical to the reviewed implementation treeE2E
Remote connection enabledagainst local API/session-ingest/event-servicexctrunner found nothing to terminate); shared services, simulator, login, Metro provenance, and CLI runtime were otherwise healthyState coverage