feat(agent-manager): run setup scripts in panel terminal - #12703
Conversation
c49acd6 to
59bfcfd
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /> | ||
| </Show> | ||
| <Show when={!contextEmpty() && !history()}> | ||
| <Show when={showDetailStack()}> |
There was a problem hiding this comment.
CRITICAL: The gate change drops the unassigned-session case, blanking the detail pane
The old gate was !contextEmpty() && !history(). contextEmpty() explicitly return false for a null selection (line 672), so the detail stack did render for unassigned sessions. The new gate is showTerminalStack(history(), selection()) = !history && selection !== null, which is false whenever selection() is null.
selectUnassigned() sets setSelection(null) and is reachable from the sidebar and focusSidebarItem. Both ChatView and the read-only banner live inside this stack (lines 2494-2501), and readOnly() (line 723) is defined as selection() === null && !!session.currentSessionID() — so it is now dead code and clicking an unassigned session renders nothing. The sibling <Show when={contextEmpty()}> at 2373 is also false in that state, so there is no fallback.
Secondary effect: the stack also hosts renderTerminalLayer and the side-terminal host, so selecting an unassigned session unmounts every live xterm — contradicting the invariant in the comment right below ("Keep terminal tabs mounted so output streams across worktree switches").
Suggest preserving the old semantics, e.g. !history && (selection !== null || !contextEmpty()).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| {/* Session-less context (e.g. a worktree mid-provisioning): the | ||
| empty state lives in the main pane so the side terminal | ||
| panel can render next to it. */} | ||
| <Show when={contextEmpty()}> |
There was a problem hiding this comment.
WARNING: Two empty states now render at the same time
The pre-existing empty state at line 2373 (<Show when={contextEmpty()}> directly under .am-detail) was kept, and the detail stack is no longer gated on !contextEmpty(). Whenever contextEmpty() && selection() !== null && !history() — exactly the new provisioning flow this PR targets — both blocks render: the outer generic "No sessions" + New Session button, and this inner spinner/progress block. .am-detail is a flex column and .am-empty-state is flex: 1, so the user sees two stacked panels.
Worse, the outer button calls handleAddSession, which now early-returns on settingUpSelection() (line ~1899) — a visibly enabled button that silently does nothing. One of the two blocks should be removed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const sel = selection() | ||
| // A live Setup script terminal shows progress and failures on its own | ||
| // tab; never cover it with the blocking overlay. | ||
| if (typeof sel === "string" && sel !== LOCAL && hasSetupTerminal(nsKey(sel), terms.sides())) return null |
There was a problem hiding this comment.
WARNING: This suppresses every later overlay for the worktree, not just the setup-progress one
hasSetupTerminal matches any Setup record regardless of state, and an exited/failed Setup tab is deliberately retained in terminal state until the user closes it. Because this early-return sits above the state.active check, once a worktree has run an embedded setup script every subsequent worktreeSetup overlay for that worktree is swallowed — including the genuine error paths from createSessionInWorktree ("Failed to create session: …", "Not connected to CLI backend").
In that flow discardWorktree then rolls the worktree back, so the user watches the worktree disappear with no error shown anywhere. Gating on a live status (running/stopping) would match the comment's stated intent ("A live Setup script terminal…") and keep error overlays working.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| export function createSideTerminal(deps: SideTerminalDeps) { | ||
| const [local, setLocal] = createSignal<TerminalDestination | undefined>(deps.saved) | ||
| const [destination, setDestination] = createSignal<TerminalDestination>(deps.saved ?? "vscode") | ||
| if (deps.saved) deps.postMessage({ type: "agentManager.terminal.destinationSelected", destination: deps.saved }) |
There was a problem hiding this comment.
WARNING: Restoring panel-local state rewrites the user's application-scoped setting
This fires on every createSideTerminal call, i.e. every webview construction/reload — not only on an explicit pick. The extension treats destinationSelected as a deliberate choice: handleDestination calls state.select() (latching local = true) and writeTerminalDestination(...) with ConfigurationTarget.Global.
So merely reopening a panel that holds a stale saved value silently reverts the global setting a user changed afterwards in the Settings UI (or that another window wrote), and broadcasts terminal.destinationChanged to every other window — the cross-window fight the choose() doc comment above says this design avoids. Before this PR a restore never wrote.
Suggest distinguishing restore from pick (separate message, or a restore: true flag that skips the config write).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| log: (message: string) => void, | ||
| ): boolean { | ||
| if (message.type !== "agentManager.terminal.destinationSelected") return false | ||
| const destination = resolveTerminalDestination(message.destination) |
There was a problem hiding this comment.
SUGGESTION: Coercing an invalid IPC payload and then persisting it
resolveTerminalDestination falls back to "vscode" for anything it doesn't recognise. That's the right behaviour when reading a stale setting, but here the coerced value is written straight into the user's global settings on the next line. A malformed or unexpected destination therefore silently overwrites a real setting instead of being ignored. Returning early when message.destination is not "vscode" | "agentManager" would make this write-path safe at the trust boundary.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const decision = ambientDecision(deps.terms.scriptStatus(ambient.terminalId), deps.selection(), ambient.contextKey) | ||
| if (decision === "wait") return | ||
| setPending(undefined) | ||
| if (decision === "hide") deps.setSidePanel(null) |
There was a problem hiding this comment.
WARNING: The auto-hide closes whatever the side panel currently shows, not just the terminal
The effect never reads deps.sidePanel() before hiding, and cancel() is only wired to terminal-panel engagement in AgentManagerApp.tsx (hide, onTerminalDestinationOpen, onClose, onCloseOthers, onStart, onStop). It is not wired to toggleDiffPanel() / the toggleDiff keybinding, nor to openReviewTab(), all of which call setSidePanel("diff" | "pr") while a reveal is pending.
Repro: setup starts and ambiently reveals the panel → user opens the diff panel → setup exits 0 → ambientDecision still returns "hide" (it only compares selection vs contextKey) and this line closes the diff panel the user deliberately opened. That breaks the "user content is never pulled away" invariant in this module's header. if (decision === "hide" && deps.sidePanel() === "terminal") would be enough.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| if (status.state === "exited" && status.exitCode !== 0) { | ||
| failureWritten = true | ||
| term.writeln(`\r\n\x1b[31m[${t("agentManager.terminal.setupFailedCode")} ${status.exitCode ?? "?"}]\x1b[0m`) |
There was a problem hiding this comment.
SUGGESTION: Concatenating the exit code onto a translated string breaks word order
t() supports {{param}} templates (webview-ui/src/context/language.tsx → resolveTemplate), and this dict already uses them elsewhere ("agentManager.worktree.versions": "{{count}} versions"). Gluing the number on the end forces English word order onto every locale — the new translations render as "Setup-Skript mit Exit-Code fehlgeschlagen 1" (de), "セットアップスクリプトが終了コードで失敗しました 1" (ja), "종료 코드로 설정 스크립트 실패 1" (ko), and lands even worse in the RTL locales (ar, fa).
A key like "setup script failed with exit code {{code}}" plus t(key, { code: status.exitCode ?? "?" }) fixes all locales and lets the split setupFailed / setupFailedCode pair collapse into one key.
Separately: streamed (line ~225) is a plain local, and the effect below only tracks props.status?.(). If the failed snapshot lands before the first frame and the socket then stays open — the exact case the comment above calls out ("or stay open when a background child outlives the script") — noteFailure() is never reached again and no annotation is written. Setting streamed through a signal, or calling noteFailure() from ws.onmessage, would close that gap.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| */ | ||
| export function terminalClosable(status: ScriptTerminalStatus | undefined): boolean { | ||
| if (status?.kind !== "setup") return true | ||
| return status.state !== "running" && status.state !== "stopping" |
There was a problem hiding this comment.
SUGGESTION: A Setup tab in stopping has neither a stop nor a close affordance
terminalStoppable returns true only for "running", and this returns false for both "running" and "stopping". In stopping the tab therefore renders no button at all (SortableTerminalTab.tsx gates both <Show> blocks on these predicates). If the stop never settles — wedged backend PTY, lost snapshot — the tab is permanently unclosable from the UI. Consider keeping close enabled in stopping, or keeping the stop button visible-but-disabled so the state stays legible.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| starts.push({ kind, config, done }) | ||
| if (opts?.startError) return Promise.reject(opts.startError) | ||
| if (opts?.gate) return opts.gate.promise | ||
| stops.length = 0 |
There was a problem hiding this comment.
SUGGESTION: A recorder that clears itself weakens future assertions
stops is reset on every start() invocation, so any test that starts twice silently loses the first handle's stop() calls and expect(ctx.stops).toEqual([...]) becomes weaker than it reads. A recorder should only append — dropping this line keeps the harness honest.
Two related test-quality notes while you're here:
harness()returns a handle with nokill, a shape production never produces (ScriptTerminalManager.startalways returns{ stop, kill }). The "times out, rejects, and stops the process tree" case therefore exerciseshalt()'s fallback rather than the real path; giving the shared harness akilland covering the fallback in one dedicated test would stop it drifting.- The new
if (message === "Setup script was stopped") returnsuppression insetup-script-task.tsand the embedded (agentManager) failure/timeoutstatus: "error"post are both untested — those are exactly the paths a regression would silently break.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| Kilo runs the script automatically whenever a new worktree is created. It uses `sh` for POSIX scripts, PowerShell for `.ps1`, and `cmd.exe` for `.cmd` / `.bat`, so executable permissions are not required. | ||
|
|
||
| Where the script runs follows the terminal destination dropdown in the Agent Manager toolbar. **Agent Manager panel** shows live output in a named `Setup` tab in the side terminal panel. After success, the panel returns to its previous state unless you interacted with it; the retained tab remains available for review. Failures keep the panel open. **VS Code terminal** runs setup as a task in the integrated terminal. The script keeps the existing five-minute timeout; when it expires, the setup process tree is terminated and the failed tab retains its partial output. |
There was a problem hiding this comment.
SUGGESTION: The timeout sentence is only true for the panel destination
"The script keeps the existing five-minute timeout; when it expires, the setup process tree is terminated and the failed tab retains its partial output" directly follows the VS Code terminal sentence and opens destination-agnostically, so it reads as applying to both. It only holds for the Agent Manager panel path (setup-script-task.ts halt() → manager.kill()). task-runner.ts merely rejects on timeout — it never calls execution.terminate() — so with VS Code terminal the task process keeps running and there is no "failed tab". Suggest scoping the termination/retained-output clause to the panel destination.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 18 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (55 files)
Notes and assumptions
Fix these issues in Kilo Cloud Reviewed by claude-opus-5 · Input: 86 · Output: 37.1K · Cached: 5M Review guidance: REVIEW.md from base branch |
The setup-terminal gate introduced in #12703 changed the detail-stack predicate from !contextEmpty() && !history() to !history && selection !== null. An unassigned session has selection === null but a live session showing, so contextEmpty() is false there. The new gate dropped that case, blanking the content pane, leaving readOnly() dead code, and unmounting every live xterm on selection. showTerminalStack now takes contextEmpty and returns !history && (selection !== null || !contextEmpty), restoring the pre-#12703 semantics while keeping the provisioning worktree (empty context with a side Setup tab) rendering the stack. Adds a showTerminalStack regression suite covering the unassigned, history, selected-context, provisioning, and empty cases.
…tup-terminal feat(agent-manager): run setup scripts in panel terminal
…g#12722) The setup-terminal gate introduced in Kilo-Org#12703 changed the detail-stack predicate from !contextEmpty() && !history() to !history && selection !== null. An unassigned session has selection === null but a live session showing, so contextEmpty() is false there. The new gate dropped that case, blanking the content pane, leaving readOnly() dead code, and unmounting every live xterm on selection. showTerminalStack now takes contextEmpty and returns !history && (selection !== null || !contextEmpty), restoring the pre-Kilo-Org#12703 semantics while keeping the provisioning worktree (empty context with a side Setup tab) rendering the stack. Adds a showTerminalStack regression suite covering the unassigned, history, selected-context, provisioning, and empty cases.
2 participants
Agent Manager setup scripts currently run as VS Code tasks even when the terminal destination is set to the Agent Manager panel. That hides provisioning output outside the worktree context, makes the displayed destination disagree with execution, and leaves failures difficult to inspect.
This routes setup through the same authenticated PTY runtime used by Run scripts when Agent Manager panel is selected, while preserving the integrated task path for VS Code terminal. Each worktree receives an independent
Setupterminal with live and replayable output, explicit stop behavior, failure annotations, process-tree cleanup, and best-effort session creation after failures or timeouts. Panel-local destination state is authoritative from the first selection, including restored panels and multi-window setting echoes.The side terminal reveals ambiently during provisioning, restores the previous layout after success unless the user interacted with it, and remains open on failure. Setup tabs created while their worktree is in the background are activated within that context without replacing an existing user-selected terminal.
Closes #12649