From 346ae48b0c32701ccde2a2cae9f5ee438d756ca0 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 31 May 2026 06:21:29 +0200 Subject: [PATCH] fix(session): prevented runtime model switches from persisting default role - Restricted `setModel` to persist settings only when `persist: true` is passed; all runtime switches (Ctrl+P, `--model`, `/model`, model picker temp selections) no longer overwrite `modelRoles.default`. - Changed `cycleRoleModels` to accept a direction ("forward"/"backward") instead of a `temporary` flag; both directions now use `applyRoleModel` without persisting. - Added `persist: true` exclusively to the model picker's "Set as default" action in `SelectorController`. - Added test suite covering persistence behavior for `setModel`, `cycleRoleModels`, and `cycleModel`. --- packages/coding-agent/CHANGELOG.md | 2 + .../src/autoresearch/tools/run-experiment.ts | 2 +- .../src/modes/controllers/input-controller.ts | 15 +- .../modes/controllers/selector-controller.ts | 1 + .../src/modes/interactive-mode.ts | 4 +- packages/coding-agent/src/modes/types.ts | 2 +- .../src/modes/utils/hotkeys-markdown.ts | 2 +- .../coding-agent/src/session/agent-session.ts | 36 ++-- .../agent-session-model-persistence.test.ts | 190 ++++++++++++++++++ .../test/agent-session-role-thinking.test.ts | 2 +- .../test/model-registry-create.test.ts | 9 +- 11 files changed, 226 insertions(+), 39 deletions(-) create mode 100644 packages/coding-agent/test/agent-session-model-persistence.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1af2cc41a..19ca95587 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -9,6 +9,7 @@ ### Changed +- Changed Shift+Ctrl+P to cycle role models backward instead of cycling forward without persisting. - Changed `search` output to preserve full virtual and internal URL paths in grouped results and `details.files` instead of collapsing them to file basenames - Changed `/omfg` to run up to three generation attempts with validation feedback and only prompt saving when no draft matches assistant history - Changed `/omfg` to show a live draft panel with generation/validation/saving status and allow canceling an active rule request with `Esc` @@ -16,6 +17,7 @@ ### Fixed +- Fixed runtime model switches (Ctrl+P cycling, `--model`, `/model`, model picker selections, and programmatic changes) so they no longer overwrite the persisted `modelRoles.default`; only the model picker's explicit "Set as default" action and settings changes persist the default. - Fixed `search` to honor line-range suffixes on virtual internal URL targets so matches outside the requested ranges are no longer returned - Fixed `search` to handle internal URLs without source files without incorrectly reporting `Path not found`, returning matches from virtual content instead - Fixed `/omfg` parsing to tolerate fenced or noisy model output, normalize generated rule names, and reject invalid regex conditions before saving diff --git a/packages/coding-agent/src/autoresearch/tools/run-experiment.ts b/packages/coding-agent/src/autoresearch/tools/run-experiment.ts index 328a3155c..80e9afa88 100644 --- a/packages/coding-agent/src/autoresearch/tools/run-experiment.ts +++ b/packages/coding-agent/src/autoresearch/tools/run-experiment.ts @@ -3,12 +3,12 @@ import * as path from "node:path"; import { Text } from "@oh-my-pi/pi-tui"; import { formatBytes } from "@oh-my-pi/pi-utils"; import * as z from "zod/v4"; +import { executeBash } from "../../exec/bash-executor"; import type { ToolDefinition } from "../../extensibility/extensions"; import type { Theme } from "../../modes/theme/theme"; import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, TailBuffer, truncateTail } from "../../session/streaming-output"; import { replaceTabs, shortenPath } from "../../tools/render-utils"; import * as git from "../../utils/git"; -import { executeBash } from "../../exec/bash-executor"; import { parseWorkDirDirtyPaths } from "../git"; import { EXPERIMENT_MAX_BYTES, diff --git a/packages/coding-agent/src/modes/controllers/input-controller.ts b/packages/coding-agent/src/modes/controllers/input-controller.ts index 95589e20d..10bad4c5f 100644 --- a/packages/coding-agent/src/modes/controllers/input-controller.ts +++ b/packages/coding-agent/src/modes/controllers/input-controller.ts @@ -8,7 +8,6 @@ import { renderSegmentTrack } from "../../modes/components/segment-track"; import { TinyTitleDownloadProgressComponent } from "../../modes/components/tiny-title-download-progress"; import { expandEmoticons } from "../../modes/emoji-autocomplete"; import { createPromptActionAutocompleteProvider } from "../../modes/prompt-action-autocomplete"; -import { theme } from "../../modes/theme/theme"; import type { InteractiveModeContext } from "../../modes/types"; import type { AgentSessionEvent } from "../../session/agent-session"; import { SKILL_PROMPT_MESSAGE_TYPE, type SkillPromptDetails } from "../../session/messages"; @@ -165,9 +164,9 @@ export class InputController { this.ctx.editor.setActionKeys("app.thinking.cycle", this.ctx.keybindings.getKeys("app.thinking.cycle")); this.ctx.editor.onCycleThinkingLevel = () => this.cycleThinkingLevel(); this.ctx.editor.setActionKeys("app.model.cycleForward", this.ctx.keybindings.getKeys("app.model.cycleForward")); - this.ctx.editor.onCycleModelForward = () => this.cycleRoleModel(); + this.ctx.editor.onCycleModelForward = () => this.cycleRoleModel("forward"); this.ctx.editor.setActionKeys("app.model.cycleBackward", this.ctx.keybindings.getKeys("app.model.cycleBackward")); - this.ctx.editor.onCycleModelBackward = () => this.cycleRoleModel({ temporary: true }); + this.ctx.editor.onCycleModelBackward = () => this.cycleRoleModel("backward"); this.ctx.editor.setActionKeys( "app.model.selectTemporary", this.ctx.keybindings.getKeys("app.model.selectTemporary"), @@ -767,10 +766,10 @@ export class InputController { } } - async cycleRoleModel(options?: { temporary?: boolean }): Promise { + async cycleRoleModel(direction: "forward" | "backward" = "forward"): Promise { try { const cycleOrder = settings.get("cycleOrder"); - const result = await this.ctx.session.cycleRoleModels(cycleOrder, options); + const result = await this.ctx.session.cycleRoleModels(cycleOrder, direction); if (!result) { this.ctx.showStatus("Only one role model available"); return; @@ -780,14 +779,12 @@ export class InputController { this.ctx.updateEditorBorderColor(); // The status line already reports the resolved model + thinking level, so // the cycle status is just a status-line-style chip track (active role - // filled), matching the plan-approval model slider. A dim suffix flags a - // temporary switch since that isn't shown elsewhere. + // filled), matching the plan-approval model slider. const track = renderSegmentTrack( cycleOrder.map(role => ({ label: role, color: getRoleInfo(role, settings).color })), cycleOrder.indexOf(result.role), ); - const tempLabel = options?.temporary ? theme.fg("dim", " (temporary)") : ""; - this.ctx.showStatus(`${track}${tempLabel}`, { dim: false }); + this.ctx.showStatus(track, { dim: false }); } catch (error) { this.ctx.showError(error instanceof Error ? error.message : String(error)); } diff --git a/packages/coding-agent/src/modes/controllers/selector-controller.ts b/packages/coding-agent/src/modes/controllers/selector-controller.ts index 4ef7372e7..ba7676e0e 100644 --- a/packages/coding-agent/src/modes/controllers/selector-controller.ts +++ b/packages/coding-agent/src/modes/controllers/selector-controller.ts @@ -426,6 +426,7 @@ export class SelectorController { await this.ctx.session.setModel(model, role, { selector, thinkingLevel: concreteThinking, + persist: true, }); if (isAuto) { this.ctx.session.setThinkingLevel(AUTO_THINKING, true); diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index fe20bb33b..383217c15 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -2812,8 +2812,8 @@ export class InteractiveMode implements InteractiveModeContext { this.#inputController.cycleThinkingLevel(); } - cycleRoleModel(options?: { temporary?: boolean }): Promise { - return this.#inputController.cycleRoleModel(options); + cycleRoleModel(direction?: "forward" | "backward"): Promise { + return this.#inputController.cycleRoleModel(direction); } toggleToolOutputExpansion(): void { diff --git a/packages/coding-agent/src/modes/types.ts b/packages/coding-agent/src/modes/types.ts index 1c3806622..e408782ba 100644 --- a/packages/coding-agent/src/modes/types.ts +++ b/packages/coding-agent/src/modes/types.ts @@ -272,7 +272,7 @@ export interface InteractiveModeContext { hasActiveOmfg(): boolean; handleOmfgEscape(): boolean; cycleThinkingLevel(): void; - cycleRoleModel(options?: { temporary?: boolean }): Promise; + cycleRoleModel(direction?: "forward" | "backward"): Promise; toggleToolOutputExpansion(): void; setToolsExpanded(expanded: boolean): void; toggleThinkingBlockVisibility(): void; diff --git a/packages/coding-agent/src/modes/utils/hotkeys-markdown.ts b/packages/coding-agent/src/modes/utils/hotkeys-markdown.ts index e19b42a2a..21f8b7f1a 100644 --- a/packages/coding-agent/src/modes/utils/hotkeys-markdown.ts +++ b/packages/coding-agent/src/modes/utils/hotkeys-markdown.ts @@ -39,7 +39,7 @@ export function buildHotkeysMarkdown(bindings: HotkeysMarkdownBindings): string `| \`${appKey(bindings, "app.suspend")}\` | Suspend to background |`, `| \`${appKey(bindings, "app.thinking.cycle")}\` | Cycle thinking level |`, `| \`${appKey(bindings, "app.model.cycleForward")}\` | Cycle role models (slow/default/smol) |`, - `| \`${appKey(bindings, "app.model.cycleBackward")}\` | Cycle role models (temporary) |`, + `| \`${appKey(bindings, "app.model.cycleBackward")}\` | Cycle role models (backward) |`, `| \`${appKey(bindings, "app.model.selectTemporary")}\` | Select model (temporary) |`, `| \`${appKey(bindings, "app.model.select")}\` | Select model (set roles) |`, `| \`${appKey(bindings, "app.plan.toggle")}\` | Toggle plan mode |`, diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 62b4f0dd9..2684b00fb 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -5091,13 +5091,13 @@ export class AgentSession { /** * Set model directly. - * Validates API key, saves to session and settings. + * Validates API key and saves to the active session. Persists settings only when requested. * @throws Error if no API key available for the model */ async setModel( model: Model, role: string = "default", - options?: { selector?: string; thinkingLevel?: ThinkingLevel }, + options?: { selector?: string; thinkingLevel?: ThinkingLevel; persist?: boolean }, ): Promise { const previousEditMode = this.#resolveActiveEditMode(); const apiKey = await this.#modelRegistry.getApiKey(model, this.sessionId); @@ -5108,10 +5108,12 @@ export class AgentSession { this.#clearActiveRetryFallback(); this.#setModelWithProviderSessionReset(model); this.sessionManager.appendModelChange(`${model.provider}/${model.id}`, role); - this.settings.setModelRole( - role, - this.#formatRoleModelValue(role, model, options?.selector, options?.thinkingLevel), - ); + if (options?.persist) { + this.settings.setModelRole( + role, + this.#formatRoleModelValue(role, model, options.selector, options.thinkingLevel), + ); + } this.settings.getStorage()?.recordModelUsage(`${model.provider}/${model.id}`); // Re-apply thinking for the newly selected model. Prefer the model's @@ -5214,9 +5216,8 @@ export class AgentSession { } /** - * Apply a resolved role model as the active model, persisting the choice to - * settings under its role. Mirrors the non-temporary branch of - * {@link cycleRoleModels} and is shared with the plan-approval model slider. + * Apply a resolved role model as the active model without changing global + * settings. Shared with role cycling and the plan-approval model slider. */ async applyRoleModel(entry: ResolvedRoleModel): Promise { await this.setModel(entry.model, entry.role); @@ -5227,24 +5228,21 @@ export class AgentSession { /** * Cycle through configured role models in a fixed order. - * Skips missing roles. + * Skips missing roles and changes only the active session model. * @param roleOrder - Order of roles to cycle through (e.g., ["slow", "default", "smol"]) - * @param options - Optional settings: `temporary` to not persist to settings + * @param direction - "forward" (default) or "backward" */ async cycleRoleModels( roleOrder: readonly string[], - options?: { temporary?: boolean }, + direction: "forward" | "backward" = "forward", ): Promise { const cycle = this.getRoleModelCycle(roleOrder); if (!cycle || cycle.models.length <= 1) return undefined; - const next = cycle.models[(cycle.currentIndex + 1) % cycle.models.length]; + const step = direction === "backward" ? -1 : 1; + const next = cycle.models[(cycle.currentIndex + step + cycle.models.length) % cycle.models.length]; - if (options?.temporary) { - await this.setModelTemporary(next.model, next.explicitThinkingLevel ? next.thinkingLevel : undefined); - } else { - await this.applyRoleModel(next); - } + await this.applyRoleModel(next); return { model: next.model, thinkingLevel: this.thinkingLevel, role: next.role }; } @@ -5288,7 +5286,6 @@ export class AgentSession { this.#clearActiveRetryFallback(); this.#setModelWithProviderSessionReset(next.model); this.sessionManager.appendModelChange(`${next.model.provider}/${next.model.id}`); - this.settings.setModelRole("default", this.#formatRoleModelValue("default", next.model)); this.settings.getStorage()?.recordModelUsage(`${next.model.provider}/${next.model.id}`); // Apply the scoped model's configured thinking level, preserving auto. @@ -5319,7 +5316,6 @@ export class AgentSession { this.#clearActiveRetryFallback(); this.#setModelWithProviderSessionReset(nextModel); this.sessionManager.appendModelChange(`${nextModel.provider}/${nextModel.id}`); - this.settings.setModelRole("default", this.#formatRoleModelValue("default", nextModel)); this.settings.getStorage()?.recordModelUsage(`${nextModel.provider}/${nextModel.id}`); // Re-apply the current thinking level (or auto) for the newly selected model this.#reapplyThinkingLevel(); diff --git a/packages/coding-agent/test/agent-session-model-persistence.test.ts b/packages/coding-agent/test/agent-session-model-persistence.test.ts new file mode 100644 index 000000000..e5241ae5d --- /dev/null +++ b/packages/coding-agent/test/agent-session-model-persistence.test.ts @@ -0,0 +1,190 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as path from "node:path"; +import { Agent } from "@oh-my-pi/pi-agent-core"; +import { type Api, Effort, getBundledModel, type Model } from "@oh-my-pi/pi-ai"; +import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +import { TempDir } from "@oh-my-pi/pi-utils"; + +describe("AgentSession model persistence", () => { + let tempDir: TempDir; + let session: AgentSession | undefined; + let sessionSettings: Settings; + const authStorages: AuthStorage[] = []; + + beforeEach(() => { + tempDir = TempDir.createSync("@pi-model-persistence-"); + }); + + afterEach(async () => { + if (session) { + await session.dispose(); + session = undefined; + } + for (const authStorage of authStorages.splice(0)) { + authStorage.close(); + } + tempDir.removeSync(); + }); + + function getAnthropicModelOrThrow(id: string): Model { + const model = getBundledModel("anthropic", id); + if (!model) throw new Error(`Expected anthropic model ${id} to exist`); + return model; + } + + function modelValue(model: Model): string { + return `${model.provider}/${model.id}`; + } + + async function createSession(options?: { + initialModel?: Model; + selectInitialModel?: (availableModels: Model[]) => Model; + modelRoles?: Record; + }): Promise<{ modelRegistry: ModelRegistry; settings: Settings; session: AgentSession }> { + const authStorage = await AuthStorage.create(path.join(tempDir.path(), `testauth-${authStorages.length}.db`)); + authStorages.push(authStorage); + authStorage.setRuntimeApiKey("anthropic", "test-key"); + const modelRegistry = new ModelRegistry( + authStorage, + path.join(tempDir.path(), `models-${authStorages.length}.yml`), + ); + const model = + options?.initialModel ?? + options?.selectInitialModel?.(modelRegistry.getAvailable()) ?? + getAnthropicModelOrThrow("claude-sonnet-4-5"); + const agent = new Agent({ + initialState: { + model, + systemPrompt: ["Test"], + tools: [], + messages: [], + thinkingLevel: Effort.Medium, + }, + }); + + sessionSettings = Settings.isolated(); + const modelRoles = options?.modelRoles; + if (modelRoles) { + for (const role in modelRoles) { + const modelRoleValue = modelRoles[role]; + if (modelRoleValue !== undefined) { + sessionSettings.setModelRole(role, modelRoleValue); + } + } + } + session = new AgentSession({ + agent, + sessionManager: SessionManager.inMemory(), + settings: sessionSettings, + modelRegistry, + }); + + return { modelRegistry, settings: sessionSettings, session }; + } + + it("switches the active model without persisting by default", async () => { + const defaultModel = getAnthropicModelOrThrow("claude-sonnet-4-5"); + const nextModel = getAnthropicModelOrThrow("claude-sonnet-4-6"); + const defaultRoleValue = modelValue(defaultModel); + + const created = await createSession({ + initialModel: defaultModel, + modelRoles: { default: defaultRoleValue }, + }); + + await created.session.setModel(nextModel); + + expect(created.session.model?.id).toBe(nextModel.id); + expect(created.settings.getModelRole("default")).toBe(defaultRoleValue); + }); + + it("persists the default role when explicitly 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 }); + + expect(created.session.model?.id).toBe(nextModel.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"); + const defaultRoleValue = modelValue(defaultModel); + const slowRoleValue = `${modelValue(slowModel)}:high`; + + const created = await createSession({ + initialModel: defaultModel, + modelRoles: { + default: defaultRoleValue, + slow: slowRoleValue, + }, + }); + + const result = await created.session.cycleRoleModels(["default", "slow"]); + + expect(result?.role).toBe("slow"); + expect(result?.model.id).toBe(slowModel.id); + expect(created.session.model?.id).toBe(slowModel.id); + expect(created.settings.getModelRole("default")).toBe(defaultRoleValue); + expect(created.settings.getModelRole("slow")).toBe(slowRoleValue); + }); + + it("cycles role models backward from the current role", async () => { + const defaultModel = getAnthropicModelOrThrow("claude-sonnet-4-5"); + const slowModel = getAnthropicModelOrThrow("claude-sonnet-4-6"); + const defaultRoleValue = modelValue(defaultModel); + const slowRoleValue = modelValue(slowModel); + + const created = await createSession({ + initialModel: defaultModel, + modelRoles: { + default: defaultRoleValue, + slow: slowRoleValue, + }, + }); + + const forward = await created.session.cycleRoleModels(["default", "slow"], "forward"); + const backward = await created.session.cycleRoleModels(["default", "slow"], "backward"); + + expect(forward?.role).toBe("slow"); + expect(backward?.role).toBe("default"); + expect(created.session.model?.id).toBe(defaultModel.id); + expect(created.settings.getModelRole("default")).toBe(defaultRoleValue); + expect(created.settings.getModelRole("slow")).toBe(slowRoleValue); + }); + + it("cycles available models without persisting the default role", async () => { + const created = await createSession({ + selectInitialModel: availableModels => { + if (availableModels.length <= 1 || !availableModels[0]) { + throw new Error("Expected at least two available models"); + } + return availableModels[0]; + }, + }); + const initialModel = created.session.model; + if (!initialModel) throw new Error("Expected initial model to be set"); + const defaultRoleValue = modelValue(initialModel); + created.settings.setModelRole("default", defaultRoleValue); + + const result = await created.session.cycleModel(); + + if (!result) throw new Error("Expected cycleModel to return a new model"); + expect(modelValue(result.model)).not.toBe(defaultRoleValue); + const activeModel = created.session.model; + if (!activeModel) throw new Error("Expected active model after cycleModel"); + expect(modelValue(activeModel)).toBe(modelValue(result.model)); + expect(created.settings.getModelRole("default")).toBe(defaultRoleValue); + }); +}); diff --git a/packages/coding-agent/test/agent-session-role-thinking.test.ts b/packages/coding-agent/test/agent-session-role-thinking.test.ts index 03a6d0c29..19df05517 100644 --- a/packages/coding-agent/test/agent-session-role-thinking.test.ts +++ b/packages/coding-agent/test/agent-session-role-thinking.test.ts @@ -173,7 +173,7 @@ describe("AgentSession role model thinking behavior", () => { }, }); - await session.setModel(slowModel); + await session.setModel(slowModel, "default", { persist: true }); expect(sessionSettings.getModelRole("default")).toBe(`${slowModel.provider}/${slowModel.id}:off`); }); diff --git a/packages/coding-agent/test/model-registry-create.test.ts b/packages/coding-agent/test/model-registry-create.test.ts index ee40c6ea5..42f2ae233 100644 --- a/packages/coding-agent/test/model-registry-create.test.ts +++ b/packages/coding-agent/test/model-registry-create.test.ts @@ -53,18 +53,19 @@ describe("ModelRegistry.create() factory (F6)", () => { } }); - test("ConfigFile.warmup is idempotent — second call is a no-op", async () => { + test("ConfigFile migration is idempotent — second load is a no-op", async () => { const yml = path.join(tempDir.path(), "models.yml"); const json = path.join(tempDir.path(), "models.json"); await Bun.write(json, JSON.stringify({ models: [] })); const cf = new ConfigFile("models", ModelsConfigSchema, yml); - await ConfigFile.warmup(cf); + cf.tryLoad(); expect(fs.existsSync(yml)).toBe(true); const mtime1 = fs.statSync(yml).mtimeMs; - // Second warmup should not rewrite the file (idempotent path). - await ConfigFile.warmup(cf); + // Second load should not rewrite the file (idempotent migration path). + cf.invalidate(); + cf.tryLoad(); const mtime2 = fs.statSync(yml).mtimeMs; expect(mtime2).toBe(mtime1); });