From 88ba42fd3719ec9d23f5fc97de72653536ed112e Mon Sep 17 00:00:00 2001 From: amosblomqvist Date: Mon, 26 Oct 2026 18:50:54 +0200 Subject: [PATCH] fix: allow repairing model slots after subscription disconnect --- app/api/settings/[section]/route.ts | 14 ++- src/server/model-catalog.ts | 14 ++- tests/models-settings-repair.test.ts | 165 +++++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 10 deletions(-) create mode 100644 tests/models-settings-repair.test.ts diff --git a/app/api/settings/[section]/route.ts b/app/api/settings/[section]/route.ts index 616db9f..a51da1c 100644 --- a/app/api/settings/[section]/route.ts +++ b/app/api/settings/[section]/route.ts @@ -10,7 +10,8 @@ * (server/settings-body.ts): a bad shape/type is a 400 carrying the * supported list where one exists. The chat and models sections * additionally pass the catalog gate (validateChatSettings / - * validateModelsSettings — unknown provider/model = 400). Thinking + * validateModelsSettings — unknown provider/model = 400, except unchanged + * model-slot selections may remain saved while unavailable). Thinking * levels never reject a write (owner, interlude 2026-09-22): a pinned * level CLAMPS to the named model's nearest supported level and the * clamped value is stored — model swaps just work, same as the @@ -63,6 +64,8 @@ export async function PUT(request: Request, ctx: { params: Promise<{ section: st if (!parsed.ok) { return Response.json({ error: parsed.error }, { status: 400 }); } + // Compare against the same record that this whole-section write replaces. + const record = readSettingsRecord() ?? { version: 1 }; let agentHealed: AgentSettings | undefined; // Section-specific cross-validation (against the catalog / the prompt @@ -73,11 +76,11 @@ export async function PUT(request: Request, ctx: { params: Promise<{ section: st return Response.json({ error: verdict.error, supported: verdict.supported }, { status: 400 }); } } - // Model slots wave (2026-09-20): the models section gets the same - // catalog gate the chat section gets — a slot must name a real - // provider+model and a thinking level it supports. + // New/replaced slot models pass the catalog gate. Unchanged saved model + // references may survive disconnect, including thinking-preference edits, + // so each unavailable slot can be repaired independently of the others. if (section === "models") { - const verdict = validateModelsSettings(parsed.settings as ModelsSettings); + const verdict = validateModelsSettings(parsed.settings as ModelsSettings, record.models); if (!verdict.ok) { return Response.json({ error: verdict.error, supported: verdict.supported }, { status: 400 }); } @@ -124,7 +127,6 @@ export async function PUT(request: Request, ctx: { params: Promise<{ section: st // Read-modify-write ONE section of the record; everything else rides // along untouched (other sections, unknown keys — sanitize preserves). - const record = readSettingsRecord() ?? { version: 1 }; const next = { ...record, [section]: sectionOut }; writeSettingsRecord(next); return Response.json({ settings: next }); diff --git a/src/server/model-catalog.ts b/src/server/model-catalog.ts index 0aa0b2a..11fbe0f 100644 --- a/src/server/model-catalog.ts +++ b/src/server/model-catalog.ts @@ -245,10 +245,17 @@ export function validateChatSettings(settings: ChatSettings): SettingsValidation return { ok: true }; } -export function validateModelsSettings(settings: ModelsSettings): SettingsValidation { +/** Validate introduced/replaced model selections, not unchanged saved pins. + * A disconnected subscription loses its available catalog, but its stored + * selections must not prevent repairing another slot or editing thinking. + * Only the SAME slot's provider+id can pass through; runtime admission stays + * separate. Callers without a stored section retain strict catalog checking. */ +export function validateModelsSettings(settings: ModelsSettings, stored?: ModelsSettings): SettingsValidation { for (const name of MODEL_SLOTS) { const slot = settings[name]; if (!slot) continue; + const previous = stored?.[name]?.model; + if (slot.model && previous && slot.model.provider === previous.provider && slot.model.id === previous.id) continue; const verdict = validateSlotSpec(name, slot); if (!verdict.ok) return verdict; } @@ -280,9 +287,8 @@ function validateSlotSpec(name: ModelSlotName, slot: { model?: { provider: strin // runtime — and the clamped value is what lands in the record. The // server is the truth: the record shows the clamped level after write. -/** Clamp one level against one model (identity when the model is unknown - * or the level already supported — the existence gate rejects unknown - * models separately, so this only ever sees real ones in practice). */ +/** Clamp one level against one model. Preserve the preference when metadata + * is unavailable for an unchanged saved pin; runtime clamps after reconnect. */ function clampLevelFor(provider: string, id: string, level: ThinkingLevel): ThinkingLevel { const model = connectionModel(provider, id); return model ? clampThinkingLevel(model, level) : level; diff --git a/tests/models-settings-repair.test.ts b/tests/models-settings-repair.test.ts new file mode 100644 index 0000000..224e08d --- /dev/null +++ b/tests/models-settings-repair.test.ts @@ -0,0 +1,165 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { after, beforeEach, test } from "node:test"; +import type { ModelsSettings, ModelSlotName } from "../src/core/settings.ts"; +import type { NativeSubscriptionSnapshot } from "../src/core/contracts/subscriptions.ts"; + +// Every app import follows isolation. No installed credentials, ambient keys, +// OAuth, or provider requests: subscription state comes from synthetic IPC. +const directory = fs.mkdtempSync(path.join(os.tmpdir(), "lh-models-repair-")); +process.env.LH_DATA_DIR = directory; +process.env.LH_AUTH_FILE = path.join(directory, "auth.json"); +process.env.LH_IGNORE_ENV_KEYS = "1"; +fs.writeFileSync(process.env.LH_AUTH_FILE, "{}"); +const originalSend = Object.getOwnPropertyDescriptor(process, "send"); +const originalConnected = Object.getOwnPropertyDescriptor(process, "connected"); +const marker = process.env.LH_DESKTOP_SERVER, secret = process.env.LH_DESKTOP_API_SECRET; +process.env.LH_DESKTOP_SERVER = "1"; +process.env.LH_DESKTOP_API_SECRET = "d".repeat(64); +Object.defineProperty(process, "connected", { configurable: true, value: true }); + +const { piModels, piEngineForSpec } = await import("../src/agent/models.ts"); +const { refreshSubscriptionModels } = await import("../src/agent/subscription-bridge.ts"); +const { modelCatalog, validateModelsSettings } = await import("../src/server/model-catalog.ts"); +const { readSettings: readSettingsRecord, writeSettings: writeSettingsRecord } = await import("../src/store/settings.ts"); +const { PUT } = await import("../app/api/settings/[section]/route.ts"); +const { clampThinkingLevel, normalizeContext } = await import("@earendil-works/pi-ai"); +const native = piModels().getModels("github-copilot")[0]!; +const copilot = { provider: "github-copilot-subscription", id: native.id }; +const replacement = { provider: "openrouter", id: "z-ai/glm-5.3-flash" }; +const slots: ModelSlotName[] = ["strong", "cheap", "vision"]; +const initial: ModelsSettings = Object.fromEntries(slots.map((slot) => [slot, { model: copilot, thinkingLevel: "high" }])); +let connected = false, revision = 0, leases = 0; +function snapshot(): NativeSubscriptionSnapshot { + return { + statuses: [{ providerId: "github-copilot", available: true, connected, pending: false, allowBackgroundUse: true, connectionId: connected ? "fixture-connection" : null, epoch: revision, authorityRevision: revision, revision }], + models: { "github-copilot": connected ? [native] : [] }, + }; +} +Object.defineProperty(process, "send", { configurable: true, value: (message: { id: string; op: string }, callback?: () => void) => { + if (message.op !== "snapshot") { leases++; throw new Error("No credential lease expected"); } + queueMicrotask(() => (process.emit as (event: string, value: unknown) => boolean)("message", { channel: "lh:subscriptions", id: message.id, op: message.op, ok: true, value: snapshot() })); + callback?.(); return true; +} }); +async function connection(value: boolean) { connected = value; revision++; await refreshSubscriptionModels(); } +async function put(body: unknown) { + return PUT(new Request("http://localhost/api/settings/models", { method: "PUT", headers: { "content-type": "application/json" }, body: JSON.stringify(body) }), { params: Promise.resolve({ section: "models" }) }); +} +beforeEach(async () => { + await connection(false); + writeSettingsRecord({ version: 1, models: initial, general: { autoUpdate: false }, graph: { rendering: false } }); +}); +after(() => { + if (originalSend) Object.defineProperty(process, "send", originalSend); else delete process.send; + if (originalConnected) Object.defineProperty(process, "connected", originalConnected); else delete (process as { connected?: boolean }).connected; + for (const [key, value] of [["LH_DESKTOP_SERVER", marker], ["LH_DESKTOP_API_SECRET", secret]]) { + if (value === undefined) delete process.env[key!]; else process.env[key!] = value; + } + fs.rmSync(directory, { recursive: true, force: true }); +}); + +for (const slot of slots) test(`${slot} can be replaced while the other disconnected slots remain saved`, async () => { + const response = await put({ ...initial, [slot]: { model: replacement, thinkingLevel: "xhigh" } }); + assert.equal(response.status, 200, JSON.stringify(await response.clone().json())); + const stored = readSettingsRecord()!; + assert.deepEqual(stored.models?.[slot], { model: replacement, thinkingLevel: "max" }); + for (const other of slots.filter((name) => name !== slot)) assert.deepEqual(stored.models?.[other], initial[other]); + assert.deepEqual(stored.general, { autoUpdate: false }); + assert.deepEqual(stored.graph, { rendering: false }); + assert.deepEqual((await response.json()).settings, stored); + assert(!modelCatalog().models.some((model) => model.provider === copilot.provider), "saving stale pins must not make them available choices"); +}); + +test("all three slots can be repaired successively without reconnecting", async () => { + for (const slot of slots) { + const response = await put({ ...readSettingsRecord()!.models, [slot]: { model: replacement } }); + assert.equal(response.status, 200); + } + assert.deepEqual(readSettingsRecord()!.models, Object.fromEntries(slots.map((slot) => [slot, { model: replacement }]))); +}); + +test("thinking preferences on unchanged disconnected models can be changed, removed, and restored", async () => { + for (const thinkingLevel of ["max", undefined, "minimal"] as const) { + const strong = { model: copilot, ...(thinkingLevel ? { thinkingLevel } : {}) }; + const response = await put({ ...initial, strong }); + assert.equal(response.status, 200); + assert.deepEqual(readSettingsRecord()!.models?.strong, strong); + } + await connection(true); + const engine = piEngineForSpec({ ...copilot, thinkingLevel: "minimal" }, false); + assert.equal(engine.level, clampThinkingLevel(native, "minimal"), "reconnected runtime clamps the saved preference"); + assert.equal((await put(readSettingsRecord()!.models)).status, 200); + assert.equal(readSettingsRecord()!.models?.strong?.thinkingLevel, engine.level, "known metadata still clamps on write"); +}); + +test("introduced or replaced unknown models still reject, without writing any section", async () => { + for (const model of [ + { provider: copilot.provider, id: "fixture-not-in-catalog" }, + { provider: "xai-subscription", id: copilot.id }, + { provider: "fixture-unknown-provider", id: copilot.id }, + { provider: replacement.provider, id: "fixture-not-in-catalog" }, + ]) { + const before = readSettingsRecord(); + assert.equal((await put({ ...initial, strong: { model } })).status, 400); + assert.deepEqual(readSettingsRecord(), before); + } + writeSettingsRecord({ version: 1, models: { strong: initial.strong } }); + assert.equal((await put({ strong: initial.strong, cheap: initial.strong })).status, 400, "a pin saved in one slot cannot authorize another slot"); + assert.equal((await put(initial)).status, 400, "missing slots cannot introduce disconnected models"); +}); + +test("a first models write cannot introduce disconnected selections", async () => { + fs.unlinkSync(path.join(directory, "settings.json")); + assert.equal((await put(initial)).status, 400); + assert.equal(readSettingsRecord(), null); +}); + +test("a replaced stale selection cannot be reintroduced without availability", async () => { + assert.equal((await put({ ...initial, strong: { model: replacement } })).status, 200); + assert.equal((await put(initial)).status, 400); + assert.deepEqual(readSettingsRecord()!.models?.strong?.model, replacement); +}); + +test("slot removal still replaces the whole section, including clearing all slots", async () => { + assert.equal((await put({ cheap: initial.cheap })).status, 200); + assert.deepEqual(readSettingsRecord()!.models, { cheap: initial.cheap }); + assert.equal((await put({})).status, 200); + assert.deepEqual(readSettingsRecord()!.models, {}); +}); + +test("unchanged model identity does not bypass body shape validation", async () => { + for (const body of [ + { ...initial, strong: { model: copilot, thinkingLevel: 7 } }, + { ...initial, strong: { thinkingLevel: "high" } }, + { ...initial, strong: { model: { provider: copilot.provider } } }, + ]) assert.equal((await put(body)).status, 400); + assert.deepEqual(readSettingsRecord()!.models, initial); +}); + +test("standalone catalog validation stays strict without a stored section", () => { + assert.equal(validateModelsSettings(initial).ok, false); + assert.equal(validateModelsSettings({ strong: { model: replacement } }).ok, true); +}); + +test("retained selections remain unavailable at runtime, with no API-key fallback or credential lease", async () => { + await connection(true); + const engine = piEngineForSpec(copilot, false); + await connection(false); + assert.equal((await put({ ...initial, cheap: { model: replacement } })).status, 200); + assert.throws(() => piEngineForSpec(copilot, false), /catalog has no model/); + let requests = 0; + const before = leases; + const key = process.env.COPILOT_GITHUB_TOKEN; + process.env.COPILOT_GITHUB_TOKEN = "fixture-must-not-be-used"; + try { + const result = await engine.streamFn(engine.model, normalizeContext({ messages: [] }), { fetch: async () => { requests++; throw new Error("Must not dispatch"); } }).result(); + assert.equal(result.stopReason, "error"); + assert.match(result.errorMessage ?? "", /No API-key fallback was used/); + assert.equal(requests, 0); + assert.equal(leases, before); + } finally { + if (key === undefined) delete process.env.COPILOT_GITHUB_TOKEN; else process.env.COPILOT_GITHUB_TOKEN = key; + } +});