fix(backend): validate configured model provider pair
This commit is contained in:
+7
-2
@@ -23,7 +23,7 @@ export interface BuildAppDeps {
|
|||||||
}
|
}
|
||||||
|
|
||||||
export function buildApp(config: AppConfig, deps?: BuildAppDeps): FastifyInstance {
|
export function buildApp(config: AppConfig, deps?: BuildAppDeps): FastifyInstance {
|
||||||
const app = Fastify({ logger: false });
|
const app = Fastify({ logger: { level: "warn" }, disableRequestLogging: true });
|
||||||
|
|
||||||
// Allow any origin in dev/e2e; tighten in production via config if needed.
|
// Allow any origin in dev/e2e; tighten in production via config if needed.
|
||||||
app.register(cors, {
|
app.register(cors, {
|
||||||
@@ -45,7 +45,12 @@ export function buildApp(config: AppConfig, deps?: BuildAppDeps): FastifyInstanc
|
|||||||
Math.round(config.ollamaEnsureTimeoutMs / 1000),
|
Math.round(config.ollamaEnsureTimeoutMs / 1000),
|
||||||
);
|
);
|
||||||
|
|
||||||
const listModels = deps?.listModels ?? createPiModelLister(config);
|
const listModels = deps?.listModels ?? createPiModelLister(config, {
|
||||||
|
warn: (detail) => app.log.warn(
|
||||||
|
{ component: "pi-model-list", detail },
|
||||||
|
"Pi enabled-model configuration warning",
|
||||||
|
),
|
||||||
|
});
|
||||||
const getSettings = deps?.getSettings ?? (() => effectiveSettings(config, loadSettings(config)));
|
const getSettings = deps?.getSettings ?? (() => effectiveSettings(config, loadSettings(config)));
|
||||||
|
|
||||||
const authenticate = authPreHandler(config.authMode);
|
const authenticate = authPreHandler(config.authMode);
|
||||||
|
|||||||
@@ -34,7 +34,11 @@ export function metaRoutes(
|
|||||||
const fn = deps.listModels ?? (async () => []);
|
const fn = deps.listModels ?? (async () => []);
|
||||||
try {
|
try {
|
||||||
return { models: await fn() };
|
return { models: await fn() };
|
||||||
} catch {
|
} catch (error) {
|
||||||
|
app.log.warn({
|
||||||
|
component: "pi-model-list",
|
||||||
|
errorType: error instanceof Error ? error.name : typeof error,
|
||||||
|
}, "Pi model listing failed");
|
||||||
// Graceful fallback: Pi may not be running; don't crash the server.
|
// Graceful fallback: Pi may not be running; don't crash the server.
|
||||||
return { models: [] as PiModel[] };
|
return { models: [] as PiModel[] };
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -25,15 +25,19 @@ export function settingsRoutes(
|
|||||||
app.put("/settings", async (req, reply) => {
|
app.put("/settings", async (req, reply) => {
|
||||||
const b = (req.body ?? {}) as Settings;
|
const b = (req.body ?? {}) as Settings;
|
||||||
if (b.model) {
|
if (b.model) {
|
||||||
let available: { id: string }[] = [];
|
let available: { provider: string; id: string }[] = [];
|
||||||
try {
|
try {
|
||||||
available = await deps.listModels();
|
available = await deps.listModels();
|
||||||
} catch {
|
} catch {
|
||||||
available = [];
|
available = [];
|
||||||
}
|
}
|
||||||
// Only validate when Pi gave us a non-empty list; otherwise allow (degraded).
|
// Only validate when Pi gave us a non-empty list; otherwise allow (degraded).
|
||||||
if (available.length > 0 && !available.some((m) => m.id === b.model)) {
|
if (available.length > 0 && !available.some(
|
||||||
return reply.code(400).send({ error: `Unknown model: ${b.model}` });
|
(candidate) => candidate.provider === b.provider && candidate.id === b.model,
|
||||||
|
)) {
|
||||||
|
return reply.code(400).send({
|
||||||
|
error: `Unknown model: ${b.provider ?? "unknown"}/${b.model}`,
|
||||||
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
const next: Settings = {
|
const next: Settings = {
|
||||||
|
|||||||
@@ -64,6 +64,33 @@ test("PUT /settings rejects an unknown model when a model list is available", as
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("PUT /settings validates provider and model as one composite identifier", async () => {
|
||||||
|
const { app, dir } = appWithTmpSettings({}, {
|
||||||
|
listModels: async () => [
|
||||||
|
{ provider: "provider-a", id: "shared-id", name: "A", reasoning: false },
|
||||||
|
],
|
||||||
|
});
|
||||||
|
try {
|
||||||
|
const wrongProvider = await app.inject({
|
||||||
|
method: "PUT", url: "/settings",
|
||||||
|
payload: {
|
||||||
|
workspace: "psd", provider: "provider-b", model: "shared-id", thinking: "low",
|
||||||
|
},
|
||||||
|
});
|
||||||
|
expect(wrongProvider.statusCode).toBe(400);
|
||||||
|
|
||||||
|
const exactPair = await app.inject({
|
||||||
|
method: "PUT", url: "/settings",
|
||||||
|
payload: {
|
||||||
|
workspace: "psd", provider: "provider-a", model: "shared-id", thinking: "low",
|
||||||
|
},
|
||||||
|
});
|
||||||
|
expect(exactPair.statusCode).toBe(200);
|
||||||
|
} finally {
|
||||||
|
rmSync(dir, { recursive: true, force: true });
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
test("PUT /settings allows any model when model list is empty (Pi unavailable)", async () => {
|
test("PUT /settings allows any model when model list is empty (Pi unavailable)", async () => {
|
||||||
const { app, dir } = appWithTmpSettings({}, { listModels: async () => [] });
|
const { app, dir } = appWithTmpSettings({}, { listModels: async () => [] });
|
||||||
try {
|
try {
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
import { test, expect } from "vitest";
|
import { test, expect, vi } from "vitest";
|
||||||
import { buildApp } from "../src/app.js";
|
import { buildApp } from "../src/app.js";
|
||||||
import { loadConfig } from "../src/config.js";
|
import { loadConfig } from "../src/config.js";
|
||||||
|
|
||||||
@@ -148,6 +148,20 @@ test("GET /models returns {models:[]} when listModels throws (graceful fallback)
|
|||||||
expect(res.json()).toEqual({ models: [] });
|
expect(res.json()).toEqual({ models: [] });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("GET /models logs a sanitized warning when listing fails", async () => {
|
||||||
|
const app = buildApp(loadConfig({ THT_HARNESS_DIR: "../harness" }), {
|
||||||
|
thtRunner: {} as any,
|
||||||
|
listModels: async () => { throw new Error("credential-value-must-not-appear"); },
|
||||||
|
});
|
||||||
|
const warn = vi.spyOn(app.log, "warn");
|
||||||
|
|
||||||
|
const res = await app.inject({ method: "GET", url: "/models" });
|
||||||
|
|
||||||
|
expect(res.json()).toEqual({ models: [] });
|
||||||
|
expect(JSON.stringify(warn.mock.calls)).not.toContain("credential-value-must-not-appear");
|
||||||
|
expect(warn).toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
test("GET /models with empty listModels stub returns empty array", async () => {
|
test("GET /models with empty listModels stub returns empty array", async () => {
|
||||||
const app = buildApp(loadConfig({ THT_HARNESS_DIR: "../harness" }), {
|
const app = buildApp(loadConfig({ THT_HARNESS_DIR: "../harness" }), {
|
||||||
thtRunner: {} as any,
|
thtRunner: {} as any,
|
||||||
|
|||||||
Reference in New Issue
Block a user