Merge pull request #2 from d-buckner/review/manual-model-ids-must-fixes

fix: never lose custom endpoint models or pins on a failed probe

app/api/custom-provider/route.ts

@@ -1,11 +1,17 @@
 /**
  * The custom endpoint's connect surface (one provider in the Keys list).
- *   GET    → non-secret config { baseUrl, models, overrides } + keyPresent (masked).
- *   POST   → connect: validate + normalize the base URL, probe /models (fail
- *            fast), store the key, persist { baseUrl, models }, re-sync the
- *            provider into piModels(). Discovery lives here because connecting
- *            and discovering are one user action. Stored user pins survive a
- *            reconnect, pruned to the ids the endpoint still advertises.
+ *   GET    → non-secret config { baseUrl, models, overrides, manualModels } +
+ *            keyPresent (masked).
+ *   POST   → connect: validate + normalize the base URL, probe /models, store
+ *            the key, persist { baseUrl, models, overrides, manualModels },
+ *            re-sync the provider into piModels(). Discovery lives here because
+ *            connecting and discovering are one user action. A failed probe is
+ *            fatal UNLESS the user typed model IDs: an endpoint that serves chat
+ *            but no model list is configured from those ids alone. A failed
+ *            probe for the endpoint already stored keeps its last snapshot, so
+ *            a blip cannot prune the stored pins. A 401/403 always fails: a
+ *            rejected key is not a missing model list. Stored user pins survive
+ *            a reconnect, pruned to the ids the connect ends up offering.
  *   PATCH  → store per-model pins only. No probe: pins must be editable with
  *            the local server switched off (the case they exist for).
  *   DELETE → disconnect: drop the stored config + the stored key and unregister
@@ -48,11 +54,25 @@ export async function POST(request: Request) {
 	const key = keyProvided ? (body!.key as string).trim() : undefined;
 
 	const manualModels = parseModelIds(body?.manualModels);
+	const record = readSettingsRecord() ?? { version: 1 };
 	const found = await discoverModels(base, key ? key : undefined);
+	// A rejected key is not a missing model list: never let typed ids turn an
+	// auth failure into a stored "connected" endpoint.
+	if (!found.ok && (found.status === 401 || found.status === 403)) {
+		return Response.json({ error: found.error }, { status: 502 });
+	}
 	if (!found.ok && !manualModels.length) {
 		return Response.json({ error: `${found.error}. If this endpoint has no /models list, add model IDs manually.` }, { status: 502 });
 	}
-	const models = mergeManualModels(found.ok ? found.models : [], manualModels);
+	// A failed probe says nothing about what the endpoint offers: keep the last
+	// snapshot of the SAME endpoint, so a transient failure cannot shrink the
+	// model list and prune its pins. A different base URL starts clean.
+	const discovered = found.ok
+		? found.models
+		: record.customProvider?.baseUrl === base
+			? (record.customProvider?.models ?? [])
+			: [];
+	const models = mergeManualModels(discovered, manualModels);
 
 	if (keyProvided) {
 		if (key) {
@@ -63,9 +83,8 @@ export async function POST(request: Request) {
 		}
 	}
 
-	const record = readSettingsRecord() ?? { version: 1 };
-	// User pins are never rewritten by discovery — only pruned to what the
-	// endpoint still advertises (the single prune law, core/settings.ts).
+	// User pins are never rewritten by discovery — only pruned to the ids this
+	// connect offers (discovered ∪ typed).
 	const overrides = sanitizeCustomModelOverrides(
 		record.customProvider?.overrides,
 		models.map((m) => m.id),

src/core/settings.ts

@@ -254,11 +254,16 @@ export interface CustomModelOverride {
 export interface CustomProviderSettings {
 	/** Normalized base URL (see normalizeBaseUrl). Empty/absent = not configured. */
 	baseUrl?: string;
-	/** Models discovered from the endpoint (refreshed on connect). */
+	/** Models this endpoint offers: advertised ids merged with the typed
+	 *  manual ones (rewritten on connect; a failed probe keeps the same
+	 *  endpoint's last snapshot). */
 	models?: CustomModel[];
 	/** User pins keyed by model id — pure user intent, never rewritten by
-	 *  discovery, pruned to the endpoint's live ids on connect. */
+	 *  discovery, pruned on connect to the ids that connect offers. */
 	overrides?: Record<string, CustomModelOverride>;
