diff --git a/plugins/provider-pi/src/bridge/bridge.settings.test.ts b/plugins/provider-pi/src/bridge/bridge.settings.test.ts index 7bce6663cf..5ef5084afb 100644 --- a/plugins/provider-pi/src/bridge/bridge.settings.test.ts +++ b/plugins/provider-pi/src/bridge/bridge.settings.test.ts @@ -60,7 +60,8 @@ it("pins model and thinking at spawn and never sends set_model or set_thinking_l options: OPTIONS, }); let seen = await harness.waitForTurnBoundary(threadId, 0); - // A turn with different options: still no settings-writing command. + // A turn with different options rebuilds the session on the new spawn + // flags (#2160): still no settings-writing command. await harness.request(3, "turn/start", { threadId, providerThreadId: threadId, @@ -92,13 +93,14 @@ it("pins model and thinking at spawn and never sends set_model or set_thinking_l const sent = commandsSent(); expect(sent.length).toBeGreaterThan(0); expect(sent.filter((c) => c === "set_model" || c === "set_thinking_level")).toEqual([]); - // The pinned model reached pi as the spawn flag: the session runs - // fake-mini (32k context), not the fake's default fake-model (200k). + // Both selections reached pi as spawn flags: the first turn ran on the + // pinned fake-mini (32k context) rather than the fake's default + // fake-model, and the turn that picked fake-model ran on its 200k window. const contextWindows = harness.messages .filter((m) => m.method === "thread/delta") .flatMap((m) => (m.params as { deltas: { kind: string; size?: number }[] }).deltas) .filter((d) => d.kind === "contextWindow") .map((d) => d.size); - expect(contextWindows.length).toBeGreaterThan(0); - expect(new Set(contextWindows)).toEqual(new Set([32_000])); + expect(contextWindows[0]).toBe(32_000); + expect(contextWindows.at(-1)).toBe(200_000); }, SETTINGS_PROCESS_TEST_TIMEOUT_MS); diff --git a/plugins/provider-pi/src/bridge/bridge.ts b/plugins/provider-pi/src/bridge/bridge.ts index c298569127..742a538c50 100644 --- a/plugins/provider-pi/src/bridge/bridge.ts +++ b/plugins/provider-pi/src/bridge/bridge.ts @@ -54,6 +54,7 @@ import type { ImageContent } from "@earendil-works/pi-ai"; import { createPiDeltaTranslator } from "../delta-translation.js"; import { buildPiSessionParams, + buildPiTurnOptions, type PiSessionParams, } from "../session-params.js"; import { BB_PI_EXTENSION_SOURCE } from "./bb-pi-extension.js"; @@ -186,6 +187,14 @@ interface ThreadSession { providerThreadId: string; /** The working directory pi runs this thread in. */ cwd: string; + /** + * The params this thread's pi child was constructed from. A turn that + * carries a different model or thinking level is reconciled against them + * and rebuilds the child from the same base. + */ + construction: PiSessionParams; + /** The model the child spawned on, resolved against pi's catalog. */ + constructionModel: { provider: string; id: string } | undefined; } let sessionSerialCounter = 0; @@ -719,21 +728,25 @@ async function buildSessionOptions(args: { }; } -async function startPiThreadSession( +/** + * Construct one pi child for a thread, register it, and start it. The + * registration precedes `start()` so the child's own startup events reach + * the thread; a start that fails takes the registration back out, so the + * caller decides what serves the thread next. + */ +async function constructPiThreadSession( threadId: string, providerThreadId: string, params: PiSessionParams, -): Promise { - const existing = sessions.get(threadId); - if (existing) { - await closeThreadSession({ - message: "Pi thread session replaced while tool call was pending", - threadId, - }); - } +): Promise { const sessionSerial = nextSessionSerial(); + const sessionOptions = await buildSessionOptions({ + params, + providerThreadId, + threadId, + }); const session = new PiRpcSession( - await buildSessionOptions({ params, providerThreadId, threadId }), + sessionOptions, createForwardToolCall(() => threadId), createOnPiEvent({ sessionSerial, threadId }), createOnSessionDone({ sessionSerial, threadId }), @@ -747,6 +760,8 @@ async function startPiThreadSession( // pi's SessionManager.open resolves the cwd from the header. A fresh // session has no file yet and runs in the requested cwd. cwd: persistedSessionCwd(providerThreadId) ?? params.cwd, + construction: params, + constructionModel: sessionOptions.model, }; sessions.set(threadId, threadSession); try { @@ -757,6 +772,7 @@ async function startPiThreadSession( { id: liveModel.id, provider: liveModel.provider, contextWindow: liveModel.contextWindow }, ]); } + return threadSession; } catch (error) { if (sessions.get(threadId) === threadSession) { sessions.delete(threadId); @@ -766,14 +782,97 @@ async function startPiThreadSession( } } +async function startPiThreadSession( + threadId: string, + providerThreadId: string, + params: PiSessionParams, +): Promise { + const existing = sessions.get(threadId); + if (existing) { + await closeThreadSession({ + message: "Pi thread session replaced while tool call was pending", + threadId, + }); + } + await constructPiThreadSession(threadId, providerThreadId, params); +} + +/** + * Retire the child a verified replacement took over from. It is idle and no + * longer registered, so nothing waits on its close: holding the turn for the + * old child's abort/leaf exchange would spend the runtime's request budget + * on a session that no longer serves anything. + */ +function retireReplacedPiChild(replaced: ThreadSession): void { + replaced.closing = true; + resolvePendingToolCalls( + replaced, + "Pi thread session replaced while tool call was pending", + ); + void replaced.session + .closeGracefully(THREAD_STOP_CLOSE_TIMEOUT_MS) + .catch(() => undefined); +} + +/** + * Swap a thread's pi child for one built from `params`, keeping the thread + * servable if the replacement never starts. + * + * The replacement is constructed, started and verified (`start()` answers + * `get_state`, waits for the extension's `ready`, and refuses a child that + * came up on another model) BEFORE the child it replaces is closed. Two pi + * children may hold one session file: the file is append-only, the outgoing + * child is idle, and pi takes no lock (verified against pi 0.84.3). So a + * replacement that dies at spawn — an unstartable `provider/id`, a + * readiness timeout, a crash — costs the turn and nothing else: the + * previous child never stopped serving, and the next turn runs on it. + * + * Closing first is what stranded the thread: `sessions` had no entry, every + * later turn answered "No active pi session", and the runtime still held the + * thread, so nothing resumed it (#2221 review). + */ +async function rebuildThreadSession( + threadId: string, + previous: ThreadSession, + params: PiSessionParams, +): Promise { + let replacement: ThreadSession; + try { + replacement = await constructPiThreadSession( + threadId, + previous.providerThreadId, + params, + ); + } catch (error) { + // The failed construction removed its own registration. Put the child + // that is still alive back in front of the thread, unless something + // else (a resume, a discard) already claimed it. + if (!sessions.has(threadId) && !previous.closing) { + sessions.set(threadId, previous); + } + throw error; + } + retireReplacedPiChild(previous); + return replacement; +} + +/** + * The native id-space boundary a newly constructed pi child opens: its turn + * and item ids may repeat, so the thread's assembly state is dropped on both + * sides of the wire. + */ +function sendSessionResetBoundary(threadId: string): void { + piDeltaTranslator.resetThread(threadId); + sendThreadDeltas(threadId, [{ kind: "session.reset" }]); +} + function sendThreadSessionResult( id: string | number, threadId: string, providerThreadId: string, ): void { sendThreadIdentity(threadId, providerThreadId); - piDeltaTranslator.resetThread(threadId); - sendThreadDeltas(threadId, [{ kind: "session.reset" }]); + sendSessionResetBoundary(threadId); sendResult(id, { providerThreadId, sessionRestorable: true }); } @@ -884,12 +983,103 @@ function recordAcceptedTurnInput(params: TurnStartParams): void { ]); } +/** + * Reconcile the execution options a turn carries with the live session before + * its input is dispatched (#2160). The runtime never diffs options: they ride + * every turn command and each bridge applies what changed. Model and thinking + * level are spawn-time flags for pi, and `set_model` / `set_thinking_level` + * write the selection into the user's global pi settings, which bb must not + * touch — so a change is applied by rebuilding the child from the thread's + * session file, the way the codex bridge rebuilds from its rollout. The + * history is on disk, so the conversation survives the replacement. + * + * Nothing is in flight at this point: the daemon steers an active thread + * instead of starting a turn on it, and a steer joins the turn that is + * already running on the model it started with. Reconciliation runs ahead of + * every dispatch, including manual compaction, so the summarization request + * also goes to the selected model. A selection the rebuild cannot serve — + * a model that does not resolve, a child that will not start on it — fails + * the turn alone: `rebuildThreadSession` keeps the live child until a + * replacement is verified, so the thread stays servable either way. + */ +async function reconcileTurnOptions( + threadId: string, + threadSession: ThreadSession, + options: TurnStartParams["options"], +): Promise { + const turnOptions = buildPiTurnOptions(options); + const construction = threadSession.construction; + // The request the construction ran with is the cheap comparison: an + // unchanged spelling settles the common turn without touching pi's + // catalog. Pi clamps a level the model does not support at spawn, so + // comparing requests (not the clamped result) also keeps a clamped + // session from rebuilding on every turn. + const changedModelRequest = + turnOptions.model !== undefined && turnOptions.model !== construction.model + ? turnOptions.model + : undefined; + const thinkingLevelChanged = + turnOptions.thinkingLevel !== undefined && + turnOptions.thinkingLevel !== construction.thinkingLevel; + if (changedModelRequest === undefined && !thinkingLevelChanged) { + return threadSession; + } + // Resolved before anything is torn down: a model that does not resolve + // fails the turn with the live session intact, and two spellings of one + // model ("fake-model", "fake-provider/fake-model") rebuild nothing. + const nextModel = + changedModelRequest === undefined + ? undefined + : await resolvePiModel(changedModelRequest, construction.cwd); + const modelChanged = + nextModel !== undefined && + (threadSession.constructionModel === undefined || + threadSession.constructionModel.provider !== nextModel.provider || + threadSession.constructionModel.id !== nextModel.id); + if (!modelChanged && !thinkingLevelChanged) { + return threadSession; + } + const replacement = await rebuildThreadSession(threadId, threadSession, { + ...construction, + ...(turnOptions.model === undefined ? {} : { model: turnOptions.model }), + ...(turnOptions.thinkingLevel === undefined + ? {} + : { thinkingLevel: turnOptions.thinkingLevel }), + }); + // A replacement session re-reports its identity and restorability, and + // its boundary deltas go out before the notification that explains them. + sendThreadIdentity(threadId, replacement.providerThreadId); + sendSessionResetBoundary(threadId); + send({ + jsonrpc: "2.0", + method: BRIDGE_NOTIFICATION_METHODS.sessionReplaced, + params: { + threadId, + providerThreadId: replacement.providerThreadId, + reason: "Execution settings changed; the pi session was rebuilt to apply them.", + contextLost: false, + }, + }); + return replacement; +} + async function handleTurnStart(id: string | number, params: TurnStartParams): Promise { - const threadSession = sessions.get(params.threadId); - if (!threadSession || threadSession.closing) { + const liveSession = sessions.get(params.threadId); + if (!liveSession || liveSession.closing) { sendError(id, -32000, "No active pi session"); return; } + let threadSession: ThreadSession; + try { + threadSession = await reconcileTurnOptions( + params.threadId, + liveSession, + params.options, + ); + } catch (error) { + sendError(id, -32000, error instanceof Error ? error.message : String(error)); + return; + } if (isStandaloneBuiltinCompactCommand(params.input)) { recordAcceptedTurnInput(params); startPiCompaction(threadSession, params.threadId); diff --git a/plugins/provider-pi/src/bridge/bridge.turn-options.test.ts b/plugins/provider-pi/src/bridge/bridge.turn-options.test.ts new file mode 100644 index 0000000000..4f5b9b9bd8 --- /dev/null +++ b/plugins/provider-pi/src/bridge/bridge.turn-options.test.ts @@ -0,0 +1,260 @@ +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { z } from "zod"; +import type { + BridgeJsonRpcObject, + BridgeJsonRpcOutputMessage, +} from "@get-bb/plugin-sdk/provider-bridge/testing"; +import { + FULL_PERMISSION_OPTIONS, + type FakePiBridgeHarness, + startFakePiBridge, +} from "./test-support.js"; + +/** + * Execution options ride every turn command and the runtime never diffs + * them, so each bridge reconciles them itself (#2160). Pi takes the model and + * the thinking level as spawn flags, so a turn that carries different ones + * rebuilds the child from the thread's session file and says so with + * `session/replaced`; a turn that carries the live ones leaves it alone. + */ + +/** A rebuild starts a second real pi child; match the process-test budget. */ +const TURN_OPTIONS_TEST_TIMEOUT_MS = 60_000; + +/** The fake's small model: a 32k context window against fake-model's 200k. */ +const MINI = { + ...FULL_PERMISSION_OPTIONS, + model: "fake-provider/fake-mini", + reasoningLevel: "medium", +}; +const FULL_MODEL = { ...MINI, model: "fake-provider/fake-model" }; + +const sessionReplacedParamsSchema = z.object({ + threadId: z.string(), + providerThreadId: z.string().nullable(), + reason: z.string(), + contextLost: z.boolean(), +}); + +let harness: FakePiBridgeHarness; + +beforeEach(async () => { + harness = await startFakePiBridge({ + prefix: "bb-pi-turn-options-", + initialize: true, + }); +}, 30_000); + +afterEach(async () => { + await harness.teardown(); +}, 30_000); + +function sessionReplacements( + threadId: string, +): z.infer[] { + return harness.messages + .filter((message) => message.method === "session/replaced") + .map((message) => sessionReplacedParamsSchema.parse(message.params)) + .filter((params) => params.threadId === threadId); +} + +/** The context window each turn reported: the fake reports its model's. */ +function contextWindowSizes(threadId: string): number[] { + return harness + .deltasOf(threadId) + .filter((delta) => delta.kind === "contextWindow") + .map((delta) => delta.size) + .filter((size): size is number => typeof size === "number"); +} + +function turnStart( + id: number, + threadId: string, + text: string, + options: BridgeJsonRpcObject, +): Promise { + return harness.request(id, "turn/start", { + threadId, + providerThreadId: threadId, + clientRequestId: `creq_abcdefghi${"23456789"[id % 8] ?? "2"}`, + input: [{ type: "text", text, mentions: [] }], + options, + }); +} + +it("rebuilds the session on the model a later turn carries", async () => { + const threadId = "thr_turn_options_model"; + await harness.startThread(threadId, { options: MINI }); + + expect((await turnStart(1, threadId, "first", MINI)).error).toBeUndefined(); + let seen = await harness.waitForTurnBoundary(threadId, 0); + // The turn carried the construction options, so it rebuilt nothing and + // ran on the pinned fake-mini. + expect(contextWindowSizes(threadId)).toEqual([32_000]); + expect(sessionReplacements(threadId)).toEqual([]); + + // The user picks another model in the composer. The runtime never diffs + // options: the change only rides the next turn command. + expect( + (await turnStart(2, threadId, "second", FULL_MODEL)).error, + ).toBeUndefined(); + seen = await harness.waitForTurnBoundary(threadId, seen); + + expect(contextWindowSizes(threadId).at(-1)).toBe(200_000); + expect(sessionReplacements(threadId)).toEqual([ + { + threadId, + providerThreadId: threadId, + reason: expect.stringContaining("Execution settings changed"), + contextLost: false, + }, + ]); + // The replacement opens a new native id space: the construction reset and + // the rebuild's reset are both on the wire. + expect( + harness.deltasOf(threadId).filter((delta) => delta.kind === "session.reset"), + ).toHaveLength(2); + + // A third turn on the options the rebuild applied changes nothing. + expect( + (await turnStart(3, threadId, "third", FULL_MODEL)).error, + ).toBeUndefined(); + await harness.waitForTurnBoundary(threadId, seen); + expect(sessionReplacements(threadId)).toHaveLength(1); +}, TURN_OPTIONS_TEST_TIMEOUT_MS); + +it("rebuilds the session on the reasoning level a later turn carries", async () => { + const threadId = "thr_turn_options_level"; + await harness.startThread(threadId, { options: MINI }); + + expect((await turnStart(1, threadId, "first", MINI)).error).toBeUndefined(); + const seen = await harness.waitForTurnBoundary(threadId, 0); + expect(sessionReplacements(threadId)).toEqual([]); + + expect( + (await turnStart(2, threadId, "second", { ...MINI, reasoningLevel: "high" })) + .error, + ).toBeUndefined(); + await harness.waitForTurnBoundary(threadId, seen); + + expect(sessionReplacements(threadId)).toHaveLength(1); +}, TURN_OPTIONS_TEST_TIMEOUT_MS); + +it("compacts with the model the compaction turn selected", async () => { + const threadId = "thr_turn_options_compact"; + await harness.startThread(threadId, { options: MINI }); + + expect((await turnStart(1, threadId, "first", MINI)).error).toBeUndefined(); + const seen = await harness.waitForTurnBoundary(threadId, 0); + + // A standalone builtin /compact is bb's manual-compaction request, not + // model input. Reconciliation runs ahead of that branch too, so the + // summarization request goes to the model the user selected. + const compaction = await harness.request(2, "turn/start", { + threadId, + providerThreadId: threadId, + clientRequestId: "creq_abcdefghij", + input: [ + { + type: "text", + text: "/compact", + mentions: [ + { + start: 0, + end: 8, + resource: { + kind: "command", + trigger: "/", + name: "compact", + source: "command", + origin: "builtin", + label: "compact", + argumentHint: null, + }, + }, + ], + }, + ], + options: FULL_MODEL, + }); + expect(compaction.result).toMatchObject({ threadId }); + + await harness.waitForDelta( + threadId, + (delta) => delta.kind === "contextWindow" && delta.size === 200_000, + seen, + ); + expect(sessionReplacements(threadId)).toHaveLength(1); +}, TURN_OPTIONS_TEST_TIMEOUT_MS); + +it("fails a turn whose model cannot be resolved and keeps the live session", async () => { + const threadId = "thr_turn_options_bad_model"; + await harness.startThread(threadId, { options: MINI }); + + expect((await turnStart(1, threadId, "first", MINI)).error).toBeUndefined(); + const seen = await harness.waitForTurnBoundary(threadId, 0); + + expect( + (await turnStart(2, threadId, "second", { ...MINI, model: "no-such-model" })) + .error, + ).toMatchObject({ + code: -32000, + message: 'Failed to resolve Pi model "no-such-model"', + }); + expect(sessionReplacements(threadId)).toEqual([]); + + // The session that was live still serves the thread: the next turn runs + // on it, on the model it was constructed with. + expect((await turnStart(3, threadId, "third", MINI)).error).toBeUndefined(); + await harness.waitForTurnBoundary(threadId, seen); + expect(contextWindowSizes(threadId).at(-1)).toBe(32_000); +}, TURN_OPTIONS_TEST_TIMEOUT_MS); + +it("keeps serving the thread when the replacement child never starts", async () => { + const threadId = "thr_turn_options_dead_replacement"; + await harness.startThread(threadId, { options: MINI }); + + expect((await turnStart(1, threadId, "first", MINI)).error).toBeUndefined(); + const seen = await harness.waitForTurnBoundary(threadId, 0); + + // The next child dies at spawn: a `provider/id` pi accepts but cannot run, + // a readiness timeout, a crash. The live child must not be closed for it. + vi.stubEnv("FAKE_PI_EXIT_BEFORE_FIRST_RESPONSE", "1"); + const failed = await turnStart(2, threadId, "second", FULL_MODEL); + vi.stubEnv("FAKE_PI_EXIT_BEFORE_FIRST_RESPONSE", undefined); + + expect(failed.error).toMatchObject({ code: -32000 }); + expect(sessionReplacements(threadId)).toEqual([]); + + // The turn failed, the thread did not. Without keeping the previous child + // this answers "No active pi session" for the rest of the thread's life: + // the runtime still holds the thread, so nothing resumes it. + const recovered = await turnStart(3, threadId, "third", MINI); + expect(recovered.error).toBeUndefined(); + await harness.waitForTurnBoundary(threadId, seen); + expect(contextWindowSizes(threadId).at(-1)).toBe(32_000); + expect(sessionReplacements(threadId)).toEqual([]); +}, TURN_OPTIONS_TEST_TIMEOUT_MS); + +it("steers the running turn without rebuilding on its options", async () => { + const threadId = "thr_turn_options_steer"; + await harness.startThread(threadId, { options: MINI }); + + // `/hold` opens a run that stays live until it is steered or aborted. + expect((await turnStart(1, threadId, "/hold", MINI)).error).toBeUndefined(); + await harness.waitForDelta(threadId, (delta) => delta.kind === "turn.open"); + + // A steer joins the turn already running on the model it started with; + // rebuilding here would kill that run, so a changed option waits for the + // next turn/start. + const steer = await harness.request(2, "turn/steer", { + threadId, + providerThreadId: threadId, + clientRequestId: "creq_abcdefghik", + expectedTurnId: "turn-1", + input: [{ type: "text", text: "steered", mentions: [] }], + options: FULL_MODEL, + }); + expect(steer.result).toMatchObject({ threadId }); + expect(sessionReplacements(threadId)).toEqual([]); +}, TURN_OPTIONS_TEST_TIMEOUT_MS); diff --git a/plugins/provider-pi/src/session-params.ts b/plugins/provider-pi/src/session-params.ts index a0a697915b..0c52f7e597 100644 --- a/plugins/provider-pi/src/session-params.ts +++ b/plugins/provider-pi/src/session-params.ts @@ -75,6 +75,25 @@ export interface PiSessionParams { thinkingLevel?: PiReasoningLevel; } +/** + * The construction-scoped option subset a turn command can change. Every turn + * command carries the full execution options and the runtime never diffs + * them, so the bridge reads these off each turn and reconciles them with the + * session it already holds. `undefined` means the turn named nothing and the + * live value stands. + */ +export interface PiTurnOptions { + model: string | undefined; + thinkingLevel: PiReasoningLevel | undefined; +} + +export function buildPiTurnOptions(options: PiSessionOptions): PiTurnOptions { + return { + model: options.model ? options.model : undefined, + thinkingLevel: toPiThinkingLevel(options.reasoningLevel), + }; +} + export function buildPiSessionParams( args: BuildPiSessionParamsArgs, ): PiSessionParams {