From 3765d80cbc9bbac3349cc03554b00db50eed65e1 Mon Sep 17 00:00:00 2001 From: roboomp Date: Sun, 28 Jun 2026 07:04:31 +0000 Subject: [PATCH] fix(coding-agent): fixed alt-m default over context Decoupled default-role persistence from live model switching when the selected model is below the current session context window. Updated the model selector regression coverage so the Alt+M Default action remains selectable and advances to thinking selection. Fixes #3708 --- packages/coding-agent/CHANGELOG.md | 1 + .../src/modes/components/model-selector.ts | 51 +++---------------- .../modes/controllers/selector-controller.ts | 19 +++++-- .../coding-agent/src/session/agent-session.ts | 20 +++++--- .../agent-session-model-persistence.test.ts | 15 ++++++ ...model-selector-role-badge-thinking.test.ts | 17 +++++-- 6 files changed, 62 insertions(+), 61 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a131828ac..3d9a5066f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -20,6 +20,7 @@ ### Fixed +- Fixed Alt+M default-role model configuration being disabled by the current session's context size. ([#3708](https://github.com/can1357/oh-my-pi/issues/3708)) - Fixed interrupted reasoning blocks being incorrectly stripped when they contained a valid signature - Fixed interrupted thinking being lost in LLM provider requests after user interrupts by properly stripping trailing reasoning blocks from assistant turns while preserving them in the UI and session history. - Fixed the live todo HUD going stale during long tool-use loops by introducing a mid-run reconciliation reminder that prompts the agent to update incomplete items. diff --git a/packages/coding-agent/src/modes/components/model-selector.ts b/packages/coding-agent/src/modes/components/model-selector.ts index af91ee3be..df3806488 100644 --- a/packages/coding-agent/src/modes/components/model-selector.ts +++ b/packages/coding-agent/src/modes/components/model-selector.ts @@ -714,10 +714,6 @@ export class ModelSelectorComponent extends Container { return this.#temporaryOnly && this.#isModelOverCurrentContext(model); } - #isDefaultRoleActionOverContextLimit(action: MenuRoleAction, model: Model): boolean { - return action.role === "default" && this.#isModelOverCurrentContext(model); - } - #formatCurrentContextLimitSuffix(model: Model): string { return ` ${theme.status.disabled} context>${formatNumber(model.contextWindow ?? 0).toLowerCase()}`; } @@ -1065,47 +1061,16 @@ export class ModelSelectorComponent extends Container { : this.#filteredModels[this.#selectedIndex]; } - #isMenuRoleIndexDisabled(index: number, selectedItem: ModelItem | CanonicalModelItem): boolean { - const action = this.#menuRoleActions[index]; - return action ? this.#isDefaultRoleActionOverContextLimit(action, selectedItem.model) : false; - } - - #coerceMenuSelectedIndex(index: number, selectedItem: ModelItem | CanonicalModelItem): number { + #coerceMenuSelectedIndex(index: number): number { const maxIndex = this.#menuRoleActions.length - 1; if (maxIndex < 0) { return 0; } - const clamped = Math.max(0, Math.min(index, maxIndex)); - if (!this.#isMenuRoleIndexDisabled(clamped, selectedItem)) { - return clamped; - } - for (let i = clamped + 1; i <= maxIndex; i++) { - if (!this.#isMenuRoleIndexDisabled(i, selectedItem)) { - return i; - } - } - for (let i = clamped - 1; i >= 0; i--) { - if (!this.#isMenuRoleIndexDisabled(i, selectedItem)) { - return i; - } - } - return clamped; + return Math.max(0, Math.min(index, maxIndex)); } - #moveMenuSelection(delta: number, selectedItem: ModelItem | CanonicalModelItem, optionCount: number): void { - let index = this.#menuSelectedIndex; - for (let step = 0; step < optionCount; step++) { - index = (index + delta + optionCount) % optionCount; - if (this.#menuStep !== "role" || !this.#isMenuRoleIndexDisabled(index, selectedItem)) { - this.#menuSelectedIndex = index; - this.#updateMenu(); - return; - } - } - this.#menuSelectedIndex = - this.#menuStep === "role" - ? this.#coerceMenuSelectedIndex(this.#menuSelectedIndex, selectedItem) - : this.#menuSelectedIndex; + #moveMenuSelection(delta: number, _selectedItem: ModelItem | CanonicalModelItem, optionCount: number): void { + this.#menuSelectedIndex = (this.#menuSelectedIndex + delta + optionCount) % optionCount; this.#updateMenu(); } @@ -1116,7 +1081,7 @@ export class ModelSelectorComponent extends Container { this.#isMenuOpen = true; this.#menuStep = "role"; this.#menuSelectedRole = null; - this.#menuSelectedIndex = this.#coerceMenuSelectedIndex(0, selectedItem); + this.#menuSelectedIndex = this.#coerceMenuSelectedIndex(0); // Collapse the model list while the action/thinking menu is open so the // menu owns the full viewport instead of stacking below a now-irrelevant // (and often off-screen) list. @@ -1149,9 +1114,7 @@ export class ModelSelectorComponent extends Container { }) : this.#menuRoleActions.map((action, index) => { const prefix = index === this.#menuSelectedIndex ? ` ${theme.nav.cursor} ` : " "; - const disabled = this.#isDefaultRoleActionOverContextLimit(action, selectedItem.model); - const suffix = disabled ? this.#formatCurrentContextLimitSuffix(selectedItem.model) : ""; - return `${prefix}${action.label}${suffix}`; + return `${prefix}${action.label}`; }); const selectedRoleName = this.#menuSelectedRole ? getRoleInfo(this.#menuSelectedRole, this.#settings).name : ""; @@ -1301,7 +1264,7 @@ export class ModelSelectorComponent extends Container { if (matchesKey(keyData, "enter") || matchesKey(keyData, "return") || keyData === "\n") { if (this.#menuStep === "role") { const action = this.#menuRoleActions[this.#menuSelectedIndex]; - if (!action || this.#isDefaultRoleActionOverContextLimit(action, selectedItem.model)) return; + if (!action) return; this.#menuSelectedRole = action.role; this.#menuStep = "thinking"; this.#menuSelectedIndex = this.#getThinkingPreselectIndex(action.role, selectedItem.model); diff --git a/packages/coding-agent/src/modes/controllers/selector-controller.ts b/packages/coding-agent/src/modes/controllers/selector-controller.ts index 5b2662ae3..db1e242de 100644 --- a/packages/coding-agent/src/modes/controllers/selector-controller.ts +++ b/packages/coding-agent/src/modes/controllers/selector-controller.ts @@ -540,19 +540,28 @@ export class SelectorController { done(); this.ctx.ui.requestRender(); } else if (role === "default") { - // Default: update agent state and persist + const contextWindow = model.contextWindow ?? 0; + const switchActiveModel = + currentContextTokens <= 0 || contextWindow <= 0 || currentContextTokens <= contextWindow; await this.ctx.session.setModel(model, role, { selector, thinkingLevel: concreteThinking, persist: true, + switchActiveModel, }); if (isAuto) { - this.ctx.session.setThinkingLevel(AUTO_THINKING, true); - } else if (concreteThinking && concreteThinking !== ThinkingLevel.Inherit) { + if (switchActiveModel) { + this.ctx.session.setThinkingLevel(AUTO_THINKING, true); + } else { + this.ctx.settings.set("defaultThinkingLevel", AUTO_THINKING); + } + } else if (switchActiveModel && concreteThinking && concreteThinking !== ThinkingLevel.Inherit) { this.ctx.session.setThinkingLevel(concreteThinking); } - this.ctx.statusLine.invalidate(); - this.ctx.updateEditorBorderColor(); + if (switchActiveModel) { + this.ctx.statusLine.invalidate(); + this.ctx.updateEditorBorderColor(); + } this.ctx.showStatus(`Default model: ${selector ?? model.id}`); // Don't call done() - selector stays open for role assignment } else { diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index be8194c8f..5701f9622 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -7863,16 +7863,17 @@ export class AgentSession { /** * Set model directly. * Validates that a credential source is configured (synchronously, without - * refreshing OAuth or running command-backed key programs) and saves to the - * active session. Persists settings only when requested. The concrete key is - * resolved lazily per request, so switching never blocks the event loop. + * refreshing OAuth or running command-backed key programs). By default, the + * active session switches immediately; callers may persist role settings + * without touching the live session via `switchActiveModel: false`. * @throws Error if no API key available for the model */ async setModel( model: Model, role: string = "default", - options?: { selector?: string; thinkingLevel?: ThinkingLevel; persist?: boolean }, + options?: { selector?: string; thinkingLevel?: ThinkingLevel; persist?: boolean; switchActiveModel?: boolean }, ): Promise { + const switchActiveModel = options?.switchActiveModel ?? true; const previousEditMode = this.#resolveActiveEditMode(); if (!this.#modelRegistry.hasConfiguredAuth(model)) { throw new Error(`No API key for ${model.provider}/${model.id}`); @@ -7880,15 +7881,20 @@ export class AgentSession { const targetModel = await this.#modelRegistry.refreshSelectedModelMetadata(model); - this.#clearActiveRetryFallback(); - this.#setModelWithProviderSessionReset(targetModel); - this.sessionManager.appendModelChange(`${targetModel.provider}/${targetModel.id}`, role); + if (switchActiveModel) { + this.#clearActiveRetryFallback(); + this.#setModelWithProviderSessionReset(targetModel); + this.sessionManager.appendModelChange(`${targetModel.provider}/${targetModel.id}`, role); + } if (options?.persist) { this.settings.setModelRole( role, this.#formatRoleModelValue(role, targetModel, options.selector, options.thinkingLevel), ); } + if (!switchActiveModel) { + return; + } this.settings.getStorage()?.recordModelUsage(`${targetModel.provider}/${targetModel.id}`); // Re-apply thinking for the newly selected model. Prefer the model's diff --git a/packages/coding-agent/test/agent-session-model-persistence.test.ts b/packages/coding-agent/test/agent-session-model-persistence.test.ts index 8d9f9a530..d02212b2b 100644 --- a/packages/coding-agent/test/agent-session-model-persistence.test.ts +++ b/packages/coding-agent/test/agent-session-model-persistence.test.ts @@ -189,6 +189,21 @@ describe("AgentSession model persistence", () => { expect(created.settings.getModelRole("default")).toBe(modelValue(nextModel)); }); + it("persists the default role without switching the active model when requested", async () => { + const defaultModel = getAnthropicModelOrThrow("claude-sonnet-4-5"); + const nextModel = getAnthropicModelOrThrow("claude-sonnet-4-6"); + + const created = await createSession({ + initialModel: defaultModel, + modelRoles: { default: modelValue(defaultModel) }, + }); + + await created.session.setModel(nextModel, "default", { persist: true, switchActiveModel: false }); + + expect(created.session.model?.id).toBe(defaultModel.id); + expect(created.settings.getModelRole("default")).toBe(modelValue(nextModel)); + }); + it("cycles role models without rewriting configured roles", async () => { const defaultModel = getAnthropicModelOrThrow("claude-sonnet-4-5"); const slowModel = getAnthropicModelOrThrow("claude-sonnet-4-6"); diff --git a/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts b/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts index 9b9c0c30f..529908858 100644 --- a/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts +++ b/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts @@ -7,6 +7,7 @@ import type { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-regis import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { ModelSelectorComponent } from "@oh-my-pi/pi-coding-agent/modes/components/model-selector"; import { getThemeByName, setThemeInstance } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import type { ConfiguredThinkingLevel } from "@oh-my-pi/pi-coding-agent/thinking"; import type { TUI } from "@oh-my-pi/pi-tui"; function normalizeRenderedText(text: string): string { @@ -66,7 +67,7 @@ function createContextTestModel(id: string, contextWindow: number): Model { function createScopedSelector( models: Model[], settings: Settings, - onSelect: (model: Model) => void, + onSelect: (model: Model, role: string | null, thinkingLevel?: ConfiguredThinkingLevel, selector?: string) => void, options?: { temporaryOnly?: boolean; currentContextTokens?: number }, ): ModelSelectorComponent { const modelRegistry = { @@ -83,7 +84,7 @@ function createScopedSelector( settings, modelRegistry, models.map(model => ({ model })), - model => onSelect(model), + (model, role, thinkingLevel, selector) => onSelect(model, role, thinkingLevel, selector), () => {}, options, ); @@ -207,7 +208,7 @@ describe("ModelSelector role badge thinking display", () => { expect(rendered).toContain("default/smol/plan/task/slow/custom roles"); }); - test("opens the role assignment menu but guards over-context default switches", async () => { + test("opens over-context default role actions for global configuration", async () => { installTestTheme(); const settings = Settings.isolated({}); const small = createContextTestModel("only-small", 4096); @@ -225,12 +226,18 @@ describe("ModelSelector role badge thinking display", () => { selector.handleInput("\n"); const afterOpen = normalizeRenderedText(selector.render(220).join("\n")); expect(afterOpen).toContain("Action for: only-small"); - expect(afterOpen).toContain("Set as DEFAULT (Default) ⦸ context>4.1k"); + expect(afterOpen).toContain("Set as DEFAULT (Default)"); + expect(afterOpen).not.toContain("context>4.1k"); selector.handleInput("\n"); const afterRoleEnter = normalizeRenderedText(selector.render(220).join("\n")); - expect(afterRoleEnter).toContain("Thinking for: Fast (only-small)"); + expect(afterRoleEnter).toContain("Thinking for: Default (only-small)"); expect(onSelect).not.toHaveBeenCalled(); + + selector.handleInput("\n"); + expect(onSelect.mock.calls[0]?.[0]).toBe(small); + expect(onSelect.mock.calls[0]?.[1]).toBe("default"); + expect(onSelect.mock.calls[0]?.[3]).toBe("test/only-small"); }); test("uses cached models for Enter while offline refresh is still pending", () => {