From 3d23d0543cd64dfdab17c08b2980d3e7a589b06c Mon Sep 17 00:00:00 2001 From: User Date: Tue, 14 Jul 2026 18:39:37 +0200 Subject: [PATCH] fix(backend): validate configured model provider pair --- backend/src/app.ts | 9 +++++++-- backend/src/routes/meta.ts | 6 +++++- backend/src/routes/settings.ts | 10 +++++++--- backend/test/routes-settings.test.ts | 27 +++++++++++++++++++++++++++ backend/test/routes-sql-meta.test.ts | 16 +++++++++++++++- 5 files changed, 61 insertions(+), 7 deletions(-) diff --git a/backend/src/app.ts b/backend/src/app.ts index b3b965ea..9ad984f8 100644 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -23,7 +23,7 @@ export interface BuildAppDeps { } 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. app.register(cors, { @@ -45,7 +45,12 @@ export function buildApp(config: AppConfig, deps?: BuildAppDeps): FastifyInstanc 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 authenticate = authPreHandler(config.authMode); diff --git a/backend/src/routes/meta.ts b/backend/src/routes/meta.ts index bfb86be5..db8149c5 100644 --- a/backend/src/routes/meta.ts +++ b/backend/src/routes/meta.ts @@ -34,7 +34,11 @@ export function metaRoutes( const fn = deps.listModels ?? (async () => []); try { 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. return { models: [] as PiModel[] }; } diff --git a/backend/src/routes/settings.ts b/backend/src/routes/settings.ts index b21b6989..0f9516fc 100644 --- a/backend/src/routes/settings.ts +++ b/backend/src/routes/settings.ts @@ -25,15 +25,19 @@ export function settingsRoutes( app.put("/settings", async (req, reply) => { const b = (req.body ?? {}) as Settings; if (b.model) { - let available: { id: string }[] = []; + let available: { provider: string; id: string }[] = []; try { available = await deps.listModels(); } catch { available = []; } // Only validate when Pi gave us a non-empty list; otherwise allow (degraded). - if (available.length > 0 && !available.some((m) => m.id === b.model)) { - return reply.code(400).send({ error: `Unknown model: ${b.model}` }); + if (available.length > 0 && !available.some( + (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 = { diff --git a/backend/test/routes-settings.test.ts b/backend/test/routes-settings.test.ts index 2265f4dc..d83dc0d0 100644 --- a/backend/test/routes-settings.test.ts +++ b/backend/test/routes-settings.test.ts @@ -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 () => { const { app, dir } = appWithTmpSettings({}, { listModels: async () => [] }); try { diff --git a/backend/test/routes-sql-meta.test.ts b/backend/test/routes-sql-meta.test.ts index 0ce8a48a..8d56f65e 100644 --- a/backend/test/routes-sql-meta.test.ts +++ b/backend/test/routes-sql-meta.test.ts @@ -1,4 +1,4 @@ -import { test, expect } from "vitest"; +import { test, expect, vi } from "vitest"; import { buildApp } from "../src/app.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: [] }); }); +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 () => { const app = buildApp(loadConfig({ THT_HARNESS_DIR: "../harness" }), { thtRunner: {} as any,