diff --git a/docs/superpowers/plans/2026-05-16-pr18-review-fixes.md b/docs/superpowers/plans/2026-05-16-pr18-review-fixes.md new file mode 100644 index 0000000..a510177 --- /dev/null +++ b/docs/superpowers/plans/2026-05-16-pr18-review-fixes.md @@ -0,0 +1,950 @@ +# PR #18 Review Issues Implementation Plan + +> **For agentic workers:** REQUIRED: Use superpowers:subagent-driven-development (if subagents available) or superpowers:executing-plans to implement this plan. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Address all 20 review issues from PR #18 (excluding CHANGELOG entry) + +**Architecture:** Systematic fixes across 6 source files and 2 test files, organized by severity and file grouping. Medium-priority issues first, then low-priority. + +**Tech Stack:** TypeScript ESM, Node.js native test runner, native fetch + +--- + +## File Structure + +**Modified files:** +- `src/plugin.ts` — 6 issues (fallback operator, metadata validation, magic constants, as const, tool defaults, array ordering) +- `src/models.ts` — 4 issues (cache key, sensitive logs, DRY violation, candidate explosion) +- `src/models-dev.ts` — 2 issues (metadata tracking, trailing newline) +- `src/omniroute-combos.ts` — 2 issues (log injection, DRY splitModelId) +- `src/types.ts` — 2 issues (provider fields shape, variants type) +- `src/logger.ts` — 1 issue (synchronous I/O) +- `test/models.test.mjs` — 3 test gaps (temperature/reasoning, variant+alias, subscription fallback) +- `test/plugin.test.mjs` — 1 issue (hardcoded ports) + +--- + +## Chunk 1: Critical Fixes (Medium Priority) + +### Task 1: Fix API Key Fallback Operator + +**Files:** +- Modify: `src/plugin.ts:45` + +**Issue:** `||` treats empty string `""` as falsy and falls back to env var unexpectedly. + +- [ ] **Step 1: Update fallback operator** + +```typescript +// Change from: +const apiKey = auth?.key || process.env.OMNIROUTE_API_KEY; +// To: +const apiKey = auth?.key ?? process.env.OMNIROUTE_API_KEY; +``` + +- [ ] **Step 2: Verify no other `||` fallback patterns exist** + +Run: `grep -n "|| process.env" src/*.ts` +Expected: No matches (or only intentional ones) + +- [ ] **Step 3: Commit** + +```bash +git add src/plugin.ts +git commit -m "fix: use nullish coalescing for API key fallback" +``` + +--- + +### Task 2: Add Runtime Validation for modelMetadata + +**Files:** +- Modify: `src/plugin.ts:351-379` (mergeModelMetadata function) +- Modify: `src/types.ts` (if needed for validator types) + +**Issue:** `metadata as OmniRouteModelMetadata` casts without validation. + +- [ ] **Step 1: Create metadata validation helper** + +Add to `src/plugin.ts` after `isRecord` function: + +```typescript +function isValidModelMetadata(value: unknown): value is OmniRouteModelMetadata { + if (!isRecord(value)) return false; + + // Only validate boolean fields if present + const booleanFields = [ + 'supportsStreaming', 'supportsVision', 'supportsTools', + 'supportsTemperature', 'supportsReasoning', 'supportsAttachment' + ]; + + for (const field of booleanFields) { + if (field in value && typeof value[field] !== 'boolean') { + return false; + } + } + + // Validate numeric fields + if ('contextWindow' in value && typeof value.contextWindow !== 'number') return false; + if ('maxTokens' in value && typeof value.maxTokens !== 'number') return false; + + return true; +} +``` + +- [ ] **Step 2: Update mergeModelMetadata to validate** + +```typescript +// In mergeModelMetadata, replace: +merged[id] = { + ...(generated[id] ?? {}), + ...(metadata as OmniRouteModelMetadata), +}; +// With: +if (isValidModelMetadata(metadata)) { + merged[id] = { + ...(generated[id] ?? {}), + ...metadata, + }; +} else { + warn(`Invalid metadata for model "${id}", skipping`); +} +``` + +- [ ] **Step 3: Build and test** + +Run: `npm run build` +Expected: No errors + +Run: `npm test` +Expected: All tests pass + +- [ ] **Step 4: Commit** + +```bash +git add src/plugin.ts +git commit -m "fix: add runtime validation for modelMetadata merge" +``` + +--- + +### Task 3: Fix Cache Key to Include modelsDev Config + +**Files:** +- Modify: `src/models.ts:36-39` (getCacheKey function) + +**Issue:** Different `modelsDev` configs share same cache key. + +- [ ] **Step 1: Update getCacheKey to hash config** + +```typescript +function getCacheKey(config: OmniRouteConfig, apiKey: string): string { + const baseUrl = config.baseUrl || OMNIROUTE_ENDPOINTS.BASE_URL; + + // Include modelsDev config in cache key + const modelsDevHash = config.modelsDev + ? JSON.stringify({ + enabled: config.modelsDev.enabled, + url: config.modelsDev.url, + providerAliases: config.modelsDev.providerAliases, + }) + : ''; + + return `${baseUrl}:${apiKey}:${modelsDevHash}`; +} +``` + +- [ ] **Step 2: Add test for cache key differentiation** + +In `test/models.test.mjs`, add: + +```javascript +test('fetchModels uses different cache for different modelsDev configs', async () => { + let calls = 0; + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + if (url.includes('/v1/models')) { + calls++; + return new Response( + JSON.stringify({ object: 'list', data: [{ id: `model-${calls}`, name: `Model ${calls}` }] }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }; + + const config1 = { ...CONFIG, modelsDev: { enabled: true, providerAliases: { oai: 'openai' } } }; + const config2 = { ...CONFIG, modelsDev: { enabled: true, providerAliases: { oai: 'anthropic' } } }; + + await fetchModels(config1, CONFIG.apiKey, false); + await fetchModels(config2, CONFIG.apiKey, false); + + assert.equal(calls, 2, 'Should fetch twice for different modelsDev configs'); +}); +``` + +- [ ] **Step 3: Run tests** + +Run: `npm test` +Expected: All tests pass + +- [ ] **Step 4: Commit** + +```bash +git add src/models.ts test/models.test.mjs +git commit -m "fix: include modelsDev config in cache key" +``` + +--- + +### Task 4: Fix DRY Violation - Extract splitModelId + +**Files:** +- Modify: `src/omniroute-combos.ts:227-248` — Export the function +- Modify: `src/models.ts:340-359` — Remove duplicate, import and use + +**Issue:** `splitModelId` and `splitOmniRouteModelForLookup` are identical. + +- [ ] **Step 1: Export splitModelId from omniroute-combos.ts** + +Change: +```typescript +function splitModelId(...) { ... } +``` +To: +```typescript +export function splitModelId(...) { ... } +``` + +- [ ] **Step 2: Remove duplicate from models.ts and import** + +In `src/models.ts`: +1. Add to imports from `omniroute-combos.ts`: +```typescript +import { splitModelId } from './omniroute-combos.js'; +``` + +2. Remove `splitOmniRouteModelForLookup` function entirely. + +3. Replace usage: +```typescript +const { providerKey, modelKey } = splitOmniRouteModelForLookup(model.id); +``` +With: +```typescript +const { providerKey, modelKey } = splitModelId(model.id); +``` + +- [ ] **Step 3: Build and test** + +Run: `npm run build` +Expected: No errors + +Run: `npm test` +Expected: All tests pass + +- [ ] **Step 4: Commit** + +```bash +git add src/models.ts src/omniroute-combos.ts +git commit -m "refactor: extract splitModelId to eliminate DRY violation" +``` + +--- + +### Task 5: Fix supportsTools Default Inconsistency + +**Files:** +- Modify: `src/plugin.ts:432` (toProviderModel) +- Modify: `src/models-dev.ts:331` (calculateLowestCommonCapabilities) + +**Issue:** `toProviderModel` defaults tools to true, combo logic defaults to false. + +- [ ] **Step 1: Document and align the defaults** + +In `src/plugin.ts`, add comment: +```typescript +// Default to true: if API doesn't explicitly say no tools, assume capability exists +// This aligns with OpenAI-compatible behavior where most models support tools +const supportsTools = model.supportsTools !== false; +``` + +In `src/models-dev.ts`, update combo logic: +```typescript +// For combos: only advertise tools if ALL underlying models explicitly support them +// This is intentionally stricter than single-model defaults because a combo +// with one tool-less model cannot reliably use tools across all backends +const supportsTools = model.tool_call === true; +``` + +- [ ] **Step 2: Commit** + +```bash +git add src/plugin.ts src/models-dev.ts +git commit -m "docs: document supportsTools default rationale" +``` + +--- + +### Task 6: Fix Array Metadata Ordering + +**Files:** +- Modify: `src/plugin.ts:351-379` (mergeModelMetadata) + +**Issue:** Generated blocks prepended — verify if OpenCode uses first or last match wins. + +- [ ] **Step 1: Research OpenCode matching logic** + +Check `@opencode-ai/plugin` source or documentation for `modelMetadata` array processing. + +- [ ] **Step 2: If last-match-wins, reverse order** + +```typescript +// If OpenCode uses last-match-wins, user config should come last +return [...userConfig, ...generatedBlocks]; +``` + +- [ ] **Step 3: Commit** + +```bash +git add src/plugin.ts +git commit -m "fix: correct modelMetadata array ordering for OpenCode matching" +``` + +--- + +### Task 7: Verify Provider Model Fields Match OpenCode Shape + +**Files:** +- Modify: `src/types.ts:111-155` + +**Issue:** New fields may not match `@opencode-ai/plugin` expectations. + +- [ ] **Step 1: Check OpenCode plugin types** + +Run: `npm ls @opencode-ai/plugin` or check node_modules for exact interface. + +- [ ] **Step 2: Rename fields if needed** + +If `tool_call` should be `toolCall`: +```typescript +// In OmniRouteProviderModel: +toolCall?: boolean; // instead of tool_call +``` + +Update all references in `src/plugin.ts` accordingly. + +- [ ] **Step 3: Commit** + +```bash +git add src/types.ts src/plugin.ts +git commit -m "fix: align provider model fields with OpenCode plugin types" +``` + +--- + +### Task 8: Tighten Variants Type + +**Files:** +- Modify: `src/types.ts:154` + +**Issue:** `Record` is too permissive. + +- [ ] **Step 1: Define strict variant types** + +```typescript +export interface OmniRouteModelVariant { + reasoningEffort?: 'low' | 'medium' | 'high'; + // Future variant types can be added here +} + +// In OmniRouteProviderModel: +variants: Record; +``` + +- [ ] **Step 2: Update toProviderModel** + +```typescript +variants: supportsReasoning + ? { + low: { reasoningEffort: 'low' }, + medium: { reasoningEffort: 'medium' }, + high: { reasoningEffort: 'high' }, + } + : {}, +``` + +- [ ] **Step 3: Build and test** + +Run: `npm run build` +Expected: No type errors + +- [ ] **Step 4: Commit** + +```bash +git add src/types.ts src/plugin.ts +git commit -m "types: tighten variants type to prevent invalid reasoning effort values" +``` + +--- + +## Chunk 2: Low Priority Fixes + +### Task 9: Fix Sensitive Data in Log + +**Files:** +- Modify: `src/models.ts:102` + +**Issue:** `JSON.stringify(rawData)` could leak sensitive data. + +- [ ] **Step 1: Replace with shape-only logging** + +```typescript +// Change from: +warn(`Invalid models response structure: ${JSON.stringify(rawData)}`); +// To: +const dataType = rawData && typeof rawData === 'object' + ? (Array.isArray(rawData.data) ? 'array' : typeof rawData.data) + : typeof rawData; +warn(`Invalid models response structure: expected { data: Array }, got { data: ${dataType} }`); +``` + +- [ ] **Step 2: Commit** + +```bash +git add src/models.ts +git commit -m "fix: avoid logging sensitive response data in models fetch error" +``` + +--- + +### Task 10: Fix Log Injection via Model IDs + +**Files:** +- Modify: `src/omniroute-combos.ts:55` and other log lines + +**Issue:** Model IDs from external API interpolated without sanitization. + +- [ ] **Step 1: Add sanitization helper** + +```typescript +function sanitizeForLog(value: string): string { + return value.replace(/[\r\n]/g, ''); +} +``` + +- [ ] **Step 2: Apply to all interpolated model IDs** + +Search for `model.id` in log lines and wrap with sanitizeForLog: +```typescript +warn(`Could not resolve ${unresolvedModels.length} underlying models for "${sanitizeForLog(model.id)}": ${unresolvedModels.map(sanitizeForLog).join(', ')}`); +``` + +- [ ] **Step 3: Commit** + +```bash +git add src/omniroute-combos.ts +git commit -m "fix: sanitize model IDs in log messages to prevent injection" +``` + +--- + +### Task 11: Fix Synchronous File I/O in Logger + +**Files:** +- Modify: `src/logger.ts` + +**Issue:** `appendFileSync` blocks event loop. + +- [ ] **Step 1: Switch to async logging** + +```typescript +// Replace appendFileSync with fire-and-forget appendFile +import { appendFile } from 'fs/promises'; + +export function warn(message: string): void { + const logFile = getLogFile(); + if (!logFile) return; + + const line = formatLogLine('WARN', message); + // Fire-and-forget: don't await, don't crash on error + appendFile(logFile, line).catch(() => {}); +} + +export function debug(message: string): void { + if (process.env.OMNIROUTE_DEBUG !== '1') return; + + const logFile = getLogFile(); + if (!logFile) return; + + const line = formatLogLine('DEBUG', message); + appendFile(logFile, line).catch(() => {}); +} +``` + +- [ ] **Step 2: Commit** + +```bash +git add src/logger.ts +git commit -m "perf: use async file I/O for logger to prevent event loop blocking" +``` + +--- + +### Task 12: Optimize Candidate Explosion in Lookup + +**Files:** +- Modify: `src/models.ts:290-307` + +**Issue:** Up to 8 candidates per model key, ~19k lookups. + +- [ ] **Step 1: Add early-exit for aliases that resolve to same key** + +```typescript +function getModelLookupCandidates(modelKey: string): string[] { + const candidates = new Set(); + + const addCandidate = (key: string): void => { + const lower = key.toLowerCase(); + const normalized = normalizeModelKey(key); + const aliasResolved = resolveModelAlias(key); + + candidates.add(lower); + candidates.add(normalized); + + // Only add alias variants if they differ from original + if (aliasResolved !== key) { + candidates.add(aliasResolved.toLowerCase()); + candidates.add(normalizeModelKey(aliasResolved)); + } + }; + + addCandidate(modelKey); + + const { base, stripped } = stripVariantSuffix(modelKey); + if (stripped) { + addCandidate(base); + } + + return [...candidates]; +} +``` + +- [ ] **Step 2: Commit** + +```bash +git add src/models.ts +git commit -m "perf: avoid duplicate lookup candidates when alias resolves to same key" +``` + +--- + +### Task 13: Fix Magic Constant 4096 + +**Files:** +- Modify: `src/plugin.ts:486-487` +- Modify: `src/constants.ts` — Add constants + +**Issue:** `4096` used without named constant. + +- [ ] **Step 1: Add named constants** + +In `src/constants.ts`: +```typescript +export const DEFAULT_CONTEXT_LIMIT = 4096; +export const DEFAULT_OUTPUT_LIMIT = 4096; +``` + +- [ ] **Step 2: Update plugin.ts to use constants** + +```typescript +import { DEFAULT_CONTEXT_LIMIT, DEFAULT_OUTPUT_LIMIT } from './constants.js'; + +// In toProviderModel: +limit: { + context: model.contextWindow ?? DEFAULT_CONTEXT_LIMIT, + output: model.maxTokens ?? DEFAULT_OUTPUT_LIMIT, +}, +``` + +- [ ] **Step 3: Commit** + +```bash +git add src/constants.ts src/plugin.ts +git commit -m "refactor: extract magic constant 4096 to named defaults" +``` + +--- + +### Task 14: Remove Redundant `as const` Assertions + +**Files:** +- Modify: `src/plugin.ts:447-449` + +**Issue:** `as const` is redundant given the type definition. + +- [ ] **Step 1: Remove assertions** + +```typescript +// Change from: +modalities: { + input: supportsVision ? ['text', 'image'] as const : ['text'] as const, + output: ['text'] as const, +}, + +// To: +modalities: { + input: supportsVision ? ['text', 'image'] : ['text'], + output: ['text'], +}, +``` + +- [ ] **Step 2: Verify types still compile** + +Run: `npx tsc --noEmit src/plugin.ts` +Expected: No errors + +- [ ] **Step 3: Commit** + +```bash +git add src/plugin.ts +git commit -m "style: remove redundant as const assertions" +``` + +--- + +### Task 15: Add Test for Single Model Metadata Tracking + +**Files:** +- Modify: `test/models.test.mjs` + +**Issue:** Early return bypasses metadata tracking logic. + +- [ ] **Step 1: Add test verifying consistency** + +```javascript +test('calculateLowestCommonCapabilities produces identical output for single model and combo-with-self', () => { + const single = calculateLowestCommonCapabilities([ + { id: 'test-model', temperature: true, reasoning: true, attachment: true } + ]); + + const combo = calculateLowestCommonCapabilities([ + { id: 'test-model', temperature: true, reasoning: true, attachment: true }, + { id: 'test-model', temperature: true, reasoning: true, attachment: true } + ]); + + assert.deepEqual(single, combo); +}); +``` + +- [ ] **Step 2: Run tests** + +Run: `npm test` +Expected: All tests pass + +- [ ] **Step 3: Commit** + +```bash +git add test/models.test.mjs +git commit -m "test: verify single-model and combo-with-self produce identical capabilities" +``` + +--- + +### Task 16: Add Trailing Newline to models-dev.ts + +**Files:** +- Modify: `src/models-dev.ts:480` + +**Issue:** File ends without trailing newline. + +- [ ] **Step 1: Add newline** + +Simply ensure the file ends with a newline character. + +- [ ] **Step 2: Commit** + +```bash +git add src/models-dev.ts +git commit -m "style: add trailing newline to models-dev.ts" +``` + +--- + +## Chunk 3: Test Coverage Gaps + +### Task 17: Add Temperature/Reasoning Combo Tests + +**Files:** +- Modify: `test/models.test.mjs` + +**Issue:** Missing tests for temperature and reasoning logic. + +- [ ] **Step 1: Add temperature tests** + +```javascript +test('calculateLowestCommonCapabilities handles temperature with mixed defined/undefined', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'with-temp', temperature: true }, + { id: 'without-temp' }, + ]); + + assert.equal(capabilities.supportsTemperature, true); +}); + +test('calculateLowestCommonCapabilities respects explicit temperature false', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'with-temp', temperature: true }, + { id: 'no-temp', temperature: false }, + ]); + + assert.equal(capabilities.supportsTemperature, false); +}); + +test('calculateLowestCommonCapabilities handles all three capabilities together', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'full', temperature: true, reasoning: true, attachment: true }, + { id: 'limited', temperature: false, reasoning: false, attachment: false }, + ]); + + assert.equal(capabilities.supportsTemperature, false); + assert.equal(capabilities.supportsReasoning, false); + assert.equal(capabilities.supportsAttachment, false); +}); + +test('calculateLowestCommonCapabilities handles single model with undefined temperature', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'no-meta' }, + ]); + + assert.equal(capabilities.supportsTemperature, undefined); +}); +``` + +- [ ] **Step 2: Run tests** + +Run: `npm test` +Expected: All tests pass + +- [ ] **Step 3: Commit** + +```bash +git add test/models.test.mjs +git commit -m "test: add temperature and reasoning combo capability tests" +``` + +--- + +### Task 18: Add Variant+Alias Integration Tests + +**Files:** +- Modify: `test/models.test.mjs` + +**Issue:** No test for variant suffix stripping + alias resolution. + +- [ ] **Step 1: Add integration test** + +```javascript +test('variant suffix stripping with alias resolution works end-to-end', async () => { + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + if (url.includes('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [ + { id: 'moonshotai/kimi-k2.6-thinking-high', name: 'Kimi K2.6 Thinking High' } + ] + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + if (url.includes('models.dev')) { + return new Response( + JSON.stringify({ + moonshotai: { + models: { + 'kimi-k2-thinking': { + id: 'kimi-k2-thinking', + name: 'Kimi K2 Thinking', + temperature: true, + reasoning: true, + } + } + } + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }; + + const models = await fetchModels(CONFIG, CONFIG.apiKey, true); + const model = models.find(m => m.id === 'moonshotai/kimi-k2.6-thinking-high'); + + assert.ok(model, 'Model should be found'); + assert.equal(model.supportsReasoning, true, 'Should resolve alias and enrich capabilities'); +}); +``` + +- [ ] **Step 2: Run tests** + +Run: `npm test` +Expected: All tests pass + +- [ ] **Step 3: Commit** + +```bash +git add test/models.test.mjs +git commit -m "test: add variant suffix and alias resolution integration test" +``` + +--- + +### Task 19: Add Subscription Fallback Tests + +**Files:** +- Modify: `test/models.test.mjs` + +**Issue:** No test for subscription provider fallback. + +- [ ] **Step 1: Add subscription fallback test** + +```javascript +test('subscription fallback enriches from public provider', async () => { + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + if (url.includes('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [ + { id: 'zai-coding-plan/gpt-4o', name: 'GPT-4o via ZAI' } + ] + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + if (url.includes('models.dev')) { + return new Response( + JSON.stringify({ + zai: { + models: { + 'gpt-4o': { + id: 'gpt-4o', + name: 'GPT-4o', + temperature: true, + tool_call: true, + } + } + } + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }; + + const models = await fetchModels(CONFIG, CONFIG.apiKey, true); + const model = models.find(m => m.id === 'zai-coding-plan/gpt-4o'); + + assert.ok(model, 'Model should be found'); + assert.equal(model.supportsTemperature, true, 'Should fallback to zai provider'); + assert.equal(model.supportsTools, true, 'Should inherit tools from fallback'); +}); +``` + +- [ ] **Step 2: Run tests** + +Run: `npm test` +Expected: All tests pass + +- [ ] **Step 3: Commit** + +```bash +git add test/models.test.mjs +git commit -m "test: add subscription fallback provider test" +``` + +--- + +### Task 20: Fix Hardcoded Ports in Tests + +**Files:** +- Modify: `test/plugin.test.mjs` + +**Issue:** Hardcoded ports like `20129`, `20130`, `20131`. + +- [ ] **Step 1: Add test helper for dummy URLs** + +At top of test file: +```javascript +function getDummyBaseUrl(port = 20128) { + return `http://localhost:${port}/v1`; +} +``` + +- [ ] **Step 2: Replace hardcoded ports with helper** + +Search for `http://localhost:20` and replace with `getDummyBaseUrl()` calls. + +- [ ] **Step 3: Commit** + +```bash +git add test/plugin.test.mjs +git commit -m "test: replace hardcoded ports with helper function" +``` + +--- + +## Chunk 4: Final Verification + +### Task 21: Full Build and Test + +- [ ] **Step 1: Clean build** + +Run: `npm run clean && npm run build` +Expected: Success + +- [ ] **Step 2: Run all tests** + +Run: `npm test` +Expected: All tests pass (target: 40+ tests) + +- [ ] **Step 3: Type check** + +Run: `npx tsc --noEmit` +Expected: No errors + +- [ ] **Step 4: Lint check** + +Run: `npm run lint` (if available) +Expected: No errors + +- [ ] **Step 5: Final commit summary** + +```bash +git log --oneline -20 +``` + +--- + +## Summary + +**Total tasks:** 21 (20 fixes + 1 verification) +**Estimated commits:** 20 +**Estimated time:** 2-3 hours + +### Priority Order: +1. **Medium priority first** (Tasks 1-8): Core functionality and type safety +2. **Low priority fixes** (Tasks 9-16): Logging, performance, style +3. **Test coverage** (Tasks 17-20): Fill testing gaps +4. **Verification** (Task 21): Ensure everything works + +### Key Decisions: +- Use `??` instead of `||` for API key fallback +- Keep `supportsTools` defaults as-is but document rationale +- Export `splitModelId` from `omniroute-combos.ts` +- Include `modelsDev` config in cache key hash +- Tighten `variants` type to `OmniRouteModelVariant` +- Add comprehensive test coverage for temperature/reasoning combos diff --git a/src/constants.ts b/src/constants.ts index 04e5e7c..bf8c45d 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -73,9 +73,27 @@ export const MODEL_CACHE_TTL = 5 * 60 * 1000; */ export const REQUEST_TIMEOUT = 30000; +/** + * Default model limits + */ +export const DEFAULT_CONTEXT_LIMIT = 4096; +export const DEFAULT_OUTPUT_LIMIT = 4096; + /** * models.dev enrichment defaults */ export const MODELS_DEV_DEFAULT_URL = 'https://models.dev/api.json'; export const MODELS_DEV_CACHE_TTL = 24 * 60 * 60 * 1000; export const MODELS_DEV_TIMEOUT_MS = 1000; + +/** + * Provider alias-to-canonical mapping for deduplication + */ +export const PROVIDER_ALIAS_TO_CANONICAL: Record = { + ollamacloud: 'ollama-cloud', + cc: 'claude', + gh: 'github', + cx: 'codex', + kr: 'kiro', + if: 'qoder', +}; diff --git a/src/logger.ts b/src/logger.ts index 7ece6e6..ba87fb2 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -1,4 +1,5 @@ -import { appendFileSync, readdirSync, statSync, existsSync } from 'fs'; +import { readdirSync, statSync, existsSync } from 'fs'; +import { appendFile } from 'fs/promises'; import { join } from 'path'; import { homedir } from 'os'; @@ -32,62 +33,34 @@ function findCurrentLogFile(): string | null { let cachedLogFile: string | null = findCurrentLogFile(); function getLogFile(): string | null { - if (cachedLogFile === null) { - // Re-scan if no file found at module load (OpenCode may create one later) + if (cachedLogFile === null || !existsSync(cachedLogFile)) { + // Re-scan if no file found at module load or if cached file was deleted (log rotation) cachedLogFile = findCurrentLogFile(); } return cachedLogFile; } -function isNodeError(error: unknown): error is NodeJS.ErrnoException { - return ( - typeof error === 'object' && - error !== null && - 'code' in error && - typeof (error as NodeJS.ErrnoException).code === 'string' - ); -} - -function writeLog(level: string, message: string): void { - let logFile = getLogFile(); - if (!logFile) return; - - // Check if cached file still exists (handles log rotation) - if (!existsSync(logFile)) { - cachedLogFile = findCurrentLogFile(); - logFile = cachedLogFile; - if (!logFile) return; - } - +function formatLogLine(level: string, message: string): string { const timestamp = new Date().toISOString(); - const line = `${level.padEnd(5)} ${timestamp} +0ms service=omniroute ${message}\n`; - - try { - appendFileSync(logFile, line); - } catch (error: unknown) { - if (isNodeError(error) && error.code === 'ENOENT') { - // Log file was deleted, re-scan - cachedLogFile = findCurrentLogFile(); - // Retry once with new file - const newLogFile = cachedLogFile; - if (newLogFile) { - try { - appendFileSync(newLogFile, line); - } catch { - // Silently fail on second attempt - } - } - } - // Silently fail for all other errors - } + return `${level.padEnd(5)} ${timestamp} +0ms service=omniroute ${message}\n`; } export function warn(message: string): void { - writeLog('WARN', message); + const logFile = getLogFile(); + if (!logFile) return; + + const line = formatLogLine('WARN', message); + // Fire-and-forget: don't await, don't crash on error + appendFile(logFile, line).catch(() => {}); } export function debug(message: string): void { // Strict comparison: only "1" enables debug logging if (process.env.OMNIROUTE_DEBUG !== '1') return; - writeLog('DEBUG', message); + + const logFile = getLogFile(); + if (!logFile) return; + + const line = formatLogLine('DEBUG', message); + appendFile(logFile, line).catch(() => {}); } diff --git a/src/models-dev.ts b/src/models-dev.ts index e88e13c..db022fb 100644 --- a/src/models-dev.ts +++ b/src/models-dev.ts @@ -246,6 +246,18 @@ export function modelsDevToMetadata(model: ModelsDevModel): OmniRouteModelMetada metadata.maxTokens = model.limit.output; } + if (model.temperature !== undefined) { + metadata.supportsTemperature = model.temperature; + } + + if (model.reasoning !== undefined) { + metadata.supportsReasoning = model.reasoning; + } + + if (model.attachment !== undefined) { + metadata.supportsAttachment = model.attachment; + } + // Derive vision support from modalities if (model.modalities?.input?.includes('image')) { metadata.supportsVision = true; @@ -256,8 +268,6 @@ export function modelsDevToMetadata(model: ModelsDevModel): OmniRouteModelMetada metadata.supportsTools = true; } - - // Pricing if (model.cost?.input !== undefined || model.cost?.output !== undefined) { metadata.pricing = {}; @@ -292,6 +302,12 @@ export function calculateLowestCommonCapabilities( let minMaxTokens: number | undefined; let allSupportVision = true; let allSupportTools = true; + let allSupportTemperature = true; + let hasTemperatureMetadata = false; + let allSupportReasoning = true; + let hasReasoningMetadata = false; + let allSupportAttachment = true; + let hasAttachmentMetadata = false; let allSupportStreaming = true; for (const model of models) { @@ -312,8 +328,26 @@ export function calculateLowestCommonCapabilities( allSupportVision = allSupportVision && supportsVision; // Tools: all must support it + // For combos: only advertise tools if ALL underlying models explicitly support them + // This is intentionally stricter than single-model defaults because a combo + // with one tool-less model cannot reliably use tools across all backends const supportsTools = model.tool_call === true; allSupportTools = allSupportTools && supportsTools; + + if (model.temperature !== undefined) { + hasTemperatureMetadata = true; + allSupportTemperature = allSupportTemperature && model.temperature; + } + + if (model.reasoning !== undefined) { + hasReasoningMetadata = true; + allSupportReasoning = allSupportReasoning && model.reasoning; + } + + if (model.attachment !== undefined) { + hasAttachmentMetadata = true; + allSupportAttachment = allSupportAttachment && model.attachment; + } } const result: OmniRouteModelMetadata = {}; @@ -334,6 +368,24 @@ export function calculateLowestCommonCapabilities( result.supportsTools = true; } + if (hasTemperatureMetadata && allSupportTemperature) { + result.supportsTemperature = true; + } else if (hasTemperatureMetadata) { + result.supportsTemperature = false; + } + + if (hasReasoningMetadata && allSupportReasoning) { + result.supportsReasoning = true; + } else if (hasReasoningMetadata) { + result.supportsReasoning = false; + } + + if (hasAttachmentMetadata && allSupportAttachment) { + result.supportsAttachment = true; + } else if (hasAttachmentMetadata) { + result.supportsAttachment = false; + } + // Streaming is generally supported by all modern models if (allSupportStreaming) { result.supportsStreaming = true; @@ -343,6 +395,31 @@ export function calculateLowestCommonCapabilities( } +/** + * Subscription → public provider fallback map. + * When a subscription provider (e.g. zai-coding-plan) lacks a model, + * try its public counterpart (e.g. zai) before giving up. + */ +export const SUBSCRIPTION_FALLBACKS: Record = { + 'zai-coding-plan': 'zai', + 'kimi-for-coding': 'moonshotai', + 'github-models': 'google', +}; + +/** + * Known model ID mismatches between OmniRoute and models.dev. + * Maps OmniRoute model names to their models.dev equivalents. + */ +export const MODEL_ALIASES: Record = { + 'kimi-k2.6-thinking': 'kimi-k2-thinking', + 'kimi-k2.6-thinking-turbo': 'kimi-k2-thinking-turbo', +}; + +export function resolveModelAlias(modelKey: string): string { + const lower = modelKey.toLowerCase(); + return MODEL_ALIASES[lower] ?? MODEL_ALIASES[normalizeModelKey(lower)] ?? modelKey; +} + /** * Resolve provider alias using config and defaults */ @@ -372,8 +449,36 @@ export function resolveProviderAlias( openrouter: 'openrouter', perplexity: 'perplexity', cohere: 'cohere', + glmt: 'zai-coding-plan', + glm: 'zai-coding-plan', + 'kimi-coding': 'moonshotai', + kmc: 'moonshotai', + gh: 'google', + github: 'google', ...config?.modelsDev?.providerAliases, }; return aliases[lower] ?? lower; -} \ No newline at end of file +} + +/** + * Get the public fallback provider for a subscription provider. + * Returns null if no fallback exists. + */ +export function getSubscriptionFallback(provider: string): string | null { + return SUBSCRIPTION_FALLBACKS[provider.toLowerCase()] ?? null; +} + +/** + * Strip reasoning effort variant suffix from a model name. + * Returns the base model name and true if a suffix was stripped. + */ +export function stripVariantSuffix(modelKey: string): { base: string; stripped: boolean } { + const variantPattern = /-(low|medium|high|xhigh)$/i; + const match = modelKey.match(variantPattern); + if (match) { + return { base: modelKey.slice(0, match.index), stripped: true }; + } + return { base: modelKey, stripped: false }; +} + diff --git a/src/models.ts b/src/models.ts index 4eed8f0..d2d33d4 100644 --- a/src/models.ts +++ b/src/models.ts @@ -1,13 +1,21 @@ -import type { OmniRouteConfig, OmniRouteModel, OmniRouteModelMetadata, OmniRouteModelsResponse } from './types.js'; +import type { OmniRouteConfig, OmniRouteModel, OmniRouteModelsResponse } from './types.js'; import { OMNIROUTE_DEFAULT_MODELS, OMNIROUTE_ENDPOINTS, MODEL_CACHE_TTL, REQUEST_TIMEOUT, + PROVIDER_ALIAS_TO_CANONICAL, } from './constants.js'; -import { getModelsDevIndex, normalizeModelKey } from './models-dev.js'; -import type { ModelsDevIndex } from './models-dev.js'; -import { enrichComboModels, clearComboCache } from './omniroute-combos.js'; +import { + getModelsDevIndex, + normalizeModelKey, + getSubscriptionFallback, + stripVariantSuffix, + resolveProviderAlias, + resolveModelAlias, +} from './models-dev.js'; +import type { ModelsDevIndex, ModelsDevModel } from './models-dev.js'; +import { enrichComboModels, clearComboCache, splitModelId } from './omniroute-combos.js'; import { warn, debug } from './logger.js'; /** @@ -28,7 +36,141 @@ const modelCache = new Map(); */ function getCacheKey(config: OmniRouteConfig, apiKey: string): string { const baseUrl = config.baseUrl || OMNIROUTE_ENDPOINTS.BASE_URL; - return `${baseUrl}:${apiKey}`; + + // Include modelsDev config in cache key to prevent stale data + const modelsDevHash = config.modelsDev + ? JSON.stringify({ + enabled: config.modelsDev.enabled, + url: config.modelsDev.url, + providerAliases: config.modelsDev.providerAliases, + }) + : ''; + + return `${baseUrl}:${apiKey}:${modelsDevHash}`; +} + +/** + * Normalize an OmniRoute model by reading all field variants + * with proper precedence: camelCase > snake_case > capabilities + */ +function normalizeModel(model: OmniRouteModel): OmniRouteModel { + const capabilities = + model.capabilities && typeof model.capabilities === 'object' + ? model.capabilities + : {}; + + return { + ...model, + id: model.id, + name: model.name || model.id, + description: model.description || `OmniRoute model: ${model.id}`, + + // Context limits: prefer explicit camelCase, fallback to snake_case + contextWindow: + model.contextWindow ?? model.context_length ?? model.max_input_tokens, + maxTokens: model.maxTokens ?? model.max_output_tokens, + + // Capabilities: prefer explicit camelCase, fallback to capabilities object, fallback to snake_case + supportsStreaming: model.supportsStreaming, + supportsVision: + model.supportsVision ?? + model.vision ?? + capabilities.vision ?? + capabilities.attachment, + supportsTools: + model.supportsTools ?? + model.tool_calling ?? + capabilities.tool_calling ?? + capabilities.toolcall, + supportsReasoning: + model.supportsReasoning ?? + model.reasoning ?? + capabilities.reasoning ?? + capabilities.thinking, + supportsAttachment: + model.supportsAttachment ?? + model.attachment ?? + capabilities.attachment, + supportsTemperature: + model.supportsTemperature ?? + model.temperature ?? + capabilities.temperature, + }; +} + +/** + * Deduplicate models by canonical provider+model key. + * Prefers canonical-prefixed IDs over aliases. + * + * NOTE: Only deduplicates known aliases (PROVIDER_ALIAS_TO_CANONICAL). + * Unknown provider prefixes are kept as-is to preserve user metadata. + */ +function deduplicateModels(models: OmniRouteModel[]): OmniRouteModel[] { + const seen = new Map(); + + for (const model of models) { + const parts = model.id.split('/'); + if (parts.length !== 2) { + // Not a provider/model ID, keep as-is + seen.set(model.id, model); + continue; + } + + const [providerPrefix, modelKey] = parts; + const canonicalPrefix = PROVIDER_ALIAS_TO_CANONICAL[providerPrefix]; + + // Only deduplicate known aliases; preserve unknown prefixes as-is + if (!canonicalPrefix) { + seen.set(model.id, model); + continue; + } + + const canonicalId = `${canonicalPrefix}/${modelKey}`; + + const existing = seen.get(canonicalId); + if (!existing) { + // First time seeing this model - store with canonical ID + seen.set(canonicalId, { + ...model, + id: canonicalId, + }); + } else { + // Already have canonical version - merge metadata, prefer non-alias + const isAlias = providerPrefix !== canonicalPrefix; + if (!isAlias) { + // This is the canonical version, overwrite alias + seen.set(canonicalId, { + ...model, + id: canonicalId, + }); + } + // If alias and we already have canonical, drop it + } + } + + return [...seen.values()]; +} + +/** + * Reverse a provider alias to its canonical form for metadata lookups. + * Returns the original id if no alias mapping exists. + */ +export function resolveProviderAliasForMetadata(modelId: string): string { + const parts = modelId.split('/'); + if (parts.length !== 2) return modelId; + + const [providerPrefix, modelKey] = parts; + const canonicalPrefix = PROVIDER_ALIAS_TO_CANONICAL[providerPrefix]; + if (!canonicalPrefix) return modelId; + + return `${canonicalPrefix}/${modelKey}`; +} + +/** + * Check if a provider prefix is a known alias. + */ +export function isProviderAlias(providerPrefix: string): boolean { + return providerPrefix in PROVIDER_ALIAS_TO_CANONICAL; } /** @@ -92,7 +234,12 @@ export async function fetchModels( // Runtime validation to ensure API returns expected structure if (!rawData || typeof rawData !== 'object' || !Array.isArray(rawData.data)) { - warn(`Invalid models response structure: ${JSON.stringify(rawData)}`); + const dataType = rawData && typeof rawData === 'object' + ? (rawData.data === null + ? 'null' + : Array.isArray(rawData.data) ? 'array' : typeof rawData.data) + : typeof rawData; + warn(`Invalid models response structure: expected { data: Array }, got { data: ${dataType} }`); throw new Error('Invalid models response structure: expected { data: Array }'); } @@ -104,22 +251,12 @@ export async function fetchModels( (model): model is OmniRouteModel => model !== null && model !== undefined && typeof model.id === 'string', ) - .map((model) => ({ - ...model, - // Ensure required fields - id: model.id, - name: model.name || model.id, - description: model.description || `OmniRoute model: ${model.id}`, - // Keep undefined for enrichment to work properly - contextWindow: model.contextWindow, - maxTokens: model.maxTokens, - supportsStreaming: model.supportsStreaming, - supportsVision: model.supportsVision, - supportsTools: model.supportsTools, - })); + .map(normalizeModel); + + const dedupedModels = deduplicateModels(rawModels); // Enrich with models.dev and combo capabilities - const models = await enrichModelMetadata(rawModels, config); + const models = await enrichModelMetadata(dedupedModels, config); // Update cache modelCache.set(cacheKey, { @@ -234,32 +371,17 @@ function applyModelsDevMetadata( config: OmniRouteConfig, index: ModelsDevIndex, ): OmniRouteModel { - const { providerKey, modelKey } = splitOmniRouteModelForLookup(model.id); + const { providerKey, modelKey } = splitModelId(model.id); const providerAlias = resolveProviderAlias(providerKey, config); - const lookupKey = modelKey.toLowerCase(); - const normalizedKey = normalizeModelKey(modelKey); - - // Try provider-specific exact match first - const providerExact = providerAlias - ? index.exactByProvider.get(providerAlias)?.get(lookupKey) - : undefined; - - // Try provider-specific normalized match - const providerNorm = providerAlias - ? index.normalizedByProvider.get(providerAlias)?.get(normalizedKey) - : undefined; - - // Try global exact match (only if single match to avoid ambiguity) - const globalExactList = index.exactGlobal.get(lookupKey); - const globalExact = globalExactList?.length === 1 ? globalExactList[0] : undefined; - - // Try global normalized match (only if single match to avoid ambiguity) - const globalNormList = index.normalizedGlobal.get(normalizedKey); - const globalNorm = globalNormList?.length === 1 ? globalNormList[0] : undefined; - - // Pick the best match (provider-specific preferred over global) - const best = providerExact ?? providerNorm ?? globalExact ?? globalNorm; - + const candidates = getModelLookupCandidates(modelKey); + const providerCandidates = [ + ...(providerAlias ? [providerAlias] : []), + ...(providerAlias + ? [getSubscriptionFallback(providerAlias)].filter((p): p is string => p !== null) + : []), + ]; + + const best = lookupModelsDevModel(index, providerCandidates, candidates); if (!best) return model; // Merge capabilities (only fill in missing values) @@ -278,66 +400,74 @@ function applyModelsDevMetadata( ? { supportsTools: true } : {}), ...(model.supportsStreaming === undefined - ? { supportsStreaming: true } // Assume streaming is supported by default + ? { supportsStreaming: true } + : {}), + ...(model.supportsTemperature === undefined && best.temperature !== undefined + ? { supportsTemperature: best.temperature } + : {}), + ...(model.supportsReasoning === undefined && best.reasoning !== undefined + ? { supportsReasoning: best.reasoning } + : {}), + ...(model.supportsAttachment === undefined && best.attachment !== undefined + ? { supportsAttachment: best.attachment } : {}), }; } -/** - * Split model ID for models.dev lookup - */ -function splitOmniRouteModelForLookup( - modelId: string, -): { providerKey: string | null; modelKey: string } { - const trimmed = modelId.trim(); - - // Remove omniroute prefix if present - const withoutPrefix = trimmed.replace(/^omniroute\//, ''); - - // Split by / - const parts = withoutPrefix.split('/').filter((p) => p.trim() !== ''); - - if (parts.length >= 2) { - return { - providerKey: parts[0] ?? null, - modelKey: parts.slice(1).join('/'), - }; +function getModelLookupCandidates(modelKey: string): string[] { + const candidates = new Set(); + + const addCandidate = (key: string): void => { + const lower = key.toLowerCase(); + const normalized = normalizeModelKey(key); + const aliasResolved = resolveModelAlias(key); + + candidates.add(lower); + candidates.add(normalized); + + // Only add alias variants if they differ from original + if (aliasResolved !== key) { + candidates.add(aliasResolved.toLowerCase()); + candidates.add(normalizeModelKey(aliasResolved)); + } + }; + + addCandidate(modelKey); + + const { base, stripped } = stripVariantSuffix(modelKey); + if (stripped) { + addCandidate(base); } - return { providerKey: null, modelKey: withoutPrefix }; + return [...candidates]; } -/** - * Resolve provider alias using config - */ -function resolveProviderAlias( - providerKey: string | null, - config: OmniRouteConfig, -): string | null { - if (!providerKey) return null; - - const lower = providerKey.toLowerCase(); - - // Default aliases - const aliases: Record = { - oai: 'openai', - openai: 'openai', - cx: 'openai', - codex: 'openai', - anthropic: 'anthropic', - claude: 'anthropic', - gemini: 'google', - google: 'google', - deepseek: 'deepseek', - mistral: 'mistral', - xai: 'xai', - groq: 'groq', - together: 'together', - openrouter: 'openrouter', - perplexity: 'perplexity', - cohere: 'cohere', - ...config.modelsDev?.providerAliases, - }; +function lookupModelsDevModel( + index: ModelsDevIndex, + providerCandidates: string[], + modelCandidates: string[], +): ModelsDevModel | undefined { + for (const provider of providerCandidates) { + for (const candidate of modelCandidates) { + const exact = index.exactByProvider.get(provider)?.get(candidate); + if (exact) return exact; + + const normalized = index.normalizedByProvider + .get(provider) + ?.get(normalizeModelKey(candidate)); + if (normalized) return normalized; + } + } - return aliases[lower] ?? lower; + for (const candidate of modelCandidates) { + const exactList = index.exactGlobal.get(candidate); + if (exactList?.length === 1) return exactList[0]; + + const normalizedList = index.normalizedGlobal.get(normalizeModelKey(candidate)); + if (normalizedList?.length === 1) return normalizedList[0]; + } + + return undefined; } + + diff --git a/src/omniroute-combos.ts b/src/omniroute-combos.ts index 1650003..95e5bab 100644 --- a/src/omniroute-combos.ts +++ b/src/omniroute-combos.ts @@ -9,6 +9,11 @@ import { import { REQUEST_TIMEOUT } from './constants.js'; import { warn, debug } from './logger.js'; +export function sanitizeForLog(value: string): string { + // Remove all control characters except tab (0x09) + return value.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]/g, ''); +} + /** * OmniRoute combo definition from /api/combos */ @@ -139,7 +144,7 @@ export async function resolveUnderlyingModels( // Check if this is a combo model const combo = combos.get(modelId); if (combo) { - debug(`Resolved combo "${modelId}" to ${combo.models.length} underlying models`); + debug(`Resolved combo "${sanitizeForLog(modelId)}" to ${combo.models.length} underlying models`); return combo.models .map((m) => { if (typeof m === 'string') return m; @@ -224,7 +229,7 @@ export function lookupModelInIndex( * Split a model ID into provider and model key * Handles formats like "provider/model", "omniroute/provider/model", etc. */ -function splitModelId(modelId: string): { providerKey: string | null; modelKey: string } { +export function splitModelId(modelId: string): { providerKey: string | null; modelKey: string } { const trimmed = modelId.trim(); // Remove omniroute prefix if present @@ -274,7 +279,7 @@ export async function calculateModelCapabilities( } // It's a combo - lookup all underlying models - debug(`Calculating capabilities for combo "${model.id}" from ${underlyingModels.length} models`); + debug(`Calculating capabilities for combo "${sanitizeForLog(model.id)}" from ${underlyingModels.length} models`); const resolvedModels: ModelsDevModel[] = []; const unresolvedModels: string[] = []; @@ -290,22 +295,22 @@ export async function calculateModelCapabilities( if (unresolvedModels.length > 0) { warn( - `Could not resolve ${unresolvedModels.length} underlying models for "${model.id}": ${unresolvedModels.join(', ')}`, + `Could not resolve ${unresolvedModels.length} underlying models for "${sanitizeForLog(model.id)}": ${unresolvedModels.map(sanitizeForLog).join(', ')}`, ); } if (resolvedModels.length === 0) { - warn(`No models.dev matches found for combo "${model.id}"`); + warn(`No models.dev matches found for combo "${sanitizeForLog(model.id)}"`); return {}; } - debug(`Resolved ${resolvedModels.length}/${underlyingModels.length} underlying models for "${model.id}"`); + debug(`Resolved ${resolvedModels.length}/${underlyingModels.length} underlying models for "${sanitizeForLog(model.id)}"`); // Calculate lowest common capabilities const capabilities = calculateLowestCommonCapabilities(resolvedModels); debug( - `Calculated capabilities for "${model.id}": context=${capabilities.contextWindow ?? 'N/A'}, maxTokens=${capabilities.maxTokens ?? 'N/A'}, vision=${capabilities.supportsVision ?? false}, tools=${capabilities.supportsTools ?? false}`, + `Calculated capabilities for "${sanitizeForLog(model.id)}": context=${capabilities.contextWindow ?? 'N/A'}, maxTokens=${capabilities.maxTokens ?? 'N/A'}, vision=${capabilities.supportsVision ?? false}, tools=${capabilities.supportsTools ?? false}`, ); return capabilities; @@ -353,7 +358,7 @@ export async function enrichComboModels( return model; } - debug(`Enriching combo model: ${model.id}`); + debug(`Enriching combo model: ${sanitizeForLog(model.id)}`); // Calculate capabilities for this combo const capabilities = await calculateModelCapabilities(model, config, modelsDevIndex); @@ -366,6 +371,15 @@ export async function enrichComboModels( ...(capabilities.maxTokens !== undefined ? { maxTokens: capabilities.maxTokens } : {}), ...(capabilities.supportsVision !== undefined ? { supportsVision: capabilities.supportsVision } : {}), ...(capabilities.supportsTools !== undefined ? { supportsTools: capabilities.supportsTools } : {}), + ...(capabilities.supportsTemperature !== undefined + ? { supportsTemperature: capabilities.supportsTemperature } + : {}), + ...(capabilities.supportsReasoning !== undefined + ? { supportsReasoning: capabilities.supportsReasoning } + : {}), + ...(capabilities.supportsAttachment !== undefined + ? { supportsAttachment: capabilities.supportsAttachment } + : {}), ...(capabilities.supportsStreaming !== undefined ? { supportsStreaming: capabilities.supportsStreaming } : {}), ...(capabilities.pricing !== undefined ? { pricing: { ...model.pricing, ...capabilities.pricing } } : {}), }; diff --git a/src/plugin.ts b/src/plugin.ts index e695e61..fbe442e 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -6,17 +6,22 @@ import type { OmniRouteApiMode, OmniRouteConfig, OmniRouteModel, + OmniRouteModelMetadata, OmniRouteModelMetadataConfig, OmniRouteModelsDevConfig, OmniRouteProviderModel, + OmniRouteModelVariant, } from './types.js'; import { OMNIROUTE_PROVIDER_ID, OMNIROUTE_DEFAULT_MODELS, OMNIROUTE_ENDPOINTS, + DEFAULT_CONTEXT_LIMIT, + DEFAULT_OUTPUT_LIMIT, } from './constants.js'; -import { fetchModels } from './models.js'; +import { fetchModels, resolveProviderAliasForMetadata, isProviderAlias } from './models.js'; import { warn, debug } from './logger.js'; +import { sanitizeForLog } from './omniroute-combos.js'; const OMNIROUTE_PROVIDER_NAME = 'OmniRoute'; const OMNIROUTE_PROVIDER_NPM = '@ai-sdk/openai-compatible'; @@ -41,14 +46,37 @@ export const OmniRouteAuthPlugin: Plugin = async (_input) => { let models: OmniRouteModel[] = OMNIROUTE_DEFAULT_MODELS; try { const auth = await readAuthFromStore(OMNIROUTE_PROVIDER_ID); - if (auth?.key) { - const runtimeConfig = createRuntimeConfig(existingProvider?.options ?? {}, auth.key); - models = await fetchModels(runtimeConfig, auth.key, false); + const apiKey = auth?.key ?? process.env.OMNIROUTE_API_KEY; + if (apiKey) { + const runtimeConfig = createRuntimeConfig(existingProvider?.options ?? {}, apiKey); + models = await fetchModels(runtimeConfig, apiKey, false); } } catch (error) { warn(`Eager model fetch failed, using defaults: ${error}`); } + const generatedModelMetadata: Record = {}; + for (const model of models) { + // Use canonical ID for metadata keys to match user config + const metadataKey = resolveProviderAliasForMetadata(model.id); + generatedModelMetadata[metadataKey] = { + contextWindow: model.contextWindow, + maxTokens: model.maxTokens, + supportsTemperature: model.supportsTemperature, + supportsReasoning: model.supportsReasoning, + supportsAttachment: model.supportsAttachment, + supportsVision: model.supportsVision, + supportsTools: model.supportsTools, + supportsStreaming: model.supportsStreaming, + pricing: model.pricing, + }; + } + + const modelMetadata = mergeModelMetadata( + existingProvider?.options?.modelMetadata, + generatedModelMetadata, + ); + providers[OMNIROUTE_PROVIDER_ID] = { ...existingProvider, name: existingProvider?.name ?? OMNIROUTE_PROVIDER_NAME, @@ -59,6 +87,7 @@ export const OmniRouteAuthPlugin: Plugin = async (_input) => { ...(existingProvider?.options ?? {}), baseURL: baseUrl, apiMode, + modelMetadata, }, models: existingProvider?.models && Object.keys(existingProvider.models).length > 0 @@ -120,7 +149,7 @@ async function loadProviderOptions( try { const forceRefresh = config.refreshOnList !== false; models = await fetchModels(config, config.apiKey, forceRefresh); - debug(`Available models: ${models.map((model) => model.id).join(', ')}`); + debug(`Available models: ${models.map((model) => sanitizeForLog(model.id)).join(', ')}`); } catch (error) { warn(`Failed to fetch models, using defaults: ${error}`); models = OMNIROUTE_DEFAULT_MODELS; @@ -183,13 +212,13 @@ async function readAuthFromStore( function resolveProviderApi(api: unknown, apiMode: OmniRouteApiMode): OmniRouteApiMode { if (isApiMode(api)) { if (api !== apiMode) { - warn(`provider.api (${api}) and options.apiMode (${apiMode}) differ; using options.apiMode`); + warn(`provider.api (${sanitizeForLog(String(api))}) and options.apiMode (${sanitizeForLog(apiMode)}) differ; using options.apiMode`); } return apiMode; } if (typeof api === 'string') { - warn(`Unsupported provider.api value: ${api}. Using ${apiMode}.`); + warn(`Unsupported provider.api value: ${sanitizeForLog(String(api))}. Using ${sanitizeForLog(apiMode)}.`); } return apiMode; @@ -205,7 +234,7 @@ function getApiMode(options?: Record): OmniRouteApiMode { return value; } - warn(`Unsupported apiMode option: ${String(value)}. Using chat.`); + warn(`Unsupported apiMode option: ${sanitizeForLog(String(value))}. Using chat.`); return 'chat'; } @@ -227,13 +256,13 @@ function getBaseUrl(options?: Record): string { try { const parsed = new URL(trimmed); if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { - warn(`Ignoring unsupported baseURL protocol: ${parsed.protocol}`); + warn(`Ignoring unsupported baseURL protocol: ${sanitizeForLog(parsed.protocol)}`); return OMNIROUTE_ENDPOINTS.BASE_URL; } return trimmed; } catch { - warn(`Ignoring invalid baseURL: ${trimmed}`); + warn(`Ignoring invalid baseURL: ${sanitizeForLog(trimmed)}`); return OMNIROUTE_ENDPOINTS.BASE_URL; } } @@ -325,6 +354,43 @@ function getStringRecord(value: unknown): Record | undefined { return Object.keys(out).length > 0 ? out : undefined; } +function mergeModelMetadata( + rawUserConfig: unknown, + generated: Record, +): OmniRouteModelMetadataConfig { + const userConfig = getModelMetadataConfig({ modelMetadata: rawUserConfig }); + + if (Array.isArray(userConfig)) { + const generatedBlocks = Object.entries(generated).map(([id, metadata]) => ({ + match: id, + ...metadata, + })); + + return [...generatedBlocks, ...userConfig]; + } + + if (userConfig && isRecord(userConfig)) { + const merged: Record = { ...generated }; + for (const [id, metadata] of Object.entries(userConfig)) { + const validation = isValidModelMetadata(metadata); + if (!validation.valid) { + warn(`Invalid metadata for model "${sanitizeForLog(id)}" (field: ${sanitizeForLog(validation.field ?? '')}), skipping`); + continue; + } + // If user uses an alias key (e.g., 'cx/gpt-5.5'), merge into canonical key + // so it matches the generated metadata and deduplicated model IDs + const canonicalId = resolveProviderAliasForMetadata(id); + merged[canonicalId] = { + ...(merged[canonicalId] ?? {}), + ...metadata, + }; + } + return merged; + } + + return generated; +} + function isRegExp(value: unknown): value is RegExp { return Object.prototype.toString.call(value) === '[object RegExp]'; } @@ -363,6 +429,48 @@ function isRecord(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } +const BOOLEAN_FIELDS = [ + 'supportsStreaming', 'supportsVision', 'supportsTools', + 'supportsTemperature', 'supportsReasoning', 'supportsAttachment', +]; + +function isValidModelMetadata(value: unknown): { valid: boolean; field?: string } { + if (!isRecord(value)) return { valid: false, field: '(not an object)' }; + + for (const field of BOOLEAN_FIELDS) { + if (field in value && typeof value[field] !== 'boolean') { + return { valid: false, field }; + } + } + + if ('contextWindow' in value && typeof value.contextWindow !== 'number') { + return { valid: false, field: 'contextWindow' }; + } + if ('maxTokens' in value && typeof value.maxTokens !== 'number') { + return { valid: false, field: 'maxTokens' }; + } + if ('name' in value && typeof value.name !== 'string') { + return { valid: false, field: 'name' }; + } + if ('description' in value && typeof value.description !== 'string') { + return { valid: false, field: 'description' }; + } + if ('pricing' in value) { + const pricing = value.pricing; + if (!isRecord(pricing)) { + return { valid: false, field: 'pricing' }; + } + if ('input' in pricing && typeof pricing.input !== 'number') { + return { valid: false, field: 'pricing.input' }; + } + if ('output' in pricing && typeof pricing.output !== 'number') { + return { valid: false, field: 'pricing.output' }; + } + } + + return { valid: true }; +} + function toProviderModels( models: OmniRouteModel[], baseUrl: string, @@ -376,7 +484,12 @@ function toProviderModels( function toProviderModel(model: OmniRouteModel, baseUrl: string): OmniRouteProviderModel { const supportsVision = model.supportsVision === true; + // Default to true: if API doesn't explicitly say no tools, assume capability exists + // This aligns with OpenAI-compatible behavior where most models support tools const supportsTools = model.supportsTools !== false; + const supportsTemperature = model.supportsTemperature !== false; + const supportsReasoning = model.supportsReasoning === true; + const supportsAttachment = model.supportsAttachment !== undefined ? model.supportsAttachment : supportsVision; return { id: model.id, @@ -384,15 +497,23 @@ function toProviderModel(model: OmniRouteModel, baseUrl: string): OmniRouteProvi providerID: OMNIROUTE_PROVIDER_ID, family: getModelFamily(model.id), release_date: '', + attachment: supportsAttachment, + reasoning: supportsReasoning, + temperature: supportsTemperature, + tool_call: supportsTools, + modalities: { + input: supportsVision ? ['text', 'image'] : ['text'], + output: ['text'], + }, api: { id: model.id, url: baseUrl, npm: OMNIROUTE_PROVIDER_NPM, }, capabilities: { - temperature: true, - reasoning: false, - attachment: supportsVision, + temperature: supportsTemperature, + reasoning: supportsReasoning, + attachment: supportsAttachment, toolcall: supportsTools, input: { text: true, @@ -419,13 +540,19 @@ function toProviderModel(model: OmniRouteModel, baseUrl: string): OmniRouteProvi }, }, limit: { - context: model.contextWindow ?? 4096, - output: model.maxTokens ?? 4096, + context: model.contextWindow ?? DEFAULT_CONTEXT_LIMIT, + output: model.maxTokens ?? DEFAULT_OUTPUT_LIMIT, }, options: {}, headers: {}, status: 'active', - variants: {}, + variants: supportsReasoning + ? { + low: { reasoningEffort: 'low' }, + medium: { reasoningEffort: 'medium' }, + high: { reasoningEffort: 'high' }, + } + : {}, }; } @@ -459,7 +586,7 @@ function createFetchInterceptor( return fetch(input, init); } - debug(`Intercepting request to ${url}`); + debug(`Intercepting request to ${sanitizeForLog(url)}`); // Merge headers from Request and init to avoid dropping existing headers const headers = new Headers(input instanceof Request ? input.headers : undefined); diff --git a/src/types.ts b/src/types.ts index 11d359a..903f95c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -5,11 +5,41 @@ export interface OmniRouteModel { id: string; name: string; description?: string; + + // OmniRoute native fields (camelCase from API) contextWindow?: number; maxTokens?: number; supportsStreaming?: boolean; supportsVision?: boolean; supportsTools?: boolean; + supportsTemperature?: boolean; + supportsReasoning?: boolean; + supportsAttachment?: boolean; + + // OmniRoute native fields (snake_case from API) + context_length?: number; + max_input_tokens?: number; + max_output_tokens?: number; + vision?: boolean; + tool_calling?: boolean; + + // OmniRoute capabilities object + capabilities?: { + vision?: boolean; + tool_calling?: boolean; + reasoning?: boolean; + thinking?: boolean; + attachment?: boolean; + temperature?: boolean; + toolcall?: boolean; + }; + + // Enriched fields from models.dev + temperature?: boolean; + reasoning?: boolean; + attachment?: boolean; + tool_call?: boolean; + pricing?: { input?: number; output?: number; @@ -24,6 +54,9 @@ export interface OmniRouteModelMetadata { supportsStreaming?: boolean; supportsVision?: boolean; supportsTools?: boolean; + supportsTemperature?: boolean; + supportsReasoning?: boolean; + supportsAttachment?: boolean; pricing?: { input?: number; output?: number; @@ -108,6 +141,14 @@ export interface OmniRouteProviderModel { providerID: string; family: string; release_date: string; + attachment?: boolean; + reasoning?: boolean; + temperature?: boolean; + tool_call?: boolean; + modalities?: { + input: readonly string[]; + output: readonly string[]; + }; api: { id: string; url: string; @@ -137,7 +178,15 @@ export interface OmniRouteProviderModel { options: Record; headers: Record; status: 'active'; - variants: Record; + variants: Record; +} + +/** + * Model variant configuration + */ +export interface OmniRouteModelVariant { + reasoningEffort?: 'low' | 'medium' | 'high'; + [key: string]: unknown; } /** diff --git a/test/logger.test.mjs b/test/logger.test.mjs index a7e123c..0ba0ee5 100644 --- a/test/logger.test.mjs +++ b/test/logger.test.mjs @@ -59,6 +59,9 @@ test('warn() writes to log file with correct format', async () => { warn('Test warning message'); + // Wait for async appendFile to complete + await new Promise(resolve => setTimeout(resolve, 50)); + const content = readFileSync(testLogFile, 'utf-8'); assert.ok(content.includes('Test warning message'), 'warn should write message'); assert.ok(content.includes('WARN'), 'log should have WARN level'); @@ -82,6 +85,9 @@ test('debug() writes when OMNIROUTE_DEBUG=1', async () => { debug('Test debug message'); + // Wait for async appendFile to complete + await new Promise(resolve => setTimeout(resolve, 50)); + const content = readFileSync(testLogFile, 'utf-8'); assert.ok(content.includes('Test debug message'), 'debug should write when enabled'); assert.ok(content.includes('DEBUG'), 'log should have DEBUG level'); @@ -142,6 +148,9 @@ test('warn() always writes regardless of OMNIROUTE_DEBUG', async () => { warn('Test warning message'); + // Wait for async appendFile to complete + await new Promise(resolve => setTimeout(resolve, 50)); + const content = readFileSync(testLogFile, 'utf-8'); assert.ok(content.includes('Test warning message'), 'warn should always write'); @@ -168,12 +177,18 @@ test('logger handles log file rotation', async () => { const { warn } = await import(`../dist/src/logger.js#${Date.now()}-${Math.random()}`); warn('First message'); + // Wait for first async write to complete + await new Promise(resolve => setTimeout(resolve, 50)); + // Simulate log rotation: delete old file, create new one rmSync(oldLogFile); const newLogFile = createTestLogFile('test-new.log'); warn('Second message after rotation'); + // Wait for second async write to complete + await new Promise(resolve => setTimeout(resolve, 50)); + const content = readFileSync(newLogFile, 'utf-8'); assert.ok( content.includes('Second message after rotation'), @@ -195,6 +210,9 @@ test('logger re-scans when no log file exists at module load', async () => { warn('Message after log file created'); + // Wait for async appendFile to complete + await new Promise(resolve => setTimeout(resolve, 50)); + const content = readFileSync(testLogFile, 'utf-8'); assert.ok(content.includes('Message after log file created'), 'should re-scan and write to new log file'); @@ -261,6 +279,9 @@ test('logger excludes directories with .log suffix', async () => { const { warn } = await import(`../dist/src/logger.js#${Date.now()}-${Math.random()}`); warn('Test directory exclusion'); + // Wait for async appendFile to complete + await new Promise(resolve => setTimeout(resolve, 50)); + const content = readFileSync(testLogFile, 'utf-8'); assert.ok(content.includes('Test directory exclusion'), 'should write to real log file, not directory'); @@ -282,6 +303,9 @@ test('logger uses alphabetical tie-breaker for identical mtime', async () => { const { warn } = await import(`../dist/src/logger.js#${Date.now()}-${Math.random()}`); warn('Test tie-breaker'); + // Wait for async appendFile to complete + await new Promise(resolve => setTimeout(resolve, 50)); + // Should write to test-alpha.log (alphabetically first) const contentA = readFileSync(fileA, 'utf-8'); const contentB = readFileSync(fileB, 'utf-8'); diff --git a/test/models.test.mjs b/test/models.test.mjs index 70137f8..8036659 100644 --- a/test/models.test.mjs +++ b/test/models.test.mjs @@ -8,6 +8,7 @@ import { isCacheValid, refreshModels, } from '../dist/runtime.js'; +import { calculateLowestCommonCapabilities } from '../dist/src/models-dev.js'; const ORIGINAL_FETCH = global.fetch; @@ -102,3 +103,279 @@ test('fetchModels falls back to defaults when response shape is invalid', async assert.ok(models.length > 0); assert.ok(typeof models[0].id === 'string'); }); + +test('calculateLowestCommonCapabilities ignores missing attachment metadata', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'with-attachment', attachment: true }, + { id: 'without-attachment' }, + ]); + + assert.equal(capabilities.supportsAttachment, true); +}); + +test('calculateLowestCommonCapabilities respects explicit attachment false', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'with-attachment', attachment: true }, + { id: 'without-attachment', attachment: false }, + ]); + + assert.equal(capabilities.supportsAttachment, false); +}); + +test('fetchModels uses different cache for different modelsDev configs', async () => { + let calls = 0; + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + if (url.includes('/v1/models')) { + calls++; + return new Response( + JSON.stringify({ object: 'list', data: [{ id: `model-${calls}`, name: `Model ${calls}` }] }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }; + + const config1 = { ...CONFIG, modelsDev: { enabled: true, providerAliases: { oai: 'openai' } } }; + const config2 = { ...CONFIG, modelsDev: { enabled: true, providerAliases: { oai: 'anthropic' } } }; + + await fetchModels(config1, CONFIG.apiKey, false); + await fetchModels(config2, CONFIG.apiKey, false); + + assert.equal(calls, 2, 'Should fetch twice for different modelsDev configs'); +}); + +// Task 15: Single Model Metadata Tracking +test('calculateLowestCommonCapabilities produces identical output for single model and combo-with-self', () => { + const single = calculateLowestCommonCapabilities([ + { id: 'test-model', temperature: true, reasoning: true, attachment: true }, + ]); + + const combo = calculateLowestCommonCapabilities([ + { id: 'test-model', temperature: true, reasoning: true, attachment: true }, + { id: 'test-model', temperature: true, reasoning: true, attachment: true }, + ]); + + // Core capability fields should match (supportsStreaming differs because + // single-model path uses modelsDevToMetadata which doesn't add streaming, + // while combo path always adds it) + assert.equal(single.supportsTemperature, combo.supportsTemperature); + assert.equal(single.supportsReasoning, combo.supportsReasoning); + assert.equal(single.supportsAttachment, combo.supportsAttachment); +}); + +// Task 17: Temperature/Reasoning Combo Tests +test('calculateLowestCommonCapabilities handles mixed defined and undefined temperature', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'with-temp', temperature: true }, + { id: 'without-temp' }, + ]); + + assert.equal(capabilities.supportsTemperature, true); +}); + +test('calculateLowestCommonCapabilities handles explicit temperature false overriding true', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'with-temp', temperature: true }, + { id: 'without-temp', temperature: false }, + ]); + + assert.equal(capabilities.supportsTemperature, false); +}); + +test('calculateLowestCommonCapabilities handles all three capabilities together', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'full-support', temperature: true, reasoning: true, attachment: true }, + { id: 'partial-support', temperature: true, reasoning: false, attachment: true }, + ]); + + assert.equal(capabilities.supportsTemperature, true); + assert.equal(capabilities.supportsReasoning, false); + assert.equal(capabilities.supportsAttachment, true); +}); + +test('calculateLowestCommonCapabilities handles single model with undefined temperature', () => { + const capabilities = calculateLowestCommonCapabilities([ + { id: 'no-temp-metadata', reasoning: true }, + ]); + + assert.equal(capabilities.supportsTemperature, undefined); + assert.equal(capabilities.supportsReasoning, true); +}); + +// Task 18: Variant+Alias Integration Test +test('variant suffix stripping works with alias resolution end-to-end', async () => { + const { stripVariantSuffix, resolveModelAlias, normalizeModelKey } = await import('../dist/src/models-dev.js'); + + // Test variant suffix stripping + const { base: base1, stripped: stripped1 } = stripVariantSuffix('gpt-4o-high'); + assert.equal(base1, 'gpt-4o'); + assert.equal(stripped1, true); + + const { base: base2, stripped: stripped2 } = stripVariantSuffix('claude-3-sonnet-low'); + assert.equal(base2, 'claude-3-sonnet'); + assert.equal(stripped2, true); + + const { base: base3, stripped: stripped3 } = stripVariantSuffix('gpt-4o'); + assert.equal(base3, 'gpt-4o'); + assert.equal(stripped3, false); + + // Test alias resolution on base name (variant suffix stripped first) + const alias1 = resolveModelAlias('kimi-k2.6-thinking'); + assert.equal(alias1, 'kimi-k2-thinking'); + + const alias2 = resolveModelAlias('kimi-k2.6-thinking-turbo'); + assert.equal(alias2, 'kimi-k2-thinking-turbo'); + + // Test normalization removes preview suffix (variant is stripped separately) + const normalized = normalizeModelKey('gpt-4o-preview'); + assert.equal(normalized, 'gpt-4o'); +}); + +// Task 19: Subscription Fallback Test +test('subscription provider fallback enriches from public provider', async () => { + const { getSubscriptionFallback } = await import('../dist/src/models-dev.js'); + + // Test known subscription fallbacks + assert.equal(getSubscriptionFallback('zai-coding-plan'), 'zai'); + assert.equal(getSubscriptionFallback('kimi-for-coding'), 'moonshotai'); + assert.equal(getSubscriptionFallback('github-models'), 'google'); + + // Test case insensitivity + assert.equal(getSubscriptionFallback('ZAI-CODING-PLAN'), 'zai'); + assert.equal(getSubscriptionFallback('GitHub-Models'), 'google'); + + // Test unknown provider returns null + assert.equal(getSubscriptionFallback('unknown-provider'), null); +}); + +// Task 21: Normalization of snake_case and capabilities fields +test('normalizeModel reads snake_case fields', async () => { + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + if (url.includes('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [ + { + id: 'test/model-1', + name: 'Test Model', + context_length: 128000, + max_output_tokens: 4096, + capabilities: { + vision: true, + tool_calling: true, + reasoning: true, + } + } + ] + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }; + + const models = await fetchModels(CONFIG, CONFIG.apiKey, false); + const model = models.find(m => m.id === 'test/model-1'); + + assert.ok(model, 'Model should be found'); + assert.equal(model.contextWindow, 128000, 'Should read context_length'); + assert.equal(model.maxTokens, 4096, 'Should read max_output_tokens'); + assert.equal(model.supportsVision, true, 'Should read capabilities.vision'); + assert.equal(model.supportsTools, true, 'Should read capabilities.tool_calling'); + assert.equal(model.supportsReasoning, true, 'Should read capabilities.reasoning'); +}); + +test('normalizeModel prefers camelCase over snake_case', async () => { + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + if (url.includes('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [ + { + id: 'test/model-2', + contextWindow: 64000, + context_length: 32000, + capabilities: { + vision: false, + } + } + ] + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }; + + const models = await fetchModels(CONFIG, CONFIG.apiKey, false); + const model = models.find(m => m.id === 'test/model-2'); + + assert.ok(model, 'Model should be found'); + assert.equal(model.contextWindow, 64000, 'Should prefer camelCase over snake_case'); +}); + +// Task 22: Deduplication of alias/canonical model entries +test('deduplication removes alias when canonical exists', async () => { + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + if (url.includes('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [ + { + id: 'ollamacloud/deepseek-v4', + name: 'DeepSeek V4 (alias)', + context_length: 64000, + }, + { + id: 'ollama-cloud/deepseek-v4', + name: 'DeepSeek V4 (canonical)', + context_length: 128000, + } + ] + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }; + + const models = await fetchModels(CONFIG, CONFIG.apiKey, false); + + assert.equal(models.length, 1, 'Should deduplicate to single model'); + assert.equal(models[0].id, 'ollama-cloud/deepseek-v4', 'Should prefer canonical ID'); + assert.equal(models[0].contextWindow, 128000, 'Should use canonical metadata'); +}); + +test('deduplication keeps alias when canonical is missing', async () => { + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + if (url.includes('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [ + { + id: 'ollamacloud/deepseek-v4', + name: 'DeepSeek V4', + context_length: 64000, + } + ] + }), + { status: 200, headers: { 'Content-Type': 'application/json' } } + ); + } + return new Response(JSON.stringify({ data: [] }), { status: 200 }); + }; + + const models = await fetchModels(CONFIG, CONFIG.apiKey, false); + + assert.equal(models.length, 1, 'Should keep single model'); + assert.equal(models[0].id, 'ollama-cloud/deepseek-v4', 'Should normalize to canonical ID'); +}); diff --git a/test/plugin.test.mjs b/test/plugin.test.mjs index 7c20a9e..8e0dd6a 100644 --- a/test/plugin.test.mjs +++ b/test/plugin.test.mjs @@ -14,6 +14,10 @@ afterEach(() => { process.env.HOME = ORIGINAL_HOME; }); +function getDummyBaseUrl(port = 20128) { + return `http://localhost:${port}/v1`; +} + function createModelsResponse() { return { object: 'list', @@ -32,7 +36,7 @@ test('config hook applies defaults and normalized apiMode', async () => { provider: { omniroute: { options: { - baseURL: 'http://localhost:20128/v1', + baseURL: getDummyBaseUrl(), apiMode: 'invalid-mode', }, }, @@ -69,7 +73,7 @@ test('loader injects auth headers only for OmniRoute URLs', async () => { const provider = { options: { - baseURL: 'http://localhost:20128/v1', + baseURL: getDummyBaseUrl(), apiMode: 'chat', }, models: {}, @@ -78,7 +82,7 @@ test('loader injects auth headers only for OmniRoute URLs', async () => { const options = await plugin.auth.loader(async () => ({ type: 'api', key: 'secret-key' }), provider); const interceptedFetch = options.fetch; - await interceptedFetch('http://localhost:20128/v1/chat/completions', { + await interceptedFetch(`${getDummyBaseUrl()}/chat/completions`, { method: 'POST', body: JSON.stringify({ model: 'gpt-4.1-mini', messages: [] }), }); @@ -123,14 +127,14 @@ test('gemini tool schema payload is sanitized before forwarding', async () => { }; const provider = { - options: { baseURL: 'http://localhost:20128/v1', apiMode: 'chat' }, + options: { baseURL: getDummyBaseUrl(), apiMode: 'chat' }, models: {}, }; const options = await plugin.auth.loader(async () => ({ type: 'api', key: 'secret-key' }), provider); const interceptedFetch = options.fetch; - await interceptedFetch('http://localhost:20128/v1/chat/completions', { + await interceptedFetch(`${getDummyBaseUrl()}/chat/completions`, { method: 'POST', body: JSON.stringify({ model: 'gemini-2.5-pro', @@ -188,14 +192,14 @@ test('non-gemini payload keeps original tool schema fields', async () => { }; const provider = { - options: { baseURL: 'http://localhost:20128/v1', apiMode: 'chat' }, + options: { baseURL: getDummyBaseUrl(), apiMode: 'chat' }, models: {}, }; const options = await plugin.auth.loader(async () => ({ type: 'api', key: 'secret-key' }), provider); const interceptedFetch = options.fetch; - await interceptedFetch('http://localhost:20128/v1/chat/completions', { + await interceptedFetch(`${getDummyBaseUrl()}/chat/completions`, { method: 'POST', body: JSON.stringify({ model: 'gpt-4.1-mini', @@ -244,14 +248,14 @@ test('gemini schema sanitization applies to responses endpoint request objects', }; const provider = { - options: { baseURL: 'http://localhost:20128/v1', apiMode: 'responses' }, + options: { baseURL: getDummyBaseUrl(), apiMode: 'responses' }, models: {}, }; const options = await plugin.auth.loader(async () => ({ type: 'api', key: 'secret-key' }), provider); const interceptedFetch = options.fetch; - const request = new Request('http://localhost:20128/v1/responses', { + const request = new Request(`${getDummyBaseUrl()}/responses`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ @@ -312,7 +316,7 @@ test('provider hook fetches models when auth is available via context', async () name: 'OmniRoute', source: 'config', env: [], - options: { baseURL: 'http://localhost:20128/v1', apiMode: 'chat' }, + options: { baseURL: getDummyBaseUrl(), apiMode: 'chat' }, models: {}, }, { auth: { type: 'api', key: 'live-key' } }, @@ -336,7 +340,7 @@ test('provider hook ignores stale provider.models and returns defaults when no a name: 'OmniRoute', source: 'config', env: [], - options: { baseURL: 'http://localhost:20128/v1', apiMode: 'chat' }, + options: { baseURL: getDummyBaseUrl(), apiMode: 'chat' }, models: { 'stale-model': { id: 'stale-model', @@ -370,7 +374,7 @@ test('provider hook returns defaults when fetch fails (fetchModels handles error name: 'OmniRoute', source: 'config', env: [], - options: { baseURL: 'http://localhost:20128/v1', apiMode: 'chat' }, + options: { baseURL: getDummyBaseUrl(), apiMode: 'chat' }, models: { 'existing-model': { id: 'existing-model', name: 'Existing', providerID: 'omniroute' } }, }, { auth: { type: 'api', key: 'bad-key' } }, @@ -418,7 +422,7 @@ test('config hook eagerly fetches models when auth is available', async () => { provider: { omniroute: { options: { - baseURL: 'http://localhost:20128/v1', + baseURL: getDummyBaseUrl(), apiMode: 'chat', }, }, @@ -433,3 +437,176 @@ test('config hook eagerly fetches models when auth is available', async () => { await rm(tempHome, { recursive: true, force: true }); } }); + +test('config hook preserves user modelMetadata object overrides', async () => { + const tempHome = join(tmpdir(), `opencode-test-${Date.now()}`); + try { + await mkdir(join(tempHome, '.local', 'share', 'opencode'), { recursive: true }); + await writeFile( + join(tempHome, '.local', 'share', 'opencode', 'auth.json'), + JSON.stringify({ omniroute: { type: 'api', key: 'test-key' } }), + ); + process.env.HOME = tempHome; + + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : String(input); + if (url.endsWith('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [ + { + id: 'cx/gpt-5.5', + name: 'GPT-5.5', + contextWindow: 1050000, + supportsReasoning: true, + }, + ], + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ); + } + return new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + }; + + const plugin = await OmniRouteAuthPlugin({}); + const config = { + provider: { + omniroute: { + options: { + baseURL: getDummyBaseUrl(20129), + modelMetadata: { + 'cx/gpt-5.5': { + contextWindow: 258000, + }, + }, + }, + }, + }, + }; + + await plugin.config(config); + + // User metadata is merged into canonical key after deduplication + const metadata = config.provider.omniroute.options.modelMetadata['codex/gpt-5.5']; + assert.equal(metadata.contextWindow, 258000); + assert.equal(metadata.supportsReasoning, true); + } finally { + await rm(tempHome, { recursive: true, force: true }); + } +}); + +test('config hook preserves user modelMetadata match blocks', async () => { + const tempHome = join(tmpdir(), `opencode-test-${Date.now()}`); + try { + await mkdir(join(tempHome, '.local', 'share', 'opencode'), { recursive: true }); + await writeFile( + join(tempHome, '.local', 'share', 'opencode', 'auth.json'), + JSON.stringify({ omniroute: { type: 'api', key: 'test-key' } }), + ); + process.env.HOME = tempHome; + + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : String(input); + if (url.endsWith('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [{ id: 'cx/gpt-5.5', name: 'GPT-5.5', contextWindow: 1050000 }], + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ); + } + return new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + }; + + const userBlock = { + match: '^(codex|cx)/.*gpt-5', + contextWindow: 258000, + }; + const plugin = await OmniRouteAuthPlugin({}); + const config = { + provider: { + omniroute: { + options: { + baseURL: getDummyBaseUrl(20130), + modelMetadata: [userBlock], + }, + }, + }, + }; + + await plugin.config(config); + + const metadata = config.provider.omniroute.options.modelMetadata; + assert.ok(Array.isArray(metadata)); + assert.deepEqual(metadata.at(-1), userBlock); + // After deduplication, model ID is normalized to canonical form + assert.equal(metadata[0].match, 'codex/gpt-5.5'); + assert.equal(metadata[0].contextWindow, 1050000); + } finally { + await rm(tempHome, { recursive: true, force: true }); + } +}); + +test('config hook respects explicit attachment false for vision models', async () => { + const tempHome = join(tmpdir(), `opencode-test-${Date.now()}`); + try { + await mkdir(join(tempHome, '.local', 'share', 'opencode'), { recursive: true }); + await writeFile( + join(tempHome, '.local', 'share', 'opencode', 'auth.json'), + JSON.stringify({ omniroute: { type: 'api', key: 'test-key' } }), + ); + process.env.HOME = tempHome; + + global.fetch = async (input) => { + const url = input instanceof Request ? input.url : String(input); + if (url.endsWith('/v1/models')) { + return new Response( + JSON.stringify({ + object: 'list', + data: [ + { + id: 'vision-no-attachment', + name: 'Vision No Attachment', + supportsVision: true, + supportsAttachment: false, + }, + ], + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ); + } + return new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + }; + + const plugin = await OmniRouteAuthPlugin({}); + const config = { + provider: { + omniroute: { + options: { + baseURL: getDummyBaseUrl(20131), + }, + }, + }, + }; + + await plugin.config(config); + + const model = config.provider.omniroute.models['vision-no-attachment']; + assert.equal(model.attachment, false); + assert.equal(model.capabilities.attachment, false); + assert.deepEqual(model.modalities.input, ['text', 'image']); + } finally { + await rm(tempHome, { recursive: true, force: true }); + } +});