Fix OpenRouter remote discovery and unify managed model sync - #1521
Conversation
![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 refactors the model import and synchronization process by moving logic from the client-side to a new server-side utility, 'importManagedModels'. It introduces a 'merge' mode for imports to preserve manually added models, standardizes model source normalization, and updates the provider models route to prioritize remote API discovery. The changes include new unit tests for catalog source normalization and sync route behavior. Feedback suggests sorting endpoint arrays to ensure deterministic change detection and correcting the logic for reporting the count of synced models to avoid misleading logs.
| const endpoints = Array.from( | ||
| new Set( | ||
| value | ||
| .map((entry) => toNonEmptyString(entry)) | ||
| .filter((entry): entry is string => Boolean(entry)) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
The normalizeSupportedEndpoints function should sort the resulting array. This ensures that even if the provider returns endpoints in a different order during subsequent syncs, the resulting model object remains consistent. This is critical for the JSON.stringify comparison used in summarizeImportedChanges to avoid false positive update reports.
| const endpoints = Array.from( | |
| new Set( | |
| value | |
| .map((entry) => toNonEmptyString(entry)) | |
| .filter((entry): entry is string => Boolean(entry)) | |
| ) | |
| ); | |
| const endpoints = Array.from( | |
| new Set( | |
| value | |
| .map((entry) => toNonEmptyString(entry)) | |
| .filter((entry): entry is string => Boolean(entry)) | |
| ) | |
| ).sort(); |
| const syncedModelsCount = | ||
| persistedModels.length > 0 | ||
| ? persistedModels.length | ||
| : importedModels.length > 0 | ||
| ? importedModels.length | ||
| : discoveredModels.length; |
There was a problem hiding this comment.
The fallback logic for syncedModelsCount is misleading. It should directly reflect the number of models returned in the models field of the response (which is persistedModels.length). Falling back to discoveredModels.length when persistedModels is empty would cause the count to include built-in models that were not actually persisted as custom models, leading to inconsistent API responses and confusing logs.
| const syncedModelsCount = | |
| persistedModels.length > 0 | |
| ? persistedModels.length | |
| : importedModels.length > 0 | |
| ? importedModels.length | |
| : discoveredModels.length; | |
| const syncedModelsCount = persistedModels.length; |
There was a problem hiding this comment.
Pull request overview
Unifies manual “import from /models” and scheduled managed model sync behind the same backend flow, normalizes “synced” source labeling, and ensures OpenRouter prefers remote /models discovery over the static image catalog fallback.
Changes:
- Route compatible-provider “manual import” through
/sync-models?mode=importand centralize managed sync logic in a newimportManagedModelshelper. - Normalize “auto-sync” / “imported” source handling to the unified
api-synclabel across sync, import, and catalog UI. - Adjust provider
/modelsdiscovery ordering so OpenRouter’s remote config path is tried before static image-provider models.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/provider-models-route.test.ts | Adds coverage asserting OpenRouter uses remote /models discovery over static image catalog. |
| tests/unit/model-sync-route.test.ts | Updates expected synced source to api-sync and adds import-mode merge behavior test coverage. |
| tests/unit/model-catalog-source.test.ts | Adds unit tests for consistent source normalization and labels. |
| src/shared/utils/modelCatalogSearch.ts | Normalizes auto-sync / imported sources to api-sync for consistent catalog labeling. |
| src/lib/providerModels/managedModelImport.ts | Introduces centralized managed import/sync logic, merging behavior, and alias syncing controls. |
| src/lib/providerModels/managedAvailableModels.ts | Adds pruneMissing option to avoid alias pruning during import/merge mode. |
| src/app/api/providers/[id]/sync-models/route.ts | Switches sync route to use the unified managed import flow and returns import-mode metadata. |
| src/app/api/providers/[id]/models/route.ts | Ensures providers with remote models config don’t short-circuit to static models first (OpenRouter fix). |
| src/app/(dashboard)/dashboard/providers/[id]/page.tsx | Updates compatible-provider import UI to call the managed sync endpoint instead of manual per-model writes. |
| const fetchedModels = modelsData.models || []; | ||
|
|
||
| // Filter out models already in the built-in registry | ||
| const registryIds = new Set(getModelsByProviderId(logProvider).map((m: any) => m.id)); | ||
|
|
||
| // Replace the full model list | ||
| const models = fetchedModels | ||
| .map((m: any) => ({ | ||
| id: m.id || m.name || m.model, | ||
| name: m.name || m.displayName || m.id || m.model, | ||
| source: "auto-sync", | ||
| ...(Array.isArray(m.supportedEndpoints) && m.supportedEndpoints.length > 0 | ||
| ? { supportedEndpoints: m.supportedEndpoints } | ||
| : {}), | ||
| ...(typeof m.inputTokenLimit === "number" ? { inputTokenLimit: m.inputTokenLimit } : {}), | ||
| ...(typeof m.outputTokenLimit === "number" ? { outputTokenLimit: m.outputTokenLimit } : {}), | ||
| ...(typeof m.description === "string" ? { description: m.description } : {}), | ||
| ...(m.supportsThinking === true ? { supportsThinking: true } : {}), | ||
| })) | ||
| .filter((m: any) => m.id && !registryIds.has(m.id)); | ||
|
|
||
| const previousModels = await getCustomModels(logProvider); | ||
| const replaced = await replaceCustomModels(logProvider, models); | ||
|
|
||
| try { | ||
| const syncedModels = normalizeDiscoveredModels(fetchedModels); | ||
| if (syncedModels.length > 0) { | ||
| await replaceSyncedAvailableModelsForConnection(logProvider, id, syncedModels); | ||
| } | ||
| } catch (e) { | ||
| console.error(`Failed to union synced available models for ${logProvider}:`, e); | ||
| } | ||
|
|
||
| const modelChanges = summarizeModelChanges(previousModels, replaced); | ||
| const { persistedModels, importedModels, discoveredModels, syncedAliases, importedChanges } = | ||
| await importManagedModels({ | ||
| providerId: logProvider, | ||
| connectionId: id, | ||
| fetchedModels, | ||
| mode, | ||
| }); |
There was a problem hiding this comment.
importManagedModels() already fetches and returns previousModels, but this route fetches previousModels again via getCustomModels(logProvider). This adds an extra DB read per sync/import and can also make the diff calculation inconsistent if models change between the two reads. Consider returning previousModels from importManagedModels and using that value here (or passing previousModels into importManagedModels).
| const syncedModelsCount = | ||
| persistedModels.length > 0 | ||
| ? persistedModels.length | ||
| : importedModels.length > 0 | ||
| ? importedModels.length | ||
| : discoveredModels.length; |
There was a problem hiding this comment.
syncedModelsCount is computed from persistedModels.length first, but persistedModels includes retained manual/custom models (especially in mode === "merge" and also in sync where manual models are preserved). This will over-report the number of models actually synced/imported in logs and API responses. Consider using importedModels.length (models written by this sync) or tracking a dedicated count of synced/imported models separately from the total persisted list.
| const syncedModelsCount = | |
| persistedModels.length > 0 | |
| ? persistedModels.length | |
| : importedModels.length > 0 | |
| ? importedModels.length | |
| : discoveredModels.length; | |
| const syncedModelsCount = importedModels.length; |
| syncedModels: syncedModelsCount, | ||
| syncedAliases, | ||
| modelChanges, | ||
| importedCount: importedChanges.total, |
There was a problem hiding this comment.
importedCount is derived from importedChanges.total (added + updated). The UI strings (importSuccessCount, etc.) read like “new models imported”, so counting updates as imports may produce misleading success messages. Consider changing importedCount to importedChanges.added, or return separate importedAddedCount / importedUpdatedCount fields and have the UI pick the appropriate one.
| importedCount: importedChanges.total, | |
| importedCount: importedChanges.added, |
| /** | ||
| * POST /api/providers/[id]/sync-models | ||
| * | ||
| * Fetches the model list from a provider's /models endpoint and replaces the | ||
| * full custom models list for that provider while refreshing the per-connection | ||
| * discovery cache. Successful syncs only write a call log when the fetched | ||
| * channel actually changes the stored model list. | ||
| * | ||
| * Used by: | ||
| * - modelSyncScheduler (auto-sync on interval) | ||
| * - Manual trigger from UI | ||
| */ | ||
| export async function POST(request: Request, { params }: { params: Promise<{ id: string }> }) { |
There was a problem hiding this comment.
Per repo guidelines, PRs that change production code under src/ should run the coverage gate (npm run test:coverage) and maintain ≥60% statements/lines/functions/branches. The PR description lists individual unit tests but not the coverage run/result; please add the coverage run output (or update tests until it passes) before merging.
| function normalizeDiscoveredModels(models: unknown): SyncedAvailableModel[] { | ||
| const items = Array.isArray(models) ? models : []; | ||
| const deduped = new Map<string, SyncedAvailableModel>(); | ||
|
|
||
| for (const item of items) { | ||
| const record = asRecord(item); | ||
| const id = | ||
| toNonEmptyString(record.id) || | ||
| toNonEmptyString(record.name) || | ||
| toNonEmptyString(record.model); | ||
| if (!id) continue; | ||
|
|
||
| const name = | ||
| toNonEmptyString(record.name) || | ||
| toNonEmptyString(record.displayName) || | ||
| toNonEmptyString(record.model) || | ||
| id; | ||
| const supportedEndpoints = normalizeSupportedEndpoints(record.supportedEndpoints); | ||
|
|
||
| deduped.set(id, { | ||
| id, | ||
| name, | ||
| source: "api-sync", | ||
| ...(supportedEndpoints ? { supportedEndpoints } : {}), | ||
| ...(typeof record.inputTokenLimit === "number" | ||
| ? { inputTokenLimit: record.inputTokenLimit } | ||
| : {}), | ||
| ...(typeof record.outputTokenLimit === "number" | ||
| ? { outputTokenLimit: record.outputTokenLimit } | ||
| : {}), | ||
| ...(typeof record.description === "string" ? { description: record.description } : {}), | ||
| ...(record.supportsThinking === true ? { supportsThinking: true } : {}), | ||
| }); | ||
| } | ||
|
|
||
| return Array.from(deduped.values()); | ||
| } |
There was a problem hiding this comment.
normalizeDiscoveredModels() duplicates the existing normalization logic in src/lib/providerModels/modelDiscovery.ts. Keeping two slightly-different copies increases the chance they drift (fields, endpoint normalization, dedupe rules). Consider importing and reusing the shared normalizeDiscoveredModels helper (or moving the pure normalization logic into a shared utility module).
|
Thanks @rdself for this great contribution! 🎉 This is a fantastic architectural improvement that unifies the sync models flow and properly unlocks OpenRouter discovery. We've officially merged it into the |
…uzapw#1521) Integrated into release/v3.7.0
…uzapw#1521) Integrated into release/v3.7.0
Summary
/modelsdiscovery for providers like OpenRouter instead of falling back to the static image catalog when a remote models config existsRoot cause
Manual
Import from /modelsand scheduled sync used different write paths and different source labels, which made the UI treat synced models differently from manually imported ones. OpenRouter also hit the static image-provider fallback before the remote/modelsconfig path, so discovery could stop at the four image models from the local catalog.Testing
https://openrouter.ai/api/v1/modelsand a real OpenAI-compatible endpoint athttps://api.bltcy.aiusing both import mode and scheduled sync semantics