refactor(cli): shared-process remote sessions with safe per-session exit - #12327
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge This incremental review covers only the diff since the last review pass (3 files, 260 lines changed, commit
No new bugs, style violations, or fork-hygiene concerns found in the changed lines. Files Reviewed (3 files)
Previous Review Summaries (6 snapshots, latest commit 7433dfe)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7433dfe)Status: No Issues Found | Recommendation: Merge This incremental review covers only the diff since the last review pass (1 file, 9 lines changed). The change moves Files Reviewed (1 file)
Previous review (commit f1c6279)Status: 5 Issues Found | Recommendation: Address before merge This PR was rewritten since the last review (force-pushed): the process-per-session spawner design was replaced with in-process concurrent sessions and detach-only Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (16 files)
Fix these issues in Kilo Cloud Previous review (commit 145d472)Status: No Issues Found | Recommendation: Merge All 4 previously reported findings were fixed in the latest commits:
No new issues found in the incremental diff. Files Reviewed (incremental — this pass)
Previous review (commit 1193023)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (incremental — this pass)
Incremental review: no new commits were pushed since the previous review pass. All 4 previously reported findings remain unresolved and are carried forward unchanged; no new issues were found and no duplicate comments were posted. Fix these issues in Kilo Cloud Previous review (commit 4f26c88)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (incremental — this pass)
Incremental review: only Fix these issues in Kilo Cloud Previous review (commit d29521b)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (14 files)
No command/shell injection issues found — the spawn engine builds argv arrays (no shell string interpolation) and only forwards schema-validated Reviewed by claude-sonnet-5 · Input: 24 · Output: 8.6K · Cached: 668.1K Review guidance: REVIEW.md from base branch |
@iscekic why do we need to spawn a new CLI. Kilo already supports running multiple sessions at the same time and you can pass a project dir for it to work on. That's how VSCode and Agent Manager works from what I know. |
|
@eshurakov I wanted process isolation, but I think you may have a good point. I'll double-check and refactor. |
…on exit Consolidate remote session handling into a single CLI process instead of spawning one process per remote-created session (addresses the PR review): - restore in-process create_session (accepts an absent sessionId and targets the connection directory); remove the session spawner, the KILO_REMOTE_ATTACH_SESSION attach-on-boot path, the child-advertisement gate, and their tests - retain instance advertisement and fire one immediate out-of-band heartbeat on (re)connect when advertising, so a headless `kilo remote` host is discoverable without delay Make /exit (wire command exit_cli, unchanged for compatibility) detach only the target session instead of terminating the CLI: - AttachedState.detach with a presence-suppression tombstone; detach also clears the target's SessionStatus so the negative-containment heartbeat fence resolves deterministically for busy/retry/offline sessions - exit_cli handler verifies ownership, cancels the active prompt, detaches and awaits the detach heartbeat, then ACKs; the interactive RemoteExit callback is invoked only after the ACK when the last owned session exits; a headless `kilo remote` host stays alive and advertising at zero sessions - add an optional canExitSession boolean to the list_commands v1 catalog (always true, independent of exitAvailable) so clients can detect safe session-exit semantics History and stored sessions are preserved on exit.
145d472 to
3df3157
Compare
iscekic
changed the title
Integrate #12394 (mobile file attachments). Resolved additive heartbeat-wire conflicts in remote-protocol.ts / remote-ws.ts by keeping both the K1 instance-advertisement field and the #12394 capabilities.attachments field; combined the round-trip tests. Typecheck + focused kilo-sessions suites green.
|
(bot) Addressed — I refactored away the process-per-session spawner. As part of the same change, The PR title/description and diff have been updated accordingly; the spawner and its tests are removed. |
The K1 in-process exit_cli seam added a static `import { SessionPrompt }`
to kilo-sessions.ts. @/session/prompt evaluates KiloSessionPrompt at module
load, so the new static edge raced that init and left the namespace in TDZ,
crashing unrelated test files with 'undefined is not an object (evaluating
KiloSessionPrompt.shouldAskPlanFollowup)'. Defer to a dynamic import at the
single call site, mirroring remote-command.ts.
Address review findings on the shared-process session lifecycle: - announce/detach no longer join the OPPOSITE in-flight operation. Joining detach's negative-containment fence made announce resolve success for a detached id (and vice versa: detach joined announce and resolved success while still attached, which exit_cli treats as license to ACK/close). Each path now joins only a same-kind in-flight op and, when the opposite op is in flight, awaits it to settle and then performs the real work. - Failed-detach rollback now releases the suppression tombstone, so a still-attached session is not dropped by the next setPresence (the tombstone loop would otherwise remove the still-present id and never clear). - Both catch/rollback branches now honor the lifecycle generation guard (mirroring the success path); a stale in-flight op that rejects after reset() no longer mutates the new lifecycle's presence/pending/suppressed sets (reset clears the same Set instances). Adds regression tests for each fix, plus AC6f covering the remote-ws detachSessionId negative-containment waiter.
…xit (Kilo-Org#12327) * refactor(cli): run remote sessions in one process with safe per-session exit Consolidate remote session handling into a single CLI process instead of spawning one process per remote-created session (addresses the PR review): - restore in-process create_session (accepts an absent sessionId and targets the connection directory); remove the session spawner, the KILO_REMOTE_ATTACH_SESSION attach-on-boot path, the child-advertisement gate, and their tests - retain instance advertisement and fire one immediate out-of-band heartbeat on (re)connect when advertising, so a headless `kilo remote` host is discoverable without delay Make /exit (wire command exit_cli, unchanged for compatibility) detach only the target session instead of terminating the CLI: - AttachedState.detach with a presence-suppression tombstone; detach also clears the target's SessionStatus so the negative-containment heartbeat fence resolves deterministically for busy/retry/offline sessions - exit_cli handler verifies ownership, cancels the active prompt, detaches and awaits the detach heartbeat, then ACKs; the interactive RemoteExit callback is invoked only after the ACK when the last owned session exits; a headless `kilo remote` host stays alive and advertising at zero sessions - add an optional canExitSession boolean to the list_commands v1 catalog (always true, independent of exitAvailable) so clients can detect safe session-exit semantics History and stored sessions are preserved on exit. * fix(cli): break module-load cycle in remote session prompt-cancel The K1 in-process exit_cli seam added a static `import { SessionPrompt }` to kilo-sessions.ts. @/session/prompt evaluates KiloSessionPrompt at module load, so the new static edge raced that init and left the namespace in TDZ, crashing unrelated test files with 'undefined is not an object (evaluating KiloSessionPrompt.shouldAskPlanFollowup)'. Defer to a dynamic import at the single call site, mirroring remote-command.ts. * fix(cli): correct AttachedState announce/detach concurrency and rollback Address review findings on the shared-process session lifecycle: - announce/detach no longer join the OPPOSITE in-flight operation. Joining detach's negative-containment fence made announce resolve success for a detached id (and vice versa: detach joined announce and resolved success while still attached, which exit_cli treats as license to ACK/close). Each path now joins only a same-kind in-flight op and, when the opposite op is in flight, awaits it to settle and then performs the real work. - Failed-detach rollback now releases the suppression tombstone, so a still-attached session is not dropped by the next setPresence (the tombstone loop would otherwise remove the still-present id and never clear). - Both catch/rollback branches now honor the lifecycle generation guard (mirroring the success path); a stale in-flight op that rejects after reset() no longer mutates the new lifecycle's presence/pending/suppressed sets (reset clears the same Set instances). Adds regression tests for each fix, plus AC6f covering the remote-ws detachSessionId negative-containment waiter.
3 participants
Summary
Refactors remote CLI sessions to run concurrently inside one CLI process instead of spawning a dedicated process per remote-created session, and makes
/exita safe per-session detach rather than a whole-CLI kill.This addresses @eshurakov's review: Kilo already supports multiple sessions in one process with per-call directory context, so process-per-session isolation was unnecessary.
What changed
create_session(with an absent session id) creates and attaches the session in the existing process against the connection's target directory. Removed the per-session spawner (session-spawner.ts), theKILO_REMOTE_ATTACH_SESSIONattach-on-boot path + retry, and the child-advertisement gate. Retained the instance advertisement and an immediate advertisement heartbeat on (re)connect./exitdetaches only the target session. Theexit_cliwire command (name unchanged for deployed-client compatibility) now: verifies ownership, cancels the active prompt, clears the session's status, detaches it (awaiting the heartbeat fence so the relay/clients see it leave), then ACKs. History is preserved. On failure it rolls back and the CLI stays alive.RemoteExitcallback), the CLI closes — but only after the ACK flushes. A headlesskilo remotehost never registers that callback, so it stays alive and advertising at zero sessions and can create new sessions again.list_commandsv1 catalog gains an optionalcanExitSession: true, emitted independently ofexitAvailable, so clients can detect the safe session-exit semantics. A companion cloud/mobile PR consumes it and fails closed against CLIs that don't advertise it.Testing
canExitSessionon interactive and headless paths), remote-sender (create_sessionin-process,exit_cliowns-check / detach / ACK-ordering / last-session-callback / headless-stays-alive / rollback), remote-ws, and an integration test proving the detach heartbeat fence resolves for busy/retry/offline sessions (fails without the status-clear fix).tsgotypecheck, the opencode annotation check, and the promise-facade ratchet all pass on the exact head.Companion PR (cloud SDK + mobile): Kilo-Org/cloud#4686