feat: enrich model capabilities via models.dev and fix provider alias resolution#18
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enriches OmniRoute model capability metadata from models.dev and improves provider/model alias resolution so OpenCode can expose more accurate model limits and feature flags.
Changes:
- Adds temperature, reasoning, and attachment capability fields to model/provider types and provider model output.
- Generates provider
modelMetadatafrom fetched/enriched models and adds reasoning-effort variants. - Adds provider aliases, subscription fallback mappings, model aliases, and reasoning variant suffix stripping for models.dev lookup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/types.ts |
Adds capability fields to OmniRoute model and provider model type definitions. |
src/plugin.ts |
Injects generated model metadata and emits top-level/capability provider fields plus reasoning variants. |
src/models.ts |
Enhances models.dev matching with aliases, suffix stripping, and fallback providers. |
src/models-dev.ts |
Defines fallback/alias helpers and variant suffix stripping utilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances model matching and metadata handling by introducing model aliases, subscription fallbacks, and reasoning effort variant stripping. It also expands the model schema to support capabilities like temperature, reasoning, and attachments, ensuring these are correctly propagated to the provider model. Feedback focuses on improving the robustness of the matching logic by using normalized keys for aliases and variant stripping, as well as removing code duplication in the provider alias resolution function.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review Roast 🔥Verdict: 2 Issues Found | Recommendation: Address before merge Reviewed with: kilo-auto/free Overview
Issue Details (click to expand)
🏆 Best part: The attachment fix is solid — 💀 Worst part: The attachment fix was a masterclass in "let us not repeat this mistake" — and yet here we are, with temperature and reasoning using the exact same buggy AND-chain pattern. This is like watching someone stub their toe, learn to wear shoes, then immediately step on a Lego barefoot. 📊 Overall: Fix temperature and reasoning with the same Files Reviewed (6 files)
Fix these issues in Kilo Cloud Reviewed by step-3.5-flash · 395,795 tokens |
… resolution - Add env var fallback for API key in config hook (OMNIROUTE_API_KEY) - Generate modelMetadata from enriched models so OpenCode applies capabilities - Add top-level capability fields (temperature, reasoning, attachment, tool_call, modalities) to provider model output - Generate reasoning effort variants (low/medium/high) for reasoning models - Add provider aliases: glmt/glm→zai-coding-plan, kimi-coding/kmc→moonshotai, gh/github→google - Add subscription fallback: zai-coding-plan→zai, kimi-for-coding→moonshotai - Add model aliases for naming mismatches (kimi-k2.6-thinking→kimi-k2-thinking) - Strip reasoning effort variant suffixes (gpt-5.5-xhigh→gpt-5.5) for lookup - Close 19 of 132 models with 4096 default context limits
Preserve user modelMetadata while merging generated capabilities, propagate new capability fields through models.dev and combo paths, and consolidate alias resolution.
Treat missing attachment capability as unknown instead of false when folding combo model capabilities, while still honoring explicit false values.
Avoid falling back to vision support when models.dev or combo metadata explicitly marks attachments unsupported.
abien
force-pushed
the
feat/model-enrichment-capabilities
branch
from
4574620 to
5df51f9
Compare
- Fix temperature AND-chain bug in calculateLowestCommonCapabilities by tracking hasTemperatureMetadata - Fix reasoning AND-chain bug in calculateLowestCommonCapabilities by tracking hasReasoningMetadata - Respect explicit supportsAttachment=false for vision models in toProviderModel - Add normalized key candidates to getModelLookupCandidates for better alias/variant matching Addresses feedback from Copilot, Gemini Code Assist, and Kilo Code Bot reviews.
|
@abien Thank you for your pull request and all the work you've put in! 😍 |
Alph4d0g
left a comment
Alph4d0g
left a comment
There was a problem hiding this comment.
Comprehensive Code Review Report — PR #18
Reviewer: AI Code Review (multi-agent comprehensive analysis)
Commit reviewed: `9ca663b` (includes fixes applied)
Status: 30/30 tests pass, build clean
Executive Summary
| Category | Score | Notes |
|---|---|---|
| Security | ✅ Good | No new attack surface; env var fallback is safe |
| Performance | Cache key collisions possible; candidate explosion in lookup | |
| Architecture | DRY violations, tight coupling, magic constants | |
| Type Safety | Missing strict null checks, implicit any risks | |
| Test Coverage | 30 tests pass; missing combo/temperature/reasoning tests | |
| Documentation | ✅ Good | CHANGELOG updated; inline docs adequate |
Verdict: Address medium issues before merge. Low issues can be fixed in follow-up.
Summary of Findings
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🟡 Medium | `src/plugin.ts:45` | ` |
| 2 | 🟡 Medium | `src/plugin.ts:351-379` | No validation on merged modelMetadata |
| 3 | 🟢 Low | `src/omniroute-combos.ts:292-294` | Log injection via model IDs |
| 4 | 🟢 Low | `src/models.ts:290-305` | Candidate explosion in lookup |
| 5 | 🟢 Low | `src/logger.ts:66` | Synchronous file I/O blocks event loop |
| 6 | 🟡 Medium | `src/models.ts:36-39` | Cache key ignores modelsDev config |
| 7 | 🟡 Medium | `src/omniroute-combos.ts` + `src/models.ts` | DRY violation: two splitModelId functions |
| 8 | 🟢 Low | `src/plugin.ts:486-489` | Magic constant 4096 |
| 9 | 🟢 Low | `src/models.ts:91-104` | Sensitive data in log on invalid response |
| 10 | 🟡 Medium | `src/types.ts:154` | `variants: Record<string, unknown>` too permissive |
| 11 | 🟢 Low | `src/models-dev.ts:296-298` | Early return on single model bypasses metadata tracking |
| 12 | 🟡 Medium | `src/plugin.ts:432` | supportsTools defaults inconsistent with combo logic |
| 13 | 🟡 Medium | `test/models.test.mjs` | Missing temperature/reasoning combo tests |
| 14 | 🟢 Low | `test/models.test.mjs` | Missing variant+alias integration tests |
| 15 | 🟢 Low | `test/models.test.mjs` | Missing subscription fallback tests |
| 16 | 🟢 Low | `test/plugin.test.mjs:476+` | Hardcoded ports in tests |
| 17 | 🟡 Medium | `src/types.ts:117-124` | New provider model fields may not match OpenCode shape |
| 18 | 🟡 Medium | `src/plugin.ts:357-363` | Array metadata ordering assumption |
| 19 | 🟢 Low | `CHANGELOG.md` | Missing PR #18 entry |
| 20 | 🟢 Low | `src/models-dev.ts:480` | Missing trailing newline |
| 21 | 🟢 Low | `src/plugin.ts:448-449` | Redundant `as const` assertions |
Risk Assessment
Low risk. The changes are additive (new fields, new aliases) with backward-compatible optional types. The fixes correct bugs in combo capability calculation. No breaking changes to public API.
Recommendation: Merge after addressing medium-severity items. Low-severity items can be addressed in a follow-up PR.
Detailed inline comments posted on specific files and lines.
| models = await fetchModels(runtimeConfig, auth.key, false); | ||
| const apiKey = auth?.key || process.env.OMNIROUTE_API_KEY; | ||
| if (apiKey) { | ||
| const runtimeConfig = createRuntimeConfig(existingProvider?.options ?? {}, apiKey); |
There was a problem hiding this comment.
🟡 Medium — API Key Fallback Operator
The || operator treats empty string "" as falsy and falls back to env var. If a user explicitly clears their key, the env var still wins unexpectedly.
Suggested fix:
const apiKey = auth?.key ?? process.env.OMNIROUTE_API_KEY;
| @@ -1,12 +1,19 @@ | |||
| import type { OmniRouteConfig, OmniRouteModel, OmniRouteModelMetadata, OmniRouteModelsResponse } from './types.js'; | |||
| import type { OmniRouteConfig, OmniRouteModel, OmniRouteModelsResponse } from './types.js'; | |||
There was a problem hiding this comment.
🟡 Medium — Cache Key Ignores modelsDev Config
Different modelsDev configurations (e.g., different providerAliases) share the same cache key. A user switching aliases won't see refreshed models until TTL expires.
Suggested fix: Hash config-relevant fields into the cache key.
| @@ -1,12 +1,19 @@ | |||
| import type { OmniRouteConfig, OmniRouteModel, OmniRouteModelMetadata, OmniRouteModelsResponse } from './types.js'; | |||
| import type { OmniRouteConfig, OmniRouteModel, OmniRouteModelsResponse } from './types.js'; | |||
There was a problem hiding this comment.
🟢 Low — Sensitive Data in Log on Invalid Response
JSON.stringify(rawData) in the warning could leak sensitive model data into logs if the API returns non-standard responses.
Suggested fix: Log only the shape, not the content: warn('Invalid models response structure: expected { data: Array }, got ' + typeof rawData?.data);
| }; | ||
| } | ||
|
|
||
| function getModelLookupCandidates(modelKey: string): string[] { |
There was a problem hiding this comment.
🟢 Low — Candidate Explosion in Lookup
getModelLookupCandidates generates up to 8 candidates per model key. With 2 provider candidates and 614 models, this is ~19,648 Map lookups. While currently negligible, this grows O(n×m×p).
Suggested fix: Early-exit if resolveModelAlias(key) === key to avoid duplicate candidates. Consider memoizing candidate lists per model key.
| 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; |
There was a problem hiding this comment.
🟡 Medium — API Key Fallback Operator
The || operator treats empty string "" as falsy and falls back to env var. If a user explicitly clears their key, the env var still wins unexpectedly.
Suggested fix: const apiKey = auth?.key ?? process.env.OMNIROUTE_API_KEY;
| }; | ||
| } | ||
|
|
||
| const modelMetadata = mergeModelMetadata( |
There was a problem hiding this comment.
🟡 Medium — No Validation on Merged modelMetadata
The mergeModelMetadata function casts metadata as OmniRouteModelMetadata without runtime validation. A malformed config could inject non-boolean values into capability flags.
Suggested fix: Add runtime type guards or use a schema validator (zod/valibot) for OmniRouteModelMetadata.
| return Object.keys(out).length > 0 ? out : undefined; | ||
| } | ||
|
|
||
| function mergeModelMetadata( |
There was a problem hiding this comment.
🟡 Medium — Array Metadata Ordering Assumption
When user provides array config, generated blocks are prepended. If OpenCode uses 'last match wins', the order is wrong. Verify OpenCode's modelMetadata array matching logic. If 'first match wins', the order is correct. If 'last match wins', this is a bug.
| @@ -377,22 +430,33 @@ function toProviderModels( | |||
| function toProviderModel(model: OmniRouteModel, baseUrl: string): OmniRouteProviderModel { | |||
There was a problem hiding this comment.
🟡 Medium — supportsTools Default Inconsistent with Combo Logic
In toProviderModel: supportsTools = model.supportsTools !== false (defaults to true). In calculateLowestCommonCapabilities: supportsTools = model.tool_call === true (defaults to false). A model without explicit tool support data will be advertised as tool-capable, but a combo containing it will lose tool capability.
Suggested fix: Align defaults or document the rationale.
| reasoning: supportsReasoning, | ||
| temperature: supportsTemperature, | ||
| tool_call: supportsTools, | ||
| modalities: { |
There was a problem hiding this comment.
🟢 Low — Redundant as const Assertions
as const is used to narrow types, but modalities is typed as readonly string[] anyway. The assertions are redundant.
Suggested fix: Remove as const or tighten the type to readonly ['text', 'image'] | readonly ['text'].
| providerID: string; | ||
| family: string; | ||
| release_date: string; | ||
| attachment?: boolean; |
There was a problem hiding this comment.
🟡 Medium — New Provider Model Fields May Not Match OpenCode Shape
New optional fields (attachment, reasoning, temperature, tool_call, modalities) are added to OmniRouteProviderModel. Verify these match exactly what @opencode-ai/plugin expects. Risk: tool_call vs toolCall naming discrepancy could cause OpenCode to ignore the field.
Suggested fix: Cross-reference with plugin type definitions.
📋 Remaining Review Items (Cannot Be Posted Inline)The following issues were identified but cannot be posted as inline comments because they affect lines not modified in this PR. They are still relevant and should be addressed in follow-up work. 🟡 Medium Priority1. Cache Key Ignores modelsDev Config (
2. DRY Violation: splitModelId Duplicated (
3. Magic Constant 4096 (
4. Variants Type Too Permissive (
🟢 Low Priority5. Early Return Bypasses Metadata Tracking (
6. Sensitive Data in Log (
7. Log Injection via Model IDs (
8. Synchronous File I/O (
🧪 Testing Gaps9. Missing Temperature/Reasoning Combo Tests (
10. Missing Variant+Alias Integration Tests (
11. Missing Subscription Fallback Tests (
12. Hardcoded Ports in Tests (
📝 Documentation13. Missing CHANGELOG Entry
14. Missing Trailing Newline (
Summary✅ Already Fixed in This Branch:
✅ Posted as Inline Comments: 10 review comments on changed lines 📌 This Follow-up Comment: 14 additional items for future PRs Recommendation: Address the 4 medium-priority items before merging if possible, or create follow-up issues for them. The low-priority items can be deferred. |
Review Fixes Complete — All 21 Tasks AddressedThis branch contains comprehensive fixes for all review issues raised in PR #18. Below is a summary of the 20 commits addressing each concern: Security & Correctness
Performance & Architecture
Type Safety & Documentation
Test Coverage
Verification✅ 38 tests pass (0 failures) cc @kilo-code @gemini-code-assist — All review items have been addressed. Ready for final review and merge. |
There was a problem hiding this comment.
Code Review
This pull request introduces support for new model capabilities, specifically temperature, reasoning, and attachment, across the OmniRoute service. Key changes include refactoring the model lookup logic to handle model aliases, subscription provider fallbacks, and reasoning effort variant suffixes. Additionally, the plugin now supports merging generated model metadata with user-provided configurations and can retrieve API keys from environment variables. New tests have been added to verify the metadata merging logic and the calculation of common capabilities. I have no feedback to provide as there were no review comments to assess.
|
Thanks for the work on this PR, @abien and @Alph4d0g. I tested 1. Deduplicate OmniRoute alias/canonical model IDsOmniRoute ollamacloud/deepseek-v4-flash
ollama-cloud/deepseek-v4-flash
ollamacloud/kimi-k2.6
ollama-cloud/kimi-k2.6OpenCode then shows duplicate models. For the OpenCode provider UX, the plugin should probably dedupe equivalent IDs and prefer the canonical Minimal implementation sketch in const rawModels = data.data
.filter((model) => model !== null && model !== undefined && typeof model.id === "string")
.map(normalizeModel)
.filter((model, _index, models) => {
if (!model.id.startsWith("ollamacloud/")) return true;
const canonicalId = `ollama-cloud/${model.id.slice("ollamacloud/".length)}`;
return !models.some((candidate) => candidate.id === canonicalId);
});More generally, this could become a small alias-canonical map if other providers have the same pattern. 2. Map OmniRoute native
|
Update: Model Normalization & Deduplication ImplementedI've pushed the implementation for comprehensive model metadata normalization and deduplication as discussed in the review. Commits Added (5)
Key ChangesNormalization Precedence: Deduplication Logic:
Metadata Handling:
VerificationFiles Changed
This addresses both claims from the review:
Ready for re-review! |
Fixes Applied from gemini-code-assist Review (PR #20)All review feedback has been addressed: 1. Array-based Metadata Merge Precedence ✅Issue: User-defined overrides in Fix: return [...validUserConfig, ...generatedBlocks];2. Array-based Metadata Validation ✅Issue: Array-based merge did not validate user-provided metadata blocks. Fix: Added validation filter for user blocks with warning logs for invalid entries: const validUserConfig = userConfig.filter((block) => {
const validation = isValidModelMetadata(block);
if (!validation.valid) {
warn(`Invalid metadata block for match "...", skipping`);
return false;
}
return true;
});3. Unknown Provider Prefix Merge ✅Issue: Unknown provider prefixes were overwritten instead of merged, losing metadata. Fix: Now merges metadata for unknown prefixes: const existing = seen.get(model.id);
seen.set(model.id, existing ? { ...existing, ...model } : model);4. Deduplication Logic Simplification ✅Issue: Dead code ( Fix: Simplified to always merge alias into existing, preferring canonical fields: if (!existing) {
seen.set(canonicalId, { ...model, id: canonicalId });
} else {
// Merge alias metadata into existing, preferring existing (canonical) fields
seen.set(canonicalId, { ...model, ...existing, id: canonicalId });
}Test ResultsAll 42 tests passing with updated expectations for the new array ordering. |
Version 1.2.0 Release Ready 🚀All changes have been implemented, tested, and documented: Version Bump
CHANGELOG UpdatedComprehensive entry for v1.2.0 documenting:
README Updated
Test ResultsThis PR is ready for final review and merge! |
* feat: enrich model capabilities via models.dev and fix provider alias resolution - Add env var fallback for API key in config hook (OMNIROUTE_API_KEY) - Generate modelMetadata from enriched models so OpenCode applies capabilities - Add top-level capability fields (temperature, reasoning, attachment, tool_call, modalities) to provider model output - Generate reasoning effort variants (low/medium/high) for reasoning models - Add provider aliases: glmt/glm→zai-coding-plan, kimi-coding/kmc→moonshotai, gh/github→google - Add subscription fallback: zai-coding-plan→zai, kimi-for-coding→moonshotai - Add model aliases for naming mismatches (kimi-k2.6-thinking→kimi-k2-thinking) - Strip reasoning effort variant suffixes (gpt-5.5-xhigh→gpt-5.5) for lookup - Close 19 of 132 models with 4096 default context limits * fix: address model metadata review feedback Preserve user modelMetadata while merging generated capabilities, propagate new capability fields through models.dev and combo paths, and consolidate alias resolution. * fix: preserve combo attachment metadata Treat missing attachment capability as unknown instead of false when folding combo model capabilities, while still honoring explicit false values. * fix: respect explicit attachment false Avoid falling back to vision support when models.dev or combo metadata explicitly marks attachments unsupported. * fix: address code review issues from PR #18 - Fix temperature AND-chain bug in calculateLowestCommonCapabilities by tracking hasTemperatureMetadata - Fix reasoning AND-chain bug in calculateLowestCommonCapabilities by tracking hasReasoningMetadata - Respect explicit supportsAttachment=false for vision models in toProviderModel - Add normalized key candidates to getModelLookupCandidates for better alias/variant matching Addresses feedback from Copilot, Gemini Code Assist, and Kilo Code Bot reviews. * fix: use nullish coalescing for API key fallback * fix: add runtime validation for modelMetadata merge * fix: improve modelMetadata validation coverage * fix: include modelsDev config in cache key * refactor: extract splitModelId to eliminate DRY violation * docs: document supportsTools default rationale * fix: avoid logging sensitive response data in models fetch error * style: add trailing newline to models-dev.ts * fix: sanitize model IDs in log messages to prevent injection * perf: use async file I/O for logger to prevent event loop blocking * fix: strengthen log sanitization and cover missed interpolation sites - Expand sanitizeForLog to strip all control chars except tab (was only \r\n) - Export sanitizeForLog from omniroute-combos.ts for reuse in plugin.ts - Sanitize model IDs and external values in plugin.ts debug/warn lines: - Available models list (line 146) - provider.api / apiMode mismatch (lines 209, 215) - Unsupported apiMode option (line 231) - Unsupported baseURL protocol (line 254) - Invalid baseURL (line 260) - Invalid metadata field (line 371) - Intercepting request URL (line 581) - Include models.ts null-data fix in same commit Refs: task-10-log-injection * perf: avoid duplicate lookup candidates when alias resolves to same key * types: tighten variants type to prevent invalid reasoning effort values * refactor: extract magic constant 4096 to named defaults * style: remove redundant as const assertions * test(tasks 15,17,18,19): Add model metadata, temperature/reasoning, variant+alias, and subscription fallback tests - Task 15: Add test verifying single model and combo-with-self produce identical capabilities - Task 17: Add temperature/reasoning combo tests (mixed defined/undefined, false override, all three capabilities, undefined single model) - Task 18: Add variant suffix stripping + alias resolution integration test - Task 19: Add subscription provider fallback enrichment test * test(task 20): Replace hardcoded ports with getDummyBaseUrl() helper - Add getDummyBaseUrl(port) helper function to eliminate magic numbers - Replace all hardcoded http://localhost:20xxx URLs with helper calls - Keeps assertions and external URLs using hardcoded strings for clarity * fix: align provider model fields with OpenCode plugin types * types: add OmniRoute native fields (snake_case, capabilities) to OmniRouteModel * feat: normalize all OmniRoute field variants (snake_case, capabilities) * feat: add provider alias-to-canonical mapping for deduplication * test: verify normalization of snake_case and capabilities fields * fix: preserve user metadata by only deduplicating known aliases and canonicalizing metadata keys * fix: address gemini-code-assist review feedback on PR #20 - Fix array-based metadata merge precedence: userConfig now comes first in first-match-wins systems instead of being overridden by generated blocks - Add validation for array-based user metadata blocks to filter out invalid entries before merging - Merge metadata for unknown provider prefixes instead of overwriting, preserving all available metadata - Simplify deduplication logic: remove unreachable isAlias check and merge alias metadata into canonical entries, preferring existing (canonical) fields - Update test expectations to match new array ordering (user first, generated second) * chore: bump version to 1.2.0, update CHANGELOG and README - Bump version from 1.1.4 to 1.2.0 in package.json - Add comprehensive CHANGELOG entry for v1.2.0 covering: - Model metadata normalization (camelCase, snake_case, capabilities) - Provider alias deduplication system - Array-based metadata validation and precedence fixes - Metadata key canonicalization - All gemini-code-assist review feedback addressed - Update README with: - New features: metadata normalization, alias deduplication, variant support - Updated OmniRouteModel type documentation with native fields - Star History graph at the bottom --------- Co-authored-by: Alexander Bien <abien@gmx.net>
Summary
OMNIROUTE_API_KEY)modelMetadatafrom enriched models so OpenCode applies capabilities (temperature, reasoning, attachment, vision, tools)glmt/glm→zai-coding-plan,kimi-coding/kmc→moonshotai,gh/github→googlezai-coding-plan→zai,kimi-for-coding→moonshotaikimi-k2.6-thinking→kimi-k2-thinking)gpt-5.5-xhigh→gpt-5.5) for lookupImpact
Testing
npm run build)opencode models --verbose --refreshafter restart