diff --git a/lib/fallback/engine.mjs b/lib/fallback/engine.mjs index e242bab..0750d64 100644 --- a/lib/fallback/engine.mjs +++ b/lib/fallback/engine.mjs @@ -18,6 +18,7 @@ import { homedir } from 'node:os'; import { join } from 'node:path'; import { ProviderError } from '../providers/base.mjs'; +import { getProviderForModel } from '../providers/index.mjs'; // ── Hard-trigger evaluation ──────────────────────────────────────────────── @@ -418,16 +419,24 @@ export function buildDefaultChain( })); } - // No explicit chain: find first loaded provider that serves this model - for (const [name, provider] of loadedProviders.entries()) { - if (provider.models && provider.models.includes(modelString)) { - return [{ - provider: name, - model: modelString, - softTriggers: softTriggerConfig[name] ?? null, - quotaSnapshot: null, - }]; - } + // No explicit chain: use getProviderForModel (SPOT for alias-aware routing). + // D17 Finding 13 fix: replaced the duplicated inline scan loop with this + // single call so alias resolution and provider lookup live in one place. + // result.canonicalModel carries the canonical ID even when user passed an + // alias. The canonical ID flows into: cache key (computeCacheKey arg), + // X-OLP-Model-Used header, and structured log events — so identical requests + // differing only in alias-vs-canonical now hit the same cache entry. + // provider.spawn continues to receive irRequest.model (the user's original + // input); each provider CLI accepts its own aliases natively per its docs, + // so no in-IR rewrite is needed for spawn correctness. + const match = getProviderForModel(loadedProviders, modelString); + if (match) { + return [{ + provider: match.name, + model: match.canonicalModel, + softTriggers: softTriggerConfig[match.name] ?? null, + quotaSnapshot: null, + }]; } // No provider found for this model diff --git a/lib/providers/index.mjs b/lib/providers/index.mjs index d973e81..c8e88a5 100644 --- a/lib/providers/index.mjs +++ b/lib/providers/index.mjs @@ -22,12 +22,17 @@ * after D-later E2E audit. Authority: ADR 0006 Tier D classification for * Mistral Vibe (Le Chat Pro); docs-based authority from * https://docs.mistral.ai/mistral-vibe/terminal/quickstart. + * + * D17 (Finding 12+13): alias resolution is centralised in getProviderForModel(). + * models-registry.json is the single source of truth for alias→canonical mapping. + * All provider plugins expose canonical IDs only in their models[] field. */ import { validateProvider } from './base.mjs'; import anthropicDefault from './anthropic.mjs'; import codexDefault from './codex.mjs'; import mistralDefault from './mistral.mjs'; +import modelsRegistryRaw from '../../models-registry.json' with { type: 'json' }; // Normalize default export pattern const anthropic = anthropicDefault; @@ -42,6 +47,28 @@ const STATIC_REGISTRY = [ mistral, ]; +// ── Alias map (built once at module load) ───────────────────────────────── +// Maps aliasString → { providerName: string, canonicalModel: string } +// Sourced from models-registry.json providers[*].aliases — the SPOT for alias +// definitions per D17 Finding 12 fix. Each provider plugin's models[] contains +// only canonical IDs; this map is consulted by getProviderForModel() to resolve +// alias strings before the direct-lookup fallback. +// +// Example entries: +// 'sonnet' → { providerName: 'anthropic', canonicalModel: 'claude-sonnet-4-6' } +// 'devstral' → { providerName: 'mistral', canonicalModel: 'devstral-2-25-12' } +// 'codex' → { providerName: 'openai', canonicalModel: 'gpt-5.3-codex' } +const _aliasMap = new Map(); +for (const [providerName, providerEntry] of Object.entries( + modelsRegistryRaw?.providers ?? {}, +)) { + for (const [alias, canonicalModel] of Object.entries( + providerEntry?.aliases ?? {}, + )) { + _aliasMap.set(alias, { providerName, canonicalModel }); + } +} + // ── Registry functions ──────────────────────────────────────────────────── /** @@ -71,22 +98,42 @@ export function loadProviders(config = {}) { } /** - * Returns the first provider in `loadedProviders` whose `models[]` includes - * the exact `modelString`. Returns null if no provider is found. + * Returns the provider in `loadedProviders` that serves `modelString`, with + * alias resolution. D17 Finding 12+13: this is the single lookup point (SPOT) + * for model→provider routing. All callers (including buildDefaultChain) must + * use this function instead of duplicating the scan loop. * - * This is the naive lookup strategy for D4. Phase 2+ may add prefix/alias - * matching once models-registry.json is consulted per AGENTS.md SPOT policy. + * Resolution order: + * 1. Alias lookup: if modelString is a known alias in models-registry.json + * AND the resolved provider is in loadedProviders (enabled), return the + * match with canonicalModel set to the canonical ID. + * 2. Direct lookup: scan loadedProviders for any p.models.includes(modelString) + * (handles canonical IDs passed directly). Returns canonicalModel=modelString. + * 3. null — no provider found. * * @param {Map} loadedProviders * @param {string} modelString - * @returns {{ provider: import('./base.mjs').ProviderContractV1, name: string }|null} + * @returns {{ provider: import('./base.mjs').ProviderContractV1, name: string, canonicalModel: string }|null} */ export function getProviderForModel(loadedProviders, modelString) { + // Step 1: alias resolution via models-registry.json + const aliasEntry = _aliasMap.get(modelString); + if (aliasEntry) { + const { providerName, canonicalModel } = aliasEntry; + const provider = loadedProviders.get(providerName); + if (provider) { + return { provider, name: providerName, canonicalModel }; + } + // Alias exists but target provider is not loaded (not enabled) — fall through. + } + + // Step 2: direct canonical lookup for (const [name, p] of loadedProviders) { if (p.models.includes(modelString)) { - return { provider: p, name }; + return { provider: p, name, canonicalModel: modelString }; } } + return null; } diff --git a/lib/providers/mistral.mjs b/lib/providers/mistral.mjs index 2c9e5d9..4fb1c71 100644 --- a/lib/providers/mistral.mjs +++ b/lib/providers/mistral.mjs @@ -742,19 +742,13 @@ function _defaultBinaryExists() { import modelsRegistryRaw from '../../models-registry.json' with { type: 'json' }; -// Build the `models` array from BOTH canonical date-stamped IDs and the -// short-form aliases per registry. ADR 0002 says `models: string[]` is "models -// this provider serves" — an alias that resolves to a real model is itself -// served. Including both makes getProviderForModel() naively route either form: -// model: "devstral-2-25-12" → mistral -// model: "devstral-2" → mistral (via alias) -// model: "devstral" → mistral (via alias) -// Phase-2 fallback engine + dashboard will use canonical IDs internally. +// Build the `models` array from canonical date-stamped IDs only — matching the +// shape used by anthropic.mjs and codex.mjs (canonical-only, no alias spread). +// D17 Finding 12 fix: alias resolution is handled in getProviderForModel() in +// lib/providers/index.mjs (the SPOT for alias-aware routing), not by inflating +// models[] with alias strings. This keeps models[] a pure canonical enumeration. const _registryEntry = modelsRegistryRaw?.providers?.mistral ?? {}; -const _registryModels = [ - ...(_registryEntry.models ?? []).map(m => m.id), - ...Object.keys(_registryEntry.aliases ?? {}), -]; +const _registryModels = (_registryEntry.models ?? []).map(m => m.id); const mistral = { name: 'mistral', diff --git a/test-features.mjs b/test-features.mjs index 9e095ba..0d555c0 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -533,6 +533,7 @@ describe('Plugin registry', () => { const r = getProviderForModel(m, 'alpha-v1'); assert.ok(r !== null); assert.equal(r.name, 'alpha'); + assert.equal(r.canonicalModel, 'alpha-v1', 'D17: canonicalModel must equal modelString on direct lookup'); }); it('getProviderForModel returns null for unknown model', () => { @@ -600,6 +601,133 @@ function makeMockSpawn(stdoutChunks, exitCode = 0) { }; } +// ── Suite D17: Alias-aware getProviderForModel (Finding 12 + 13) ───────────── +// +// Tests that getProviderForModel resolves aliases from models-registry.json +// to canonical IDs and routes to the correct (enabled) provider. + +describe('D17 — alias-aware getProviderForModel', () => { + + // ── Anthropic aliases ──────────────────────────────────────────────── + it('D17: alias "sonnet" → anthropic, canonical claude-sonnet-4-6', () => { + const loaded = new Map([['anthropic', anthropic]]); + const r = getProviderForModel(loaded, 'sonnet'); + assert.ok(r !== null); + assert.equal(r.name, 'anthropic'); + assert.equal(r.canonicalModel, 'claude-sonnet-4-6'); + }); + + it('D17: alias "claude" → anthropic, canonical claude-sonnet-4-6', () => { + const loaded = new Map([['anthropic', anthropic]]); + const r = getProviderForModel(loaded, 'claude'); + assert.ok(r !== null); + assert.equal(r.name, 'anthropic'); + assert.equal(r.canonicalModel, 'claude-sonnet-4-6'); + }); + + it('D17: alias "opus" → anthropic, canonical claude-opus-4-7', () => { + const loaded = new Map([['anthropic', anthropic]]); + const r = getProviderForModel(loaded, 'opus'); + assert.ok(r !== null); + assert.equal(r.name, 'anthropic'); + assert.equal(r.canonicalModel, 'claude-opus-4-7'); + }); + + it('D17: alias "haiku" → anthropic, canonical claude-haiku-4-5', () => { + const loaded = new Map([['anthropic', anthropic]]); + const r = getProviderForModel(loaded, 'haiku'); + assert.ok(r !== null); + assert.equal(r.name, 'anthropic'); + assert.equal(r.canonicalModel, 'claude-haiku-4-5'); + }); + + // ── OpenAI aliases ──────────────────────────────────────────────────── + it('D17: alias "codex" → openai, canonical gpt-5.3-codex', () => { + const loaded = new Map([['openai', codex]]); + const r = getProviderForModel(loaded, 'codex'); + assert.ok(r !== null); + assert.equal(r.name, 'openai'); + assert.equal(r.canonicalModel, 'gpt-5.3-codex'); + }); + + it('D17: alias "gpt5" → openai, canonical gpt-5.5', () => { + const loaded = new Map([['openai', codex]]); + const r = getProviderForModel(loaded, 'gpt5'); + assert.ok(r !== null); + assert.equal(r.name, 'openai'); + assert.equal(r.canonicalModel, 'gpt-5.5'); + }); + + // ── Mistral aliases ─────────────────────────────────────────────────── + it('D17: alias "devstral" → mistral, canonical devstral-2-25-12', () => { + const loaded = new Map([['mistral', mistral]]); + const r = getProviderForModel(loaded, 'devstral'); + assert.ok(r !== null); + assert.equal(r.name, 'mistral'); + assert.equal(r.canonicalModel, 'devstral-2-25-12'); + }); + + it('D17: alias "devstral-small" → mistral, canonical devstral-small-2-25-12', () => { + const loaded = new Map([['mistral', mistral]]); + const r = getProviderForModel(loaded, 'devstral-small'); + assert.ok(r !== null); + assert.equal(r.name, 'mistral'); + assert.equal(r.canonicalModel, 'devstral-small-2-25-12'); + }); + + // ── Canonical pass-through ──────────────────────────────────────────── + it('D17: canonical "claude-sonnet-4-6" → anthropic, canonicalModel unchanged', () => { + const loaded = new Map([['anthropic', anthropic]]); + const r = getProviderForModel(loaded, 'claude-sonnet-4-6'); + assert.ok(r !== null); + assert.equal(r.name, 'anthropic'); + assert.equal(r.canonicalModel, 'claude-sonnet-4-6'); + }); + + // ── Unknown model → null ────────────────────────────────────────────── + it('D17: unknown model "gpt-4-imaginary" → null', () => { + const loaded = new Map([['anthropic', anthropic], ['openai', codex], ['mistral', mistral]]); + assert.equal(getProviderForModel(loaded, 'gpt-4-imaginary'), null); + }); + + // ── Alias points to disabled provider → null ───────────────────────── + it('D17: alias "devstral" with only anthropic loaded → null (mistral not enabled)', () => { + // The alias is known (devstral → mistral) but mistral is not in loadedProviders. + const loaded = new Map([['anthropic', anthropic]]); + assert.equal(getProviderForModel(loaded, 'devstral'), null); + }); + + it('D17: alias "sonnet" with only mistral loaded → null (anthropic not enabled)', () => { + const loaded = new Map([['mistral', mistral]]); + assert.equal(getProviderForModel(loaded, 'sonnet'), null); + }); + + // ── buildDefaultChain with alias ────────────────────────────────────── + it('D17: buildDefaultChain("sonnet") → chain carries canonical claude-sonnet-4-6', () => { + const loaded = new Map([['anthropic', anthropic]]); + const chain = buildDefaultChain('sonnet', loaded, {}, {}); + assert.ok(chain !== null); + assert.equal(chain.length, 1); + assert.equal(chain[0].provider, 'anthropic'); + assert.equal(chain[0].model, 'claude-sonnet-4-6'); + }); + + it('D17: buildDefaultChain("devstral") → chain carries canonical devstral-2-25-12', () => { + const loaded = new Map([['mistral', mistral]]); + const chain = buildDefaultChain('devstral', loaded, {}, {}); + assert.ok(chain !== null); + assert.equal(chain.length, 1); + assert.equal(chain[0].provider, 'mistral'); + assert.equal(chain[0].model, 'devstral-2-25-12'); + }); + + it('D17: buildDefaultChain("unknown-alias") with no providers → null', () => { + const loaded = new Map(); + assert.equal(buildDefaultChain('unknown-alias', loaded, {}, {}), null); + }); + +}); + describe('Anthropic plugin (D4)', () => { // ── Test 1: Contract conformance ────────────────────────────────────── @@ -2692,51 +2820,65 @@ describe('Mistral Vibe plugin (D8)', () => { assert.ok(mistral.displayName.toLowerCase().includes('mistral')); }); - // ── Test 4: models include all registry IDs + aliases ──────────────── - it('mistral.models includes every canonical registry id AND every alias', () => { - // The plugin merges canonical IDs and alias keys into models[] so that - // getProviderForModel routes either form. Test that the registry's - // canonical IDs are a subset, and the aliases are all included too. + // ── Test 4: models is canonical-only (D17 Finding 12 fix) ─────────── + it('mistral.models contains only canonical registry IDs — no alias strings', () => { + // D17 fix: models[] is canonical-only across all plugins. Alias routing + // is the responsibility of getProviderForModel() in lib/providers/index.mjs. const registryIds = modelsRegistry.providers.mistral.models.map(m => m.id); const registryAliases = Object.keys(modelsRegistry.providers.mistral.aliases ?? {}); for (const id of registryIds) { assert.ok(mistral.models.includes(id), `canonical id ${id} missing from mistral.models`); } for (const alias of registryAliases) { - assert.ok(mistral.models.includes(alias), `alias ${alias} missing from mistral.models`); + assert.ok(!mistral.models.includes(alias), `alias ${alias} must NOT appear in mistral.models (D17)`); } - assert.equal(mistral.models.length, registryIds.length + registryAliases.length); + assert.equal(mistral.models.length, registryIds.length, `Expected ${registryIds.length} canonical IDs, got ${mistral.models.length}`); }); - it('mistral.models contains canonical IDs + short-form aliases', () => { - // Per canonical models registry (docs.mistral.ai/getting-started/models/ - // models_overview, D8 review-2 finding): canonical IDs are date-stamped - // (devstral-2-25-12, devstral-small-2-25-12). Vibe config.toml uses short - // forms (devstral-2, devstral-small-2). Plugin exposes BOTH so - // getProviderForModel routes either form. + it('mistral.models contains exactly the two canonical date-stamped IDs', () => { + // D17 fix: canonical-only shape. Length: 2 canonical IDs. assert.ok(mistral.models.includes('devstral-2-25-12'), 'missing canonical devstral-2-25-12'); assert.ok(mistral.models.includes('devstral-small-2-25-12'), 'missing canonical devstral-small-2-25-12'); - assert.ok(mistral.models.includes('devstral-2'), 'missing short-form alias devstral-2'); - assert.ok(mistral.models.includes('devstral-small-2'), 'missing short-form alias devstral-small-2'); - assert.ok(mistral.models.includes('devstral'), 'missing alias devstral'); - assert.ok(mistral.models.includes('devstral-small'), 'missing alias devstral-small'); - // Length: 2 canonical + 4 aliases = 6 total - assert.equal(mistral.models.length, 6, `Expected 6 entries (2 canonical + 4 aliases), got ${mistral.models.length}`); + assert.ok(!mistral.models.includes('devstral-2'), 'alias devstral-2 must NOT be in models[] (D17)'); + assert.ok(!mistral.models.includes('devstral-small-2'), 'alias devstral-small-2 must NOT be in models[] (D17)'); + assert.ok(!mistral.models.includes('devstral'), 'alias devstral must NOT be in models[] (D17)'); + assert.ok(!mistral.models.includes('devstral-small'), 'alias devstral-small must NOT be in models[] (D17)'); + // Length: 2 canonical only + assert.equal(mistral.models.length, 2, `Expected 2 canonical IDs, got ${mistral.models.length}`); }); // ── Test 5: getProviderForModel finds mistral for each model ────────── - it('getProviderForModel finds mistral for devstral-2', () => { + it('getProviderForModel finds mistral for canonical devstral-2-25-12', () => { + const loaded = new Map([['mistral', mistral]]); + const result = getProviderForModel(loaded, 'devstral-2-25-12'); + assert.ok(result !== null); + assert.equal(result.name, 'mistral'); + assert.equal(result.canonicalModel, 'devstral-2-25-12'); + }); + + it('getProviderForModel finds mistral for alias devstral-2 → canonical devstral-2-25-12 (D17)', () => { + // D17: alias resolution now handled in getProviderForModel, not in models[]. const loaded = new Map([['mistral', mistral]]); const result = getProviderForModel(loaded, 'devstral-2'); assert.ok(result !== null); assert.equal(result.name, 'mistral'); + assert.equal(result.canonicalModel, 'devstral-2-25-12'); }); - it('getProviderForModel finds mistral for devstral-small-2', () => { + it('getProviderForModel finds mistral for alias devstral → canonical devstral-2-25-12 (D17)', () => { + const loaded = new Map([['mistral', mistral]]); + const result = getProviderForModel(loaded, 'devstral'); + assert.ok(result !== null); + assert.equal(result.name, 'mistral'); + assert.equal(result.canonicalModel, 'devstral-2-25-12'); + }); + + it('getProviderForModel finds mistral for alias devstral-small-2 → canonical devstral-small-2-25-12 (D17)', () => { const loaded = new Map([['mistral', mistral]]); const result = getProviderForModel(loaded, 'devstral-small-2'); assert.ok(result !== null); assert.equal(result.name, 'mistral'); + assert.equal(result.canonicalModel, 'devstral-small-2-25-12'); }); // ── Test 6: irToMistral translation ────────────────────────────────── @@ -3224,7 +3366,11 @@ describe('Mistral Vibe plugin (D8)', () => { const p = loaded.get('mistral'); const { valid, errors } = validateProvider(p); assert.equal(valid, true, `Contract errors: ${errors.join('; ')}`); - assert.ok(p.models.includes('devstral-2')); + // D17: models[] is canonical-only. Verify via getProviderForModel instead of direct inclusion. + assert.ok(p.models.includes('devstral-2-25-12'), 'canonical devstral-2-25-12 must be in models[]'); + const r = getProviderForModel(loaded, 'devstral-2'); + assert.ok(r !== null, 'alias devstral-2 must route to mistral via getProviderForModel (D17)'); + assert.equal(r.name, 'mistral'); }); // ── Test 16: auth artifact reading helpers ────────────────────────────