Repository navigation
feat(cli): pause a subagent's task when the user interrupts it - #14752
calebnorman wants to merge 4 commits into
Conversation
8e46755 to
03046ea
Compare
| ) | ||
|
|
||
| // the parent was not aborted: it finished its step and made its next model call | ||
| expect(result.parts.some((part) => part.type === "text" && part.text === "parent recovered")).toBe(true) |
There was a problem hiding this comment.
Please move all kilo specific code to kilo files and code. The goal should be to reduce kilo change markers as much as possible. We also have a skill for that.
There was a problem hiding this comment.
moved code into our managed files.
| /** Child session metadata key that marks a task paused by the user. Must match the TUI. */ | ||
| export const KEY = "kilo.task" | ||
|
|
||
| export const NOTICE = |
There was a problem hiding this comment.
I am a bit unsure what this is for, @kilocode-bot does this introduce a new tool and why is it needed for this change?
There was a problem hiding this comment.
No, it doesn't add a tool. task-pause.ts is a helper module for the existing task tool. It sits under src/kilocode/tool/ because runTask uses it. That placement is from the PR description and the file header. I haven't read the rest of the file.
What it does: it's a registry of paused tasks, keyed by child session ID. When a user interrupts a subagent, runTask waits in this registry instead of returning. The next prompt to that child resumes the wait, and the task then waits on that new turn and delivers its result normally.
Why it's needed:
- Before this change, an interrupt either hung the parent or ended the task as a failure. It hung because the task waited on the child's drain, which also holds for each background grandchild. It failed because the turn's
AbortedErrorbecame the tool result. - The goal is for Esc on a subagent to pause the task, as in VS Code, not end it. The task's job stays
running, and the parent stays blocked or pending. - Only a
treeabort of the child (Cancel), session delete, or instance dispose ends a paused task early. - Keeping this logic in a separate Kilo-owned file also keeps the changes to upstream
tasktool code small, which I assume is the usual reason for thesrc/kilocode/split.
Related pieces in the PR:
- The registry is projected into the child session's
metadataaskilo.task: { status: paused }through theGoalState.projecthook. That's how the TUI shows "Interrupted · send a prompt to resume". - The registry is live, so a marker left behind by a crashed process is never shown as paused.
I can look at specific functions in the file if you tell me which part is unclear.
An interrupt (session-scope abort) of a subagent stops its turn but keeps the task's job running: the task waits in a Kilo-owned pause until the next prompt to the child resumes it. Only a tree cancel, delete or dispose ends it early. The pause is exposed on the child session's metadata, and a paused background task posts a board notice to its parent. The subagent tree-cancel test moves into test/kilocode.
Esc in the prompt and double Esc in a subagent view now interrupt with session scope. A paused subagent keeps its steering prompt open to resume the task, its footer says so, and the parent's task card shows the task waiting instead of a spinner.
691a461 to
f865790
Compare
Adversarial reviewThe overall design works: the pause logic sits in Kilo files, the shared-file footprint is small and annotated, and the tests drive the real runtime with no mocks. I checked the admission race first. It's fine: 1.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| Effect.acquireUseRelease( | ||
| Effect.gen(function* () { | ||
| const entry = { done: yield* Deferred.make<void>(), notice: notice(messageID) } | ||
| pauses.set(input.child, entry) |
There was a problem hiding this comment.
WARNING: Potential registry leak / permanently stuck "paused" state
pauses.set(input.child, entry) registers the pause before the fallible publish/entry.notice work in the acquire phase. Effect.acquireUseRelease only runs the release (which deletes the entry) when acquire succeeds, so if entry.notice raises a defect (e.g. a DB defect from BoardStore.post that escapes the typed Effect.catch in KiloSessionSteering.post), acquire fails, the entry is never removed, and the child keeps exposing kilo.task: { status: "paused" } forever while nothing can resume it. Consider catchCause-ing the notice or making the removal unconditional.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (!pauses.has(input.id)) return yield* jobs.extend(input) | ||
| yield* direct.pipe( | ||
| Effect.provideService(Detached, true), | ||
| Effect.catchCause((cause) => Effect.logWarning("paused task prompt failed", { cause })), |
There was a problem hiding this comment.
WARNING: A failed prompt to a paused task is swallowed and reported as success
In the paused branch direct is forked, every cause is caught here and only logged, and extend still returns true. The caller then reports "Additional context sent to the running background task". But if ops.prompt fails before KiloSessionAdmission.admit runs (e.g. a session lookup error), resume is never called and the task stays paused indefinitely, with no error surfaced to the parent or user. Consider propagating a failure (or a non-success result) so the caller does not claim the prompt was delivered.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| yield* run.prompt.cancel(run.child, "session") | ||
| yield* paused(run.child) | ||
| const board = yield* BoardStore.read({ sessionID: run.chat.id }) | ||
| expect(board.messages).toEqual([ |
There was a problem hiding this comment.
WARNING: This assertion can race the pause registration
paused() polls KiloTaskPause.paused(), which becomes true as soon as pauses.set runs in hold's acquire, before the board notice is posted. The BoardStore.read immediately after can therefore observe an empty board and flake under load. Poll the board until the notice arrives instead of using the in-memory registry as a barrier. The same pattern appears at line 697 (asserting two notices).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| expect(row).not.toContain("⋯") | ||
| expect(row).toContain("Parent up") | ||
| // a narrow footer drops prev/next while the paused hint shows | ||
| expect(row?.includes("Next right")).toBe(width >= 96) |
There was a problem hiding this comment.
SUGGESTION: Width threshold does not match the component
subagent-footer.tsx computes narrow = width - 4 < 96, so prev/next are hidden for width < 100, but this asserts they are visible for width >= 96. It passes today only because the loop tests 80 and 120; a width of 96-99 would fail a correct implementation. Derive the expectation from the same condition (or from the rendered compact behavior).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 3715b64)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3715b64)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (18 files)
Fix these issues in Kilo Cloud Previous review (commit f865790)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (20 files)
Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
…_id window - Key the detached marker by child ID so a task_id-resumed turn's nested tasks still drain and pause. - Take the direct path for a stopped child whose task has not paused yet, and resume a pause whose stop was cleared before it registered. - Split the pause registry into a dependency-free module projected from session reads alongside goal state, move subagent board posting into its own module, and fork the delivered prompt in the instance scope. - Cancel from a VS Code subagent view uses tree scope, since that view cannot resume a paused task; document the scopes in the SDK.
|
All points are addressed in 3715b64. 1. 2.
3. Non-TUI clients. I shipped the VS Code change with this PR:
4. Smaller items.
Checks:
|
| Effect.acquireUseRelease( | ||
| Effect.gen(function* () { | ||
| const entry = { done: yield* Deferred.make<void>(), notice: notice(messageID) } | ||
| State.set(input.child, entry) |
There was a problem hiding this comment.
WARNING: Pause-registry entry can leak when the acquire phase is interrupted
State.set(input.child, entry) registers the entry before the rest of the Effect.acquireUseRelease acquire phase, which is still interruptible: the stopped re-check calls KiloSessionControl.paused, publish calls sessions.touch, and entry.notice calls input.jobs.get plus the board post. Effect does not run release when acquire is interrupted, so an interrupt landing in that window (a tree/job cancel or instance dispose while the turn is entering its pause) skips State.remove. The module-global pauses map then keeps the entry for the life of the process: project keeps reporting kilo.task: { status: "paused" } for a child that is no longer paused, and extend keeps taking the direct-prompt path for that child ID. Consider doing the fallible work before State.set, registering the entry in the use phase, or masking the registration so removal is guaranteed even on interruption.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * an interrupt (`session`) would pause the parent's task, and this view has no prompt to resume it. | ||
| */ | ||
| export function cancelScope(parentID: string | null | undefined): "session" | "tree" { | ||
| return parentID ? "tree" : "session" |
There was a problem hiding this comment.
WARNING: Cancel in a subagent view can pause the parent when the session is not hydrated yet
cancelScope maps an unknown parentID to a root session ("session"), but the abort is coming from a subagent's own view. currentSession() is store.sessions[currentSessionID()] (session.tsx:2740), and the subagent viewer only calls setCurrentSessionID on open (App.tsx:369); the record carrying parentID arrives later through the async registerSession (SubAgentViewerProvider.ts:64-71). If Cancel is pressed in that window, the abort is sent with scope: "session", which pauses the parent's task instead of cancelling the subagent — the behavior this change is meant to avoid, and this view has no prompt to resume it. Prefer deriving the scope from data already available at abort time (for example the tracked session list/metadata) instead of defaulting an unknown parent to root.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…sk_id prompts - The pause registers as the only acquire step; the board notice and the marker publish run in the use step, so a failing notice cannot leave the child shown as paused. - A task_id sent to a paused task returns once the child admits the prompt, and fails with the cause when the run fails before that. - Wait for the board notice in tests instead of reading it right after the pause registers, and match the footer's narrow threshold.
|
All four issues are addressed in 4d38eb7.
Checks:
|
github-actions
Bot
added
area:cli
github-actions
Bot
added
needs-triage
Follows #14701 (double Esc in a subagent view) and #14702 (steering a subagent from its view), both now in
main.What
Interrupting a subagent now pauses its task instead of ending it, so the TUI matches VS Code:
scope: "session". It stops that session's turn and its foreground descendants, and background descendants keep running. This covers the TUI prompt's double-Esc and feat(cli): interrupt a subagent from its view with double Esc #14701's subagent-view double-Esc; both used to sendtree.runningandrunTaskwaits in a Kilo-owned pause (src/kilocode/tool/task-pause.ts). A foreground parent stays blocked in its task call, and a background parent's result stays pending.task_idcall. The task then waits for that turn and delivers its result normally. If that turn is interrupted too, the task pauses again.treeabort on the subagent (the VS Code task card's or agent bar's Stop) is the only way to end a paused task early. Session delete and instance dispose also tear the pause down. Parent Esc and Ctrl+B promotion behave as before.A foreground task is meant to work until it completes, so nothing returns early with a partial or "interrupted" result.
Why
Before this change, an interrupt either hung the parent or ended the task as a failure. It hung because the task waited on the child's drain, which also holds for each background grandchild. It failed because the turn's
AbortedErrorsurfaced as the tool result. The TUI also senttreefrom Esc, which cancelled background work that VS Code keeps running.Notes for review
Shared-file footprint. The pause logic lives in Kilo files.
src/tool/task.tskeeps the upstreamrunTaskshape and callsKiloTaskPause.settle, andKiloTaskPause.extendwrapsbackground.extendfortask_id.src/session/prompt.tscalls a singleKiloSessionAdmission.admithook. The TUI subagent-view keys live in feat(cli): interrupt a subagent from its view with double Esc #14701'suseSubagentKeys, and the task card and footer read the pause throughkilocode/task-pause-view.tsx. feat(cli): interrupt a subagent from its view with double Esc #14701's tree-cancel test moved fromtest/session/prompt.test.tstotest/kilocode/task-pause.test.ts.Pause condition. A run pauses only when the child's latest assistant message ended with an
AbortedError, the child is paused inKiloSessionControl, and the job is still running.AbortedErrorhas other sources (provider aborts, directSessionRunState.cancel, dispose), and only a user stop setspaused.Visibility. The pause is exposed on the child session's existing
metadatarecord as"kilo.task": { status: "paused" }. It's projected from the live registry through theGoalState.projecthook on every session read and update, so a marker left by a crashed process is never shown. There's no new endpoint or event, and no change toBackgroundJobor the session status schemas.TUI display.
Board notice. A paused background task, or a foreground task promoted while paused, posts a shared-board
INFOto the parent using feat(tui): steer running subagents from their view #14702's posting path. Foreground pauses never post.task_idon a paused job.background.extendwould queue behind the pause forever, so the task tool sends the prompt to the child directly. Its admission resumes the paused run.VS Code (not changed here).
WorkingIndicator's cancel-retry sendsscope: "session"to the current session. In the read-only sidebar sub-agent viewer, that session can be a child, so clicking it now pauses the task. VS Code has no steering prompt in that view yet.