Skip to content

fix(session-ingest): resolve a forwarded exit_cli when ownership ends - #4827

Merged
iscekic merged 7 commits into
mainfrom
remote-cli-69f6
Jul 29, 2026
Merged

fix(session-ingest): resolve a forwarded exit_cli when ownership ends#4827
iscekic merged 7 commits into
mainfrom
remote-cli-69f6

Conversation

@iscekic

@iscekic iscekic commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Two remote-CLI defects, one small fix each.

Defect A — a successful /exit reported as "Session owner changed". Root cause: the CLI ACKs
exit_cli only after the detach heartbeat that removes the session, so the relay always failed
the 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:
failPendingCommandsForOwnerChange resolves it with result: {} (byte-identical to the CLI's own
ACK, and the only shape the SDK's strict exitSessionResponseSchema accepts) when no next owner
appears, and failPendingCommandsForSocket does the same when the owning CLI socket is really
gone (cliGone = !replaced). Genuine takeovers, reconnect socket replacements, and every other
command keep failing with SESSION_OWNER_CHANGED exactly as before. No mobile or SDK change: with
the 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:

  1. Half 1 (a plain kilo TUI with /remote, or config auto-enable) is already fixed upstream.
    In the released CLI (@kilocode/cli 7.4.16) only the kilo remote command path advertises the
    instance; kilocode main commit a19d44c3ef (PR Advertise the instance from enableRemote and report attention status on the heartbeat kilocode#12532) moved advertisement
    into KiloSessions.enableRemote(), so every enable path advertises. That commit is not an
    ancestor of the v7.4.16 release tag — the fix awaits a CLI release, and no code change is
    warranted in either repository.
  2. Half 2 (headless kilo remote on ≤ 7.4.16) is not reproducible on the current baseline. On
    7.4.16 the kilo remote handler already advertises before enableRemote(), and it was observed
    working: a headless kilo remote appeared in the picker, while a relay-enabled TUI on the same
    version 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 the
    user's "kilo remote" was the TUI's /remote — i.e. Half 1. Remaining open candidates: org
    context (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 full
    heartbeat. 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; relaxing getConnectedInstances() to synthesize rows would invent
hostname/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 if
one 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 be
coverage theatre.

Verification

  • Defect A confirmed test-first: the new relay case "heartbeat drops the session mid-exit_cli"
    failed with SESSION_OWNER_CHANGED before the fix, then passed after (the fix, not the
    validation, changed — the relay's exit_cli validation 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.
  • Defect B Run 1 (released CLI 7.4.16, planner observation 2026-07-28): headless kilo remote
    appeared in the Run-on picker (Igor-MacBook.local / remote-cli-69f6); with the TUI's
    relay 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.)
  • Defect A /exit E2E on the simulator (plan 1.7, iOS simulator, account
    e2e-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" success
    toast shown, back on the Agents list with the session row still present, no "Session owner
    changed" toast (assertNotVisible), no "Try again" retry CTA (assertNotVisible)
    , and
    both 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.ts and
    exit-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.
  • Defect B Run 2 E2E (plan 2.4): not run — recorded as a test-environment limitation per
    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 into
    JSON.parse; SWAP_EXIT=1, no product signal), and by the time a retry was possible the
    section'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 on main with its own coverage) and
    the existing getConnectedInstances tests, and Run 1 above already refutes Half 2 on the
    released 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:

Before After
"Run `kilo remote` in a project on your computer to connect one." "Run `kilo remote` in a project on your computer, or update Kilo CLI if one is already running."

Reviewer Notes

  • Focus: UserConnectionDO.ts failPendingCommandsForOwnerChange /
    failPendingCommandsForSocket — the only behavioral change is the payload branch for forwarded
    exit_cli; call sites did not move, send/delete ordering is unchanged, and the
    'CLI disconnected' string path is preserved.
  • The pre-forward SESSION_OWNER_CHANGED in handleWebCommand is deliberately untouched: nothing
    was sent there, and reporting success would claim an exit worked when it was never forwarded.
  • A reconnect socket replacement keeps the retryable error for exit_cli on purpose: the CLI
    process is alive and the exit's outcome is genuinely unknown.

iscekic added 2 commits July 28, 2026 15:37
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.
@iscekic iscekic self-assigned this Jul 28, 2026
@kilo-code-bot

kilo-code-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The relay fix for forwarded exit_cli (resolving with result: {} only when the owning CLI/session ownership is genuinely gone, and still failing with SESSION_OWNER_CHANGED for takeovers, reconnects, and every other command) is correctly scoped and covered by new tests; the one-line mobile empty-state copy change is trivial and low risk.

Files Reviewed (3 files)
  • services/session-ingest/src/dos/UserConnectionDO.ts
  • services/session-ingest/src/dos/UserConnectionDO.test.ts
  • apps/mobile/src/app/(app)/agent-chat/instance-picker.tsx

Reviewed by claude-sonnet-5 · Input: 30 · Output: 5.7K · Cached: 812.1K

Review guidance: REVIEW.md from base branch main

@iscekic

iscekic commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

(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).

@kilo-code-bot

kilo-code-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Re-review of 62d556620 (was 08dc4250b): claim verified — no-op for review purposes

I diffed 08dc4250b..62d556620 and cross-checked the effective PR diff (branch vs. its merge-base with origin/main) at both heads.

1. Commit contents

New commits in range (git log --graph 08dc4250b..62d556620):

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

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.

@kilo-code-bot

kilo-code-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

The re-review is already posted: #4827 (comment)

Verdict: confirmed no-op — only origin/main merges plus one docs-only commit (.kilo_workflow/learnings/maestro-remote-exit-flow.md). No changes to PR-owned files (session-ingest, mobile picker). Prior "Merge" recommendation on 08dc4250b stands for 62d556620. No further action needed.

@iscekic iscekic added the human-ready The PR is ready for human review. label Jul 28, 2026
@iscekic
iscekic requested a review from pandemicsyn July 29, 2026 00:07
@iscekic
iscekic merged commit adc725c into main Jul 29, 2026
22 checks passed
@iscekic
iscekic deleted the remote-cli-69f6 branch July 29, 2026 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human-ready The PR is ready for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants