diff --git a/.changeset/responsive-plan-approvals.md b/.changeset/responsive-plan-approvals.md new file mode 100644 index 00000000000..9600a4d3b53 --- /dev/null +++ b/.changeset/responsive-plan-approvals.md @@ -0,0 +1,5 @@ +--- +"@kilocode/cli": patch +--- + +Allow plan approval submissions to complete after planning finishes. diff --git a/packages/opencode/src/kilocode/plan-followup.ts b/packages/opencode/src/kilocode/plan-followup.ts index 6d57baa3c78..b676add2b94 100644 --- a/packages/opencode/src/kilocode/plan-followup.ts +++ b/packages/opencode/src/kilocode/plan-followup.ts @@ -26,7 +26,6 @@ import z from "zod" const agents = lazy(() => makeRuntime(Agent.Service, Agent.defaultLayer)) const providers = lazy(() => makeRuntime(Provider.Service, Provider.defaultLayer)) -const questions = lazy(() => makeRuntime(Question.Service, Question.defaultLayer)) const todo = lazy(() => makeRuntime(Todo.Service, Todo.defaultLayer)) const llm = lazy(() => makeRuntime(LLM.Service, LLM.defaultLayer)) const pending = new Map() @@ -38,20 +37,6 @@ export const PlanFollowupRuntime = { model(providerID: ProviderID, modelID: ModelID): Promise { return providers().runPromise((svc) => svc.getModel(providerID, modelID)) }, - question: { - ask(input: Parameters[0]) { - return questions().runPromise((svc) => svc.ask(input)) - }, - list() { - return questions().runPromise((svc) => svc.list()) - }, - reject(requestID: Parameters[0]) { - return questions().runPromise((svc) => svc.reject(requestID)) - }, - reply(input: Parameters[0]) { - return questions().runPromise((svc) => svc.reply(input)) - }, - }, todo: { get(sessionID: SessionID) { return todo().runPromise((svc) => svc.get(sessionID)) @@ -288,8 +273,15 @@ export namespace PlanFollowup { return msg } - function prompt(input: { sessionID: SessionID; abort: AbortSignal }) { - const promise = PlanFollowupRuntime.question.ask({ + type QuestionRuntime = { + ask: (input: Parameters[0]) => Promise> + list: () => Promise> + reject: (requestID: Parameters[0]) => Promise + } + + function prompt(input: { sessionID: SessionID; abort: AbortSignal; question: QuestionRuntime }) { + if (input.abort.aborted) return Promise.resolve(undefined) + const promise = input.question.ask({ sessionID: input.sessionID, questions: [ { @@ -323,10 +315,15 @@ export namespace PlanFollowup { }) const listener = () => - PlanFollowupRuntime.question.list().then((qs) => { - const match = qs.find((q) => q.sessionID === input.sessionID) - if (match) PlanFollowupRuntime.question.reject(match.id) - }) + input.question + .list() + .then((qs) => { + const match = qs.find((q) => q.sessionID === input.sessionID) + return match ? input.question.reject(match.id) : undefined + }) + .catch((error) => { + log.warn("failed to reject aborted plan follow-up question", { sessionID: input.sessionID, error }) + }) input.abort.addEventListener("abort", listener, { once: true }) return promise @@ -478,6 +475,7 @@ export namespace PlanFollowup { sessionID: SessionID messages: MessageV2.WithParts[] abort: AbortSignal + question: QuestionRuntime }): Promise<"continue" | "break"> { if (input.abort.aborted) return "break" @@ -491,7 +489,7 @@ export namespace PlanFollowup { const user = latest.find((msg) => msg.info.role === "user")?.info if (!user || user.role !== "user" || !user.model) return "break" - const answers = await prompt({ sessionID: input.sessionID, abort: input.abort }) + const answers = await prompt({ sessionID: input.sessionID, abort: input.abort, question: input.question }) if (!answers) { Telemetry.trackPlanFollowup(input.sessionID, "dismissed") return "break" diff --git a/packages/opencode/src/kilocode/session/prompt.ts b/packages/opencode/src/kilocode/session/prompt.ts index 44863ac01a5..29e269e04c7 100644 --- a/packages/opencode/src/kilocode/session/prompt.ts +++ b/packages/opencode/src/kilocode/session/prompt.ts @@ -14,6 +14,7 @@ import { PlanFollowup } from "@/kilocode/plan-followup" import { KiloSession } from "@/kilocode/session" import { KiloSessionMessageOrder } from "@/kilocode/session/message-order" import { Permission } from "@/permission" +import { Question } from "@/question" import { environmentDetails, type EditorContext } from "@/kilocode/editor-context" import { Identifier } from "@/id/id" import { Filesystem } from "@/util/filesystem" @@ -48,6 +49,7 @@ export namespace KiloSessionPrompt { sessionID: SessionID messages: MessageV2.WithParts[] abort: AbortSignal + question: Pick }): Promise<"continue" | "break"> { if (!shouldAskPlanFollowup({ messages: input.messages, abort: input.abort })) return "break" const ask = InstanceState.bind(PlanFollowup.ask) @@ -55,6 +57,16 @@ export namespace KiloSessionPrompt { sessionID: input.sessionID, messages: input.messages, abort: input.abort, + // Keep the request in the listener-local Question service so HTTP replies can resolve it. + question: { + ask: InstanceState.bind((request: Parameters[0]) => + Effect.runPromise(input.question.ask(request)), + ), + list: InstanceState.bind(() => Effect.runPromise(input.question.list())), + reject: InstanceState.bind((requestID: Parameters[0]) => + Effect.runPromise(input.question.reject(requestID)), + ), + }, }) return action === "continue" ? "continue" : "break" } diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index bf640322cc9..3dc5f453f49 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -1544,7 +1544,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the KiloSessionPrompt.shouldAskPlanFollowup({ messages: msgs, abort: AbortSignal.any([]) }) ) { const action = yield* Effect.promise((signal) => - KiloSessionPrompt.askPlanFollowup({ sessionID, messages: msgs, abort: signal }), + KiloSessionPrompt.askPlanFollowup({ sessionID, messages: msgs, abort: signal, question }), ) if (action === "continue") continue yield* slog.info("exiting loop") @@ -1561,7 +1561,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the ) { // kilocode_change start - ask follow-up when plan_exit tool was called const action = yield* Effect.promise((signal) => - KiloSessionPrompt.askPlanFollowup({ sessionID, messages: msgs, abort: signal }), + KiloSessionPrompt.askPlanFollowup({ sessionID, messages: msgs, abort: signal, question }), ) if (action === "continue") continue // kilocode_change end diff --git a/packages/opencode/test/kilocode/plan-exit-detection.test.ts b/packages/opencode/test/kilocode/plan-exit-detection.test.ts index a8bd82fa2e4..2452df74727 100644 --- a/packages/opencode/test/kilocode/plan-exit-detection.test.ts +++ b/packages/opencode/test/kilocode/plan-exit-detection.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test" +import { AsyncResource } from "async_hooks" import { Effect } from "effect" import fs from "fs/promises" import path from "path" @@ -7,7 +8,10 @@ import { SessionID, MessageID, PartID } from "../../src/session/schema" import { ModelID, ProviderID } from "../../src/provider/schema" import { Instance } from "../../src/project/instance" import { WithInstance } from "../../src/project/with-instance" -import { PlanFollowup, PlanFollowupRuntime } from "../../src/kilocode/plan-followup" +import { PlanFollowup } from "../../src/kilocode/plan-followup" +import { KiloSessionPrompt } from "../../src/kilocode/session/prompt" +import { makeRuntime } from "../../src/effect/run-service" +import { Question } from "../../src/question" import { Session } from "../../src/session/session" import { MessageV2 } from "../../src/session/message-v2" import { SessionPrompt } from "../../src/session/prompt" @@ -29,6 +33,22 @@ const sessions = { Effect.runPromise(Session.Service.use((svc) => svc.updatePart(part)).pipe(Effect.provide(Session.defaultLayer))), } +const runtime = makeRuntime(Question.Service, Question.defaultLayer) +const questions = { + ask(input: Parameters[0]) { + return runtime.runPromise((svc) => svc.ask(input)) + }, + list() { + return runtime.runPromise((svc) => svc.list()) + }, + reject(requestID: Parameters[0]) { + return runtime.runPromise((svc) => svc.reject(requestID)) + }, + reply(input: Parameters[0]) { + return runtime.runPromise((svc) => svc.reply(input)) + }, +} + const model = { providerID: ProviderID.make("openai"), modelID: ModelID.make("gpt-4"), @@ -122,7 +142,7 @@ async function seed(input: { async function waitQuestion(sessionID: string) { for (let i = 0; i < 50; i++) { - const list = await PlanFollowupRuntime.question.list() + const list = await questions.list() const question = list.find((item) => item.sessionID === sessionID) if (question) return question await Bun.sleep(10) @@ -145,6 +165,7 @@ describe("plan_exit detection", () => { expect(SessionPrompt.shouldAskPlanFollowup({ messages: seeded.messages, abort: AbortSignal.any([]) })).toBe(true) const pending = PlanFollowup.ask({ + question: questions, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -154,10 +175,126 @@ describe("plan_exit detection", () => { expect(question).toBeDefined() if (!question) return expect(question.questions[0].header).toBe("Implement") - await PlanFollowupRuntime.question.reject(question.id) + await questions.reject(question.id) await expect(pending).resolves.toBe("break") })) + test("KiloSessionPrompt resolves plan follow-up through the supplied question service", () => + withInstance(async () => { + const seeded = await seed({ + text: "Here is the plan", + tools: [ + { + tool: "plan_exit", + input: {}, + output: "Plan is ready. Ending planning turn.", + }, + ], + }) + + const result = await Effect.runPromise( + Effect.gen(function* () { + const question = yield* Question.Service + const pending = KiloSessionPrompt.askPlanFollowup({ + sessionID: seeded.sessionID, + messages: seeded.messages, + abort: AbortSignal.any([]), + question, + }) + const item = yield* Effect.gen(function* () { + for (let i = 0; i < 50; i++) { + const request = (yield* question.list()).find((entry) => entry.sessionID === seeded.sessionID) + if (request) return request + yield* Effect.sleep("10 millis") + } + throw new Error("timed out waiting for listener-local plan follow-up question") + }) + yield* question.reply({ requestID: item.id, answers: [[PlanFollowup.ANSWER_CONTINUE]] }) + return yield* Effect.promise(() => pending) + }).pipe(Effect.provide(Question.defaultLayer)), + ) + + expect(result).toBe("continue") + })) + + test("KiloSessionPrompt cleans listener-local plan follow-up when aborted outside instance context", () => { + const outside = new AsyncResource("plan-followup-abort-test") + return withInstance(async () => { + const seeded = await seed({ + text: "Here is the plan", + tools: [ + { + tool: "plan_exit", + input: {}, + output: "Plan is ready. Ending planning turn.", + }, + ], + }) + + const result = await Effect.runPromise( + Effect.gen(function* () { + const question = yield* Question.Service + const abort = new AbortController() + const pending = KiloSessionPrompt.askPlanFollowup({ + sessionID: seeded.sessionID, + messages: seeded.messages, + abort: abort.signal, + question, + }) + yield* Effect.gen(function* () { + for (let i = 0; i < 50; i++) { + const request = (yield* question.list()).find((entry) => entry.sessionID === seeded.sessionID) + if (request) return request + yield* Effect.sleep("10 millis") + } + throw new Error("timed out waiting for listener-local plan follow-up question") + }) + outside.runInAsyncScope(() => abort.abort()) + const action = yield* Effect.promise(() => + Promise.race([pending, Bun.sleep(1_000).then(() => "timeout" as const)]), + ) + expect(yield* question.list()).toEqual([]) + return action + }).pipe(Effect.provide(Question.defaultLayer)), + ) + + expect(result).toBe("break") + }).finally(() => outside.emitDestroy()) + }) + + test("PlanFollowup skips prompt when aborted while resolving the plan", () => + withInstance(async () => { + const seeded = await seed({ + text: "Here is the plan", + tools: [ + { + tool: "plan_exit", + input: {}, + output: "Plan is ready. Ending planning turn.", + }, + ], + }) + const abort = new AbortController() + const pending = PlanFollowup.ask({ + question: questions, + sessionID: seeded.sessionID, + messages: seeded.messages, + abort: abort.signal, + }) + abort.abort() + + const result = await Promise.race([pending, Bun.sleep(1_000).then(() => "timeout" as const)]) + const list = () => questions.list().then((qs) => qs.filter((q) => q.sessionID === seeded.sessionID)) + try { + expect(result).toBe("break") + expect(await list()).toEqual([]) + } finally { + for (const item of await list()) { + await questions.reject(item.id) + } + } + })) + test("JetBrains client enables plan follow-up with custom answer", () => withInstance(async () => { const prev = process.env.KILO_CLIENT @@ -179,6 +316,7 @@ describe("plan_exit detection", () => { ) const pending = PlanFollowup.ask({ + question: questions, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -197,7 +335,7 @@ describe("plan_exit detection", () => { expect(question.questions[0].options.find((item) => item.label === PlanFollowup.ANSWER_CONTINUE)?.mode).toBe( "code", ) - await PlanFollowupRuntime.question.reject(question.id) + await questions.reject(question.id) await expect(pending).resolves.toBe("break") } finally { if (prev === undefined) delete process.env.KILO_CLIENT @@ -219,6 +357,7 @@ describe("plan_exit detection", () => { }) const pending = PlanFollowup.ask({ + question: questions, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -227,7 +366,7 @@ describe("plan_exit detection", () => { const question = await waitQuestion(seeded.sessionID) expect(question).toBeDefined() if (!question) return - await PlanFollowupRuntime.question.reply({ + await questions.reply({ requestID: question.id, answers: [[PlanFollowup.ANSWER_CONTINUE]], }) @@ -250,7 +389,7 @@ describe("plan_exit detection", () => { text: "Here is a partial plan, I have questions", }) expect(SessionPrompt.shouldAskPlanFollowup({ messages: seeded.messages, abort: AbortSignal.any([]) })).toBe(false) - const list = await PlanFollowupRuntime.question.list() + const list = await questions.list() expect(list).toHaveLength(0) })) @@ -329,7 +468,7 @@ describe("plan_exit detection", () => { expect(SessionPrompt.shouldAskPlanFollowup({ messages, abort: AbortSignal.any([]) })).toBe(false) // Confirm no questions were posted - const list = await PlanFollowupRuntime.question.list() + const list = await questions.list() expect(list).toHaveLength(0) })) @@ -435,6 +574,7 @@ describe("plan_exit detection", () => { await Bun.write(plan, "Do implementation step 1") const pending = PlanFollowup.ask({ + question: questions, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -443,7 +583,7 @@ describe("plan_exit detection", () => { const question = await waitQuestion(seeded.sessionID) expect(question).toBeDefined() if (!question) return - await PlanFollowupRuntime.question.reply({ + await questions.reply({ requestID: question.id, answers: [[PlanFollowup.ANSWER_CONTINUE]], }) @@ -537,6 +677,7 @@ describe("plan_exit detection", () => { // PlanFollowup.ask should find plan text from the earlier assistant and show prompt const pending = PlanFollowup.ask({ + question: questions, sessionID: session.id, messages, abort: AbortSignal.any([]), @@ -546,7 +687,7 @@ describe("plan_exit detection", () => { expect(question).toBeDefined() if (!question) return expect(question.questions[0].header).toBe("Implement") - await PlanFollowupRuntime.question.reply({ + await questions.reply({ requestID: question.id, answers: [[PlanFollowup.ANSWER_CONTINUE]], }) diff --git a/packages/opencode/test/kilocode/plan-followup.test.ts b/packages/opencode/test/kilocode/plan-followup.test.ts index 222d48ea6ef..c47da02b166 100644 --- a/packages/opencode/test/kilocode/plan-followup.test.ts +++ b/packages/opencode/test/kilocode/plan-followup.test.ts @@ -14,6 +14,7 @@ import { Question } from "../../src/question" import { Session } from "../../src/session/session" import { MessageV2 } from "../../src/session/message-v2" import { AppRuntime } from "../../src/effect/app-runtime" +import { makeRuntime } from "../../src/effect/run-service" import { SessionStatus } from "../../src/session/status" import { Todo } from "../../src/session/todo" import { Global } from "@opencode-ai/core/global" @@ -25,15 +26,19 @@ import { tmpdir } from "../fixture/fixture" Log.init({ print: false }) process.env.KILO_CLIENT = "cli" +const runtime = makeRuntime(Question.Service, Question.defaultLayer) const question = { + ask(input: Parameters[0]) { + return runtime.runPromise((svc) => svc.ask(input)) + }, list() { - return AppRuntime.runPromise(Question.Service.use((svc) => svc.list())) + return runtime.runPromise((svc) => svc.list()) }, reply(input: Parameters[0]) { - return AppRuntime.runPromise(Question.Service.use((svc) => svc.reply(input))) + return runtime.runPromise((svc) => svc.reply(input)) }, reject(requestID: Parameters[0]) { - return AppRuntime.runPromise(Question.Service.use((svc) => svc.reject(requestID))) + return runtime.runPromise((svc) => svc.reject(requestID)) }, } @@ -266,6 +271,7 @@ describe("plan follow-up", () => { withInstance(async () => { const seeded = await seed({ text: "1. Step one\n2. Step two" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -283,6 +289,7 @@ describe("plan follow-up", () => { withInstance(async () => { const seeded = await seed({ text: "1. Build" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -312,6 +319,7 @@ describe("plan follow-up", () => { withInstance(async () => { const seeded = await seed({ text: "1. Build" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -343,6 +351,7 @@ describe("plan follow-up", () => { process.env.KILO_CLIENT = "vscode" const seeded = await seed({ text: "1. Build" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -370,6 +379,7 @@ describe("plan follow-up", () => { withInstance(async () => { const seeded = await seed({ text: "1. Build" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -428,6 +438,7 @@ describe("plan follow-up", () => { } const seeded = await seed({ text: "1. Build\n2. Test" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -460,6 +471,7 @@ describe("plan follow-up", () => { withInstance(async () => { const seeded = await seed({ text: "1. Build\n2. Test" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -497,6 +509,7 @@ describe("plan follow-up", () => { KiloSessionPromptQueue.retarget(seeded.sessionID, original) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -613,6 +626,7 @@ describe("plan follow-up", () => { }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -719,6 +733,7 @@ describe("plan follow-up", () => { fn: async () => sessions(), }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -795,6 +810,7 @@ describe("plan follow-up", () => { } const seeded = await seed({ text: "1. Build\n2. Test" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -847,6 +863,7 @@ describe("plan follow-up", () => { } const seeded = await seed({ text: "1. Build\n2. Test" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -882,6 +899,7 @@ describe("plan follow-up", () => { } const seeded = await seed({ text: "1. Build\n2. Test", variant: planVar }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -942,6 +960,7 @@ describe("plan follow-up", () => { }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -1031,6 +1050,7 @@ describe("plan follow-up", () => { } const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -1107,6 +1127,7 @@ describe("plan follow-up", () => { } const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -1205,6 +1226,7 @@ describe("plan follow-up", () => { } const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -1246,6 +1268,7 @@ describe("plan follow-up", () => { withInstance(async () => { const seeded = await seed({ text: " " }) const result = await PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), @@ -1261,6 +1284,7 @@ describe("plan follow-up", () => { abort.abort() const result = await PlanFollowup.ask({ + question, sessionID: SessionID.make("ses_test"), messages: [], abort: abort.signal, @@ -1274,6 +1298,7 @@ describe("plan follow-up", () => { const abort = new AbortController() const seeded = await seed({ text: "1. Step one\n2. Step two" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: abort.signal, @@ -1293,6 +1318,7 @@ describe("plan follow-up", () => { withInstance(async () => { const seeded = await seed({ text: "1. Build\n2. Test" }) const pending = PlanFollowup.ask({ + question, sessionID: seeded.sessionID, messages: seeded.messages, abort: AbortSignal.any([]), diff --git a/script/check-opencode-promise-facades.ts b/script/check-opencode-promise-facades.ts index 2508ddb7d42..44e0742f0c4 100644 --- a/script/check-opencode-promise-facades.ts +++ b/script/check-opencode-promise-facades.ts @@ -37,7 +37,7 @@ const testAllow: Record = { "effect/app-runtime-logger.test.ts": { count: 6, reason: "tests AppRuntime behavior" }, "kilocode/config-resilience.test.ts": { count: 4, reason: "existing runtime integration test" }, "kilocode/config-validation.test.ts": { count: 2, reason: "existing runtime integration test" }, - "kilocode/plan-followup.test.ts": { count: 7, reason: "existing runtime integration test" }, + "kilocode/plan-followup.test.ts": { count: 4, reason: "existing runtime integration test" }, "kilocode/server/config-overlay.test.ts": { count: 3, reason: "server config cache integration test" }, "kilocode/session/platform-attribution.test.ts": { count: 5, reason: "existing runtime integration test" }, "kilocode/session-prompt-queue.test.ts": { count: 5, reason: "prompt queue legacy instance bridge regression" },