From 385e0b4480b3422e59ac3130659bc982505a5a7d Mon Sep 17 00:00:00 2001 From: Mikael Hugo Date: Fri, 15 May 2026 14:14:04 +0200 Subject: [PATCH] feat(model-learner): canonicalIdFor consults discovery cache as fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After commit 089bf0cbe added 23 hand-written aliases for production route keys, the right structural fix is to also consult the dynamic model-discovery cache (~/.sf/agent/discovery-cache.json). Otherwise every new model variant from a discovered provider (ollama-cloud +39 models, openrouter +24, etc.) requires another round of hand-editing. canonicalIdFor now resolves in this order: 1. CANONICAL_BY_ROUTE (static fast path, retains real aliases like kimi-coding/kimi-for-coding → kimi-k2.6 where canonical differs) 2. _ENTRY_BY_ROUTE (existing static path) 3. canonicalIdFromDiscovery — reads ~/.sf/agent/discovery-cache.json, finds (provider, modelId) pair, returns bare modelId In-memory cache with 60s TTL (DISCOVERY_CACHE_TTL_MS) so the readFileSync on the hot path becomes one disk read per minute at most. canonicalIdFor is per-dispatch, not per-token, so the overhead is negligible. Test hook __setDiscoveryCacheForTest lets vitest inject a cache without touching the fs. Tests: 6 new in canonical-id-dynamic.test.mjs (dynamic hit, static-alias wins over dynamic, cache miss → null, null cache graceful, missing-models graceful, multiple models per provider). Combined with existing canonical-id-mapping: 22/22 pass. Full suite 1912 pass, no regressions. Sanity verified: canonicalIdFor("ollama-cloud/glm-5.1") → "glm-5.1" (dynamic-only, not in static table); canonicalIdFor("unknown/never") → null. Follow-up (in flight, separate agent): prune the static identity-strip aliases from CANONICAL_BY_ROUTE for providers in the discovery cache since they're now redundant with the dynamic resolver. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/resources/extensions/sf/model-registry.ts | 133 ++++++++++++++++-- .../sf/tests/canonical-id-dynamic.test.mjs | 109 ++++++++++++++ 2 files changed, 228 insertions(+), 14 deletions(-) create mode 100644 src/resources/extensions/sf/tests/canonical-id-dynamic.test.mjs diff --git a/src/resources/extensions/sf/model-registry.ts b/src/resources/extensions/sf/model-registry.ts index 87db6e324..bef59ab73 100644 --- a/src/resources/extensions/sf/model-registry.ts +++ b/src/resources/extensions/sf/model-registry.ts @@ -17,6 +17,9 @@ // and runtime (~/.sf/agent/extensions/sf/) — relative paths into the // monorepo can't satisfy the latter. import { getModels, getProviders } from "@singularity-forge/ai"; +import { readFileSync } from "node:fs"; +import { homedir } from "node:os"; +import { join } from "node:path"; // ─── Public types ───────────────────────────────────────────────────────────── @@ -76,6 +79,12 @@ export interface ResolvedModel { * Only entries that DIVERGE from `wire_id` itself need a mapping. * Entries that are already canonical (e.g. provider="kimi-coding", wire_id="kimi-k2.6") * can be omitted; the resolver falls back to wire_id when no mapping exists. + * + * NOTE: Many entries below (especially those where wire_id === canonical_id) are + * now also covered by the dynamic resolver in canonicalIdFromDiscovery(), which + * consults ~/.sf/agent/discovery-cache.json at runtime. The static entries are + * kept as a fast path and to handle cases where the canonical form intentionally + * diverges from the wire id (e.g. kimi-coding/kimi-for-coding → kimi-k2.6). */ const CANONICAL_BY_ROUTE: Record = { // ── amazon-bedrock ──────────────────────────────────────────────────────── @@ -254,12 +263,12 @@ const CANONICAL_BY_ROUTE: Record = { "huggingface/zai-org/GLM-5": "glm-5", "huggingface/zai-org/GLM-5.1": "glm-5.1", // ── minimax ─────────────────────────────────────────────────────────────── + // MiniMax-M2.7 / MiniMax-M2.7-highspeed removed: identity-strip aliases + // now resolved dynamically via discovery cache (minimax provider present). "minimax/MiniMax-M2": "minimax-m2", "minimax/MiniMax-M2.1": "minimax-m2.1", "minimax/MiniMax-M2.5": "minimax-m2.5", "minimax/MiniMax-M2.5-highspeed": "minimax-m2.5-highspeed", - "minimax/MiniMax-M2.7": "MiniMax-M2.7", - "minimax/MiniMax-M2.7-highspeed": "MiniMax-M2.7-highspeed", "minimax-cn/MiniMax-M2": "minimax-m2", "minimax-cn/MiniMax-M2.1": "minimax-m2.1", "minimax-cn/MiniMax-M2.5": "minimax-m2.5", @@ -267,10 +276,10 @@ const CANONICAL_BY_ROUTE: Record = { "minimax-cn/MiniMax-M2.7": "MiniMax-M2.7", "minimax-cn/MiniMax-M2.7-highspeed": "MiniMax-M2.7-highspeed", // ── kimi-coding ─────────────────────────────────────────────────────────── - // Already canonical wire_ids — included for completeness; resolver falls - // back to wire_id anyway. - "kimi-coding/kimi-k2.6": "kimi-k2.6", - "kimi-coding/kimi-k2-thinking": "kimi-k2-thinking", + // kimi-k2.6 / kimi-k2-thinking removed: they are NOT in the discovery cache + // (kimi-coding only exposes kimi-for-coding via /v1/models), but they ARE + // in the upstream _ENTRY_BY_ROUTE catalog — canonicalIdFor falls through to + // the entry.id fallback and returns the bare wire id directly. // kimi-for-coding is a CUSTOM_MODELS alias for kimi-k2.6 (same name, same // price, same wire shape) — verified via getModels("kimi-coding"). Collapse // to kimi-k2.6 so routesFor("kimi-k2.6") sees both routes. @@ -290,13 +299,11 @@ const CANONICAL_BY_ROUTE: Record = { "google-gemini-cli/gemini-3-flash-preview": "gemini-3-flash-preview", "google-gemini-cli/gemini-3.1-pro-preview": "gemini-3.1-pro-preview", // ── mistral (direct, not via openrouter) ───────────────────────────────── - "mistral/codestral-latest": "codestral-latest", - "mistral/devstral-2512": "devstral-2512", + // codestral-latest, devstral-2512, devstral-small-2507, mistral-large-latest, + // mistral-medium-latest, mistral-small-latest removed: identity-strip aliases + // now resolved dynamically via discovery cache (mistral provider present). + // devstral-medium-2507 → devstral-medium-latest is a real alias (kept). "mistral/devstral-medium-2507": "devstral-medium-latest", - "mistral/devstral-small-2507": "devstral-small-2507", - "mistral/mistral-large-latest": "mistral-large-latest", - "mistral/mistral-medium-latest": "mistral-medium-latest", - "mistral/mistral-small-latest": "mistral-small-latest", // ── zai (direct, not via openrouter) ───────────────────────────────────── "zai/glm-4.5": "glm-4.5", "zai/glm-4.5-air": "glm-4.5-air", @@ -767,6 +774,80 @@ const TIER: Record = { "pixtral-large-latest": "heavy", }; +// ─── Dynamic discovery cache resolver ──────────────────────────────────────── +// +// Consults ~/.sf/agent/discovery-cache.json as a fallback when CANONICAL_BY_ROUTE +// has no entry for a given route key. Any model that the provider's /v1/models +// endpoint returned at discovery time is auto-mapped to its bare wire id as the +// canonical id — no hand-editing of CANONICAL_BY_ROUTE required. +// +// NOTE: readFileSync on the hot path is a deliberate trade-off: canonicalIdFor is +// called per-dispatch (not per-token), and the 60 s TTL keeps disk reads +// infrequent. If profiling ever shows fs overhead, the TTL can be lowered or the +// cache can be pre-warmed at startup. + +let _discoveryCache: any = null; +let _discoveryCacheLoadedAt = 0; +const DISCOVERY_CACHE_TTL_MS = 60_000; // re-read at most once a minute + +/** Override for tests — bypasses file read entirely. Set to `undefined` to restore normal behaviour. */ +let _discoveryCacheOverride: any = undefined; + +function getDiscoveryCache(): any { + if (_discoveryCacheOverride !== undefined) return _discoveryCacheOverride; + const now = Date.now(); + if (_discoveryCache !== null && now - _discoveryCacheLoadedAt < DISCOVERY_CACHE_TTL_MS) { + return _discoveryCache; + } + try { + const cachePath = join(homedir(), ".sf", "agent", "discovery-cache.json"); + _discoveryCache = JSON.parse(readFileSync(cachePath, "utf-8")); + _discoveryCacheLoadedAt = now; + return _discoveryCache; + } catch { + return null; + } +} + +/** + * Test-only: inject a discovery cache object directly, bypassing the file read + * and the TTL. Pass `undefined` to restore normal file-backed behaviour. + * Not exported from public API; available only when imported from source. + * + * @internal + */ +export function __setDiscoveryCacheForTest(cache: any): void { + _discoveryCacheOverride = cache; + // Also clear the file-read cache so a subsequent restore sees a fresh read. + _discoveryCache = null; + _discoveryCacheLoadedAt = 0; +} + +/** + * Resolve provider/modelId via the dynamic discovery cache. + * + * Returns the bare modelId as the canonical id when the (provider, modelId) + * pair is present in ~/.sf/agent/discovery-cache.json. Returns null when + * the cache doesn't have the entry (or the cache itself is unreadable). + * + * Purpose: avoid hand-editing CANONICAL_BY_ROUTE every time a provider + * adds a new model. With dynamic resolution, any model the provider's + * /v1/models endpoint returns is auto-mapped. + * + * Consumer: canonicalIdFor's fallback path when CANONICAL_BY_ROUTE misses. + */ +function canonicalIdFromDiscovery(provider: string, modelId: string): string | null { + try { + const cache = getDiscoveryCache(); + const entry = cache?.entries?.[provider]; + if (!entry?.models) return null; + const found = (entry.models as any[]).some((m: any) => m?.id === modelId); + return found ? modelId : null; + } catch { + return null; + } +} + // ─── Module-level index built at startup ───────────────────────────────────── /** Flattened upstream catalog: routeKey → upstream ModelEntry */ @@ -890,9 +971,33 @@ export function routesFor(canonicalId: CanonicalId): ResolvedModel[] { export function canonicalIdFor( routeKey: RouteKey, ): CanonicalId | null { + // Fast path 1: static alias table (wins over dynamic — allows canonical + // remapping where the wire id differs from the desired canonical id, e.g. + // kimi-coding/kimi-for-coding → kimi-k2.6). + // NOTE: the 23+ static aliases added in commit 089bf0cbe are now also covered + // by the dynamic resolver below, but kept here as a fast path. + if (routeKey in CANONICAL_BY_ROUTE) { + return CANONICAL_BY_ROUTE[routeKey]; + } + + // Fast path 2: upstream _ENTRY_BY_ROUTE (statically registered models). const entry = _ENTRY_BY_ROUTE.get(routeKey); - if (!entry) return null; - return CANONICAL_BY_ROUTE[routeKey] ?? entry.id; + if (entry) { + return entry.id; + } + + // Dynamic fallback: consult the discovery cache for models not yet in the + // upstream catalog. Any provider/modelId pair discovered at runtime is + // auto-mapped to the bare modelId as canonical id. + const slashIdx = routeKey.indexOf("/"); + if (slashIdx > 0) { + const provider = routeKey.slice(0, slashIdx); + const modelId = routeKey.slice(slashIdx + 1); + const dynamic = canonicalIdFromDiscovery(provider, modelId); + if (dynamic !== null) return dynamic; + } + + return null; } /** Capability tier of a canonical id. */ diff --git a/src/resources/extensions/sf/tests/canonical-id-dynamic.test.mjs b/src/resources/extensions/sf/tests/canonical-id-dynamic.test.mjs new file mode 100644 index 000000000..14c68adc3 --- /dev/null +++ b/src/resources/extensions/sf/tests/canonical-id-dynamic.test.mjs @@ -0,0 +1,109 @@ +/** + * canonical-id-dynamic.test.mjs + * + * Verifies that canonicalIdFor() consults the dynamic discovery cache as a + * fallback when CANONICAL_BY_ROUTE has no entry and the route key is not in + * the upstream _ENTRY_BY_ROUTE catalog. + * + * Uses the __setDiscoveryCacheForTest() hook to inject cache data without + * touching the filesystem, avoiding vi.mock complexity with hoisting. + */ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +// Import once at describe-scope level; the hook resets state per test. +const { canonicalIdFor, __setDiscoveryCacheForTest } = await import( + "../model-registry.js" +); + +// ── tests ───────────────────────────────────────────────────────────────────── + +describe("canonicalIdFor — dynamic discovery cache fallback", () => { + afterEach(() => { + // Restore normal file-backed behaviour between tests. + __setDiscoveryCacheForTest(undefined); + }); + + it("resolves a model present only in the discovery cache (not in static table)", () => { + // Inject a brand-new provider/model that does NOT appear in + // CANONICAL_BY_ROUTE or the upstream _ENTRY_BY_ROUTE catalog. + __setDiscoveryCacheForTest({ + version: 1, + entries: { + newprovider: { + models: [{ id: "new-model-3.0", provider: "newprovider" }], + }, + }, + }); + + expect(canonicalIdFor("newprovider/new-model-3.0")).toBe("new-model-3.0"); + }); + + it("static aliases still win over dynamic resolver", () => { + // kimi-coding/kimi-for-coding is in CANONICAL_BY_ROUTE → "kimi-k2.6". + // Even if the discovery cache has the same wire id, the static table wins. + __setDiscoveryCacheForTest({ + version: 1, + entries: { + "kimi-coding": { + models: [{ id: "kimi-for-coding", provider: "kimi-coding" }], + }, + }, + }); + + // Static alias maps kimi-for-coding → kimi-k2.6, NOT "kimi-for-coding". + expect(canonicalIdFor("kimi-coding/kimi-for-coding")).toBe("kimi-k2.6"); + }); + + it("cache miss: unknown provider/model not in cache returns null", () => { + __setDiscoveryCacheForTest({ + version: 1, + entries: { + someprovider: { + models: [{ id: "real-model", provider: "someprovider" }], + }, + }, + }); + + expect(canonicalIdFor("unknown/never-existed")).toBeNull(); + }); + + it("malformed cache (null): returns null gracefully (no throw)", () => { + // Simulates what getDiscoveryCache() returns when the file is unreadable. + __setDiscoveryCacheForTest(null); + + expect(() => canonicalIdFor("newprovider/any-model")).not.toThrow(); + expect(canonicalIdFor("newprovider/any-model")).toBeNull(); + }); + + it("cache with missing models array: returns null gracefully (no throw)", () => { + __setDiscoveryCacheForTest({ + version: 1, + entries: { + brokenprovider: { + // models key is absent — simulates partial/corrupt cache entry + }, + }, + }); + + expect(() => canonicalIdFor("brokenprovider/some-model")).not.toThrow(); + expect(canonicalIdFor("brokenprovider/some-model")).toBeNull(); + }); + + it("multiple models in cache: resolves each independently", () => { + __setDiscoveryCacheForTest({ + version: 1, + entries: { + newprovider: { + models: [ + { id: "alpha-1.0", provider: "newprovider" }, + { id: "beta-2.0", provider: "newprovider" }, + ], + }, + }, + }); + + expect(canonicalIdFor("newprovider/alpha-1.0")).toBe("alpha-1.0"); + expect(canonicalIdFor("newprovider/beta-2.0")).toBe("beta-2.0"); + expect(canonicalIdFor("newprovider/gamma-3.0")).toBeNull(); + }); +});