mirror of
https://github.com/Kilo-Org/kilocode.git
synced 2026-08-29 03:44:06 +08:00
fix: activate plan handoff sessions and retry recovered question replies (#12511)
Two regressions in the plan handoff flow: Choosing "Start new session" after a plan created the session in the background but left the UI on the completed plan. Since #10466 the sidebar only activates sessionCreated messages that match a pending draft tab, and followup sessions carry no draftID. registerSession now takes an activate flag, adoptPendingFollowup passes it, and the webview opens and focuses the tab when set. Replying to a recovered question took two submits. The first reply hit the recorded stale directory, got a 404, and the handler redrew the question instead of retrying. Recovery now rediscovers the directory and retries the reply or reject there once, and only marks the question stale when a complete scan confirms it is gone.
This commit is contained in:
committed by
GitHub
parent
5bd5b90009
commit
9e1b54d875
@@ -0,0 +1,5 @@
|
||||
---
|
||||
"kilo-code": patch
|
||||
---
|
||||
|
||||
Open plan implementation sessions immediately and submit recovered plan choices without requiring a second click.
|
||||
@@ -768,7 +768,7 @@ export class KiloProvider implements vscode.WebviewViewProvider, TelemetryProper
|
||||
}
|
||||
|
||||
/** Register a session created externally and notify the webview. */
|
||||
public registerSession(session: Session): void {
|
||||
public registerSession(session: Session, activate = false): void {
|
||||
this.stopCurrentSessionProcesses(session.id)
|
||||
this.setCurrentSession(session)
|
||||
this.contextSessionID = session.id
|
||||
@@ -776,6 +776,7 @@ export class KiloProvider implements vscode.WebviewViewProvider, TelemetryProper
|
||||
this.postMessage({
|
||||
type: "sessionCreated",
|
||||
session: this.sessionToWebview(session),
|
||||
...(activate ? { activate: true } : {}),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -4479,7 +4480,7 @@ export class KiloProvider implements vscode.WebviewViewProvider, TelemetryProper
|
||||
this.pendingFollowup = null
|
||||
this.trackDirectory(session.id, session.directory)
|
||||
for (const cb of this.followupListeners) cb(session, session.directory)
|
||||
this.registerSession(session)
|
||||
this.registerSession(session, true)
|
||||
void this.handleLoadMessages(session.id)
|
||||
return true
|
||||
}
|
||||
|
||||
@@ -28,6 +28,8 @@ interface QuestionRecovery {
|
||||
readonly complete: boolean
|
||||
}
|
||||
|
||||
type QuestionRoute = { kind: "retry"; dir: string } | { kind: "stale" } | { kind: "failed" }
|
||||
|
||||
function isNotFoundError(error: unknown): boolean {
|
||||
const record = (value: unknown) =>
|
||||
value && typeof value === "object" ? (value as Record<string, unknown>) : undefined
|
||||
@@ -41,18 +43,18 @@ function isNotFoundError(error: unknown): boolean {
|
||||
)
|
||||
}
|
||||
|
||||
function stale(ctx: QuestionContext, requestID: string): void {
|
||||
async function recover(ctx: QuestionContext, requestID: string): Promise<QuestionRoute> {
|
||||
const result = await fetchAndSendPendingQuestions(ctx, requestID)
|
||||
if (!result) return { kind: "failed" }
|
||||
if (result.seen.has(requestID)) {
|
||||
const dir = ctx.getQuestionDirectory(requestID)
|
||||
return dir ? { kind: "retry", dir } : { kind: "failed" }
|
||||
}
|
||||
// Absence only proves staleness when every directory was scanned.
|
||||
if (!result.complete) return { kind: "failed" }
|
||||
ctx.clearQuestionDirectory(requestID)
|
||||
ctx.postMessage({ type: "questionResolved", requestID })
|
||||
void fetchAndSendPendingQuestions(ctx)
|
||||
}
|
||||
|
||||
async function recover(ctx: QuestionContext, requestID: string): Promise<boolean> {
|
||||
const result = await fetchAndSendPendingQuestions(ctx)
|
||||
if (!result?.complete || result.seen.has(requestID)) return false
|
||||
ctx.clearQuestionDirectory(requestID)
|
||||
ctx.postMessage({ type: "questionResolved", requestID })
|
||||
return true
|
||||
return { kind: "stale" }
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -61,7 +63,10 @@ async function recover(ctx: QuestionContext, requestID: string): Promise<boolean
|
||||
* called after child-session sync and after SSE reconnects so missed
|
||||
* question.asked events don't leave the server blocked indefinitely.
|
||||
*/
|
||||
export async function fetchAndSendPendingQuestions(ctx: QuestionContext): Promise<QuestionRecovery | undefined> {
|
||||
export async function fetchAndSendPendingQuestions(
|
||||
ctx: QuestionContext,
|
||||
omit?: string,
|
||||
): Promise<QuestionRecovery | undefined> {
|
||||
if (!ctx.client) return
|
||||
try {
|
||||
for (;;) {
|
||||
@@ -94,6 +99,9 @@ export async function fetchAndSendPendingQuestions(ctx: QuestionContext): Promis
|
||||
if (ctx.getQuestionRevision() !== revision) continue
|
||||
for (const item of pending) {
|
||||
ctx.recordQuestionDirectory(item.question.id, item.dir)
|
||||
// The omitted request is mid-reply; its card is still visible, so
|
||||
// reposting it would only churn the webview.
|
||||
if (item.question.id === omit) continue
|
||||
ctx.postMessage({
|
||||
type: "questionRequest",
|
||||
question: {
|
||||
@@ -126,19 +134,26 @@ export async function handleQuestionReply(
|
||||
}
|
||||
|
||||
const sid = sessionID ?? ctx.currentSessionId
|
||||
const origin = ctx.getQuestionDirectory(requestID)
|
||||
const dir = origin ?? ctx.getWorkspaceDirectory(sid)
|
||||
const dir = ctx.getQuestionDirectory(requestID) ?? ctx.getWorkspaceDirectory(sid)
|
||||
|
||||
try {
|
||||
await ctx.client.question.reply({ requestID, answers, directory: dir }, { throwOnError: true })
|
||||
ctx.clearQuestionDirectory(requestID)
|
||||
return true
|
||||
} catch (error) {
|
||||
if (isNotFoundError(error) && origin) {
|
||||
stale(ctx, requestID)
|
||||
return false
|
||||
const route = isNotFoundError(error) ? await recover(ctx, requestID) : undefined
|
||||
if (route?.kind === "stale") return false
|
||||
if (route?.kind === "retry" && route.dir !== dir) {
|
||||
try {
|
||||
await ctx.client.question.reply({ requestID, answers, directory: route.dir }, { throwOnError: true })
|
||||
ctx.clearQuestionDirectory(requestID)
|
||||
return true
|
||||
} catch (retry) {
|
||||
console.error("[Kilo New] KiloProvider: Failed to reply to recovered question:", retry)
|
||||
ctx.postMessage({ type: "questionError", requestID })
|
||||
return false
|
||||
}
|
||||
}
|
||||
if (isNotFoundError(error) && (await recover(ctx, requestID))) return false
|
||||
console.error("[Kilo New] KiloProvider: Failed to reply to question:", error)
|
||||
ctx.postMessage({ type: "questionError", requestID })
|
||||
return false
|
||||
@@ -157,19 +172,26 @@ export async function handleQuestionReject(
|
||||
}
|
||||
|
||||
const sid = sessionID ?? ctx.currentSessionId
|
||||
const origin = ctx.getQuestionDirectory(requestID)
|
||||
const dir = origin ?? ctx.getWorkspaceDirectory(sid)
|
||||
const dir = ctx.getQuestionDirectory(requestID) ?? ctx.getWorkspaceDirectory(sid)
|
||||
|
||||
try {
|
||||
await ctx.client.question.reject({ requestID, directory: dir }, { throwOnError: true })
|
||||
ctx.clearQuestionDirectory(requestID)
|
||||
return true
|
||||
} catch (error) {
|
||||
if (isNotFoundError(error) && origin) {
|
||||
stale(ctx, requestID)
|
||||
return false
|
||||
const route = isNotFoundError(error) ? await recover(ctx, requestID) : undefined
|
||||
if (route?.kind === "stale") return false
|
||||
if (route?.kind === "retry" && route.dir !== dir) {
|
||||
try {
|
||||
await ctx.client.question.reject({ requestID, directory: route.dir }, { throwOnError: true })
|
||||
ctx.clearQuestionDirectory(requestID)
|
||||
return true
|
||||
} catch (retry) {
|
||||
console.error("[Kilo New] KiloProvider: Failed to reject recovered question:", retry)
|
||||
ctx.postMessage({ type: "questionError", requestID })
|
||||
return false
|
||||
}
|
||||
}
|
||||
if (isNotFoundError(error) && (await recover(ctx, requestID))) return false
|
||||
console.error("[Kilo New] KiloProvider: Failed to reject question:", error)
|
||||
ctx.postMessage({ type: "questionError", requestID })
|
||||
return false
|
||||
|
||||
@@ -140,7 +140,7 @@ describe("KiloProvider follow-up sessions", () => {
|
||||
revert: null,
|
||||
summary: null,
|
||||
},
|
||||
draftID: undefined,
|
||||
activate: true,
|
||||
},
|
||||
])
|
||||
})
|
||||
|
||||
@@ -14,6 +14,7 @@ import {
|
||||
restoreTabs,
|
||||
restoreTrackedTabs,
|
||||
showTabStrip,
|
||||
tabsForCreatedSession,
|
||||
trackedSessionInventory,
|
||||
type LocalTabState,
|
||||
} from "../../webview-ui/src/utils/local-tabs"
|
||||
@@ -61,6 +62,25 @@ const tracked = () =>
|
||||
)
|
||||
|
||||
describe("local session tabs", () => {
|
||||
it("opens explicitly activated sessions in the foreground", () => {
|
||||
expect(tabsForCreatedSession(state(["s1"], "s1"), "s2", undefined, true)).toEqual({
|
||||
ids: ["s1", "s2"],
|
||||
active: "s2",
|
||||
})
|
||||
})
|
||||
|
||||
it("promotes a matching pending draft into the created session", () => {
|
||||
expect(tabsForCreatedSession(state([pending()], pending()), "s1", pending(), undefined)).toEqual({
|
||||
ids: ["s1"],
|
||||
active: "s1",
|
||||
})
|
||||
})
|
||||
|
||||
it("ignores created sessions without activation or a pending draft", () => {
|
||||
expect(tabsForCreatedSession(state(["s1"], "s1"), "s2", undefined, undefined)).toBeUndefined()
|
||||
expect(tabsForCreatedSession(state(["s1"], "s1"), "s2", "sidebar-pending:gone", undefined)).toBeUndefined()
|
||||
})
|
||||
|
||||
it("hides the tab strip when only one tab remains", () => {
|
||||
expect(showTabStrip([pending()])).toBe(false)
|
||||
expect(showTabStrip([pending(), "sidebar-pending:2"])).toBe(true)
|
||||
|
||||
@@ -29,7 +29,7 @@ function ctx(
|
||||
dirs?: Map<string, string>
|
||||
extra?: string[]
|
||||
pending?: Record<string, QuestionRequest[]>
|
||||
errors?: { list?: Record<string, unknown>; reply?: unknown; reject?: unknown }
|
||||
errors?: { list?: Record<string, unknown>; reply?: unknown | unknown[]; reject?: unknown | unknown[] }
|
||||
changeOnList?: string
|
||||
removeOnList?: string
|
||||
} = {},
|
||||
@@ -42,7 +42,11 @@ function ctx(
|
||||
const dirs = opts.dirs ?? new Map<string, string>()
|
||||
let revision = 0
|
||||
let changed = false
|
||||
let reply = 0
|
||||
let reject = 0
|
||||
const removed = new Set<string>()
|
||||
const failure = (value: unknown | unknown[] | undefined, index: number) =>
|
||||
Array.isArray(value) ? value[index] : value
|
||||
const client = {
|
||||
question: {
|
||||
list: async (args: { directory?: string }) => {
|
||||
@@ -63,12 +67,14 @@ function ctx(
|
||||
},
|
||||
reply: async (args: unknown) => {
|
||||
replies.push(args)
|
||||
if (opts.errors?.reply) throw opts.errors.reply
|
||||
const error = failure(opts.errors?.reply, reply++)
|
||||
if (error) throw error
|
||||
return { data: true }
|
||||
},
|
||||
reject: async (args: unknown) => {
|
||||
rejects.push(args)
|
||||
if (opts.errors?.reject) throw opts.errors.reject
|
||||
const error = failure(opts.errors?.reject, reject++)
|
||||
if (error) throw error
|
||||
return { data: true }
|
||||
},
|
||||
},
|
||||
@@ -167,26 +173,96 @@ describe("question handlers", () => {
|
||||
expect(messages).toContainEqual({ type: "questionResolved", requestID: "req-stale" })
|
||||
})
|
||||
|
||||
it("keeps fallback-directory 404s retryable while recovering the request route", async () => {
|
||||
it("retries a reply through the recovered request directory", async () => {
|
||||
const error = new Error("Question request not found", {
|
||||
cause: { status: 404, body: { name: "NotFoundError" } },
|
||||
})
|
||||
const dir = "/workspace/.kilo/worktrees/origin"
|
||||
const { fake, messages, questionDirs } = ctx({
|
||||
const { fake, messages, replies, questionDirs } = ctx({
|
||||
tracked: ["ses-root"],
|
||||
extra: [dir],
|
||||
pending: { [dir]: [pending("req-misrouted", "ses-root")] },
|
||||
errors: { reply: error },
|
||||
errors: { reply: [error] },
|
||||
})
|
||||
questionDirs.set("req-misrouted", "/workspace/.kilo/worktrees/stale")
|
||||
|
||||
const ok = await handleQuestionReply(fake, "req-misrouted", [["Continue"]], "ses-root")
|
||||
|
||||
expect(ok).toBe(true)
|
||||
expect(replies).toEqual([
|
||||
{
|
||||
requestID: "req-misrouted",
|
||||
answers: [["Continue"]],
|
||||
directory: "/workspace/.kilo/worktrees/stale",
|
||||
},
|
||||
{
|
||||
requestID: "req-misrouted",
|
||||
answers: [["Continue"]],
|
||||
directory: dir,
|
||||
},
|
||||
])
|
||||
expect(messages).not.toContainEqual({ type: "questionResolved", requestID: "req-misrouted" })
|
||||
expect(messages).not.toContainEqual({ type: "questionError", requestID: "req-misrouted" })
|
||||
expect(messages).not.toContainEqual({
|
||||
type: "questionRequest",
|
||||
question: pending("req-misrouted", "ses-root"),
|
||||
})
|
||||
expect(questionDirs.has("req-misrouted")).toBe(false)
|
||||
})
|
||||
|
||||
it("retries even when an unrelated directory fails to list", async () => {
|
||||
const error = new Error("Question request not found", {
|
||||
cause: { status: 404, body: { name: "NotFoundError" } },
|
||||
})
|
||||
const dir = "/workspace/.kilo/worktrees/origin"
|
||||
const failing = "/workspace/.kilo/worktrees/failing"
|
||||
const { fake, messages, replies, questionDirs } = ctx({
|
||||
tracked: ["ses-root"],
|
||||
extra: [dir, failing],
|
||||
pending: { [dir]: [pending("req-misrouted", "ses-root")] },
|
||||
errors: { list: { [failing]: new Error("temporary failure") }, reply: [error] },
|
||||
})
|
||||
questionDirs.set("req-misrouted", "/workspace/.kilo/worktrees/stale")
|
||||
const spy = spyOn(console, "error").mockImplementation(() => {})
|
||||
|
||||
const ok = await handleQuestionReply(fake, "req-misrouted", [["Continue"]], "ses-root")
|
||||
spy.mockRestore()
|
||||
|
||||
expect(ok).toBe(false)
|
||||
expect(messages).not.toContainEqual({ type: "questionResolved", requestID: "req-misrouted" })
|
||||
expect(messages).toContainEqual({ type: "questionError", requestID: "req-misrouted" })
|
||||
expect(questionDirs.get("req-misrouted")).toBe(dir)
|
||||
expect(ok).toBe(true)
|
||||
expect(replies.map((args) => (args as { directory: string }).directory)).toEqual([
|
||||
"/workspace/.kilo/worktrees/stale",
|
||||
dir,
|
||||
])
|
||||
expect(messages).not.toContainEqual({ type: "questionError", requestID: "req-misrouted" })
|
||||
expect(questionDirs.has("req-misrouted")).toBe(false)
|
||||
})
|
||||
|
||||
it("retries a reject through the recovered request directory", async () => {
|
||||
const error = new Error("Question request not found", {
|
||||
cause: { status: 404, body: { name: "NotFoundError" } },
|
||||
})
|
||||
const dir = "/workspace/.kilo/worktrees/origin"
|
||||
const { fake, messages, rejects, questionDirs } = ctx({
|
||||
tracked: ["ses-root"],
|
||||
extra: [dir],
|
||||
pending: { [dir]: [pending("req-misrouted", "ses-root")] },
|
||||
errors: { reject: [error] },
|
||||
})
|
||||
questionDirs.set("req-misrouted", "/workspace/.kilo/worktrees/stale")
|
||||
|
||||
const ok = await handleQuestionReject(fake, "req-misrouted", "ses-root")
|
||||
|
||||
expect(ok).toBe(true)
|
||||
expect(rejects).toEqual([
|
||||
{ requestID: "req-misrouted", directory: "/workspace/.kilo/worktrees/stale" },
|
||||
{ requestID: "req-misrouted", directory: dir },
|
||||
])
|
||||
expect(messages).not.toContainEqual({ type: "questionError", requestID: "req-misrouted" })
|
||||
expect(messages).not.toContainEqual({
|
||||
type: "questionRequest",
|
||||
question: pending("req-misrouted", "ses-root"),
|
||||
})
|
||||
expect(questionDirs.has("req-misrouted")).toBe(false)
|
||||
})
|
||||
|
||||
it("removes a fallback question when recovery confirms it is stale", async () => {
|
||||
|
||||
@@ -21,10 +21,9 @@ import {
|
||||
insertSessionTabAfter,
|
||||
isPendingTab,
|
||||
openSessionTab,
|
||||
pendingTabForCreated,
|
||||
reconcileTabs,
|
||||
replacePendingTab,
|
||||
restoreTabs,
|
||||
tabsForCreatedSession,
|
||||
type LocalTabState,
|
||||
} from "../utils/local-tabs"
|
||||
import {
|
||||
@@ -186,10 +185,8 @@ export const LocalTabsProvider: ParentComponent = (props) => {
|
||||
}
|
||||
if (message.type === "sessionCreated") {
|
||||
if (message.draftID && promotePendingDraftDiscard(message.draftID, message.session.id)) return
|
||||
const draft = pendingTabForCreated(ids(), message.draftID)
|
||||
if (!draft) return
|
||||
const before = active()
|
||||
const next = replacePendingTab(current(), draft, message.session.id)
|
||||
const next = tabsForCreatedSession(current(), message.session.id, message.draftID, message.activate)
|
||||
if (!next) return
|
||||
fresh.add(message.session.id)
|
||||
apply(next)
|
||||
focus(next.active)
|
||||
|
||||
@@ -168,6 +168,7 @@ export interface SessionCreatedMessage {
|
||||
type: "sessionCreated"
|
||||
session: SessionInfo
|
||||
draftID?: string
|
||||
activate?: boolean
|
||||
}
|
||||
|
||||
export interface SessionForkedMessage {
|
||||
|
||||
@@ -109,6 +109,22 @@ export function pendingTabForCreated(
|
||||
return ids.includes(draft) && check(draft) ? draft : undefined
|
||||
}
|
||||
|
||||
// Tab outcome for a created session: explicit activation opens it in the
|
||||
// foreground, otherwise only a matching pending draft promotes into it.
|
||||
// Callers never combine activate with a draftID; activation wins if they do.
|
||||
export function tabsForCreatedSession(
|
||||
state: LocalTabState,
|
||||
id: string,
|
||||
draftID: string | undefined,
|
||||
activate: boolean | undefined,
|
||||
check: PendingTabCheck = isPendingTab,
|
||||
): LocalTabState | undefined {
|
||||
if (activate) return openSessionTab(state, id)
|
||||
const draft = pendingTabForCreated(state.ids, draftID, check)
|
||||
if (!draft) return undefined
|
||||
return replacePendingTab(state, draft, id)
|
||||
}
|
||||
|
||||
export function nextTabAfterClose(ids: readonly string[], id: string): string | undefined {
|
||||
const index = ids.indexOf(id)
|
||||
if (index === -1) return undefined
|
||||
|
||||
Reference in New Issue
Block a user