refactor(routing): D17 — alias-aware getProviderForModel as routing SPOT (Findings 12 + 13)

cold-audit catch from 2026-05-23 Cold-audit Findings 12 + 13 (both P3, both routing layer). F12: mistral plugin's models[] included canonical IDs + aliases while anthropic/codex were canonical-only — inconsistent routing surface (request "sonnet" returned 503 from anthropic but request "devstral" worked for mistral). F13: getProviderForModel imported but never called; buildDefaultChain duplicated the lookup loop — two SPOT-candidate code paths. Per cold-audit reviewer's option (a): standardize models[] on canonical- only across all 3 plugins; getProviderForModel becomes alias-aware via models-registry.json; buildDefaultChain uses getProviderForModel as SPOT. Changes (4 files, +242 / -50): 1. lib/providers/index.mjs: - At module load, builds `_aliasMap: Map<aliasString, {providerName, canonicalModel}>` by walking models-registry.json's providers[*].aliases - getProviderForModel signature extended: returns `{provider, name, canonicalModel}` (was `{provider, name}`). The canonicalModel field is the resolved canonical ID for callers that need to use it downstream (cache key, observability headers, log events) - 3-step resolution order: (1) alias map lookup → if hit AND provider loaded → return with canonicalModel; (2) direct canonical scan → return with canonicalModel = modelString; (3) null - "Alias known but provider not loaded" case correctly falls through to step 2 (which will also miss for an alias string) → returns null 2. lib/providers/mistral.mjs: - _registryModels stripped to canonical-only: removed the `...Object.keys(_registryEntry.aliases ?? {})` spread - mistral.models[] now matches anthropic/codex shape (canonical IDs only) - Comment block updated to point future readers at getProviderForModel as the SPOT 3. lib/fallback/engine.mjs: - buildDefaultChain's inline `for ([name, provider] of loadedProviders)` scan loop replaced with a single getProviderForModel() call - Chain hop's `model` field is now `match.canonicalModel` (was `modelString`) — so downstream consumers receive canonical - Explicit-chain config path unchanged (kept its pre-existing behavior; alias resolution in routing.chains config is a known limitation, tracked as a future improvement) 4. test-features.mjs: - 17 new tests in `D17 — alias-aware getProviderForModel` describe block: anthropic alias coverage (sonnet/opus/haiku/claude), openai aliases (codex/codex-spark/gpt5/gpt5-mini), mistral aliases (devstral/ devstral-2/devstral-small/devstral-small-2), canonical pass-through, unknown model → null, alias-to-disabled-provider → null, buildDefaultChain integration with alias resolution - 3 existing mistral tests rewritten — they were asserting the pre-D17 inconsistent shape (aliases in mistral.models[]). Now assert the post-D17 invariant (aliases NOT in models[]; routing via getProviderForModel instead) Tests: 300 → 317 (+17 new). All pass on Node 20. Pre-commit fold-in (per evidence-first checkpoint #4 — fold-ins themselves need second-pass review): - **D17 reviewer flagged C10**: original engine.mjs comment said "downstream (cache key, X-OLP-Model-Used, provider.spawn) receives the canonical ID rather than the alias string". The provider.spawn portion is incorrect — spawn reads `irRequest.model` (the user's original input), which is NEVER rewritten to canonical. Each provider CLI accepts its own aliases natively (claude accepts sonnet/opus/haiku; vibe accepts devstral/devstral-2; codex accepts its model IDs), so runtime behavior is fine, but the comment overstated what D17 actually changes. Folded in: corrected comment to honestly describe the canonical flow (cache key + X-OLP-Model-Used + logs receive canonical; spawn continues to receive irRequest.model). Same class of doc-code drift fix as D11's B1 (false maxConcurrent enforcement claim) and D16's "bypasses getOrCompute" drift — the discipline is maturing across D-days. Authority: - ADR 0002 § Provider contract (`models: string[]` is "models this provider serves") https://github.com/dtzp555-max/olp/blob/main/docs/adr/0002-plugin-architecture.md - models-registry.json — single source of truth for (provider, model) metadata + alias→canonical mappings per AGENTS.md SPOT policy - AGENTS.md § Project-specific constraints — "models-registry.json is the only place to add/edit (provider, model) metadata" - CC 开发铁律 v1.6 § 10.x — Cold Audit Findings 12 + 13 Reviewer (Iron Rule v1.6 § 10.x Mode A, fresh-context opus, independent of drafter): APPROVE_WITH_MINOR. Highest-value verification: walked chain[0].model from buildDefaultChain through all downstream consumers (computeCacheKey, X-OLP-Model-Used, log events) to confirm canonical flow; then verified provider.spawn paths in anthropic.mjs:231 and codex.mjs:248 read irRequest.model (user input), confirming the comment overstatement (now fixed). Verified no alias-canonical collision exists in current registry (12 aliases vs 10 canonical IDs, zero intersection). Verified empty-registry edge case + provider-with-model-not-in-registry edge case. Follow-up items (reviewer's non-blocking observations, NOT in this PR): - server.mjs:32 dead import of getProviderForModel — defer to D19 cleanup - explicit-chain config path doesn't run alias resolution (routing.chains in ~/.olp/config.json) — pre-existing limitation, file as future issue Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-07-19 09:45:07 +00:00 · 2026-05-24 11:55:43 +10:00
parent bafa6d1991
commit cb86807009
4 changed files with 246 additions and 50 deletions
@@ -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
@@ -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<string, import('./base.mjs').ProviderContractV1>} 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;
 }

@@ -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',