diff --git a/CHANGELOG.md b/CHANGELOG.md index 64c50dec2..b1e87877b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ Docs: https://docs.clawd.bot - Agents: preserve subagent announce thread/topic routing + queued replies across channels. (#1241) — thanks @gnarco. - Agents: avoid treating timeout errors with "aborted" messages as user aborts, so model fallback still runs. - Diagnostics: export OTLP logs, correct queue depth tracking, and document message-flow telemetry. +- Model catalog: avoid caching import failures, log transient discovery errors, and keep partial results. (#1332) — thanks @dougvk. - Doctor: clarify plugin auto-enable hint text in the startup banner. - Gateway: clarify unauthorized handshake responses with token/password mismatch guidance. - UI: keep config form enums typed, preserve empty strings, protect sensitive defaults, and deepen config search. (#1315) — thanks @MaudeBot. diff --git a/src/agents/model-catalog.test.ts b/src/agents/model-catalog.test.ts new file mode 100644 index 000000000..2a6d97934 --- /dev/null +++ b/src/agents/model-catalog.test.ts @@ -0,0 +1,82 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import type { ClawdbotConfig } from "../config/config.js"; +import { + __setModelCatalogImportForTest, + loadModelCatalog, + resetModelCatalogCacheForTest, +} from "./model-catalog.js"; + +type PiSdkModule = typeof import("@mariozechner/pi-coding-agent"); + +vi.mock("./models-config.js", () => ({ + ensureClawdbotModelsJson: vi.fn().mockResolvedValue({ agentDir: "/tmp", wrote: false }), +})); + +vi.mock("./agent-paths.js", () => ({ + resolveClawdbotAgentDir: () => "/tmp/clawdbot", +})); + +describe("loadModelCatalog", () => { + beforeEach(() => { + resetModelCatalogCacheForTest(); + }); + + afterEach(() => { + __setModelCatalogImportForTest(); + resetModelCatalogCacheForTest(); + vi.restoreAllMocks(); + }); + + it("retries after import failure without poisoning the cache", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + let call = 0; + + __setModelCatalogImportForTest(async () => { + call += 1; + if (call === 1) { + throw new Error("boom"); + } + return { + discoverAuthStorage: () => ({}), + discoverModels: () => [{ id: "gpt-4.1", name: "GPT-4.1", provider: "openai" }], + } as unknown as PiSdkModule; + }); + + const cfg = {} as ClawdbotConfig; + const first = await loadModelCatalog({ config: cfg }); + expect(first).toEqual([]); + + const second = await loadModelCatalog({ config: cfg }); + expect(second).toEqual([{ id: "gpt-4.1", name: "GPT-4.1", provider: "openai" }]); + expect(call).toBe(2); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); + + it("returns partial results on discovery errors", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); + + __setModelCatalogImportForTest( + async () => + ({ + discoverAuthStorage: () => ({}), + discoverModels: () => ({ + getAll: () => [ + { id: "gpt-4.1", name: "GPT-4.1", provider: "openai" }, + { + get id() { + throw new Error("boom"); + }, + provider: "openai", + name: "bad", + }, + ], + }), + }) as unknown as PiSdkModule, + ); + + const result = await loadModelCatalog({ config: {} as ClawdbotConfig }); + expect(result).toEqual([{ id: "gpt-4.1", name: "GPT-4.1", provider: "openai" }]); + expect(warnSpy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/agents/model-catalog.ts b/src/agents/model-catalog.ts index 8043eb9b6..3a3db7a28 100644 --- a/src/agents/model-catalog.ts +++ b/src/agents/model-catalog.ts @@ -18,10 +18,22 @@ type DiscoveredModel = { reasoning?: boolean; }; +type PiSdkModule = typeof import("@mariozechner/pi-coding-agent"); + let modelCatalogPromise: Promise | null = null; +let hasLoggedModelCatalogError = false; +const defaultImportPiSdk = () => import("@mariozechner/pi-coding-agent"); +let importPiSdk = defaultImportPiSdk; export function resetModelCatalogCacheForTest() { modelCatalogPromise = null; + hasLoggedModelCatalogError = false; + importPiSdk = defaultImportPiSdk; +} + +// Test-only escape hatch: allow mocking the dynamic import to simulate transient failures. +export function __setModelCatalogImportForTest(loader?: () => Promise) { + importPiSdk = loader ?? defaultImportPiSdk; } export async function loadModelCatalog(params?: { @@ -34,16 +46,21 @@ export async function loadModelCatalog(params?: { if (modelCatalogPromise) return modelCatalogPromise; modelCatalogPromise = (async () => { + const models: ModelCatalogEntry[] = []; + const sortModels = (entries: ModelCatalogEntry[]) => + entries.sort((a, b) => { + const p = a.provider.localeCompare(b.provider); + if (p !== 0) return p; + return a.name.localeCompare(b.name); + }); try { + const cfg = params?.config ?? loadConfig(); + await ensureClawdbotModelsJson(cfg); // IMPORTANT: keep the dynamic import *inside* the try/catch. // If this fails once (e.g. during a pnpm install that temporarily swaps node_modules), // we must not poison the cache with a rejected promise (otherwise all channel handlers // will keep failing until restart). - const piSdk = await import("@mariozechner/pi-coding-agent"); - - const models: ModelCatalogEntry[] = []; - const cfg = params?.config ?? loadConfig(); - await ensureClawdbotModelsJson(cfg); + const piSdk = await importPiSdk(); const agentDir = resolveClawdbotAgentDir(); const authStorage = piSdk.discoverAuthStorage(agentDir); const registry = piSdk.discoverModels(authStorage, agentDir) as @@ -71,14 +88,17 @@ export async function loadModelCatalog(params?: { modelCatalogPromise = null; } - return models.sort((a, b) => { - const p = a.provider.localeCompare(b.provider); - if (p !== 0) return p; - return a.name.localeCompare(b.name); - }); - } catch { + return sortModels(models); + } catch (error) { + if (!hasLoggedModelCatalogError) { + hasLoggedModelCatalogError = true; + console.warn(`[model-catalog] Failed to load model catalog: ${String(error)}`); + } // Don't poison the cache on transient dependency/filesystem issues. modelCatalogPromise = null; + if (models.length > 0) { + return sortModels(models); + } return []; } })();