Archived
fix(runtime): reload model config at service boundaries
This commit is contained in:
@@ -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<string> {
|
||||
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<OAuthLoginFlowService["start"]>[0][] = [];
|
||||
disposed = false;
|
||||
|
||||
@@ -50,7 +50,7 @@ export class AuthService {
|
||||
}
|
||||
|
||||
async authProviders(mode: "login" | "logout", authType?: AuthType): Promise<AuthProvidersResponse> {
|
||||
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<void> {
|
||||
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;
|
||||
|
||||
@@ -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<void> {
|
||||
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,
|
||||
}],
|
||||
},
|
||||
},
|
||||
}));
|
||||
}
|
||||
|
||||
@@ -1158,7 +1158,7 @@ export class PiSessionService implements SessionRouteService {
|
||||
|
||||
async availableModels(ref: PiSessionLookup): Promise<ClientSessionModel[]> {
|
||||
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<ClientSessionStatus> {
|
||||
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);
|
||||
|
||||
Reference in New Issue
Block a user