diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1e55758e9..12e66bead 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixed +- Fixed plan approval applying the wrong execution model when the model-tier slider stayed on the active plan model; the selection is now compared against the pre-plan model that exit would restore, so choosing the planning tier is retained. ([#3554](https://github.com/can1357/oh-my-pi/issues/3554)) + - Fixed the `eval` Julia kernel showing only runner-internal backtrace frames (`at top-level scope (./none:N)`, `at main (…runner-…jl:635)`) with no exception type or message, making cell errors undebuggable. The host renderer (`packages/coding-agent/src/eval/kernel-base.ts`) displays a non-empty `traceback` verbatim and only falls back to `ename: evalue` when it is empty; the Python and Ruby runners embed the rendered error in `traceback`, but the Julia runner (`packages/coding-agent/src/eval/jl/runner.jl`) built `traceback` from stack frames only, so the message was dropped. `emit_error` now seeds `traceback` with the `showerror` output (matching the REPL's `ERROR:` text) ahead of the frames. ### Fixed diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index ab1fab100..5084028c9 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -3021,16 +3021,17 @@ export class InteractiveMode implements InteractiveModeContext { // Capture the operator's tier choice and hand it to #approvePlan, which // applies it AFTER #exitPlanMode. #exitPlanMode normally restores // #planModePreviousModelState (the model from before plan mode), so - // applying the slider choice any earlier would be silently reverted — - // the bug that made "continue with slow" keep executing on the default - // model. For compact-context approval, the plan model is kept through - // compaction, then a successful compaction transitions to the slider model - // (or restores the pre-plan model when no slider choice was made). - // `cycle.currentIndex` is exactly that restored model, so any chosen tier - // differing from it needs an explicit executionModel — this also covers - // leaving the slider on its `default` anchor while planning ran elsewhere. + // applying the slider choice any earlier would be silently reverted. + // Treat the choice as implicit only when it matches that restored model; + // comparing against cycle.currentIndex is wrong while plan review is + // still running on the plan model. + const restoredModel = this.#planModePreviousModelState?.model; + const restoredIndex = + cycle && restoredModel + ? cycle.models.findIndex(entry => modelsAreEqual(entry.model, restoredModel)) + : -1; const executionModel = - cycle && selectedTierIndex !== cycle.currentIndex ? cycle.models[selectedTierIndex] : undefined; + cycle && selectedTierIndex !== restoredIndex ? cycle.models[selectedTierIndex] : undefined; await this.#approvePlan(latestPlanContent, { planFilePath, title: details.title, diff --git a/packages/coding-agent/test/interactive-mode-plan-review.test.ts b/packages/coding-agent/test/interactive-mode-plan-review.test.ts index cd5db7850..8f74d8d0b 100644 --- a/packages/coding-agent/test/interactive-mode-plan-review.test.ts +++ b/packages/coding-agent/test/interactive-mode-plan-review.test.ts @@ -743,6 +743,48 @@ describe("InteractiveMode plan review rendering", () => { expect(session.model?.id).toBe(slow.id); }); + it("retains the plan model when the slider selection matches the active plan tier", async () => { + const planModel = session.modelRegistry.find("anthropic", "claude-opus-4-5"); + const prePlanModel = session.modelRegistry.find("anthropic", "claude-sonnet-4-5"); + if (!planModel || !prePlanModel) throw new Error("Expected sonnet + opus to exist in registry"); + + session.settings.setModelRole("default", "anthropic/claude-sonnet-4-5"); + session.settings.setModelRole("slow", "anthropic/claude-opus-4-5"); + session.settings.setModelRole("plan", "anthropic/claude-opus-4-5"); + + const planFilePath = "local://PLAN.md"; + const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, { + getArtifactsDir: () => session.sessionManager.getArtifactsDir(), + getSessionId: () => session.sessionManager.getSessionId(), + }); + await Bun.write(resolvedPlanPath, "# Plan\n\nKeep executing on the planning tier."); + + await mode.handlePlanModeCommand(); + expect(session.model?.id).toBe(planModel.id); + + vi.spyOn(session, "getContextUsage").mockReturnValue(undefined); + vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); + + vi.spyOn(mode, "showPlanReview").mockImplementation( + async (_planContent, _title, _options, _dialogOptions, extra?: { slider?: HookSelectorSlider }) => { + const slider = extra?.slider; + expect(slider).toBeDefined(); + const slowIndex = slider!.segments.findIndex(segment => segment.label === "slow"); + expect(slowIndex).toBeGreaterThanOrEqual(0); + slider!.onChange?.(slowIndex); + return "Approve and keep context"; + }, + ); + + await mode.handlePlanApproval({ + planFilePath, + planExists: true, + title: "PLAN", + }); + + expect(session.model?.id).toBe(planModel.id); + }); + it("compaction runs on the plan model and restores the pre-plan model after success", async () => { const planModel = session.modelRegistry.find("anthropic", "claude-opus-4-5"); const prePlanModel = session.modelRegistry.find("anthropic", "claude-sonnet-4-5"); @@ -825,10 +867,8 @@ describe("InteractiveMode plan review rendering", () => { if (!planModel || !execModel) throw new Error("Expected sonnet + opus to exist in registry"); // Plan model (opus) differs from the execution tier the operator slides to - // (default = sonnet) so the assertions distinguish the new defer-restore + - // success-gated transition from the old "restore pre-plan before compaction" - // path: under the old behavior compaction would have run on sonnet and the - // restore (not applyRoleModel) would have produced the final model. + // (default = sonnet). Successful compaction must keep running on opus, then + // end on the slider-selected default tier. session.settings.setModelRole("default", "anthropic/claude-sonnet-4-5"); session.settings.setModelRole("slow", "anthropic/claude-opus-4-5"); session.settings.setModelRole("plan", "anthropic/claude-opus-4-5"); @@ -851,7 +891,6 @@ describe("InteractiveMode plan review rendering", () => { compactModelId = session.model?.id; return "ok"; }); - const applyRoleSpy = vi.spyOn(session, "applyRoleModel"); vi.spyOn(mode, "showPlanReview").mockImplementation( async (_planContent, _title, _options, _dialogOptions, extra?: { slider?: HookSelectorSlider }) => { @@ -871,12 +910,9 @@ describe("InteractiveMode plan review rendering", () => { title: "PLAN", }); - // Compaction ran on the plan model (defer-restore kept it warm) … + // Compaction ran on the plan model (defer-restore kept it warm), then the + // successful transition ended on the slider-selected default tier. expect(compactModelId).toBe(planModel.id); - // … and the slider-selected execution tier was applied via applyRoleModel - // (the executionModel branch, not the pre-plan restore which goes through - // setModelTemporary), only after the successful compaction. - expect(applyRoleSpy.mock.calls.some(call => call[0]?.model?.id === execModel.id)).toBe(true); expect(session.model?.id).toBe(execModel.id); });