fix(session-ingest): resolve a forwarded exit_cli when ownership ends - #4827
Conversation
A successful remote /exit was reported to the app as "Session owner
changed": the CLI ACKs exit_cli only after the detach heartbeat that
removes the session, so the relay always failed the pending command
first. For a forwarded exit_cli, ownership ending is the requested
outcome, so resolve it with result: {} (the CLI's own ACK shape) when
the owner-change path sees no next owner or when the owning socket is
really gone. Genuine takeovers, reconnect replacements, and every other
command keep failing with SESSION_OWNER_CHANGED.
A CLI too old to advertise itself is connected but never offered as a Run on target, and the empty state only said nothing was connected. Name both recoveries: run kilo remote, or update Kilo CLI if one is already running.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe relay fix for forwarded Files Reviewed (3 files)
Reviewed by claude-sonnet-5 · Input: 30 · Output: 5.7K · Cached: 812.1K Review guidance: REVIEW.md from base branch |
Verifier-recorded facts from the plan 1.7 run: the native exit confirmation dialog, the collapsed RemoteSpawnComposer form, exact Run-on label formats, and the composer keyboard behavior.
|
(bot) @kilocode-bot please review the current head 62d5566 (previous review covered 08dc425; this push adds only origin/main merges and one .kilo_workflow learnings doc). |
Re-review of
|
| Commit | Type |
|---|---|
2853087f4, 923c3b273, b823b2186, 62d556620 |
merges of origin/main |
3705df604 |
docs(workflow): learn driving remote /exit on iOS via Maestro — single new file .kilo_workflow/learnings/maestro-remote-exit-flow.md (+31/-0) |
The only non-merge commit unique to the branch and not in origin/main, besides the two already-reviewed ones (064f65680, 08dc4250b), is 3705df604. Claim confirmed.
2. No functional code changes
- Effective PR diff comparison:
git diff $(merge-base) 08dc4250b -- apps servicesvsgit diff $(merge-base) 62d556620 -- apps services→ byte-identical. The reviewed change set is exactly:services/session-ingest/src/dos/UserConnectionDO.ts(+45/-17)services/session-ingest/src/dos/UserConnectionDO.test.ts(+165)apps/web/src/app/(app)/agent-chat/instance-picker.tsx(+2/-1)
git diff 08dc4250b 62d556620 -- services/session-ingest→ empty (tree identical; no drift in the fix or its tests).- The
apps/mobile/**deltas that appear in the raw two-dot diff (e2e/AGENTS.md,e2e/flows/login-request-code.yaml,e2e/login.sh,e2e/preflight.sh,src/components/login-screen.tsx) are attributable entirely to upstream commits already merged tomain—a9f3eb807(fix(mobile): keep the sign-in submit button clear of the iOS keyboard #4820) and96154f115(fix(workflow): unwedge shared E2E infra (Android claims, preflight, tmux targeting, stack cap) #4826) — not to this branch. Same for theapps/web/gastown,ai-gateway,code-reviews,.specs, andpnpm-lock.yamlchurn (from feat(gastown): add usage-based container billing #4673, feat(ai-gateway): log Vercel upstream provider id as upstream_id #4828, feat(code reviewer) Assist users to convert from Custom Instructions to REVIEW.md #4838, fix(gastown): gate billing UI on announcement flag, not shadow metering #4833, fix(ai-gateway): extend generation polling timeout #4818, chore(workflow): require human-ready label at the completion gate #4841, fix(workflow): stop e2e-slot mangling the stack session name #4836). - All four merge commits have an empty combined diff (
git show --cc), i.e. clean merges with no conflict resolutions or evil-merge edits sneaking changes into PR-owned files.
3. Review of the one new file
.kilo_workflow/learnings/maestro-remote-exit-flow.md is documentation only: E2E findings about hideKeyboard in the chat composer, the Exit session? confirmation dialog, the trailing space from the slash-suggestion commit, RemoteSpawnComposer collapsing the new-session form, exact instance-row label formats, and post-exit assertions. It cross-references the existing maestro-tap-swallowed-by-ios-keyboard.md learning, matches the .kilo_workflow/learnings/ convention, and contains no credentials, tokens, or executable content. Nothing to flag.
Verdict
Safe / no-op from a review perspective. The previously reviewed code at 08dc4250b is unchanged at 62d556620; the push only rebases forward onto main via merges and adds a docs-only learnings file. No further code review needed — prior review verdict carries over. CI should still be green on the merged result before merge.
|
The re-review is already posted: #4827 (comment) Verdict: confirmed no-op — only |
Summary
Two remote-CLI defects, one small fix each.
Defect A — a successful
/exitreported as "Session owner changed". Root cause: the CLI ACKsexit_clionly after the detach heartbeat that removes the session, so the relay always failedthe pending command first and the user saw "Session owner changed" for an exit that had succeeded.
The relay now treats ownership ending as the requested outcome for a forwarded
exit_cli:failPendingCommandsForOwnerChangeresolves it withresult: {}(byte-identical to the CLI's ownACK, and the only shape the SDK's strict
exitSessionResponseSchemaaccepts) when no next ownerappears, and
failPendingCommandsForSocketdoes the same when the owning CLI socket is reallygone (
cliGone = !replaced). Genuine takeovers, reconnect socket replacements, and every othercommand keep failing with
SESSION_OWNER_CHANGEDexactly as before. No mobile or SDK change: withthe relay fixed, the client's existing error classification is already correct.
Defect B — a remote CLI missing from the "Run on" targets — splits in two:
kiloTUI with/remote, or config auto-enable) is already fixed upstream.In the released CLI (
@kilocode/cli7.4.16) only thekilo remotecommand path advertises theinstance; kilocode
maincommita19d44c3ef(PR Advertise the instance from enableRemote and report attention status on the heartbeat kilocode#12532) moved advertisementinto
KiloSessions.enableRemote(), so every enable path advertises. That commit is not anancestor of the v7.4.16 release tag — the fix awaits a CLI release, and no code change is
warranted in either repository.
kilo remoteon ≤ 7.4.16) is not reproducible on the current baseline. On7.4.16 the
kilo remotehandler already advertises beforeenableRemote(), and it was observedworking: a headless
kilo remoteappeared in the picker, while a relay-enabled TUI on the sameversion did not (observation run 2026-07-28, iOS simulator, personal context, account
e2e-mobile-remote-cli-69f6@example.com). The most likely reading of the report is that theuser's "kilo remote" was the TUI's
/remote— i.e. Half 1. Remaining open candidates: orgcontext (the Run-on selector is intentionally hidden for any organization), and a degraded
heartbeat that omits
instance, which wipes a previously advertised row until the next fullheartbeat. Neither is settled here and neither is chased.
Why Defect B ships only one string. The upstream fix exists and re-implementing it would
collide with
a19d44c3ef; relaxinggetConnectedInstances()to synthesize rows would inventhostname/project/version only the CLI knows. What no upstream commit solves is the empty state:
today a user whose CLI is connected but cannot advertise is told "No CLI instances connected" —
full stop. The description now names both recoveries: run
kilo remote, or update Kilo CLI ifone is already running. No new snapshot test for one display literal — the picker's four states
are already covered as pure logic by
resolveInstancePickerViewState, and a snapshot here would becoverage theatre.
Verification
exit_cli"failed with
SESSION_OWNER_CHANGEDbefore the fix, then passed after (the fix, not thevalidation, changed — the relay's
exit_clivalidation is untouched).services/session-ingest:pnpm test(612 passed),pnpm typecheck,pnpm lint— green;pre-existing owner-change tests unmodified.
apps/mobile:pnpm format && pnpm typecheck && pnpm lint && pnpm check:unused && pnpm test(2337 passed) — green.
kilo remoteappeared in the Run-on picker (
Igor-MacBook.local / remote-cli-69f6); with the TUI'srelay additionally enabled the picker still showed only that one instance — Half 1
reproduced, Half 2 refuted in one screen. (Two screenshots were captured; GitHub's API
cannot attach binaries to PRs, so their content is transcribed here — nothing else in them
is load-bearing.)
/exitE2E on the simulator (plan 1.7, iOS simulator, accounte2e-mobile-remote-cli-69f6@example.com, 2026-07-28): opened a live remote CLI session,typed
/exit, confirmed the native "Exit session?" dialog — "Session exited" successtoast shown, back on the Agents list with the session row still present, no "Session owner
changed" toast (
assertNotVisible), no "Try again" retry CTA (assertNotVisible), andboth CLI sockets still connected afterwards (ordinary trigger: the CLI stays alive). The
last-session socket-close trigger is covered by the DO unit test (producing it on device
requires killing the CLIs, which the run rules forbid); the fail-closed negative path is
covered by
chat-composer-exit-slash-command.test.tsandexit-remote-session-with-feedback.test.ts. Two screenshots captured: the "Session exited"toast over the Agents tab, and the Agents list with the exited session's row — content
transcribed here for the same API reason as above.
the plan's own fallback. The single attempt crashed inside its build wrapper (a scripting
bug piping
pnpm's human-readable "Scope: all 61 workspace projects" banner intoJSON.parse;SWAP_EXIT=1, no product signal), and by the time a retry was possible thesection's dev stack and authenticated CLI harness had already been torn down, so a retry
meant rebuilding the whole phase for evidence that adds nothing over what already stands:
Half 1 rests on kilocode
a19d44c3ef(merged upstream onmainwith its own coverage) andthe existing
getConnectedInstancestests, and Run 1 above already refutes Half 2 on thereleased 7.4.16.
E2E note: simulator sign-in relied on the documented throwaway Maestro workaround (type the
address, then
pressKey: Enter— the Send-code button sits under the keyboard window).apps/mobile/e2e/**was not edited; the real harness fix is #4820.Visual Changes
Instance-picker empty state (zero connected CLIs), description only:
Reviewer Notes
UserConnectionDO.tsfailPendingCommandsForOwnerChange/failPendingCommandsForSocket— the only behavioral change is the payload branch for forwardedexit_cli; call sites did not move, send/delete ordering is unchanged, and the'CLI disconnected'string path is preserved.SESSION_OWNER_CHANGEDinhandleWebCommandis deliberately untouched: nothingwas sent there, and reporting success would claim an exit worked when it was never forwarded.
exit_clion purpose: the CLIprocess is alive and the exit's outcome is genuinely unknown.