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`.
This commit is contained in:
can1357
2026-05-31 06:21:29 +02:00
parent 3fe5d98eab
commit 346ae48b0c
11 changed files with 226 additions and 39 deletions
+2
View File
@@ -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
@@ -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,
@@ -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<void> {
async cycleRoleModel(direction: "forward" | "backward" = "forward"): Promise<void> {
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));
}
@@ -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);
@@ -2812,8 +2812,8 @@ export class InteractiveMode implements InteractiveModeContext {
this.#inputController.cycleThinkingLevel();
}
cycleRoleModel(options?: { temporary?: boolean }): Promise<void> {
return this.#inputController.cycleRoleModel(options);
cycleRoleModel(direction?: "forward" | "backward"): Promise<void> {
return this.#inputController.cycleRoleModel(direction);
}
toggleToolOutputExpansion(): void {
+1 -1
View File
@@ -272,7 +272,7 @@ export interface InteractiveModeContext {
hasActiveOmfg(): boolean;
handleOmfgEscape(): boolean;
cycleThinkingLevel(): void;
cycleRoleModel(options?: { temporary?: boolean }): Promise<void>;
cycleRoleModel(direction?: "forward" | "backward"): Promise<void>;
toggleToolOutputExpansion(): void;
setToolsExpanded(expanded: boolean): void;
toggleThinkingBlockVisibility(): void;
@@ -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 |`,
@@ -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<void> {
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<void> {
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<RoleModelCycleResult | undefined> {
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();
@@ -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<Api> {
const model = getBundledModel("anthropic", id);
if (!model) throw new Error(`Expected anthropic model ${id} to exist`);
return model;
}
function modelValue(model: Model<Api>): string {
return `${model.provider}/${model.id}`;
}
async function createSession(options?: {
initialModel?: Model<Api>;
selectInitialModel?: (availableModels: Model<Api>[]) => Model<Api>;
modelRoles?: Record<string, string>;
}): 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);
});
});
@@ -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`);
});
@@ -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);
});