From d8c02f2bb3889e209c5f9daa9b3d84c750db0790 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 17 Jul 2026 03:57:03 +0200 Subject: [PATCH] fix(coding-agent): roll back failed plan model restores --- .../src/modes/interactive-mode.ts | 42 +++++++++---------- .../coding-agent/test/issue-816-repro.test.ts | 25 +++++++++++ 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index 4e5abce7d..36de765db 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -2354,31 +2354,31 @@ export class InteractiveMode implements InteractiveModeContext { if (this.#planModePreviousTools !== undefined) { await this.session.setActiveToolsByName(this.#planModePreviousTools); } + if (this.#planModePreviousModelState) { + if (!options?.deferModelRestore) { + await this.#restorePlanPreviousModel(this.#planModePreviousModelState); + } + // If #applyPlanModeModel queued a deferred switch to the plan-role model + // (because the session was streaming on entry), drop it now: we are + // leaving plan mode, so flushing it on the next agent_end would land the + // session on the plan-role model after the user has exited plan mode + // (issue #816). This runs even when deferModelRestore is set + // (compact-approval path): otherwise the stale plan switch survives and + // flushPendingModelSwitch() later clobbers the restored/execution model. + // Only clear when the pending target matches the plan-role model — leave + // any unrelated user-queued switch intact. + const pending = this.#pendingModelSwitch; + if (pending) { + const planResolution = this.session.resolveRoleModelWithThinking("plan"); + if (planResolution.model && modelsAreEqual(pending.model, planResolution.model)) { + this.#pendingModelSwitch = undefined; + } + } + } } catch (error) { this.session.setPlanModeState(planModeState); throw error; } - if (this.#planModePreviousModelState) { - if (!options?.deferModelRestore) { - await this.#restorePlanPreviousModel(this.#planModePreviousModelState); - } - // If #applyPlanModeModel queued a deferred switch to the plan-role model - // (because the session was streaming on entry), drop it now: we are - // leaving plan mode, so flushing it on the next agent_end would land the - // session on the plan-role model after the user has exited plan mode - // (issue #816). This runs even when deferModelRestore is set - // (compact-approval path): otherwise the stale plan switch survives and - // flushPendingModelSwitch() later clobbers the restored/execution model. - // Only clear when the pending target matches the plan-role model — leave - // any unrelated user-queued switch intact. - const pending = this.#pendingModelSwitch; - if (pending) { - const planResolution = this.session.resolveRoleModelWithThinking("plan"); - if (planResolution.model && modelsAreEqual(pending.model, planResolution.model)) { - this.#pendingModelSwitch = undefined; - } - } - } this.session.setPlanProposalHandler?.(null); this.planModeEnabled = false; // Suppress cache-miss marker on the next turn: plan exit changes the system diff --git a/packages/coding-agent/test/issue-816-repro.test.ts b/packages/coding-agent/test/issue-816-repro.test.ts index b17345354..8a5ecf312 100644 --- a/packages/coding-agent/test/issue-816-repro.test.ts +++ b/packages/coding-agent/test/issue-816-repro.test.ts @@ -94,6 +94,31 @@ describe("issue #816 — plan mode pendingModelSwitch leak", () => { expect(setModelSpy).not.toHaveBeenCalled(); }); + it("keeps plan state coherent when restoring the previous model fails", async () => { + const planModel = modelRegistry.find("anthropic", "claude-haiku-4-5"); + if (!planModel) throw new Error("Expected claude-haiku-4-5 in registry"); + + vi.spyOn(session, "resolveRoleModelWithThinking").mockReturnValue({ + model: planModel, + thinkingLevel: undefined, + explicitThinkingLevel: false, + warning: undefined, + }); + vi.spyOn(session, "sendPlanModeContext").mockResolvedValue(undefined); + const setModelSpy = vi.spyOn(session, "setModelTemporary"); + + await mode.handlePlanModeCommand(); + expect(mode.planModeEnabled).toBe(true); + expect(session.getPlanModeState()?.enabled).toBe(true); + + setModelSpy.mockRejectedValueOnce(new Error("model restore failed")); + vi.spyOn(mode, "showHookConfirm").mockResolvedValue(true); + await expect(mode.handlePlanModeCommand()).rejects.toThrow("model restore failed"); + + expect(mode.planModeEnabled).toBe(true); + expect(session.getPlanModeState()?.enabled).toBe(true); + }); + it("does not enter plan mode when plan.enabled is false", async () => { session.settings.set("plan.enabled", false); const warning = vi.spyOn(mode, "showWarning").mockImplementation(() => {});