Fix mobile attention, model-selection and PR-review defects - #4769
Merged
Conversation
Choosing a model inside a cloud-agent session was silently discarded. The write path stored React state plus a per-organization and a per-user preference, while the read path resolved from the session's stored agent.model, the streamed message model, or the first catalog entry — no overlap, and nothing wrote a per-session model. Because currentModel is in the config-sync effect's deps, a session with a stored agent.model had the pick reverted within a frame, before any send could use it. Add a cloud-agent model override held in the session manager, deliberately separate from the remote override: that one carries remote-only clear rules (owner connection, catalog protocol) which would wipe a cloud-agent selection. The override wins in resolveSessionConfigSelection, survives the sync effect, and is what send uses. The existing organization and user preferences are unchanged — they remain the default for the next new session. CloudAgentSession now persists agent.model and variant after a message is successfully admitted, so a cold relaunch reads back the model that actually ran. Persisting at resolve time would have recorded a model for a run that never started. The message queue stays read-only; persistence wraps admitSubmittedMessage instead. Selecting a model without sending still falls back to the stored model after leaving the screen. That is intended: the chip reflects what actually ran, which matches existing remote behaviour.
A live CLI session blocked on a structured question rendered in Active Now as a healthy green row with no NEEDS INPUT badge — the one state the badge exists for was the one state it could not reach. Live rows take their status from the CLI heartbeat, which only ever reports idle or busy. The correct question/permission status is already in cli_sessions_v2 and already fetched into the same hook, but it was discarded for live ids. Overlay the stored attention status onto live rows. The resolution runs in the tRPC router rather than only in client helpers, because the live cache refreshes with fetchQuery, which replaces the cached result wholesale — sticky state held only on the client is wiped on every enrichment, reconnect and cli.connected refresh. Serving the resolved value covers already-released CLIs that will never report attention on the heartbeat. On the client, attention is sticky against heartbeats and connect snapshots: a non-attention status cannot overwrite a held attention status, so the badge does not flicker. It clears on session.status.updated, whose payload is dual-shaped and is handled in both variants, subscribed through onSystemEvent only so it cannot be applied twice.
…ssions GitHub rejects both APPROVE and REQUEST_CHANGES on your own pull request with a 422, so the error itself was right, but the message named "approve" whichever event was submitted — telling someone who requested changes that they can't approve their own PR. The helper is event-agnostic, so rather than thread the submitted event through it for one string, the wording is now event-neutral: "you can't review your own pull request", which is accurate for both. The surrounding behaviour is deliberately unchanged: the message still renders inline and the pending comment queue is still retained, so the user can switch to a COMMENT review and submit without losing drafted comments.
…opening The attention ack fired unconditionally when the session screen mounted, so opening a blocked session, reading the question and backing out without answering cleared the NEEDS INPUT badge while the CLI was still waiting. The ack store is in-memory only, so the badge came back after an app restart — the app gave two different answers to whether the same session needed input. Ack when the user actually responds instead: after a successful answer, skip, or permission response. All three handlers route through the session manager, which serves remote and cloud-agent sessions alike, so the three call sites cover both. Each ack sits after the await inside the existing try, so a failed submit leaves the badge up. The ack is keyed by the kilo session id, passed in from the route. The manager's sessionId holds the cloud-agent id, which is nullable and is not the attention key — acking under it would silently never clear the badge. Removing the on-mount ack also fixes a quieter bug: opening any session wrote a pending ack that could swallow the next genuine raise on it.
…nects cli_sessions_v2.status was written only from an incoming session_status item and never reset on disconnect, so a CLI that died with a question pending left the row pinned at question forever — a permanent NEEDS INPUT badge on a session that opens to an empty transcript. A row found locally had been stuck for ten hours. On disconnect, sessions owned by the departing connection that are in an attention status are moved to retry. Not idle: idle is the only status treated as completed by the ingest attention path, so a synthetic idle would risk a false "Task completed" push on every CLI disconnect. retry already exists in the enum and in the UI, is what the CLI itself maps local-offline to, and is not an attention status, so the badge clears. The reset is awaited before cli.disconnected is broadcast. That ordering is load-bearing: the mobile client reacts to the broadcast by refetching stored history, and it has no handler for session.status.updated on that cache, so a fire-and-forget write would race the refetch and leave the badge stuck until a manual pull-to-refresh. Scope is deliberately narrow. Only the disconnecting connection's owned sessions are touched, only rows already in question or permission, and the existing stale-socket guard still short-circuits so a reconnecting CLI resets nothing. Identity comes from the authenticated attachment's kiloUserId; without it the reset is skipped and logged rather than guessed. A failed reset is logged and still lets the disconnect broadcast proceed. Rows already stuck from earlier disconnects are not backfilled; that is a separate one-off cleanup.
The type is only referenced as the return type of planLiveSystemEventActions in the same module, so exporting it tripped knip's unused-export check and would have failed check:unused in CI.
…ct replaced guard Device E2E showed the DEF-5 attention reset never ran on a real CLI death: the row stayed at question and the badge stayed up. The reset code, its ordering and its identity handling were all correct — it was never reached. Under wrangler/workerd, ctx.getWebSockets() still includes the closing WebSocket during webSocketClose. The stale-reconnect guard matched only on connectionId, so it found the closing socket itself, concluded a replacement was already active, and returned early. That early return is not specific to this feature. It also skips ownership cleanup (sessionOwners, connectionSessions, protocol version, capabilities, heartbeat tracking) and the cli.disconnected broadcast — so on any real disconnect none of that ran either. The "Stale CLI socket closed (already replaced)" line was appearing on ordinary disconnects, which in hindsight was the symptom. This fix therefore repairs more than the attention reset. Excluding the closing socket from the scan keeps the guard doing its actual job: a genuine reconnect has a second, different socket carrying the same connectionId, and that case still short-circuits. The unit tests missed this because they removed the socket from the mock before invoking close, modelling a cleaner teardown than the runtime provides. The new test leaves the closing socket listed, as workerd does, and fails against the old predicate.
Contributor
Author
|
(bot) Device E2E complete on
Two notes worth recording. DEF-5 initially failed E2E and the root cause was broader than this feature. The reset code, its ordering and its identity handling were all correct — it was never reached. Under workerd, The overlay is worth more than "covering old CLIs". Even with a patched CLI, the DB flips to |
iscekic
enabled auto-merge (squash)
July 25, 2026 13:29
jeanduplessis
approved these changes
Jul 25, 2026
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
2 participants
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.
Fixes four defects found by an end-to-end verification run on iOS against a local stack, each confirmed by an independent code triage before any code was written.
What was broken
A live CLI session waiting on you showed no NEEDS INPUT badge. Live rows take their status from the CLI heartbeat, which only ever reports
idle/busy. The correctquestion/permissionstatus was already incli_sessions_v2, already fetched into the same hook, and discarded for live ids — so the badge could never reach the one state it exists for.Choosing a model in a cloud-agent session was silently ignored. The write path stored React state plus a per-org and a per-user preference; the read path resolved from the session's stored
agent.model, the streamed message model, or the first catalog entry. No overlap, and nothing wrote a per-session model. Sessions with a storedagent.modelhad the pick reverted within a frame, before any send could use it.The attention badge cleared on viewing rather than answering. Opening a blocked session and backing out without answering cleared the badge while the CLI was still waiting — and because the ack store is in-memory only, it reappeared after an app restart. The app gave two different answers to the same question depending on whether the process had restarted.
A CLI that died with a question pending left the row pinned at
questionforever, producing a permanent NEEDS INPUT badge on a session that opens to an empty transcript. A row found locally had been stuck for ten hours.Plus a copy fix: submitting Request changes on your own PR reported that you can't approve your own PR.
Notable implementation points
The live-status overlay resolves in the tRPC router, not only in client helpers. The live cache refreshes with
fetchQuery, which replaces the cached result wholesale, so sticky state held only on the client is wiped on every enrichment, reconnect andcli.connectedrefresh. Serving the resolved value also means this works for already-released CLIs that will never report attention on the heartbeat — it does not depend on the companion CLI change in Kilo-Org/kilocode.The disconnect reset targets
retry, notidle.idleis the only status the ingest attention path treats as completed, so a syntheticidlewould risk a false "Task completed" push on every disconnect. The reset is awaited beforecli.disconnectedis broadcast, because the client reacts to that broadcast by refetching stored history and has no handler forsession.status.updatedon that cache — a fire-and-forget write would race the refetch.Cloud-agent model persistence happens after successful admission, not at resolve time; persisting at resolve would record a model for a run that never started. Selecting a model without sending still falls back after leaving the screen — intended, since the chip reflects what actually ran, matching existing remote behaviour.
Deliberately not changed
The
CLOUDeyebrow (it is the repo name, not a platform badge), the focus-gated in-session model selector (it renders when the composer is focused), plain-text questions not raising needs-input (only structuredquestion/permissionevents are attention states), and PR-review comment path assignment (paths are captured at line selection; a unit test already asserts it). All four were reported as bugs during verification and disproved by triage.Rows already stuck at
questionfrom earlier disconnects are not backfilled — that is a separate one-off cleanup.Checks
apps/mobiletypecheck, lint, format, knip, 1871 tests ·session-ingesttypecheck, 594 tests ·cloud-agent-nexttypecheck, 2359 tests ·apps/webtypecheck, 9376 tests. Independent reviewer over the full diff: no findings.Device E2E is running separately and will be reported in a comment.