fix(coding-agent): avoided blocking auth resolution during model switches

- Updated AgentSession.setModel and setModelTemporary to validate credentials with hasConfiguredAuth instead of eagerly resolving API keys. - Changed ModelRegistry.hasConfiguredAuth to probe configured credentials without executing command-backed key programs or refreshing OAuth tokens. - Added model switch auth tests that verify resolver calls were removed and unconfigured models are rejected synchronously.
2026-06-13 15:39:04 +02:00
parent 2baabead25
commit b7b201ae48
5 changed files with 158 additions and 9 deletions
@@ -94,7 +94,6 @@ describe("applyCanonicalLimitFallback", () => {
 		expect(proxy.maxTokens).toBe(40960);
 	});

-
 	it("leaves holes null when no canonical-family reference exists", () => {
 		const models: ModelSpec<Api>[] = [
 			spec({ id: "some-bespoke-model-xyz", provider: "custom", contextWindow: null, maxTokens: null }),
@@ -32,6 +32,7 @@
 - Fixed the "Working…" loader disappearing after an empty-Enter interrupt-and-flush of a queued steering message: the input path now acknowledges the interrupt immediately, `EventController` recreates the loader before applying the `Interrupting…` label, and turn-end teardown is deferred while `AgentSession.interruptAndFlushQueuedMessages()` is still arming the queued continuation. This covers both stale `agent_end` events that land after the resumed `agent_start` and the gap before `agent.continue()` flips `session.isStreaming`, so the agent no longer looks stopped while it keeps running in the background.
 - Fixed concurrent `omp --session` startups (e.g. cmux pane restore after an unclean shutdown) crashing with `SQLITE_BUSY_RECOVERY` while the agent SQLite databases were still under WAL recovery. The auth credential store and `AgentStorage.open()` retry the `SQLITE_BUSY` family with bounded backoff, and every shared SQLite open path (`AgentStorage`, history, autoresearch, memories, github cache, auto-QA grievances, catalog model cache, stats) now installs the busy handler before the first lock-taking statement so transient WAL recovery contention waits instead of crashing ([#2421](https://github.com/can1357/oh-my-pi/issues/2421)).
 - Mnemopi `per-project` / `per-project-tagged` bank derivation is now stable for one cwd, ignoring the surrounding git layout. Previously the bank id was hashed from `git.repo.resolveSync(cwd)?.repoRoot ?? path.resolve(cwd)`, so adding or removing a `.git` anywhere above the working directory silently repointed the same conversation to a new bank and stranded its memories (e.g. `/home/x/projects/repo` flipping between `projects-…` and `repo-…`). The derivation in `packages/coding-agent/src/mnemopi/config.ts` now hashes `path.resolve(cwd)` directly, and session startup widens the recall set with any sibling bank under `<dbDir>/banks/` whose `working_memory` rows already carry the active cwd in `metadata_json.$.cwd`, so memories stranded by the old, less-stable derivation become visible again on the next session without manual migration ([#2412](https://github.com/can1357/oh-my-pi/issues/2412)).
+- Fixed model switching (Ctrl+P role cycling and the alt+p / `/switch` / `/models` selector) intermittently freezing the UI for several seconds. `AgentSession.setModel`/`setModelTemporary` ran an eager `await modelRegistry.getApiKey(model)` purely as an existence pre-flight and discarded the value — but `getApiKey` does real work: it synchronously executes command-backed key programs (`apiKey: "!cmd"`, `execSync` with a 10s timeout, blocking the event loop) and refreshes OAuth tokens over the network when one crosses the expiry window (the "fine for a few switches, then a multi-second stall" symptom). Switching now uses the synchronous, side-effect-free `ModelRegistry.hasConfiguredAuth` check; the concrete key (command execution + OAuth refresh) is still resolved lazily per request via the existing resolver, so an unconfigured provider still fails fast with `No API key` while a healthy switch never touches the network or spawns a subprocess. `hasConfiguredAuth` no longer runs the command program or refreshes tokens either, matching its documented "probe without resolving an API key" contract.

 ## [15.12.3] - 2026-06-12

@@ -1733,11 +1733,20 @@ export class ModelRegistry {
 	 * paths that pre-flight auth before model resolution) can probe a model
 	 * without resolving an API key. Returns true for keyless providers as well
 	 * as providers with stored credentials. See issue #993.
+	 *
+	 * Side-effect-free and synchronous: a command-backed key (`!cmd`) counts as
+	 * configured by its presence alone — the program is NOT executed — and OAuth
+	 * tokens are NOT refreshed (`authStorage.hasAuth`). This is what keeps the
+	 * model-switch pre-flight off the event loop's hot path; the real key
+	 * (command execution + OAuth refresh) is resolved lazily per request via
+	 * {@link ModelRegistry.resolver}.
 	 */
 	hasConfiguredAuth(model: Model<Api>): boolean {
-		const commandKey = this.#resolveCommandBackedApiKey(model.provider);
+		const keyConfig = this.#customProviderApiKeys.get(model.provider);
 		return (
-			commandKey.configured || this.#keylessProviders.has(model.provider) || this.authStorage.hasAuth(model.provider)
+			isCommandConfigValue(keyConfig) ||
+			this.#keylessProviders.has(model.provider) ||
+			this.authStorage.hasAuth(model.provider)
 		);
 	}

@@ -5690,7 +5690,10 @@ export class AgentSession {

 	/**
 	 * Set model directly.
-	 * Validates API key and saves to the active session. Persists settings only when requested.
+	 * 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.
 	 * @throws Error if no API key available for the model
 	 */
 	async setModel(
@@ -5699,8 +5702,7 @@ export class AgentSession {
 		options?: { selector?: string; thinkingLevel?: ThinkingLevel; persist?: boolean },
 	): Promise<void> {
 		const previousEditMode = this.#resolveActiveEditMode();
-		const apiKey = await this.#modelRegistry.getApiKey(model, this.sessionId);
-		if (!apiKey) {
+		if (!this.#modelRegistry.hasConfiguredAuth(model)) {
 			throw new Error(`No API key for ${model.provider}/${model.id}`);
 		}

@@ -5723,7 +5725,9 @@ export class AgentSession {

 	/**
 	 * Set model temporarily (for this session only).
-	 * Validates API key, saves to session log but NOT to settings.
+	 * Validates that a credential source is configured (synchronously, without
+	 * refreshing OAuth or running command-backed key programs), saves to session
+	 * log but NOT to settings.
 	 * @throws Error if no API key available for the model
 	 */
 	async setModelTemporary(
@@ -5732,8 +5736,7 @@ export class AgentSession {
 		options?: { ephemeral?: boolean },
 	): Promise<void> {
 		const previousEditMode = this.#resolveActiveEditMode();
-		const apiKey = await this.#modelRegistry.getApiKey(model, this.sessionId);
-		if (!apiKey) {
+		if (!this.#modelRegistry.hasConfiguredAuth(model)) {
 			throw new Error(`No API key for ${model.provider}/${model.id}`);
 		}

@@ -0,0 +1,137 @@
+import { afterAll, afterEach, beforeAll, describe, expect, it, spyOn } from "bun:test";
+import * as path from "node:path";
+import { Agent } from "@oh-my-pi/pi-agent-core";
+import { type Api, Effort, type Model } from "@oh-my-pi/pi-ai";
+import { getBundledModel } from "@oh-my-pi/pi-catalog/models";
+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";
+
+// Switching the active model (Ctrl+P role cycling, /models selection) must be a
+// cheap, synchronous operation. It used to call the async `getApiKey`, which can
+// block the event loop on a command-backed key program (`execSync`) or stall on
+// a network OAuth refresh. The real key is resolved lazily per request via the
+// resolver, so the switch only needs a synchronous "is a credential configured"
+// pre-flight (`hasConfiguredAuth`) — never the resolver.
+describe("AgentSession model switch auth pre-flight", () => {
+	let sharedDir: TempDir;
+	let authStorage: AuthStorage;
+	let registry: ModelRegistry;
+	let session: AgentSession | undefined;
+	const spies: Array<{ mockRestore: () => void }> = [];
+
+	beforeAll(async () => {
+		sharedDir = TempDir.createSync("@pi-model-switch-auth-");
+		authStorage = await AuthStorage.create(path.join(sharedDir.path(), "auth.db"));
+		authStorage.setRuntimeApiKey("anthropic", "test-key");
+		registry = new ModelRegistry(authStorage, path.join(sharedDir.path(), "models.yml"));
+	});
+
+	afterAll(() => {
+		authStorage.close();
+		sharedDir.removeSync();
+	});
+
+	afterEach(async () => {
+		for (const spy of spies.splice(0)) spy.mockRestore();
+		if (session) {
+			await session.dispose();
+			session = undefined;
+		}
+	});
+
+	function modelOrThrow(id: string): Model<Api> {
+		const model = getBundledModel("anthropic", id);
+		if (!model) throw new Error(`Expected anthropic model ${id} to exist`);
+		return model;
+	}
+
+	function makeSession(initialModel: Model<Api>, roles?: Record<string, string>): AgentSession {
+		const settings = Settings.isolated();
+		if (roles) {
+			for (const role in roles) settings.setModelRole(role, roles[role]);
+		}
+		const agent = new Agent({
+			initialState: {
+				model: initialModel,
+				systemPrompt: ["Test"],
+				tools: [],
+				messages: [],
+				thinkingLevel: Effort.Medium,
+			},
+		});
+		session = new AgentSession({
+			agent,
+			sessionManager: SessionManager.inMemory(),
+			settings,
+			modelRegistry: registry,
+		});
+		return session;
+	}
+
+	it("switches the active model via the synchronous auth check, not the resolver", async () => {
+		const from = modelOrThrow("claude-sonnet-4-5");
+		const to = modelOrThrow("claude-sonnet-4-6");
+		const s = makeSession(from);
+
+		const getApiKeySpy = spyOn(registry, "getApiKey");
+		const hasAuthSpy = spyOn(registry, "hasConfiguredAuth");
+		spies.push(getApiKeySpy, hasAuthSpy);
+
+		await s.setModel(to);
+
+		expect(s.model?.id).toBe(to.id);
+		expect(hasAuthSpy).toHaveBeenCalled();
+		expect(getApiKeySpy).not.toHaveBeenCalled();
+	});
+
+	it("cycles role models without invoking the resolver", async () => {
+		const from = modelOrThrow("claude-sonnet-4-5");
+		const slow = modelOrThrow("claude-sonnet-4-6");
+		const s = makeSession(from, {
+			default: `${from.provider}/${from.id}`,
+			slow: `${slow.provider}/${slow.id}`,
+		});
+
+		const getApiKeySpy = spyOn(registry, "getApiKey");
+		spies.push(getApiKeySpy);
+
+		const result = await s.cycleRoleModels(["default", "slow"]);
+
+		expect(result?.role).toBe("slow");
+		expect(result?.model.id).toBe(slow.id);
+		expect(s.model?.id).toBe(slow.id);
+		expect(getApiKeySpy).not.toHaveBeenCalled();
+	});
+
+	it("temporary switch also avoids the resolver", async () => {
+		const from = modelOrThrow("claude-sonnet-4-5");
+		const to = modelOrThrow("claude-sonnet-4-6");
+		const s = makeSession(from);
+
+		const getApiKeySpy = spyOn(registry, "getApiKey");
+		spies.push(getApiKeySpy);
+
+		await s.setModelTemporary(to);
+
+		expect(s.model?.id).toBe(to.id);
+		expect(getApiKeySpy).not.toHaveBeenCalled();
+	});
+
+	it("rejects the switch synchronously when no credential is configured, without calling the resolver", async () => {
+		const from = modelOrThrow("claude-sonnet-4-5");
+		const to = modelOrThrow("claude-sonnet-4-6");
+		const s = makeSession(from);
+
+		const getApiKeySpy = spyOn(registry, "getApiKey");
+		const hasAuthSpy = spyOn(registry, "hasConfiguredAuth").mockReturnValue(false);
+		spies.push(getApiKeySpy, hasAuthSpy);
+
+		await expect(s.setModel(to)).rejects.toThrow(/No API key/);
+		expect(s.model?.id).toBe(from.id);
+		expect(getApiKeySpy).not.toHaveBeenCalled();
+	});
+});