diff --git a/packages/catalog/test/canonical-limit-fallback.test.ts b/packages/catalog/test/canonical-limit-fallback.test.ts index bd9725eac..68bf326ea 100644 --- a/packages/catalog/test/canonical-limit-fallback.test.ts +++ b/packages/catalog/test/canonical-limit-fallback.test.ts @@ -94,7 +94,6 @@ describe("applyCanonicalLimitFallback", () => { expect(proxy.maxTokens).toBe(40960); }); - it("leaves holes null when no canonical-family reference exists", () => { const models: ModelSpec[] = [ spec({ id: "some-bespoke-model-xyz", provider: "custom", contextWindow: null, maxTokens: null }), diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index b7ff15473..28ed2e71a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 `/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 diff --git a/packages/coding-agent/src/config/model-registry.ts b/packages/coding-agent/src/config/model-registry.ts index c1285c6fb..aad1f8257 100644 --- a/packages/coding-agent/src/config/model-registry.ts +++ b/packages/coding-agent/src/config/model-registry.ts @@ -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): 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) ); } diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index eaa36f474..fcbe3a55f 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -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 { 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 { 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}`); } diff --git a/packages/coding-agent/test/agent-session-model-switch-auth.test.ts b/packages/coding-agent/test/agent-session-model-switch-auth.test.ts new file mode 100644 index 000000000..2dcf69b57 --- /dev/null +++ b/packages/coding-agent/test/agent-session-model-switch-auth.test.ts @@ -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 { + const model = getBundledModel("anthropic", id); + if (!model) throw new Error(`Expected anthropic model ${id} to exist`); + return model; + } + + function makeSession(initialModel: Model, roles?: Record): 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(); + }); +});