+	/** Model ids the user typed by hand, for an endpoint that publishes no
+	 *  usable /models list. Kept apart from the snapshot so a reconnect can
+	 *  re-merge them after discovery refreshes. */
 	manualModels?: string[];
 }
 

src/server/custom-provider.ts

@@ -101,8 +101,12 @@ export function syncCustomProvider(cp: CustomProviderSettings | undefined): void
 	);
 }
 
-/** Outcome of a model-discovery probe. */
-export type DiscoverResult = { ok: true; models: CustomModel[] } | { ok: false; error: string };
+/** Outcome of a model-discovery probe. `status` is the endpoint's HTTP status
+ *  when it answered at all, so callers can tell a rejected key from a missing
+ *  model list. */
+export type DiscoverResult =
+	| { ok: true; models: CustomModel[] }
+	| { ok: false; error: string; status?: number };
 
 /** Probe GET {baseUrl}/models for model ids. Scheme validated (http/https
  *  only); keyless endpoints answer without a bearer. Non-2xx or unparseable
@@ -118,7 +122,7 @@ export async function discoverModels(baseUrl: string, apiKey?: string): Promise<
 			headers: key ? { authorization: `Bearer ${key}` } : {},
 			signal: controller.signal,
 		});
-		if (!res.ok) return { ok: false, error: `Endpoint returned ${res.status} ${res.statusText}` };
+		if (!res.ok) return { ok: false, error: `Endpoint returned ${res.status} ${res.statusText}`, status: res.status };
 		const body = (await res.json().catch(() => null)) as unknown;
 		const list = extractModels(body);
 		if (list === null) return { ok: false, error: "Endpoint response was not a model list" };
@@ -131,6 +135,9 @@ export async function discoverModels(baseUrl: string, apiKey?: string): Promise<
 	}
 }
 
+/** The connect's model list: what the endpoint advertises, plus every typed id
+ *  it does not advertise (at default limits, so the pins can correct them).
+ *  Discovered metadata always wins over a typed id of the same name. */
 export function mergeManualModels(discovered: readonly CustomModel[], manualIds: readonly string[]): CustomModel[] {
 	const byId = new Map(discovered.map((m) => [m.id, m]));
 	for (const id of manualIds) if (!byId.has(id)) byId.set(id, { id });

tests/custom-provider-route.test.ts

@@ -0,0 +1,129 @@
+/**
+ * POST /api/custom-provider — the connect policy around manually typed model
+ * ids. Two rules earn a route test (the pure pieces live in
+ * custom-endpoint.test.ts):
+ *   1. Typed ids make a FAILED probe survivable, but never a rejected key.
+ *   2. A failed probe never shrinks the snapshot it already stored: that prune
+ *      would take the model metadata and the user's pins with it.
+ */
+import assert from "node:assert/strict";
+import * as fs from "node:fs";
+import * as os from "node:os";
+import * as path from "node:path";
+import { after, before, describe, it } from "node:test";
+
+const dir = fs.mkdtempSync(path.join(os.tmpdir(), "lh-custom-route-"));
+process.env.LH_DATA_DIR = dir;
+process.env.LH_AUTH_FILE = path.join(dir, "auth.json");
+const SETTINGS_FILE = path.join(dir, "settings.json");
+
+const realFetch = globalThis.fetch;
+after(() => {
+	globalThis.fetch = realFetch;
+	fs.rmSync(dir, { recursive: true, force: true });
+	delete process.env.LH_DATA_DIR;
+	delete process.env.LH_AUTH_FILE;
+});
+
+type Route = typeof import("../app/api/custom-provider/route.ts");
+let route: Route;
+before(async () => {
+	route = await import("../app/api/custom-provider/route.ts");
+});
+
+const BASE = "http://h/v1";
+/** A connected endpoint with two discovered models, a pin on each, one typed id. */
+const CONNECTED = {
+	version: 1,
+	customProvider: {
+		baseUrl: BASE,
+		models: [{ id: "disc-1", context: 131072 }, { id: "disc-2" }],
+		overrides: { "disc-1": { context: 200000 }, "disc-2": { maxTokens: 8192 } },
+		manualModels: ["m1"],
+	},
+};
+
+function seed(record: unknown): void {
+	fs.writeFileSync(SETTINGS_FILE, JSON.stringify(record, null, "\t"), "utf-8");
+}
+
+function stored(): { customProvider?: { models?: { id: string }[]; overrides?: unknown; manualModels?: string[] } } {
+	return JSON.parse(fs.readFileSync(SETTINGS_FILE, "utf-8"));
+}
+
+function answerWith(status: number, statusText: string, body?: unknown): void {
+	globalThis.fetch = (async () => (body === undefined ? new Response("nope", { status, statusText }) : Response.json(body))) as typeof fetch;
+}
+
+function post(payload: unknown): Promise<Response> {
+	return route.POST(
+		new Request("http://localhost/api/custom-provider", {
+			method: "POST",
+			headers: { "content-type": "application/json" },
+			body: JSON.stringify(payload),
+		}),
+	);
+}
+
+describe("POST /api/custom-provider — typed ids and a failed probe", () => {
+	it("a transient failure keeps the stored models, their metadata, and their pins", async () => {
+		seed(CONNECTED);
+		answerWith(503, "Service Unavailable");
+
+		const res = await post({ baseUrl: BASE, manualModels: "m1" });
+		assert.equal(res.status, 200);
+
+		const cp = stored().customProvider;
+		assert.deepEqual(cp?.models?.map((m) => m.id), ["disc-1", "disc-2", "m1"]);
+		assert.deepEqual(cp?.models?.[0], { id: "disc-1", context: 131072 }, "detected metadata survives");
+		assert.deepEqual(cp?.overrides, CONNECTED.customProvider.overrides, "the prune must not fire on a failed probe");
+		assert.deepEqual(cp?.manualModels, ["m1"]);
+	});
+
+	it("a rejected key fails the connect even when ids are typed", async () => {
+		seed(CONNECTED);
+		answerWith(401, "Unauthorized");
+		const before = stored();
+
+		const res = await post({ baseUrl: BASE, key: "sk-abcdefghijk", manualModels: "m1" });
+		assert.equal(res.status, 502);
+		assert.deepEqual(stored(), before, "nothing is stored on an auth failure");
+	});
+
+	it("a new endpoint with no model list starts from the typed ids alone", async () => {
+		seed(CONNECTED);
+		answerWith(404, "Not Found");
+
+		const res = await post({ baseUrl: "http://other/v1", manualModels: "y, x" });
+		assert.equal(res.status, 200);
+
+		const cp = stored().customProvider;
+		assert.deepEqual(cp?.models?.map((m) => m.id), ["x", "y"], "the previous endpoint's models do not follow the URL");
+		assert.equal(cp?.overrides, undefined, "no pin survives a list the old pins do not belong to");
+		assert.deepEqual(cp?.manualModels, ["y", "x"], "the typed order is kept as typed");
+	});
+
+	it("no model list and no typed ids still refuses, with the hint", async () => {
+		answerWith(404, "Not Found");
+
+		const res = await post({ baseUrl: BASE });
+		assert.equal(res.status, 502);
+		const body = (await res.json()) as { error: string };
+		assert.match(body.error, /404/);
+		assert.match(body.error, /add model IDs manually/);
+	});
+
+	it("a working probe merges the typed ids and keeps discovered metadata", async () => {
+		seed(CONNECTED);
+		answerWith(200, "OK", { data: [{ id: "disc-1", context_length: 65536 }] });
+
+		const res = await post({ baseUrl: BASE, manualModels: "disc-1, m2" });
+		assert.equal(res.status, 200);
+
+		const cp = stored().customProvider;
+		assert.deepEqual(cp?.models?.map((m) => m.id), ["disc-1", "m2"]);
+		assert.deepEqual(cp?.models?.[0], { id: "disc-1", context: 65536 }, "discovery is not replaced by a bare typed id");
+		assert.deepEqual(cp?.overrides, { "disc-1": { context: 200000 } }, "pins follow the ids this connect offers");
+		assert.deepEqual(cp?.manualModels, ["disc-1", "m2"]);
+	});
+});