From 45f068ef05695c9b783e143dae7ed18de0769ff9 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Fri, 17 Jul 2026 23:48:32 +0200 Subject: [PATCH] fix(runtime): reload model config at service boundaries --- src/server/sessions/authService.test.ts | 63 +++++++++++++-- src/server/sessions/authService.ts | 17 ++-- .../piSessionService.promptQueue.test.ts | 78 ++++++++++++++++++- src/server/sessions/piSessionService.ts | 10 +-- 4 files changed, 141 insertions(+), 27 deletions(-) diff --git a/src/server/sessions/authService.test.ts b/src/server/sessions/authService.test.ts index 0e49ece..1751677 100644 --- a/src/server/sessions/authService.test.ts +++ b/src/server/sessions/authService.test.ts @@ -1,4 +1,4 @@ -import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { ModelRuntime } from "@earendil-works/pi-coding-agent"; @@ -15,22 +15,28 @@ afterEach(async () => { }); describe("AuthService", () => { - it("saves API keys and emits a global auth change", async () => { - const { auth, credentials, changes } = await createAuthService(); + it("saves API keys and emits a global auth change after the runtime refreshes", async () => { + const { auth, runtime, credentials, changes } = await createAuthService(); + const reloadConfig = vi.spyOn(runtime, "reloadConfig").mockResolvedValue(undefined); + const refresh = vi.spyOn(runtime, "refresh"); await expect(auth.saveApiKey("anthropic", "sk-test")).resolves.toEqual({ accepted: true }); await expect(credentials.read("anthropic")).resolves.toEqual({ type: "api_key", key: "sk-test" }); + expect(reloadConfig).toHaveBeenCalledOnce(); + expect(refresh).toHaveBeenCalledOnce(); expect(changes).toEqual([{}]); auth.dispose(); }); - it("logs out providers and emits the removed provider id", async () => { - const { auth, credentials, changes } = await createAuthService({ anthropic: { type: "api_key", key: "sk-test" } }); + it("logs out providers and emits the removed provider id after the runtime refreshes", async () => { + const { auth, runtime, credentials, changes } = await createAuthService({ anthropic: { type: "api_key", key: "sk-test" } }); + const refresh = vi.spyOn(runtime, "refresh"); await expect(auth.logoutProvider("anthropic")).resolves.toEqual({ accepted: true }); await expect(credentials.read("anthropic")).resolves.toBeUndefined(); + expect(refresh).toHaveBeenCalledOnce(); expect(changes).toEqual([{ removedProviderId: "anthropic" }]); auth.dispose(); }); @@ -164,6 +170,37 @@ describe("AuthService", () => { auth.dispose(); }); + it("reloads models.json before enumerating and validating OAuth providers", async () => { + const agentDir = await tempAgentDir(); + const modelsPath = join(agentDir, "models.json"); + const runtime = await ModelRuntime.create({ + credentials: new InMemoryCredentialStore(), + modelsPath, + allowModelNetwork: false, + }); + const authFlows = new CapturingOAuthLoginFlowService(); + const auth = await AuthService.create({ runtime, authFlows }); + + await writeFile(modelsPath, radiusModelsConfig("First Radius")); + const response = await auth.authProviders("login", "oauth"); + expect(response.providers).toEqual(expect.arrayContaining([ + expect.objectContaining({ id: "test-radius", name: "First Radius", authType: "oauth" }), + ])); + + await writeFile(modelsPath, radiusModelsConfig("Updated Radius")); + await expect(auth.startOAuthLogin("test-radius")).resolves.toMatchObject({ + providerId: "test-radius", + providerName: "Updated Radius", + status: "running", + }); + expect(authFlows.startCalls.at(0)).toMatchObject({ + providerId: "test-radius", + providerName: "Updated Radius", + runtime, + }); + auth.dispose(); + }); + it("stores credentials in the configured agent directory", async () => { const agentDir = await tempAgentDir(); const auth = await AuthService.create({ agentDir }); @@ -174,7 +211,7 @@ describe("AuthService", () => { auth.dispose(); }); - it("refreshes auth state after OAuth login completes", async () => { + it("emits an auth change after OAuth login completes without refreshing twice", async () => { const runtime = await ModelRuntime.create({ credentials: new InMemoryCredentialStore(), modelsPath: null, @@ -202,7 +239,7 @@ describe("AuthService", () => { startOptions.onComplete(); await vi.waitFor(() => { expect(changes).toEqual([{}]); }); - expect(refresh).toHaveBeenCalledOnce(); + expect(refresh).not.toHaveBeenCalled(); auth.dispose(); expect(authFlows.disposed).toBe(true); }); @@ -241,6 +278,18 @@ async function tempAgentDir(): Promise { return dir; } +function radiusModelsConfig(name: string): string { + return JSON.stringify({ + providers: { + "test-radius": { + name, + baseUrl: "https://radius.example.test/v1", + oauth: "radius", + }, + }, + }); +} + class CapturingOAuthLoginFlowService extends OAuthLoginFlowService { readonly startCalls: Parameters[0][] = []; disposed = false; diff --git a/src/server/sessions/authService.ts b/src/server/sessions/authService.ts index 386e320..ad7d650 100644 --- a/src/server/sessions/authService.ts +++ b/src/server/sessions/authService.ts @@ -50,7 +50,7 @@ export class AuthService { } async authProviders(mode: "login" | "logout", authType?: AuthType): Promise { - await this.runtime.refresh(); + await this.runtime.reloadConfig(); const providers = mode === "logout" ? await getLogoutProviderOptions(this.runtime) : getLoginProviderOptions(this.runtime, authType); return { providers }; } @@ -74,13 +74,13 @@ export class AuthService { notify: () => undefined, }; await this.runtime.login(providerId, "api_key", interaction); - await this.refreshAuthState(); + this.emit({}); return { accepted: true }; } async logoutProvider(providerId: string): Promise<{ accepted: true }> { await this.runtime.logout(providerId); - await this.refreshAuthState({ removedProviderId: providerId }); + this.emit({ removedProviderId: providerId }); return { accepted: true }; } @@ -91,7 +91,7 @@ export class AuthService { providerName: provider.name, runtime: this.runtime, onComplete: () => { - void this.refreshAuthState(); + this.emit({}); }, }); } @@ -108,17 +108,12 @@ export class AuthService { return this.authFlows.cancel(flowId); } - private async refreshAuthState(change: AuthChange = {}): Promise { - await this.runtime.refresh(); - this.emit(change); - } - private emit(change: AuthChange): void { for (const listener of this.listeners) listener(change); } private async requireApiKeyLoginProvider(providerId: string) { - await this.runtime.refresh(); + await this.runtime.reloadConfig(); const provider = getLoginProviderOptions(this.runtime, "api_key").find((option) => option.id === providerId); if (provider !== undefined) return provider; @@ -130,7 +125,7 @@ export class AuthService { } private async requireOAuthLoginProvider(providerId: string) { - await this.runtime.refresh(); + await this.runtime.reloadConfig(); const provider = getLoginProviderOptions(this.runtime, "oauth").find((option) => option.id === providerId); if (provider === undefined) throw new Error(`OAuth provider not found: ${providerId}`); return provider; diff --git a/src/server/sessions/piSessionService.promptQueue.test.ts b/src/server/sessions/piSessionService.promptQueue.test.ts index c4d4514..e9781cc 100644 --- a/src/server/sessions/piSessionService.promptQueue.test.ts +++ b/src/server/sessions/piSessionService.promptQueue.test.ts @@ -1,5 +1,9 @@ +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { createAssistantMessageEventStream, InMemoryCredentialStore, type AssistantMessage } from "@earendil-works/pi-ai"; import type { StreamFn } from "@earendil-works/pi-agent-core"; +import { ModelRuntime } from "@earendil-works/pi-coding-agent"; import { describe, expect, it, vi } from "vitest"; import { PiSessionService } from "./piSessionService.js"; import { CapturingSessionEventHub, createTestModelRuntime, fakeRuntime, runtimeCreator, seedCredential, sessionGateway, sessionRecord, sessionRef, TEST_MODEL_ID, TEST_MODEL_PROVIDER, testModel, testModelRuntime, type RuntimeCreator } from "./piSessionService.testSupport.js"; @@ -347,12 +351,56 @@ describe("PiSessionService prompt, queue, and auth warnings", () => { await service.dispose(); }); + it("reloads models.json before listing and selecting models", async () => { + const agentDir = await mkdtemp(join(tmpdir(), "pi-web-model-runtime-")); + try { + const modelsPath = join(agentDir, "models.json"); + await writeLocalModelsConfig(modelsPath, "initial-model"); + const modelRuntime = await ModelRuntime.create({ + credentials: new InMemoryCredentialStore(), + modelsPath, + allowModelNetwork: false, + }); + const setSessionModel = vi.fn(() => Promise.resolve()); + const fake = fakeRuntime("models-session", { modelRuntime, setModel: setSessionModel }); + const service = new PiSessionService(new CapturingSessionEventHub(), { + agentDir, + modelRuntime, + createAgentRuntime: runtimeCreator(fake.runtime), + sessionManager: sessionGateway([sessionRecord("models-session")]), + heartbeatIntervalMs: 60_000, + }); + + try { + await writeLocalModelsConfig(modelsPath, "listed-model"); + const listed = await service.availableModels(sessionRef("models-session")); + expect(listed).toEqual(expect.arrayContaining([ + expect.objectContaining({ provider: "test-local", id: "listed-model" }), + ])); + expect(listed).not.toEqual(expect.arrayContaining([ + expect.objectContaining({ provider: "test-local", id: "initial-model" }), + ])); + + await writeLocalModelsConfig(modelsPath, "selected-model"); + await expect(service.setModel(sessionRef("models-session"), "test-local", "selected-model")).resolves.toBeDefined(); + expect(setSessionModel).toHaveBeenCalledWith(expect.objectContaining({ + provider: "test-local", + id: "selected-model", + })); + } finally { + await service.dispose(); + } + } finally { + await rm(agentDir, { recursive: true, force: true }); + } + }); + it("refreshes auth state and dedupes warnings when logout removes the current model's credentials", async () => { const hub = new CapturingSessionEventHub(); - // The shared model runtime reads a live credential store; auth changes are - // simulated by mutating the store and refreshing the runtime (the same - // sequence AuthService performs before emitting an AuthChange), then - // notifying the service via applyAuthChange. + // The shared model runtime reads a live credential store. Mutating the store + // and refreshing here simulates the committed snapshot that + // ModelRuntime.login()/logout() establishes before AuthService emits. + // applyAuthChange then only needs to notify active sessions. const credentials = new InMemoryCredentialStore(); await seedCredential(credentials, "anthropic", { type: "api_key", key: "sk-test" }); const modelRuntime = await createTestModelRuntime(credentials); @@ -409,3 +457,25 @@ describe("PiSessionService prompt, queue, and auth warnings", () => { await service.dispose(); }); }); + +async function writeLocalModelsConfig(path: string, modelId: string): Promise { + await writeFile(path, JSON.stringify({ + providers: { + "test-local": { + name: "Test Local", + baseUrl: "http://127.0.0.1:1234/v1", + apiKey: "offline-test-key", + api: "openai-completions", + models: [{ + id: modelId, + name: modelId, + reasoning: false, + input: ["text"], + cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 }, + contextWindow: 1_000, + maxTokens: 100, + }], + }, + }, + })); +} diff --git a/src/server/sessions/piSessionService.ts b/src/server/sessions/piSessionService.ts index 5899e3c..1e8d9cd 100644 --- a/src/server/sessions/piSessionService.ts +++ b/src/server/sessions/piSessionService.ts @@ -1158,7 +1158,7 @@ export class PiSessionService implements SessionRouteService { async availableModels(ref: PiSessionLookup): Promise { const session = await this.getOrOpen(ref); - await session.modelRuntime.refresh(); + await session.modelRuntime.reloadConfig(); const models = session.scopedModels.length > 0 ? session.scopedModels.map((scoped) => scoped.model) : session.modelRuntime.getAvailableSnapshot(); @@ -1168,7 +1168,7 @@ export class PiSessionService implements SessionRouteService { async setModel(ref: PiSessionLookup, provider: string, modelId: string): Promise { await this.assertWritable(ref); const session = await this.getOrOpen(ref); - await session.modelRuntime.refresh(); + await session.modelRuntime.reloadConfig(); const candidates = session.scopedModels.length > 0 ? session.scopedModels.map((scoped) => scoped.model) : session.modelRuntime.getAvailableSnapshot(); @@ -2019,9 +2019,9 @@ export class PiSessionService implements SessionRouteService { } applyAuthChange(change: AuthChange = {}): void { - // The shared model runtime is refreshed by AuthService before it emits the - // change (and every session shares that runtime), so no refresh is needed - // here — this keeps the subscribe callback synchronous. + // ModelRuntime.login()/logout() refresh the shared runtime before AuthService + // emits the change, so no refresh is needed here. Keeping this synchronous + // also lets every active session observe the same committed auth snapshot. for (const active of this.active.values()) { const { session } = active.runtime; this.syncCurrentModelAuthWarning(session, change.removedProviderId);