fix(vscode): recover Agent Manager terminals after exit - #12812
Conversation
When an embedded Agent Manager terminal ends, the next typed input now starts a fresh shell in the same tab and delivers that input exactly once instead of requiring a second keystroke. The original scrollback remains visible above a neutral divider so the ended shell is clearly separated from the replacement. - TerminalManager owns a logical terminal ID with a mutable PTY ID and exposes a deduplicated restart method that returns the fresh wsUrl. - TerminalRouter posts one restarted message back to the webview. - TerminalTab buffers input while the replacement attaches and flushes it only after replacement shell output settles, with a one-second fallback for silent shells. - Restartable terminals show a distinct ended marker that advertises both typing and closing; Run/Setup terminals keep the original close-only text. - Localized the new marker in all Agent Manager locales. - Replaces the previous dispose-on-webview-reload behavior with a reconciliation pass so a fresh webview does not orphan running PTYs. Verified end-to-end in an isolated VS Code instance: two consecutive exit-then-one-input cycles both execute the original input without requiring a second keystroke, and pwd confirms the replacement shell starts in the original worktree cwd.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return | ||
| } | ||
| clearTimeout(resizeTimer) | ||
| if (readyTimer) clearTimeout(readyTimer) |
There was a problem hiding this comment.
WARNING: Resize cancels the pending flush without ever re-arming it
These two lines clear readyTimer and fallbackTimer but nothing re-arms them, and the handles are not reset to undefined. If a host resize lands inside the recovery window (after restarted opened the new socket, before the flush fires), both the 100 ms readyTimer and the 1 s fallbackTimer are gone, so flush() never runs: pending is never sent and disconnected stays true.
From then on every keystroke only appends to pending, and requestRestart() is a no-op because restartRequested is still true (it is only reset in flush() or on terminal.error) — the tab silently swallows all input until the user closes it. scheduleFlush() re-arms only when new PTY output arrives, which a quiet shell will not produce.
Suggestion: either leave the flush timers alone here (a resize does not invalidate buffered input) or call scheduleFlush() again after syncSize() settles. Resetting the handles to undefined after clearTimeout would also keep the if (readyTimer) guards in flush()/onclose meaningful.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| socketEnded = true | ||
| noteFailure() | ||
| if (props.restartable) { | ||
| disconnected = true |
There was a problem hiding this comment.
WARNING: A closed WebSocket is not proof the shell exited — restart can kill a live PTY
/pty/:id/connect is an attach/detach endpoint with replay (packages/opencode/src/server/routes/instance/httpapi/handlers/pty.ts); the PTY outlives the socket. The server only sends close code 1000 when the PTY actually ends — 1001 is server shutdown and 1006 is an abnormal/proxy drop, and after either of those the shell (and anything it is running) can still be alive.
In those cases this branch prints endedRestartable, and the next keystroke makes TerminalManager.restartEntry pty.remove() a live PTY — killing a dev server or a long build the user had running and discarding scrollback that a re-attach could have replayed.
Suggestion: gate the restartable path on event.code === 1000 (the handler receives the CloseEvent), and for other codes re-attach to the same ptyID first — buildWsUrl uses a plain auth_token, so the same URL can be reused, and the server answers 4404 when the session is really gone. client.pty.get({ ptyID }) on the extension side would give the same signal.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| const open = (url: string) => { | ||
| if (closed || !url) return | ||
| const next = new WebSocket(url) |
There was a problem hiding this comment.
SUGGESTION: open() replaces ws without closing the previous socket
Today the restart path only runs after onclose set ws = undefined, so this is latent. But the router posts one agentManager.terminal.restarted per restart message it receives (and TerminalManager.restart dedupes concurrent requests to the same wsUrl), so a second restarted for this terminal would orphan the current socket: its handlers short-circuit on ws !== next, onCleanup only closes the newest ws, and the server-side attachment stays open for the lifetime of the webview.
A ws?.close() before ws = next makes this self-defending at no cost.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| themeObserver.observe(document.body, { attributes: true, attributeFilter: ["class"] }) | ||
|
|
||
| onCleanup(() => { | ||
| closed = true |
There was a problem hiding this comment.
SUGGESTION: Cleanup misses the two new timers
resizeTimer is cleared below, but readyTimer and fallbackTimer are not, so a scheduled flush can outlive the component and keep this whole onMount closure — including the already-disposed Terminal — reachable for up to a second after unmount. It is harmless today only because flush() bails on a non-OPEN socket; adding clearTimeout(readyTimer) / clearTimeout(fallbackTimer) next to clearTimeout(resizeTimer) makes the teardown explicit and leak-free.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try { | ||
| const client = this.deps.getClient() | ||
| const old = entry.ptyID | ||
| const created = await client.pty.create({ |
There was a problem hiding this comment.
WARNING: Two paths here can orphan the freshly created PTY
- If
client.pty.update(...)below throws, thecatchrethrows beforeentry.ptyID = info.id, so the new PTY is never recorded — neitherclose()nordispose()can ever remove it and it lives untilkilo serveexits. Worth also checkingupdate'serrorfield: as the comments onclose()/resize()note, the SDK reports API failures there rather than throwing, so a failed resize is currently silent. - If the user closes the tab while this
createis in flight,close()deletes the entry and removes the oldptyID; this function then mutates a detachedentryand the new PTY is leaked the same way.
Re-checking this.entries.get(entry.terminalId) === entry after the await, and removing info.id when the entry is gone or when a later step fails, would close both holes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| void this.manager | ||
| .restart(m.terminalId, m.cols, m.rows) | ||
| .then((wsUrl) => { | ||
| if (!wsUrl) return |
There was a problem hiding this comment.
SUGGESTION: Silent undefined leaves the webview latched
restart() returns undefined when the entry is gone, and this branch then posts nothing at all. The webview has already set restartRequested = true, and that flag is only cleared in flush() or on agentManager.terminal.error — so with no reply the tab keeps buffering every keystroke into pending forever with no toast and no visible feedback.
Posting an agentManager.terminal.error (or a dedicated "cannot restart" reply) on the !wsUrl path would let the webview reset and tell the user what happened.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge
Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (30 files)
Assumptions: PTY lifetime behaviour was verified against Fix these issues in Kilo Cloud Reviewed by claude-opus-5 · Input: 70 · Output: 24.6K · Cached: 3M Review guidance: REVIEW.md from base branch |
* fix(vscode): recover Agent Manager terminal with one input after exit When an embedded Agent Manager terminal ends, the next typed input now starts a fresh shell in the same tab and delivers that input exactly once instead of requiring a second keystroke. The original scrollback remains visible above a neutral divider so the ended shell is clearly separated from the replacement. - TerminalManager owns a logical terminal ID with a mutable PTY ID and exposes a deduplicated restart method that returns the fresh wsUrl. - TerminalRouter posts one restarted message back to the webview. - TerminalTab buffers input while the replacement attaches and flushes it only after replacement shell output settles, with a one-second fallback for silent shells. - Restartable terminals show a distinct ended marker that advertises both typing and closing; Run/Setup terminals keep the original close-only text. - Localized the new marker in all Agent Manager locales. - Replaces the previous dispose-on-webview-reload behavior with a reconciliation pass so a fresh webview does not orphan running PTYs. Verified end-to-end in an isolated VS Code instance: two consecutive exit-then-one-input cycles both execute the original input without requiring a second keystroke, and pwd confirms the replacement shell starts in the original worktree cwd. * fix(vscode): avoid duplicate prompt after terminal recovery
2 participants
Agent Manager terminal tabs can remain on a dead PTY after a shell exits, and the next command may be discarded. This makes the terminal appear available while forcing the user to type again, which is especially disruptive after switching worktrees or using
Cmd+/.The terminal now keeps the existing tab and scrollback, clearly advertises that the shell ended, and starts a replacement shell in the same working directory when the user types. Input is buffered while the replacement attaches and delivered once, while a bare Enter only revives the prompt without creating a duplicate empty command. Run and Setup terminals keep their existing close-only behavior and are not rerun automatically.