diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 5673a5a7d..db8350a5a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -6,6 +6,8 @@ - Fixed MCP OAuth discovery rejecting Atlassian-style cross-host issuer metadata during the resource-server fallback probe; issuer matching now remains enforced for advertised auth-server candidates but no longer blocks fallback metadata where the resource host and authorization-server issuer differ. ([#3551](https://github.com/can1357/oh-my-pi/issues/3551)) +- Fixed plan approval applying the wrong execution model when the model-tier slider sat on the model that exit would restore. The match check now compares the selected role's effective thinking level against the pre-plan thinking level, so picking the active planning tier is retained and picking a same-model tier with an explicit thinking suffix (e.g. `default = sonnet:off` while plan-mode raised thinking to `high`) goes through `applyRoleModel` instead of silently restoring the pre-plan level. ([#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..4ed306b44 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -3021,16 +3021,24 @@ 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 applying the selected role + // would land on the same end state as the restore — same model AND + // the same effective thinking level. A role with an explicit thinking + // suffix that differs from the restored thinking level must still go + // through applyRoleModel, otherwise approving on the same model with a + // different configured thinking level silently keeps the pre-plan level. + const restoredState = this.#planModePreviousModelState; + const restoredIndex = + cycle && restoredState + ? cycle.models.findIndex(entry => { + if (!modelsAreEqual(entry.model, restoredState.model)) return false; + if (!entry.explicitThinkingLevel) return true; + return entry.thinkingLevel === restoredState.thinkingLevel; + }) + : -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..55e0bc2e3 100644 --- a/packages/coding-agent/test/interactive-mode-plan-review.test.ts +++ b/packages/coding-agent/test/interactive-mode-plan-review.test.ts @@ -1,7 +1,7 @@ import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "bun:test"; import * as fs from "node:fs/promises"; import * as path from "node:path"; -import { Agent, AgentBusyError } from "@oh-my-pi/pi-agent-core"; +import { Agent, AgentBusyError, ThinkingLevel } from "@oh-my-pi/pi-agent-core"; import type { AssistantMessage, Usage } from "@oh-my-pi/pi-ai"; import { KeybindingsManager } from "@oh-my-pi/pi-coding-agent/config/keybindings"; import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; @@ -743,6 +743,98 @@ 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("treats matching-model slider tier as explicit when its thinking differs from the pre-plan thinking", async () => { + const sonnet = session.modelRegistry.find("anthropic", "claude-sonnet-4-5"); + const opus = session.modelRegistry.find("anthropic", "claude-opus-4-5"); + if (!sonnet || !opus) throw new Error("Expected sonnet + opus to exist in registry"); + + // default tier explicitly turns thinking off on sonnet; the session enters + // plan mode with thinking already bumped to high. A model-only match check + // treats the slider's "stay on default" pick as implicit, so #exitPlanMode + // restores thinking=high instead of the configured off override. The fix + // must compare thinking levels too and pass the default entry through + // applyRoleModel. + session.settings.setModelRole("default", "anthropic/claude-sonnet-4-5:off"); + session.settings.setModelRole("slow", "anthropic/claude-opus-4-5"); + session.settings.setModelRole("plan", "anthropic/claude-opus-4-5"); + session.setThinkingLevel(ThinkingLevel.High); + + const planFilePath = "local://PLAN.md"; + const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, { + getArtifactsDir: () => session.sessionManager.getArtifactsDir(), + getSessionId: () => session.sessionManager.getSessionId(), + }); + await Bun.write(resolvedPlanPath, "# Plan\n\nDifferent thinking on the same model."); + + await mode.handlePlanModeCommand(); + expect(session.model?.id).toBe(opus.id); + + vi.spyOn(session, "getContextUsage").mockReturnValue(undefined); + vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); + const applyRoleSpy = vi.spyOn(session, "applyRoleModel"); + + vi.spyOn(mode, "showPlanReview").mockImplementation( + async (_planContent, _title, _options, _dialogOptions, extra?: { slider?: HookSelectorSlider }) => { + const slider = extra?.slider; + expect(slider).toBeDefined(); + const defaultIndex = slider!.segments.findIndex(segment => segment.label === "default"); + expect(defaultIndex).toBeGreaterThanOrEqual(0); + slider!.onChange?.(defaultIndex); + return "Approve and keep context"; + }, + ); + + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN" }); + + const defaultApply = applyRoleSpy.mock.calls.find(call => call[0]?.role === "default"); + expect(defaultApply).toBeDefined(); + expect(defaultApply?.[0]?.model.id).toBe(sonnet.id); + expect(defaultApply?.[0]?.thinkingLevel).toBe(ThinkingLevel.Off); + expect(defaultApply?.[0]?.explicitThinkingLevel).toBe(true); + }); + 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 +917,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 +941,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 +960,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); });